From 36fedd8fc674a7a97c18173ebd25249782435f6c Mon Sep 17 00:00:00 2001 From: Brent Date: Thu, 27 Aug 2026 20:42:52 -0400 Subject: [PATCH] Make redirect handling configurable in all five languages (th-86dc77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Go ClientBuilder.WithFollowRedirects .NET SmooFetchOptions.FollowRedirects / WithFollowRedirects Rust uses Option 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) --- .changeset/redirect-policy.md | 30 ++++ .../SmooAI.Fetch.Tests/RedirectPolicyTests.cs | 159 ++++++++++++++++++ dotnet/SmooAI.Fetch/SmooFetch.cs | 30 +++- dotnet/SmooAI.Fetch/SmooFetchBuilder.cs | 19 +++ dotnet/SmooAI.Fetch/SmooFetchOptions.cs | 16 ++ go/fetch/builder.go | 51 +++++- go/fetch/builder_test.go | 77 +++++++++ go/fetch/client.go | 18 +- python/src/smooai_fetch/_client.py | 6 +- python/src/smooai_fetch/_types.py | 15 ++ python/tests/test_integration.py | 37 ++++ rust/fetch/src/builder.rs | 4 + rust/fetch/src/client.rs | 25 ++- rust/fetch/src/types.rs | 19 +++ rust/fetch/tests/builder_tests.rs | 4 + rust/fetch/tests/fetch_tests.rs | 85 ++++++++++ src/fetch.spec.ts | 52 ++++++ src/fetch.ts | 19 ++- 18 files changed, 641 insertions(+), 25 deletions(-) create mode 100644 .changeset/redirect-policy.md create mode 100644 dotnet/SmooAI.Fetch.Tests/RedirectPolicyTests.cs diff --git a/.changeset/redirect-policy.md b/.changeset/redirect-policy.md new file mode 100644 index 0000000..c577594 --- /dev/null +++ b/.changeset/redirect-policy.md @@ -0,0 +1,30 @@ +--- +'@smooai/fetch': minor +--- + +Make redirect handling configurable in all five languages + +Redirects were followed unconditionally everywhere, and TypeScript went further: +`merge({}, init, { redirect: 'follow' })` put the literal last, so a caller +passing `redirect: 'manual'` had it **silently overwritten**. Python hardcoded +`follow_redirects=True` into the httpx kwargs; Rust, Go and .NET set nothing and +inherited platform defaults that follow up to 10 hops. + +That is a security gap, not just an ergonomic one. A caller who resolves a +hostname and checks it against an SSRF allowlist has that guard defeated by a 302 +to an internal address, because the check was performed on the original host. And +RFC 8461 forbids following redirects when fetching an MTA-STS policy. + +- **TypeScript** — `redirect` is honoured (defaults first, caller last) +- **Python** — `FetchOptions.follow_redirects` +- **Rust** — `RequestInit.follow_redirects: Option` (`None` inherits, so a + client-level default survives a per-request `..Default::default()`) +- **Go** — `ClientBuilder.WithFollowRedirects`, applied to a caller-supplied + `*http.Client` too +- **.NET** — `SmooFetchOptions.FollowRedirects` / `WithFollowRedirects` + +Honouring the option was not sufficient on its own: in TS, Rust, Go and .NET a +3xx is neither "ok" nor "redirected", so it was raised as an error and the option +was undone a line later. Each now returns a deliberately-unfollowed 3xx as an +ordinary response. Defaults are unchanged — everything still follows unless a +caller says otherwise. diff --git a/dotnet/SmooAI.Fetch.Tests/RedirectPolicyTests.cs b/dotnet/SmooAI.Fetch.Tests/RedirectPolicyTests.cs new file mode 100644 index 0000000..9636c3f --- /dev/null +++ b/dotnet/SmooAI.Fetch.Tests/RedirectPolicyTests.cs @@ -0,0 +1,159 @@ +using System.Net; +using SmooAI.Fetch; + +namespace SmooAI.Fetch.Tests; + +/// +/// th-86dc77 — redirects were followed unconditionally (SocketsHttpHandler defaults +/// AllowAutoRedirect to true) with no way for a caller to decline. +/// +/// Following one defeats an SSRF check performed on the original host — a 302 to an +/// internal address bypasses a guard applied to the original hostname — and RFC 8461 +/// forbids it outright when fetching an MTA-STS policy. +/// +public class RedirectPolicyTests +{ + private sealed record Reply(bool Arrived); + + /// Serves a 302 at /start pointing at /landing, and 200 at /landing. + private static (HttpListener listener, string url, Func landed) StartRedirectingServer() + { + var landedFlag = false; + var port = GetFreePort(); + var prefix = $"http://127.0.0.1:{port}/"; + var listener = new HttpListener(); + listener.Prefixes.Add(prefix); + listener.Start(); + + _ = Task.Run(async () => + { + while (listener.IsListening) + { + HttpListenerContext ctx; + try + { + ctx = await listener.GetContextAsync(); + } + catch + { + return; + } + + if (ctx.Request.Url!.AbsolutePath == "/landing") + { + landedFlag = true; + ctx.Response.StatusCode = 200; + ctx.Response.ContentType = "application/json"; + var body = System.Text.Encoding.UTF8.GetBytes("{\"arrived\":true}"); + await ctx.Response.OutputStream.WriteAsync(body); + } + else + { + // The 302 carries a JSON body on purpose. Without one, a + // deserialization failure would surface as the SAME + // HttpResponseError this library raises for a bad status, + // and the throwing-path test below could not tell the two + // apart. + ctx.Response.StatusCode = 302; + ctx.Response.RedirectLocation = $"{prefix}landing"; + ctx.Response.ContentType = "application/json"; + var redirectBody = System.Text.Encoding.UTF8.GetBytes("{\"arrived\":false}"); + await ctx.Response.OutputStream.WriteAsync(redirectBody); + } + + ctx.Response.Close(); + } + }); + + return (listener, prefix, () => landedFlag); + } + + private static int GetFreePort() + { + var l = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); + l.Start(); + var port = ((System.Net.IPEndPoint)l.LocalEndpoint).Port; + l.Stop(); + return port; + } + + [Fact] + public void FollowRedirects_defaults_to_true() + { + Assert.True(new SmooFetchOptions().FollowRedirects); + } + + [Fact] + public async Task A_redirect_is_not_followed_when_the_caller_opts_out() + { + var (listener, url, landed) = StartRedirectingServer(); + try + { + var fetch = SmooFetchBuilder.Create() + .WithNoRetry() + .WithFollowRedirects(false) + .Build(); + + using var request = new HttpRequestMessage(HttpMethod.Get, $"{url}start"); + var response = await fetch.SendAsync(request); + + Assert.Equal(HttpStatusCode.Found, response.StatusCode); + // The landing route is mounted so following would visibly succeed — + // this asserts the hop did not happen, not that it 404'd. + Assert.False(landed(), "the redirect was followed despite WithFollowRedirects(false)"); + } + finally + { + listener.Stop(); + } + } + + [Fact] + public async Task Redirects_are_followed_by_default() + { + var (listener, url, landed) = StartRedirectingServer(); + try + { + var fetch = SmooFetchBuilder.Create().WithNoRetry().Build(); + + using var request = new HttpRequestMessage(HttpMethod.Get, $"{url}start"); + var response = await fetch.SendAsync(request); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.True(landed(), "default behaviour must be unchanged"); + } + finally + { + listener.Stop(); + } + } + + /// + /// The throwing path. SendAsync never raises, so it cannot show that a + /// deliberately-unfollowed 3xx is returned rather than thrown — without the + /// IsAcceptable change the option would be honoured and then immediately + /// undone by an . + /// + [Fact] + public async Task An_unfollowed_redirect_is_not_thrown_on_the_json_path() + { + var (listener, url, _) = StartRedirectingServer(); + try + { + var fetch = SmooFetchBuilder.Create() + .WithNoRetry() + .WithFollowRedirects(false) + .Build(); + + var reply = await fetch.GetAsync($"{url}start"); + + // Reached deserialization at all, which means the 3xx passed the + // status gate rather than being thrown as an HttpResponseError. + Assert.False(reply.Arrived); + } + finally + { + listener.Stop(); + } + } +} diff --git a/dotnet/SmooAI.Fetch/SmooFetch.cs b/dotnet/SmooAI.Fetch/SmooFetch.cs index f687cb6..41639c1 100644 --- a/dotnet/SmooAI.Fetch/SmooFetch.cs +++ b/dotnet/SmooAI.Fetch/SmooFetch.cs @@ -108,6 +108,12 @@ public static SmooFetch Create(Action? configure = null) handler.ConnectTimeout = connectTimeout; } + // Defaults to true on the handler, so only the opt-out is worth setting. + if (!options.FollowRedirects) + { + handler.AllowAutoRedirect = false; + } + var client = new HttpClient(handler) { Timeout = System.Threading.Timeout.InfiniteTimeSpan }; return new SmooFetch(client, ownsHttpClient: true, options, logger: null); } @@ -245,12 +251,32 @@ private async Task SendJsonNoResponseAsync( { using var request = BuildRequest(method, path, body, hasBody); using var response = await SendAsync(request, cancellationToken).ConfigureAwait(false); - if (!response.IsSuccessStatusCode) + if (!IsAcceptable(response)) { await ThrowHttpResponseErrorAsync(response, cancellationToken).ConfigureAwait(false); } } + /// + /// Whether a response should be returned rather than thrown. + /// + /// + /// A 3xx when is false is the ANSWER, + /// not a failure — the caller asked to see the redirect rather than follow it. Without + /// this, honouring the option leaves it useless: the 302 comes back and is immediately + /// thrown as an because it is not 2xx. + /// + private bool IsAcceptable(HttpResponseMessage response) + { + if (response.IsSuccessStatusCode) + { + return true; + } + + var status = (int)response.StatusCode; + return !_options.FollowRedirects && status >= 300 && status < 400; + } + private HttpRequestMessage BuildRequest(HttpMethod method, string path, object? body, bool hasBody) { var uri = ResolveUri(path); @@ -346,7 +372,7 @@ private static async Task CloneRequestAsync(HttpRequestMessa private async Task ReadJsonResponseAsync(HttpResponseMessage response, CancellationToken cancellationToken) { - if (!response.IsSuccessStatusCode) + if (!IsAcceptable(response)) { await ThrowHttpResponseErrorAsync(response, cancellationToken).ConfigureAwait(false); } diff --git a/dotnet/SmooAI.Fetch/SmooFetchBuilder.cs b/dotnet/SmooAI.Fetch/SmooFetchBuilder.cs index 72010e5..6c4e91c 100644 --- a/dotnet/SmooAI.Fetch/SmooFetchBuilder.cs +++ b/dotnet/SmooAI.Fetch/SmooFetchBuilder.cs @@ -76,6 +76,20 @@ public SmooFetchBuilder WithConnectTimeout(TimeSpan connectTimeout) return this; } + /// + /// Controls whether redirects are followed. Defaults to true. + /// + /// + /// Set false when a redirect must not be followed: it defeats an SSRF check + /// performed on the original host, and RFC 8461 forbids it when fetching an MTA-STS + /// policy. The 3xx is then returned rather than thrown. + /// + public SmooFetchBuilder WithFollowRedirects(bool followRedirects) + { + _options.FollowRedirects = followRedirects; + return this; + } + /// Register an async auth-token provider. public SmooFetchBuilder WithAuthTokenProvider(AuthTokenProvider provider, string scheme = "Bearer") { @@ -231,6 +245,11 @@ public SmooFetch Build() o.RetryPolicy = _options.RetryPolicy; o.Timeout = _options.Timeout; o.ConnectTimeout = _options.ConnectTimeout; + // NOTE: this method copies options field by field, so anything added + // to SmooFetchOptions and NOT listed here is silently dropped — + // WithFollowRedirects was honoured by the handler and lost in transit + // until a test caught it. + o.FollowRedirects = _options.FollowRedirects; o.AuthTokenProvider = _options.AuthTokenProvider; o.AuthScheme = _options.AuthScheme; o.JsonOptions = _options.JsonOptions; diff --git a/dotnet/SmooAI.Fetch/SmooFetchOptions.cs b/dotnet/SmooAI.Fetch/SmooFetchOptions.cs index 33bae2b..1d3ee39 100644 --- a/dotnet/SmooAI.Fetch/SmooFetchOptions.cs +++ b/dotnet/SmooAI.Fetch/SmooFetchOptions.cs @@ -32,6 +32,22 @@ public sealed class SmooFetchOptions /// public TimeSpan? ConnectTimeout { get; set; } + /// + /// Whether redirects are followed automatically. Defaults to true, matching + /// . + /// + /// + /// Set false when a redirect must not be followed. Two cases where following one + /// is wrong rather than merely surprising: a caller that resolved the target hostname and + /// checked it against an SSRF allowlist has that guard defeated by a 302 to an internal + /// address, because the check was performed on the original host; and RFC 8461 forbids + /// fetching an MTA-STS policy through a redirect. With false the 3xx is returned + /// as an ordinary response to inspect. + /// Ignored when is true (the factory owns the handler); + /// configure AllowAutoRedirect on the primary handler at DI registration instead. + /// + public bool FollowRedirects { get; set; } = true; + /// Optional auth token provider. When set, the returned token is added as Authorization: Bearer {token}. public AuthTokenProvider? AuthTokenProvider { get; set; } diff --git a/go/fetch/builder.go b/go/fetch/builder.go index c0bd72b..1141774 100644 --- a/go/fetch/builder.go +++ b/go/fetch/builder.go @@ -17,6 +17,7 @@ type ClientBuilder struct { circuitBreakerOpts *CircuitBreakerOptions circuitBreakerName string hooks *LifecycleHooks + followRedirects *bool authProvider AuthTokenProvider authScheme string } @@ -31,6 +32,28 @@ func NewClientBuilder() *ClientBuilder { } } +// WithFollowRedirects controls whether redirects are followed. Defaults to +// true, matching net/http. +// +// Set false when a redirect must not be followed automatically. Two cases where +// following one is wrong rather than merely surprising: +// +// - The caller resolved the target hostname and checked it against an SSRF +// allowlist. A 302 to an internal address defeats that guard entirely, +// because the check was performed on the original host. +// - RFC 8461 forbids fetching an MTA-STS policy through a redirect. +// +// With false the 3xx is returned as an ordinary response to inspect, via +// http.ErrUseLastResponse. +// +// This applies to a caller-supplied *http.Client too, unlike the connect +// timeout — an SSRF control that silently did not apply because you brought +// your own client would be worse than no option at all. +func (b *ClientBuilder) WithFollowRedirects(follow bool) *ClientBuilder { + b.followRedirects = &follow + return b +} + // WithHTTPClient sets the underlying *http.Client. func (b *ClientBuilder) WithHTTPClient(c *http.Client) *ClientBuilder { b.httpClient = c @@ -169,15 +192,16 @@ func (b *ClientBuilder) WithContainerOptions(opts FetchContainerOptions) *Client // Build constructs the Client from the builder configuration. func (b *ClientBuilder) Build() *Client { c := &Client{ - httpClient: b.httpClient, - baseHeaders: b.baseHeaders, - retry: b.retryOpts, - timeout: b.timeoutOpts, - connectTimeout: b.connectTimeout, - rateLimitRetry: b.rateLimitRetryOpts, - hooks: b.hooks, - authProvider: b.authProvider, - authScheme: b.authScheme, + httpClient: b.httpClient, + baseHeaders: b.baseHeaders, + retry: b.retryOpts, + timeout: b.timeoutOpts, + connectTimeout: b.connectTimeout, + rateLimitRetry: b.rateLimitRetryOpts, + followRedirects: b.followRedirects, + hooks: b.hooks, + authProvider: b.authProvider, + authScheme: b.authScheme, } if c.authScheme == "" { c.authScheme = "Bearer" @@ -192,6 +216,15 @@ func (b *ClientBuilder) Build() *Client { } } + // Applied to whichever client is in use, including a caller-supplied one. + // This is a security control, and one that silently did not apply because + // the caller brought their own client would be worse than not offering it. + if b.followRedirects != nil && !*b.followRedirects { + c.httpClient.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + } + if c.baseHeaders == nil { c.baseHeaders = make(http.Header) } diff --git a/go/fetch/builder_test.go b/go/fetch/builder_test.go index 2bff375..fcbd6ca 100644 --- a/go/fetch/builder_test.go +++ b/go/fetch/builder_test.go @@ -389,3 +389,80 @@ func TestClientBuilder_WithCircuitBreakerStateChange(t *testing.T) { t.Errorf("expected Closed→Open as first transition, got %d→%d", observed[0].from, observed[0].to) } } + +// th-86dc77 — redirects were followed unconditionally (net/http's default), +// with no way for a caller to say otherwise. Following one defeats an SSRF +// check performed on the original host, and RFC 8461 forbids it when fetching +// an MTA-STS policy. +func TestWithFollowRedirectsFalseReturnsThe3xx(t *testing.T) { + var landed bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/landing" { + landed = true + w.WriteHeader(http.StatusOK) + return + } + w.Header().Set("Location", "/landing") + w.WriteHeader(http.StatusFound) + })) + defer server.Close() + + client := NewClientBuilder().WithFollowRedirects(false).Build() + resp, err := Fetch[any](context.Background(), client, http.MethodGet, server.URL+"/start", nil, nil) + if err != nil { + t.Fatalf("a 3xx is a response, not a transport error: %v", err) + } + if resp.StatusCode != http.StatusFound { + t.Errorf("expected 302, got %d", resp.StatusCode) + } + // The landing handler is mounted so that following would visibly succeed — + // this asserts the hop did not happen, not that it 404'd. + if landed { + t.Error("the redirect was followed despite WithFollowRedirects(false)") + } +} + +func TestRedirectsAreFollowedByDefault(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/landing" { + w.WriteHeader(http.StatusOK) + return + } + w.Header().Set("Location", "/landing") + w.WriteHeader(http.StatusFound) + })) + defer server.Close() + + client := NewClientBuilder().Build() + resp, err := Fetch[any](context.Background(), client, http.MethodGet, server.URL+"/start", nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("default must still follow; got %d", resp.StatusCode) + } +} + +// The option must survive WithHTTPClient — an SSRF control that silently did +// not apply because the caller brought their own client would be worse than +// not offering one. +func TestFollowRedirectsAppliesToACallerSuppliedClient(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", "/landing") + w.WriteHeader(http.StatusFound) + })) + defer server.Close() + + client := NewClientBuilder(). + WithHTTPClient(&http.Client{Timeout: 5 * time.Second}). + WithFollowRedirects(false). + Build() + + resp, err := Fetch[any](context.Background(), client, http.MethodGet, server.URL+"/start", nil, nil) + if err != nil { + t.Fatalf("a 3xx is a response, not a transport error: %v", err) + } + if resp.StatusCode != http.StatusFound { + t.Errorf("expected 302 through a caller-supplied client, got %d", resp.StatusCode) + } +} diff --git a/go/fetch/client.go b/go/fetch/client.go index 8bee4d1..f399bdf 100644 --- a/go/fetch/client.go +++ b/go/fetch/client.go @@ -27,9 +27,13 @@ type Client struct { rateLimiter *SlidingWindowRateLimiter rateLimitRetry *RateLimitRetryOptions circuitBreaker *CircuitBreaker - hooks *LifecycleHooks - authProvider AuthTokenProvider - authScheme string + // followRedirects is nil when unset (follow, the net/http default). Kept on + // the Client so the success check below can tell a deliberately-unfollowed + // 3xx from an unexpected one. + followRedirects *bool + hooks *LifecycleHooks + authProvider AuthTokenProvider + authScheme string } // transportWithConnectTimeout clones the default transport (preserving proxy, @@ -314,7 +318,13 @@ func executeHTTPRequest[T any]( } // Parse response - isOK := resp.StatusCode >= 200 && resp.StatusCode < 300 + // A 3xx when the caller opted OUT of following is the answer, not a + // failure — they asked to see the redirect. Without this, honouring + // WithFollowRedirects(false) leaves the option useless: the 302 comes back + // and is immediately raised as an HTTP error because it is not 2xx. + unfollowedRedirect := client.followRedirects != nil && !*client.followRedirects && + resp.StatusCode >= 300 && resp.StatusCode < 400 + isOK := (resp.StatusCode >= 200 && resp.StatusCode < 300) || unfollowedRedirect isJSON := false contentType := resp.Header.Get("Content-Type") if strings.Contains(contentType, "application/json") { diff --git a/python/src/smooai_fetch/_client.py b/python/src/smooai_fetch/_client.py index ba6622f..5a75a77 100644 --- a/python/src/smooai_fetch/_client.py +++ b/python/src/smooai_fetch/_client.py @@ -51,7 +51,11 @@ def _build_request_kwargs( kwargs: dict[str, Any] = { "method": opts.method, "url": url, - "follow_redirects": True, + # Caller-controlled: following a redirect defeats an SSRF check + # performed on the original host, and RFC 8461 forbids it outright when + # fetching an MTA-STS policy. Defaults True, so existing callers are + # unaffected. + "follow_redirects": opts.follow_redirects, } # Headers diff --git a/python/src/smooai_fetch/_types.py b/python/src/smooai_fetch/_types.py index c321cac..6694951 100644 --- a/python/src/smooai_fetch/_types.py +++ b/python/src/smooai_fetch/_types.py @@ -302,3 +302,18 @@ class FetchOptions: auth_scheme: str = "Bearer" """Auth scheme prefix used with `auth_token_provider`. Defaults to "Bearer".""" + + follow_redirects: bool = True + """Whether to follow HTTP redirects. Defaults to True. + + Set False when a redirect must not be followed automatically. Two cases + where following one is wrong rather than merely surprising: + + - The caller resolved the target hostname and checked it against an SSRF + allowlist. A 302 to an internal address defeats that guard entirely, + because the check was performed on the original host. + - RFC 8461 forbids fetching an MTA-STS policy through a redirect. + + With False, a 3xx is returned as an ordinary response for the caller to + inspect rather than raising. + """ diff --git a/python/tests/test_integration.py b/python/tests/test_integration.py index d4c9d08..452f37f 100644 --- a/python/tests/test_integration.py +++ b/python/tests/test_integration.py @@ -321,3 +321,40 @@ async def test_201_created(self): assert response.ok assert response.status_code == 201 assert response.data == {"id": "new-id"} + + +class TestRedirectPolicy: + """th-86dc77 — follow_redirects was hardcoded True in the httpx kwargs and + not exposed on FetchOptions, so a caller could not decline to follow. + + Following one defeats an SSRF check performed on the original host (a 302 + to an internal address bypasses it), and RFC 8461 forbids it outright when + fetching an MTA-STS policy. + """ + + def test_follow_redirects_defaults_to_true(self): + """Unset must mean follow, so existing callers are unaffected.""" + assert FetchOptions().follow_redirects is True + + @respx.mock + async def test_redirect_not_followed_when_caller_opts_out(self): + start = respx.get(URL).mock(return_value=httpx.Response(302, headers={"Location": f"{URL}/landing"})) + landing = respx.get(f"{URL}/landing").mock(return_value=httpx.Response(200, json={"arrived": True})) + + response = await fetch(URL, FetchOptions(follow_redirects=False, retry=None)) + + assert response.status_code == 302, "the redirect must not have been followed" + assert start.called + # Mounted so that following would visibly succeed — this asserts the hop + # did not happen, not that it 404'd. + assert not landing.called, "the redirect was followed despite follow_redirects=False" + + @respx.mock + async def test_redirects_are_followed_by_default(self): + respx.get(URL).mock(return_value=httpx.Response(302, headers={"Location": f"{URL}/landing"})) + landing = respx.get(f"{URL}/landing").mock(return_value=httpx.Response(200, json={"arrived": True})) + + response = await fetch(URL, FetchOptions(retry=None)) + + assert response.status_code == 200, "default behaviour must be unchanged" + assert landing.called diff --git a/rust/fetch/src/builder.rs b/rust/fetch/src/builder.rs index 035df1b..2bfc454 100644 --- a/rust/fetch/src/builder.rs +++ b/rust/fetch/src/builder.rs @@ -371,6 +371,10 @@ impl FetchClient { method: init.method, headers: merged_headers, body: init.body.or_else(|| default.body.clone()), + // Per-request wins, but only when it SAYS something — + // `None` inherits, so a client default of `Some(false)` + // survives a per-request `..Default::default()`. + follow_redirects: init.follow_redirects.or(default.follow_redirects), } } None => init, diff --git a/rust/fetch/src/client.rs b/rust/fetch/src/client.rs index ff2d0f4..1894942 100644 --- a/rust/fetch/src/client.rs +++ b/rust/fetch/src/client.rs @@ -174,12 +174,16 @@ async fn do_single_request( init: &RequestInit, connect_timeout_ms: Option, ) -> Result, FetchError> { - let client = match connect_timeout_ms { - Some(ms) => reqwest::Client::builder() - .connect_timeout(Duration::from_millis(ms)) - .build()?, - None => reqwest::Client::new(), - }; + let mut client_builder = reqwest::Client::builder(); + if let Some(ms) = connect_timeout_ms { + client_builder = client_builder.connect_timeout(Duration::from_millis(ms)); + } + if init.follow_redirects == Some(false) { + // Per-client, not per-request, in reqwest — which is why a caller could + // not previously express this without building its own client. + client_builder = client_builder.redirect(reqwest::redirect::Policy::none()); + } + let client = client_builder.build()?; let mut request_builder = client.request(init.method.to_reqwest(), url); @@ -246,7 +250,14 @@ async fn do_single_request( let fetch_response = FetchResponse::new(status, status_text, headers, body, is_json, data); - if fetch_response.ok { + // A 3xx when the caller opted OUT of following is the answer, not a + // failure — they asked to see the redirect. Honouring `follow_redirects` + // without this leaves the option useless: reqwest returns the 302, and it + // is then raised as an `HttpResponse` error because it is not 2xx. + let is_unfollowed_redirect = + init.follow_redirects == Some(false) && (300..400).contains(&(status as u32)); + + if fetch_response.ok || is_unfollowed_redirect { Ok(fetch_response) } else { Err(FetchError::from_response(&fetch_response, None)) diff --git a/rust/fetch/src/types.rs b/rust/fetch/src/types.rs index ffd0644..3e55397 100644 --- a/rust/fetch/src/types.rs +++ b/rust/fetch/src/types.rs @@ -223,4 +223,23 @@ pub struct RequestInit { pub headers: HashMap, /// Request body (serialized as a string). pub body: Option, + /// Whether to follow HTTP redirects. `None` inherits the client default, + /// which is to follow. + /// + /// Set `Some(false)` when a redirect must not be followed automatically. + /// Two cases where following one is wrong rather than merely surprising: + /// + /// - The caller resolved the target hostname and checked it against an + /// SSRF allowlist. A 302 to an internal address defeats that guard + /// entirely, because the check was performed on the original host. + /// - RFC 8461 forbids fetching an MTA-STS policy through a redirect. + /// + /// With `Some(false)` the 3xx is returned as an ordinary response. + /// + /// `Option` rather than `bool` for two reasons: it keeps + /// `#[derive(Default)]` correct (a bare `bool` would default to FALSE and + /// silently flip behaviour for every `..Default::default()` caller), and it + /// lets [`crate::builder`]'s `merge_init` tell "unset" from "explicitly + /// true" so a client-level default is not clobbered by a per-request init. + pub follow_redirects: Option, } diff --git a/rust/fetch/tests/builder_tests.rs b/rust/fetch/tests/builder_tests.rs index 9cb3ba0..0a734f3 100644 --- a/rust/fetch/tests/builder_tests.rs +++ b/rust/fetch/tests/builder_tests.rs @@ -76,6 +76,7 @@ async fn test_builder_with_default_headers() { method: Method::GET, headers: default_headers, body: None, + follow_redirects: None, }) .build(); @@ -218,6 +219,7 @@ async fn test_builder_merge_headers() { method: Method::GET, headers: default_headers, body: None, + follow_redirects: None, }) .build(); @@ -229,6 +231,7 @@ async fn test_builder_merge_headers() { method: Method::GET, headers: request_headers, body: None, + follow_redirects: None, }; let response = client.fetch(&url, init).await.unwrap(); @@ -262,6 +265,7 @@ async fn test_builder_post_with_body() { method: Method::POST, headers, body: Some(r#"{"name":"test"}"#.to_string()), + follow_redirects: None, }; let response = client.fetch(&url, init).await.unwrap(); diff --git a/rust/fetch/tests/fetch_tests.rs b/rust/fetch/tests/fetch_tests.rs index 8c9ec9a..f395c0b 100644 --- a/rust/fetch/tests/fetch_tests.rs +++ b/rust/fetch/tests/fetch_tests.rs @@ -77,6 +77,7 @@ async fn test_basic_post_request_with_body() { method: Method::POST, headers, body: Some(r#"{"key":"value"}"#.to_string()), + follow_redirects: None, }; let options = FetchOptions { connect_timeout_ms: None, @@ -339,3 +340,87 @@ async fn test_default_method_is_get() { assert!(response.ok); } + +// th-86dc77 — redirects were followed unconditionally (reqwest's default of up +// to 10 hops), with no way for a caller to say otherwise. `follow_redirects` is +// `Option` so that `None` inherits and a client-level default is not +// clobbered by a per-request `..Default::default()`. +#[test] +fn request_init_defaults_to_following_redirects() { + let init = RequestInit::default(); + assert_eq!( + init.follow_redirects, None, + "unset must mean inherit, not 'do not follow'" + ); +} + +#[tokio::test] +async fn a_redirect_is_not_followed_when_the_caller_opts_out() { + let server = MockServer::start().await; + Mock::given(path("/start")) + .respond_with(ResponseTemplate::new(302).insert_header("location", "/landing")) + .mount(&server) + .await; + // Mounted so that FOLLOWING would visibly succeed — the assertion below is + // about the 302 coming back, not about the hop 404ing. + Mock::given(path("/landing")) + .respond_with(ResponseTemplate::new(200).set_body_string("arrived")) + .mount(&server) + .await; + + let init = RequestInit { + follow_redirects: Some(false), + ..Default::default() + }; + let result = client::fetch::( + &format!("{}/start", server.uri()), + init, + Some(FetchOptions { + connect_timeout_ms: None, + timeout: Some(TimeoutOptions { timeout_ms: 5000 }), + retry: None, + }), + None, + None, + None, + None, + ) + .await; + + let response = result.expect("a 3xx is a response, not a transport error"); + assert_eq!( + response.status, 302, + "the redirect must not have been followed" + ); +} + +#[tokio::test] +async fn a_redirect_is_followed_by_default() { + let server = MockServer::start().await; + Mock::given(path("/start")) + .respond_with(ResponseTemplate::new(302).insert_header("location", "/landing")) + .mount(&server) + .await; + Mock::given(path("/landing")) + .respond_with(ResponseTemplate::new(200).set_body_string("arrived")) + .mount(&server) + .await; + + let response = client::fetch::( + &format!("{}/start", server.uri()), + RequestInit::default(), + Some(FetchOptions { + connect_timeout_ms: None, + timeout: Some(TimeoutOptions { timeout_ms: 5000 }), + retry: None, + }), + None, + None, + None, + None, + ) + .await + .expect("follows by default"); + + assert_eq!(response.status, 200, "default behaviour must be unchanged"); +} diff --git a/src/fetch.spec.ts b/src/fetch.spec.ts index a4ae3f3..6700f82 100644 --- a/src/fetch.spec.ts +++ b/src/fetch.spec.ts @@ -789,6 +789,58 @@ describe('Test fetch', () => { expect(mockFetch.mock.calls[0][1]?.redirect).toBe('follow'); }); + // th-86dc77 — `redirect` used to be forced to 'follow' by + // `merge({}, init, { redirect: 'follow' })`, the literal winning + // because merge lets later sources win. The test above passed under + // that bug: it asks for 'follow' and gets 'follow'. These do not. + test('honours redirect: manual instead of forcing follow', async () => { + const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; + mockFetch.mockResolvedValue(fakeResponse(true, 200)); + + await fetch(URL_TO_USE, { method: 'GET', redirect: 'manual' }); + + expect(mockFetch.mock.calls[0][1]?.redirect).toBe('manual'); + }); + + test('honours redirect: error', async () => { + const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; + mockFetch.mockResolvedValue(fakeResponse(true, 200)); + + await fetch(URL_TO_USE, { method: 'GET', redirect: 'error' }); + + expect(mockFetch.mock.calls[0][1]?.redirect).toBe('error'); + }); + + test('still defaults to follow when the caller says nothing', async () => { + const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; + mockFetch.mockResolvedValue(fakeResponse(true, 200)); + + await fetch(URL_TO_USE, { method: 'GET' }); + + expect(mockFetch.mock.calls[0][1]?.redirect).toBe('follow'); + }); + + // Honouring the option is not enough on its own: a 3xx under manual is + // neither `ok` nor `redirected`, so it would otherwise be thrown as an + // HTTPResponseError and the option would be useless. + test('returns a 3xx under redirect: manual rather than throwing', async () => { + const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; + mockFetch.mockResolvedValue(fakeResponse(false, 302, {}, '', false)); + + const response = await fetch(URL_TO_USE, { method: 'GET', redirect: 'manual' }); + + expect(response.status).toBe(302); + }); + + // ...but a 3xx WITHOUT manual is still an error, so the change does not + // quietly swallow unexpected redirects for everyone else. + test('a 3xx without redirect: manual still throws', async () => { + const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; + mockFetch.mockResolvedValue(fakeResponse(false, 302, {}, '', false)); + + await expect(fetch(URL_TO_USE, { method: 'GET', options: { retry: { attempts: 1, initialIntervalMs: 1 } } })).rejects.toThrow(); + }); + test('Test fetch with referrer', async () => { const mockFetch = global.fetch as MockedFunction<(url: RequestInfo, init?: RequestInit) => Promise>; mockFetch.mockResolvedValue(fakeResponse(true, 200)); diff --git a/src/fetch.ts b/src/fetch.ts index 41a59d1..0db3005 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -615,7 +615,16 @@ async function doGlobalFetch( init?: RequestInit, options?: RequestOptions, ): Promise>> { - const useInit: RequestInit = merge({}, init, { redirect: 'follow' }); + // Defaults FIRST, caller's init LAST — `merge` lets later sources win, so + // the old order (`merge({}, init, { redirect: 'follow' })`) silently + // overwrote an explicit `redirect: 'manual' | 'error'` with 'follow'. + // + // Following redirects is the right default, but it must be a default and + // not a mandate: a caller that resolved a hostname and checked it against + // an SSRF allowlist has that guard defeated by a 302 to an internal + // address, and RFC 8461 forbids following them at all when fetching an + // MTA-STS policy. + const useInit: RequestInit = merge({}, { redirect: 'follow' } as RequestInit, init); // Stringify JSON body if needed if ((useInit?.headers as Record)?.['Content-Type'] === 'application/json' && typeof useInit.body === 'object') { @@ -672,7 +681,13 @@ async function doGlobalFetch( responseWithBody.dataString = dataString; responseWithBody.data = data; - if (responseClone.ok || responseClone.redirected) { + // A 3xx under `redirect: 'manual'` is the ANSWER, not a failure — the + // caller asked to see the redirect rather than follow it. It is neither + // `ok` nor `redirected` (nothing was followed), so without this it would + // throw and the option would be useless even once it is honoured. + const isManualRedirect = useInit.redirect === 'manual' && responseClone.status >= 300 && responseClone.status < 400; + + if (responseClone.ok || responseClone.redirected || isManualRedirect) { return responseWithBody; } else { if (!read) {