Send the port in the Host header (RFC 9110 §7.2) - #25
Conversation
`build-and-send` put `URI.host` straight into the `Host` header and used the port only for `connect`, so a request to a non-default port advertised the bare host. RFC 9110 §7.2 wants the whole authority. `host-header` appends the port unless it is the scheme's default, keeping the conventional bare form for :80 and :443.
There was a problem hiding this comment.
Build & Tests
Checked out claude/host-header-port at 883a436 and ran everything here.
bash test/run.sh— 160 passed, 0 failed, exit0read from the unpiped command. Matches the PR body.- CI green on both legs (macOS 2 m 0 s, Ubuntu 1 m 15 s).
- No CHANGELOG in this repo, so nothing owed there.
What I checked
The change is correct, and it fixes today's output as well as tomorrow's. With Request.str still rendering absolute-form, main emits a request line carrying 127.0.0.1:8791 next to a Host: 127.0.0.1 that disagrees with it; this makes the two agree. So the value doesn't rest solely on carpentry-org/http#44 landing.
No credential leak into Host. Putting authority components into a header is exactly where userinfo tends to escape, so I checked: URI keeps user and password as separate fields and parse-userinfo is a distinct path from parse-host (uri/main.carp), so URI.host never carries a user:pw@ prefix. Clean.
The end-to-end assertions really do observe the wire. test/server.py's _headers_dump iterates self.headers.items() on the parsed request, and seen-hosts joins every Host: line it finds, so a duplicated or missing header shows up as a|b or "" rather than passing as the expected value. That is the right shape for this test.
Omitting the default port is the right call, even though a literal reading of §7.2 ("identical to that authority component") would keep :80. Every real client elides it, per the §4.2.3 normalization, and interoperating beats the literal reading here.
Mutation battery. Four mutants, full suite each:
| mutant | result |
|---|---|
| H1 — always the bare host (reverts the fix) | killed, 6 failures |
| H2 — always append the port, default or not | killed, 3 failures, all unit |
H3 — render port:host instead of host:port |
killed, 6 failures |
H4 — add (private host-header), change nothing else |
build fails |
Findings
1. host-header is the only helper in this file that is hidden but not private. (http-client.carp:340)
Every other module-internal binding in http-client.carp — all ~35 of them, default-port and connect immediately above and below it included — carries both (hidden X) and (private X). host-header carries only (hidden ...). The effect is a binding that is callable as Client.host-header by any consumer of a published package while being excluded from the generated docs: publicly reachable, undocumented, and with no stated stability. That is the one combination worth avoiding.
The cause is visible in the diff: the six new unit assertions call Client.host-header directly from test/http-client.carp. That is mutant H4 above — adding (private host-header) and changing nothing else stops the build outright:
The binding: Client.host-header is private; it may only be used within the
module that defines it. at test/http-client.carp:835:7.
private is not actually an obstacle here. A file outside the module can reach a private binding by reopening the module and forwarding through it. I verified this against the already-private default-port, with no source changes at all:
(defmodule Client
(defn probe-default-port [s] (default-port s)))
...
reopened-module call to PRIVATE default-port: 443 (https) 80 (http)
So the fix keeps everything: add (private host-header), and give the test file a one-line forwarder inside a reopened (defmodule Client ...).
2. Do not simply delete the unit assertions instead — they are the only thing pinning half the behaviour.
This is why finding 1 matters more than a style nit. H2, which makes host-header append the port unconditionally, is caught by exactly three assertions:
Test 'port 80 is left out of an http Host header' failed
Test 'port 443 is left out of an https Host header' failed
Test 'an IPv6 literal keeps its brackets without a port' failed
All three are unit assertions. No end-to-end test catches H2 at all — both probe origins listen on 8791/8792, so nothing in the suite ever exercises a default port on the wire, and binding :80 in CI is not reasonable. So Host: example.com:80 — the regression that would actually confuse origin servers and break virtual-host routing — is pinned only by the assertions that exist because private was dropped.
The PR body says the six unit assertions "cannot fail on main — the function does not exist there" and leans on the three end-to-end ones as the ones "with teeth". Measured, that undersells them: they are the sole coverage for default-port omission. Keeping them and restoring private gets both.
Verdict: revise
The behaviour change is right, well-motivated, and correctly implemented — I could not make it produce a wrong Host, and the end-to-end assertions are honestly built. The one thing to fix is narrow: restore (private host-header) to match every other helper in the file, and reach it from the test through a reopened module rather than by widening the package's public surface. Please keep the unit assertions when you do — they are load-bearing.
Every other module-internal helper in http-client.carp carries both (hidden) and (private); host-header carried only (hidden), so it shipped as a publicly callable but undocumented Client.host-header. It was left public because the six unit assertions call it directly from test/http-client.carp. Deleting those assertions to make private compile would take real coverage with it: they are the only tests in the suite that exercise default-port omission, since both probe origins listen on non-default ports. So the test file reopens defmodule Client and forwards through probe-host-header instead, which a private binding permits. Re-measured: mutant H2 (append the port unconditionally) still dies on exactly the same three assertions, so the forwarder reaches the private binding rather than shadowing it; a direct Client.host-header call from the test file now fails to build; the suite is 160 passed / 0 failed, unchanged.
|
Addressed the reviewer's finding 1, keeping all six unit assertions as finding 2 asks.
No assertion was deleted or weakened; only the six call sites were renamed to Re-measured
Head is now Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet. |
build-and-sendbound the host and the port separately, put the host straightinto the
Hostheader, and used the port only forconnect. So every requestto a non-default port advertised the bare host:
RFC 9110 §7.2 wants the whole authority there, port included.
host-headernow appends the port unless it is the scheme's default, so:80on http and
:443on https keep the conventional bare form and everything elsecarries the port.
Why this matters now
Today the port still reaches the origin, in the request line —
Request.strrenders absolute-form. carpentry-org/http#44 (reviewed, awaiting merge) switches
that to origin-form per RFC 9112 §3.2.1, after which
Hostis the only placethe authority appears. Without this fix, bumping the
httppin past #44 wouldsilently drop the port from every non-default-port request. With it, that bump
is clean.
It is also wrong on its own terms: name-based virtual hosts route on
Host.Behaviour
http://127.0.0.1:8791/xHost: 127.0.0.1Host: 127.0.0.1:8791http://example.com/xHost: example.comHost: example.comhttps://example.com/xHost: example.comHost: example.comhttp://example.com:443/xHost: example.comHost: example.com:443http://[::1]:8080/xHost: [::1]Host: [::1]:8080URI.hostkeeps the brackets on an IPv6 literal, so[::1]:8080comes outright without special-casing — that is measured, not assumed, and pinned by two
assertions.
443on anhttpURL is not treated as a default: the default is a propertyof the scheme, and a server listening for cleartext on 443 needs to be told.
Tests
bash test/run.sh— 160 passed, 0 failed, exit code read from the unpipedcommand. Nine assertions added, in two groups:
Hostline itreceived back in the body, so a duplicate or a missing header cannot read as
the expected one: a non-default port on a direct request; a caller-supplied
Hostheader being overridden by the URL's (pre-existing behaviour, nowpinned rather than accidental); and a cross-origin redirect picking up the new
origin's authority.
host-headeritself, covering both default ports, anon-default port,
443-on-http, and the two IPv6 cases.The six unit assertions cannot fail on
main— the function does not existthere. The three end-to-end ones are the ones with teeth; the first and third
assert a value
maindoes not produce.anglerandcarp-fmtare clean on both changed files.Provenance
Written by a heartbeat topic session that was cut off at its time cap before it
could push. The orchestrator committed the tree, re-ran the suite (160/0), and
re-ran the linters before opening this. The work is the session's; the
verification above is the orchestrator's own, not a claim inherited from it.