fix(redirect): fix throwOnMaxRedirect edge

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

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

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

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 left> 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 })