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) {