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
F14046935: D16491.diff
Thu, Nov 14, 12:45 AM
F14034568: D16491.diff
Sun, Nov 10, 1:03 AM
F14033435: D16491.id39687.diff
Sat, Nov 9, 5:39 PM
F14033152: D16491.id39687.diff
Sat, Nov 9, 4:55 PM
F14031903: D16491.diff
Sat, Nov 9, 12:12 PM
F14020991: D16491.diff
Wed, Nov 6, 4:01 AM
F13972447: D16491.id39686.diff
Thu, Oct 17, 7:03 PM
F13959396: D16491.id39687.diff
Oct 14 2024, 7:13 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
Branch
xport1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13565
Build 17467: Run Core Tests
Build 17466: arc lint + arc unit

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.