Page MenuHomePhabricator

Don't include default port (80, 443) in "Host:" headers from Conduit
ClosedPublic

Authored by epriestley on Sep 5 2016, 1:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 3, 10:04 AM
Unknown Object (File)
Thu, May 2, 3:00 PM
Unknown Object (File)
Mon, Apr 29, 1:40 PM
Unknown Object (File)
Mon, Apr 29, 3:12 AM
Unknown Object (File)
Thu, Apr 25, 3:30 AM
Unknown Object (File)
Fri, Apr 19, 5:16 PM
Unknown Object (File)
Tue, Apr 16, 5:15 AM
Unknown Object (File)
Tue, Apr 9, 11:31 PM
Subscribers
None
Tokens
"Doubloon" token, awarded by michaeljs1990.

Details

Summary

Fixes T11587.

In Q473, a user had lighttpd configured with a nonstandard port (8080), and their configuration did not work because we omitted the port from the Host: header.

There is a good argument that our behavior was in the wrong, and D16464 fixed this, by adding the port to the Host: header.

However, in T11587, this broke a different user who had a configuration which did not expect a port number in a port 80 request.

Use a behavior which is more similar to what browsers do:

  • If the protocol is http and the port is 80, or the protocol is https and the port is 443, omit the port.
  • Otherwise, include the port.

I think there's a good argument that our current behavior is spec-compliant and servers that do not treat Host: domain.com:80 the same as Host: domain.com are in the wrong, but realistically the lifetime support cost of fighting this battle is almost certainly higher than following the "empirical spec" and doing what browsers do.

Test Plan
  • Added some debugging code to dump the strings.
  • Ran arc list to test non-signed requests.
  • Ran bin/repository reparse --message ... on a cluster device to test signed requests.
  • In both cases, saw proper outputs (for Host: domain.com, domain.com:123; for signing: domain.com:80, domain.com:123).

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Don't include default port (80, 443) in "Host:" headers from Conduit.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, michaeljs1990.
  • Slightly more documentation.
chad edited edge metadata.
This revision is now accepted and ready to land.Sep 5 2016, 2:25 PM
This revision was automatically updated to reflect the committed changes.