Make redirect handling configurable in all five languages - #120
Merged
Conversation
Redirects were followed unconditionally everywhere, and TypeScript went
further than "no option": doGlobalFetch did
merge({}, init, { redirect: 'follow' })
with the literal LAST, so a caller passing redirect:'manual' or 'error' had it
silently overwritten. There was a test covering this — it asked for 'follow'
and asserted 'follow', so it passed under the bug and would have kept passing
forever. Python hardcoded follow_redirects=True into the httpx kwargs and never
exposed it; Rust, Go and .NET set nothing at all and inherited platform
defaults that follow up to ten hops.
This is a security gap and not only an ergonomic one. A caller that resolves a
hostname and checks it against an SSRF allowlist has that guard defeated
entirely by a 302 to an internal address, because the check was performed on
the original host. RFC 8461 separately forbids following redirects when
fetching an MTA-STS policy. api-prime hit both and had to hand-roll its own
reqwest client to get around this library.
TypeScript redirect is honoured (defaults first, caller last)
Python FetchOptions.follow_redirects
Rust RequestInit.follow_redirects: Option<bool>
Go ClientBuilder.WithFollowRedirects
.NET SmooFetchOptions.FollowRedirects / WithFollowRedirects
Rust uses Option<bool> rather than bool for two reasons: a bare bool would make
derive(Default) produce FALSE and silently flip behaviour for every existing
..Default::default() caller, and None lets merge_init tell "unset" from
"explicitly true" so a client-level default is not clobbered by a per-request
init. Go applies the setting to a caller-supplied *http.Client too — unlike the
connect timeout — because a security control that silently did not apply
because you brought your own client is worse than not offering one.
Honouring the option was NOT sufficient by itself, which four of the five
languages proved in turn: a 3xx is neither "ok" nor "redirected", so it was
raised as an error and the option undone a line later. TS, Rust, Go and .NET
each now return a deliberately-unfollowed 3xx as an ordinary response. Python
already did, via is_redirect.
Two things the tests caught that review would not have. .NET's
SmooFetchBuilder.Build() copies options field by field, so FollowRedirects was
honoured by the handler and dropped in transit until a test failed — a standing
hazard for every future option, now noted in the code. And .NET wraps JSON
deserialization failures in the same HttpResponseError it raises for a bad
status, so the redirect fixture serves a JSON body deliberately; without one
the throwing-path test cannot tell the two apart.
Defaults are unchanged everywhere: everything still follows unless a caller
says otherwise. Each language's opt-out was mutation-tested — reverting the fix
turns the new tests red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 36fedd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Redirects were followed unconditionally in all five languages. TypeScript went further than "no option":
The literal is last, and
mergelets later sources win — so a caller passingredirect: 'manual'had it silently overwritten.There was already a test for this. It asks for
'follow'and asserts'follow', so it passed under the bug and would have kept passing forever.follow_redirects=Truehardcoded into the httpx kwargshttp.Clientdefault, 10 hopsAllowAutoRedirecttrueWhy it matters beyond ergonomics
A caller that resolves a hostname and checks it against an SSRF allowlist has that guard defeated entirely by a 302 to an internal address — the check was performed on the original host. RFC 8461 separately forbids following redirects when fetching an MTA-STS policy.
api-primehit both and had to hand-roll its ownreqwestclient to work around this library.The API
redirecthonoured (defaults first, caller last)FetchOptions.follow_redirectsRequestInit.follow_redirects: Option<bool>ClientBuilder.WithFollowRedirectsSmooFetchOptions.FollowRedirects/WithFollowRedirectsTwo deliberate choices. Rust uses
Option<bool>: a bareboolwould makederive(Default)producefalseand silently flip behaviour for every existing..Default::default()caller, andNoneletsmerge_initdistinguish "unset" from "explicitly true" so a client-level default isn't clobbered. Go applies the setting to a caller-supplied*http.Clienttoo — unlike the connect timeout — because a security control that silently doesn't apply because you brought your own client is worse than not offering one.Honouring the option wasn't enough
Four of the five languages proved this in turn: a 3xx is neither
oknorredirected, so it was raised as an error and the option undone a line later. TS, Rust, Go and .NET each now return a deliberately-unfollowed 3xx as an ordinary response. Python already did, viais_redirect.Two things only the tests caught
.NET's
SmooFetchBuilder.Build()copies options field by field.FollowRedirectswas honoured by the handler and dropped in transit — the option worked when set directly and did nothing through the builder. That's a standing hazard for every future option, now noted in the code..NET wraps JSON deserialization failures in the same
HttpResponseErrorit raises for a bad status. The redirect fixture serves a JSON body deliberately; without one the throwing-path test can't tell "threw for the 3xx" from "threw because an empty body won't deserialize".Verification
Every language mutation-tested — reverting each fix turns its new tests red.
tsc --noEmit,cargo fmt/clippy -D warnings,go vet,dotnet buildall cleanDefaults are unchanged everywhere: everything still follows unless a caller says otherwise, so this is a
minor.Pearl: th-86dc77
🤖 Generated with Claude Code