Skip to content

fix: keep credentials on the host the request started from - #13

Merged
umputun merged 4 commits into
masterfrom
fix/redirect-credential-leak
Aug 19, 2026
Merged

fix: keep credentials on the host the request started from#13
umputun merged 4 commits into
masterfrom
fix/redirect-credential-leak

Conversation

@paskal

@paskal paskal commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

http.Client strips Authorization and 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:

client := requester.New(http.Client{}, middleware.BasicAuth("svc", "secret")).Client()
client.Get(origin.URL) // origin answers 302 to another host, which receives Authorization: Basic c3ZjOnNlY3JldA==

An open redirect or a compromised endpoint on the origin was enough.

The rule now matches what http.Client uses 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.

  • BasicAuth is scoped, always
  • Header is scoped for keys the client itself treats as credentials, i.e. Authorization, Www-Authenticate, Cookie, Cookie2, Proxy-Authorization and Proxy-Authenticate. Any other header keeps going on every hop as before
  • SecretHeader(key, value) is new and gives the same protection to a custom header, the X-Auth of the README example being the case that needs it

What breaks: BasicAuth and 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 as User-Agent or a tracing key are untouched.

Three deliberate calls worth your view:

  1. On a hop away from the original host, a key the client does not recognise as a credential is removed entirely, the caller's own value included, since the client copies those from the original request to every hop. For the recognised keys only the middleware's own value is removed, because values there can belong to the destination, put in by the cookie jar or by a CheckRedirect hook. The cost is that a hook setting exactly the same credential value loses it.
  2. Hosts are compared as written. The standard client canonicalises through IDNA before comparing, which needs 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.
  3. The chain is walked over 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.Client following a 302 across hosts.

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.
@paskal
paskal requested a review from umputun as a code owner August 19, 2026 06:01
@umputun

umputun commented Aug 19, 2026

Copy link
Copy Markdown
Member

the host-chain walk and domainOrSubdomain are right, including rejecting the evil-example.com suffix case and refusing anything with a colon or percent. One thing in dropHeader needs changing before this goes in, though.

For the six recognised credential keys, the value this middleware set on the previous hop can never be present on the next one. http.Client copies redirect headers from a clone of the very first request, not from the hop that just ran, and net/http/client.go:761 says so directly:

// 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 stripSensitiveHeaders is already set, so those keys are not copied at all. Anything present by the time the middleware runs therefore belongs to the destination, put there by CheckRedirect or by the jar. The value comparison then removes a legitimate destination value whenever it happens to equal the configured one, so a CheckRedirect hook setting the same Basic credential, or a destination cookie matching the configured Cookie value, is silently erased.

Off-host, for a recognised key, the middleware can just not set anything and leave the header alone. Del is still right for an unrecognised SecretHeader key, since the client does copy those. Worth a test where the destination value is exactly equal to the configured one, the current tests pass because the two differ.

Two smaller things. README line 57 still shows Header("X-Auth", "123456789"), and line 67 uses SecretHeader with the same header name, so the first example teaches the leaking form for a header that looks like a credential. X-Trace would make the contrast do its job.

And a custom RoundTripper may legitimately return a response with Request nil, which requester supports. onOriginalHost then cannot resolve the origin and returns false, so a same-host redirect through such a transport drops the credential. No leak, and failing closed is the right default, but it either wants resp.Request filled in when the wrapped transport leaves it nil, or a line in the docs.

paskal added 2 commits August 19, 2026 07:54
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.
@paskal

paskal commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Changed as you described, and you are right about the copier: makeHeadersCopier clones the initial request's headers before the first RoundTrip, so the value this middleware sets is never in that clone, and on a cross-host hop the recognised keys are not copied at all.

  • recognised credential keys off the original host: nothing is set and nothing is removed, so a CheckRedirect hook or the jar keeps its value even when it equals the configured one. Test covers exactly that, with Authorization set to the configured Basic value and Cookie set to the configured cookie value, both surviving
  • an unrecognised SecretHeader key still goes with Del, since the client does copy those from the original request
  • Response.Request is filled in by the middleware when the transport below leaves it unset, so a same-host redirect through such a transport keeps its credentials. Test drives a real http.Client over a transport that returns a 302 with Request nil. The doc comment keeps the fail-closed note for a chain that never passed through the middleware
  • README: the Header example is X-Trace now, the SecretHeader one stays X-Auth, and the intro example uses SecretHeader since its comment calls it an auth header

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 _example dependency bump, since it was not worth a PR of its own.

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.
@umputun
umputun merged commit d04fffb into master Aug 19, 2026
7 checks passed
@umputun
umputun deleted the fix/redirect-credential-leak branch August 19, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants