Strip userinfo from the rendered request-target (RFC 9110 §4.2.4) - #35
Conversation
RFC 9110 §4.2.4 and §4.2.5: "A sender MUST NOT generate a userinfo subcomponent (and its "@" delimiter) when an 'http'/'https' URI reference is generated within a message as a request target or header field value." `Request.target` rendered the whole URI, so a request built from a URI carrying credentials put them on the wire — and therefore into server and proxy logs — as `GET http://USER:PW@h.example/a/c HTTP/1.1`. The strip happens on a copy, so the credentials stay on the request's URI for whoever wants to turn them into an `Authorization` header. The empty-path special case (RFC 9112 §3.2.1) now tests the stripped URI, which also gives a userinfo-only URI a valid `/` target instead of the `//user@` it rendered before.
There was a problem hiding this comment.
Build & Tests
carp -x test/http.carp on 9dbdb86 → 397 passed, 0 failed. CI green on both runners, both check runs at the current head sha. angler, carp-fmt --check and carp -x gendocs.carp (no docs/ diff) all clean. No CHANGELOG in this repo.
Teeth reproduce exactly: with only the http.carp hunk reverted, the suite is 394 / 3, and the three that fail are the three named in the description. The two control assertions (request-target without userinfo is unchanged, the credentials stay on the request's URI) pass in both trees, which is what makes them controls rather than padding.
I also merged this with #34 and ran the result, since merge-tree being clean is not the same as the two changes being compatible. The merged tree carries both (RFC 9110 §4.2.4 at http.carp:315, 29 ETag occurrences) and is 452 passed, 0 failed.
Findings
Nothing blocking. What I checked:
Differential over the whole request-target surface
28 parsed URIs plus 6 synthetic ones the parser cannot produce, rendered through Request.str on master and on the branch. 21 rows change, and every one of them removes userinfo and changes nothing else. Two of those rows are worth naming because the description does not:
http://:PW@h.example/a/c master: GET http://:PW@h.example/a/c branch: GET http://h.example/a/c
http://@h.example/a/c master: GET http://@h.example/a/c branch: GET http://h.example/a/c
A password with no user also reached the wire on master — URI.parse gives user = Just "" there, so URI.str's apply on the user branch fires and renders :PW@. The fix covers it because it blanks the component rather than the rendering, but it is a second leak shape and a reason the fix is in the right place.
The same property shows up as a non-regression I specifically went looking for: a USER:PW@ sequence in the query is untouched.
http://USER:PW@h.example/a%2Fb?u=USER:PW@x#z -> GET http://h.example/a%2Fb?u=USER:PW@x#z
A textual scrub would have eaten that. Setting user/password to Nothing and re-rendering cannot.
Everything without userinfo is byte-identical across the two trees, including //:8080 and // from degenerate synthetic URIs, mailto:a@b, path-only, query-only and fragment-only URIs, [::1]:8080, and the default-port elisions for :80/:443.
The dropped user conjunct in the empty-target guard
Removing (Maybe.nothing? (URI.user u)) is safe rather than merely tidy: v has user = Nothing by construction, so the conjunct would be vacuously true. I checked the guard cannot now produce an empty target, which is the failure mode that removing a conjunct invites — a URI reaching URI.str with everything blank but password still renders / (SYN password-only), because a password alone never renders without a user.
The behaviour change the description calls out is real and is an improvement: a userinfo-only URI went from GET //solo@ HTTP/1.1 to GET / HTTP/1.1, and //solo@?a=1 to /?a=1.
The stored URI really does survive, on both construction paths
The test pins it for Request.get. It also holds for a parsed request, which matters more — a server reading credentials off an incoming absolute-form request line still gets them, while a forwarded copy no longer carries them:
GET http://USER:PW@h.example/a/c HTTP/1.1 (parsed)
Request.uri user = Just(USER)
Request.uri password = Just(PW)
Request.str = GET http://h.example/a/c HTTP/1.1
This is genuinely the wire form, not just a rendering
Before checking out this branch I pointed a raw socket at http-client (which pins http@0.4.2, i.e. master) and logged the actual request lines:
GET http://USER:PW@127.0.0.1:8899/a/hop1 HTTP/1.1 ; hop 1
GET http://USER:PW@127.0.0.1:8899/a/c HTTP/1.1 ; hop 2, relative Location
GET http://USER:PW@127.0.0.1:8899/ HTTP/1.1 ; hop 2, Location: /
Client.build-and-send sends (Request.str &req) verbatim, and those bytes are exactly what Request.str returns on master, so the premise holds on a real socket and not only in a test harness. Credentials were on the wire on every hop.
Two leads, both out of scope here and correctly named as such
- Fragments are still rendered into the target, which I confirmed (
GET http://h.example:8080/a?x=1#f). RFC 9112 §3.2.1 makes that a MUST NOT too, and it is now literally one moreURI.set-fragment … (Maybe.Nothing)in the samelet— the hard part (deciding to blank components rather than post-process the string) is done by this PR. - The reason userinfo could reach a request line at all is absolute-form.
httpdoes not force it —targetrenders whatever URI it is handed — buthttp-client'sbuild-and-sendhands it the whole parsed URI, so every request it makes goes out asGET http://host/path. RFC 9112 §3.2.1 says a client making a request directly to an origin server sends only the path and query; origin-form would have made both this bug and the fragment one unreachable.test/http.carp:429deliberately pins absolute-form here, so that belongs inhttp-clientrather than in this PR, but it is worth knowing that this fix is treating a symptom of that choice.
Verdict: merge
A MUST NOT violation that put credentials into every server and proxy log, fixed by blanking the components rather than scrubbing the string — which is why a USER:PW@ inside a query survives and a :PW@ with an empty user does not. 21 changed rows across a 34-URI differential, all of them removals of userinfo and nothing else, and the merged tree with #34 is green.
Request.targetrendered the request-target as(URI.str u), i.e. the whole URI including any userinfo. A request built from a URI that carries credentials therefore put them on the wire in the request line — one of the most routinely logged parts of a request, so they end up in server and proxy logs.RFC 9110 §4.2.4 (
http) and §4.2.5 (https):Before / after, for
(Request.get (URI.parse "http://USER:PW@h.example/a/c") …):What changed
targetrenders a copy of the URI withuserandpasswordset toNothing. The stored URI is untouched, so a caller can still read the credentials offRequest.uriand turn them into anAuthorizationheader — a test pins that.The form of the target is not touched: absolute-form stays absolute-form (
"absolute-form request-target is left alone"still passes), and the empty-path rule from RFC 9112 §3.2.1 still applies. That rule now looks at the stripped copy rather than the original, which is what makes a userinfo-only URI render as/instead of the//user@it produced before; without that, stripping would have left it with an empty target.Fragments are still rendered into the target — RFC 9112 §3.2.1 says they should not be. Out of scope here.
Tests
Five assertions next to the three existing request-target tests in
test/http.carp. Reverting only thehttp.carphunk fails three of them:The other two pass in both trees on purpose:
request-target without userinfo is unchangedis the no-regression control, andthe credentials stay on the request’s URIpins that only the rendering changed.Checks
carp -x test/http.carp— 397 passed, 0 failedcarp -x gendocs.carp— clean, no docs diff (targetis private)carp-fmt --check http.carp test/http.carp— cleanangler http.carp test/http.carp— cleangit merge-tree --write-treeagainst #34's head (185af3a) merges cleanly; that PR reopensdefmodule Requestfurther down the file and does not touchtarget.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.