From ec00b18377462df305604406aa04268114296692 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 23 Sep 2026 20:14:06 +0000 Subject: [PATCH] fix(redirect): fix throwOnMaxRedirect edge When throwOnMaxRedirect is true, and exactly the limit of redirects was hit, the client would throw even if the next response was not a redirect. This fixes it. Signed-off-by: James M Snell --- docs/docs/api/Interceptors.md | 7 +++--- docs/docs/api/RedirectHandler.md | 23 ++++++++++-------- lib/handler/redirect-handler.js | 18 +++++++++----- test/interceptors/redirect.js | 40 ++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/docs/docs/api/Interceptors.md b/docs/docs/api/Interceptors.md index 4eefc17aec6..06da8ee66b8 100644 --- a/docs/docs/api/Interceptors.md +++ b/docs/docs/api/Interceptors.md @@ -163,9 +163,10 @@ Follows HTTP redirects (3xx responses) automatically. * `maxRedirections` {number} Maximum number of redirects to follow. Passing `0` disables redirect following entirely. **Default:** `undefined` (inherits from the per-request `maxRedirections` option). - * `throwOnMaxRedirect` {boolean} When `true`, throws an error once the - redirect limit is reached instead of returning the final redirect response. - **Default:** `false`. + * `throwOnMaxRedirect` {boolean} When `true`, throws an error when a + redirect would be followed but the redirect limit has been reached, instead + of returning that redirect response. A non-redirect response after exactly + `maxRedirections` redirects is returned normally. **Default:** `false`. * `stripHeadersOnRedirect` {string[]} List of header names to remove from the request when following any redirect. **Default:** `[]`. * `stripHeadersOnCrossOriginRedirect` {string[]} List of header names to diff --git a/docs/docs/api/RedirectHandler.md b/docs/docs/api/RedirectHandler.md index cedb0291d8a..c90cb37b62f 100644 --- a/docs/docs/api/RedirectHandler.md +++ b/docs/docs/api/RedirectHandler.md @@ -57,9 +57,10 @@ added: v4.0.0 * `opts` {DispatchOptions} The dispatch options for the request. In addition to the standard dispatch options, the following redirect-specific fields are recognized: - * `throwOnMaxRedirect` {boolean} When `true`, an error is thrown once - `maxRedirections` is reached instead of returning the last redirect response. - **Default:** `false`. + * `throwOnMaxRedirect` {boolean} When `true`, an error is thrown when a + redirect would be followed but `maxRedirections` has been reached, instead of + returning that redirect response. A non-redirect response after exactly + `maxRedirections` redirects is returned normally. **Default:** `false`. * `stripHeadersOnRedirect` {string[]} Header names to remove from the request on every redirect hop. **Default:** `null`. * `stripHeadersOnCrossOriginRedirect` {string[]} Additional header names to @@ -165,13 +166,15 @@ added: v7.0.0 * `statusMessage` {string} The HTTP status message. Inspects the response headers to decide whether to follow a redirect. When the -status code is redirectable, the `maxRedirections` limit has not been reached, and -the request body has not been consumed, the `location` property is set, the method -and headers are adjusted for the next hop, and the response is not forwarded. -Otherwise the event is forwarded to the wrapped `handler`. - -Throws when `throwOnMaxRedirect` is `true` and the number of recorded redirects -has reached `maxRedirections`, and throws an [`InvalidArgumentError`][] if a +status code is redirectable, the response has a `Location` header, the +`maxRedirections` limit has not been reached, and the request body has not been +consumed, the `location` property is set, the method and headers are adjusted +for the next hop, and the response is not forwarded. Otherwise the event is +forwarded to the wrapped `handler`. + +Throws when `throwOnMaxRedirect` is `true` and a redirect that would otherwise be +followed arrives after `maxRedirections` redirects have already been followed, +and throws an [`InvalidArgumentError`][] if a redirect loop is detected (for example when a [`Client`][] or [`Pool`][] is used for a cross-origin redirect; use an [`Agent`][] instead). diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index 7b0a0cf927d..1daf6030b8e 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -62,10 +62,6 @@ class RedirectHandler { return } - if (this.opts.throwOnMaxRedirect && this.history.length >= this.maxRedirections) { - throw new Error('max redirects') - } - let removeContentHeaders = statusCode === 303 // https://tools.ietf.org/html/rfc7231#section-6.4.2 @@ -91,9 +87,19 @@ class RedirectHandler { this.opts.body = null } - this.location = this.history.length >= this.maxRedirections || util.isDisturbed(this.opts.body) || redirectableStatusCodes.indexOf(statusCode) === -1 + this.location = util.isDisturbed(this.opts.body) || redirectableStatusCodes.indexOf(statusCode) === -1 ? null - : headers.location + : headers.location || null + + // Only a redirect that would otherwise be followed counts against the + // limit. The final response after exactly maxRedirections redirects, or a + // 3xx without a Location header, is delivered as usual. + if (this.location && this.history.length >= this.maxRedirections) { + if (this.opts.throwOnMaxRedirect) { + throw new Error('max redirects') + } + this.location = null + } const requestOrigin = this.opts[kRequestOrigin] === undefined ? this.opts.origin diff --git a/test/interceptors/redirect.js b/test/interceptors/redirect.js index 69c2c5c2087..b7b64ac3731 100644 --- a/test/interceptors/redirect.js +++ b/test/interceptors/redirect.js @@ -486,6 +486,46 @@ for (const factory of [ await t.completed }) + for (const [finalStatus, finalBody] of [[200, 'final'], [404, 'missing'], [304, ''], [300, 'choices']]) { + test(`should not throw on a final ${finalStatus} after exactly maxRedirections redirects with throwOnMaxRedirect`, async t => { + // / redirects until 0, then answers with finalStatus + // (without a Location header). + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + const left = Number(req.url.slice(1)) + if (left > 0) { + res.writeHead(302, { location: `/${left - 1}` }) + res.end() + return + } + res.writeHead(finalStatus) + res.end(finalBody) + }).listen(0) + after(() => server.close()) + await once(server, 'listening') + + const origin = `http://localhost:${server.address().port}` + const dispatcher = new undici.Agent().compose( + redirect({ maxRedirections: 2, throwOnMaxRedirect: true }) + ) + after(() => dispatcher.close()) + + const response = await undici.request(`${origin}/2`, { dispatcher }) + + t.assert.strictEqual(response.statusCode, finalStatus) + t.assert.strictEqual(await response.body.text(), finalBody) + t.assert.deepStrictEqual(response.context.history, [ + new URL(`${origin}/2`), + new URL(`${origin}/1`), + new URL(`${origin}/0`) + ]) + + await t.assert.rejects( + undici.request(`${origin}/3`, { dispatcher }), + { message: 'max redirects' } + ) + }) + } + test('should not allow invalid throwOnMaxRedirect arguments', async t => { t = tspl(t, { plan: 1 })