fix: keep credentials on the host the request started from - #13
Conversation
Header and BasicAuth restored credentials on every hop, including the ones the client stripped when a redirect left the original host, so a 302 to another host received them. Credentials are now set while the chain stays on the original host or one of its subdomains, matching how the standard client decides it, and SecretHeader gives the same protection to a custom header carrying a secret.
|
the host-chain walk and For the six recognised credential keys, the value this middleware set on the previous hop can never be present on the next one. // The headers to copy are from the very initial request.
// We use a closured callback to keep a reference to these original headers.
ireqhdr = cloneOrMakeHeader(ireq.Header)On a cross-host hop Off-host, for a recognised key, the middleware can just not set anything and leave the header alone. Two smaller things. README line 57 still shows And a custom |
Off the original host the client copies no credential header from the original request, so what the middleware sees there belongs to the destination, set by a CheckRedirect hook or by the cookie jar. Recognised keys are now left untouched instead of being matched by value, which erased a destination value equal to the configured one. A header the client doesn't recognise as a credential is still removed whole, since that one does get copied to every hop. The middleware fills the request in on the response when the transport below leaves it unset, so a redirect through such a transport keeps its credentials on the original host.
lcw to v1.2.0, which drops hashicorp/errwrap and go-multierror and brings go-redis v9.18.0 and go.uber.org/atomic. The go directive stays at 1.23, so go-redis stops at v9.18.0: v9.19.0 needs go 1.24 and the current x/sys needs 1.25.
|
Changed as you described, and you are right about the copier:
One thing to weigh, since the simplification leans on the client doing the stripping. The module allows any go 1.23 patch release, and two of the relevant fixes are late: credentials could be restored later in a redirect chain before 1.23.5 (GO-2025-3420), and proxy credentials were not stripped before 1.23.10 (GO-2025-3751). On such a toolchain a value the middleware set can still be on a foreign hop, and the middleware no longer takes it off. Removing only the exact configured value would cover it, at the cost of the destination-value case you asked me to fix. Happy to add it back behind that trade-off if you would rather not depend on the toolchain being current. Also folded #14 in here as one commit, the |
The job checks out, builds, lints and posts coverage to coveralls, none of which writes to the repository, so the token gets contents: read as the rest of the org does it.
http.ClientstripsAuthorizationand the other credential headers before a redirect hop that leaves the original host. Both middlewares sat below it and put the credential back, so a followed 3xx handed it to whatever host the redirect pointed at:An open redirect or a compromised endpoint on the origin was enough.
The rule now matches what
http.Clientuses for its own headers: a credential is set while the redirect chain stays on the host the request started from or on one of its subdomains, and is gone once the chain leaves. Leaving and coming back does not bring it back.BasicAuthis scoped, alwaysHeaderis scoped for keys the client itself treats as credentials, i.e.Authorization,Www-Authenticate,Cookie,Cookie2,Proxy-AuthorizationandProxy-Authenticate. Any other header keeps going on every hop as beforeSecretHeader(key, value)is new and gives the same protection to a custom header, theX-Authof the README example being the case that needs itWhat breaks:
BasicAuthand credential-named headers no longer reach a host outside the original one. Code relying on that stops working, which is the point of the change. Plain headers such asUser-Agentor a tracing key are untouched.Three deliberate calls worth your view:
CheckRedirecthook. The cost is that a hook setting exactly the same credential value loses it.golang.org/x/net, and the module has no dependencies outside the standard library. The difference only shows on an internationalised host redirecting between its unicode and punycode forms, where the credential stays behind rather than leaking.Response.Request, set by the standard transport. A base transport that leaves it unset makes the origin unknowable, and that is treated as a hop away from the origin rather than towards it.Tests cover the matching rule as a table over synthesised chains, including subdomains, ports, case, a chain returning to the original host, and a broken chain, plus an end-to-end pass through a real
http.Clientfollowing a 302 across hosts.