feat: implement throwOnMaxRedirect option for RedirectHandler (#2563)

* feat: implement throwOnMaxRedirect option for RedirectHandler feat: add RedirectHandler invalid URL protocol test feat: test repair & added flag fix :lint feat: added doc for redirectHandler feat: added doc for redirectHandler feat: added type for maxRedirection option delete notes test: added for redirectionLimitReached * added sidebar for apidocs * added test for redirectionLimitReached type * feat: Add handling for throwOnMaxRedirect flag in RedirectHandler * Update RedirectHandler.js Co-authored-by: Carlos Fuentes <me@metcoder.dev> * fix: added error info & flow with return * fix: type test repair & lint --------- Co-authored-by: Mert Can Altin <mert.altin@trendyol.com> Co-authored-by: Carlos Fuentes <me@metcoder.dev>
nodejs · Jan 17, 2024 · a7551ce · a7551ce
1 parent f97b38e
commit a7551ce
Show file tree

Hide file tree

Showing 7 changed files with 151 additions and 7 deletions.
diff --git a/docs/api/RedirectHandler.md b/docs/api/RedirectHandler.md
@@ -0,0 +1,96 @@
+# Class: RedirectHandler
+
+A class that handles redirection logic for HTTP requests.
+
+## `new RedirectHandler(dispatch, maxRedirections, opts, handler, redirectionLimitReached)`
+
+Arguments:
+
+- **dispatch** `function` - The dispatch function to be called after every retry.
+- **maxRedirections** `number` - Maximum number of redirections allowed.
+- **opts** `object` - Options for handling redirection.
+- **handler** `object` - An object containing handlers for different stages of the request lifecycle.
+- **redirectionLimitReached** `boolean` (default: `false`) - A flag that the implementer can provide to enable or disable the feature. If set to `false`, it indicates that the caller doesn't want to use the feature and prefers the old behavior.
+
+Returns: `RedirectHandler`
+
+### Parameters
+
+- **dispatch** `(options: Dispatch.DispatchOptions, handlers: Dispatch.DispatchHandlers) => Promise<Dispatch.DispatchResponse>` (required) - Dispatch function to be called after every redirection.
+- **maxRedirections** `number` (required) - Maximum number of redirections allowed.
+- **opts** `object` (required) - Options for handling redirection.
+- **handler** `object` (required) - Handlers for different stages of the request lifecycle.
+- **redirectionLimitReached** `boolean` (default: `false`) - A flag that the implementer can provide to enable or disable the feature. If set to `false`, it indicates that the caller doesn't want to use the feature and prefers the old behavior.
+
+### Properties
+
+- **location** `string` - The current redirection location.
+- **abort** `function` - The abort function.
+- **opts** `object` - The options for handling redirection.
+- **maxRedirections** `number` - Maximum number of redirections allowed.
+- **handler** `object` - Handlers for different stages of the request lifecycle.
+- **history** `Array` - An array representing the history of URLs during redirection.
+- **redirectionLimitReached** `boolean` - Indicates whether the redirection limit has been reached.
+
+### Methods
+
+#### `onConnect(abort)`
+
+Called when the connection is established.
+
+Parameters:
+
+- **abort** `function` - The abort function.
+
+#### `onUpgrade(statusCode, headers, socket)`
+
+Called when an upgrade is requested.
+
+Parameters:
+
+- **statusCode** `number` - The HTTP status code.
+- **headers** `object` - The headers received in the response.
+- **socket** `object` - The socket object.
+
+#### `onError(error)`
+
+Called when an error occurs.
+
+Parameters:
+
+- **error** `Error` - The error that occurred.
+
+#### `onHeaders(statusCode, headers, resume, statusText)`
+
+Called when headers are received.
+
+Parameters:
+
+- **statusCode** `number` - The HTTP status code.
+- **headers** `object` - The headers received in the response.
+- **resume** `function` - The resume function.
+- **statusText** `string` - The status text.
+
+#### `onData(chunk)`
+
+Called when data is received.
+
+Parameters:
+
+- **chunk** `Buffer` - The data chunk received.
+
+#### `onComplete(trailers)`
+
+Called when the request is complete.
+
+Parameters:
+
+- **trailers** `object` - The trailers received.
+
+#### `onBodySent(chunk)`
+
+Called when the request body is sent.
+
+Parameters:
+
+- **chunk** `Buffer` - The chunk of the request body sent.
diff --git a/docsify/sidebar.md b/docsify/sidebar.md
@@ -23,6 +23,7 @@
   * [MIME Type Parsing](/docs/api/ContentType.md "Undici API - MIME Type Parsing")
   * [CacheStorage](/docs/api/CacheStorage.md "Undici API - CacheStorage")
   * [Util](/docs/api/Util.md "Undici API - Util")
+  * [RedirectHandler](/docs/api/RedirectHandler.md "Undici API - RedirectHandler")
 * Best Practices
   * [Proxy](/docs/best-practices/proxy.md "Connecting through a proxy")
   * [Client Certificate](/docs/best-practices/client-certificate.md "Connect using a client certificate")

diff --git a/lib/handler/RedirectHandler.js b/lib/handler/RedirectHandler.js
@@ -38,6 +38,7 @@ class RedirectHandler {
     this.maxRedirections = maxRedirections
     this.handler = handler
     this.history = []
+    this.redirectionLimitReached = false
 
     if (util.isStream(this.opts.body)) {
       // TODO (fix): Provide some way for the user to cache the file to e.g. /tmp
@@ -91,6 +92,16 @@ class RedirectHandler {
       ? null
       : parseLocation(statusCode, headers)
 
+    if (this.opts.throwOnMaxRedirect && this.history.length >= this.maxRedirections) {
+      if (this.request) {
+        this.request.abort(new Error('max redirects'))
+      }
+
+      this.redirectionLimitReached = true
+      this.abort(new Error('max redirects'))
+      return
+    }
+
     if (this.opts.origin) {
       this.history.push(new URL(this.opts.path, this.opts.origin))
     }

diff --git a/test/redirect-request.js b/test/redirect-request.js
@@ -267,6 +267,30 @@ for (const factory of [
     t.equal(body.length, 0)
   })
 
+  t.test('should follow a redirect chain up to the allowed number of times for redirectionLimitReached', async t => {
+    const server = await startRedirectingServer(t)
+
+    try {
+      const { statusCode, headers, body: bodyStream, context: { history } } = await request(t, server, undefined, `http://${server}/300`, {
+        maxRedirections: 2,
+        throwOnMaxRedirect: true
+      })
+
+      const body = await bodyStream.text()
+
+      t.equal(statusCode, 300)
+      t.equal(headers.location, `http://${server}/300/2`)
+      t.same(history.map(x => x.toString()), [`http://${server}/300`, `http://${server}/300/1`])
+      t.equal(body.length, 0)
+    } catch (error) {
+      if (error.message.startsWith('max redirects')) {
+        t.pass('Max redirects handled correctly')
+      } else {
+        t.fail(`Unexpected error: ${error.message}`)
+      }
+    }
+  })
+
   t.test('when a Location response header is NOT present', async t => {
     const redirectCodes = [300, 301, 302, 303, 307, 308]
     const server = await startRedirectingWithoutLocationServer(t)

diff --git a/test/types/index.test-d.ts b/test/types/index.test-d.ts
@@ -17,7 +17,7 @@ expectAssignable<typeof FileReader>(Undici.FileReader)
 const client = new Undici.Client('', {})
 const handler: Dispatcher.DispatchHandlers =  {}
 
-expectAssignable<RedirectHandler>(new Undici.RedirectHandler(client, 10, {
+const redirectHandler = new Undici.RedirectHandler(client, 10, {
   path: '/', method: 'GET'
-}, handler))
-expectAssignable<DecoratorHandler>(new Undici.DecoratorHandler(handler))
+}, handler, false) as RedirectHandler;
+expectAssignable<RedirectHandler>(redirectHandler);
diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts
@@ -131,6 +131,8 @@ declare namespace Dispatcher {
     opaque?: unknown;
     /** Default: 0 */
     maxRedirections?: number;
+    /** Default: false */
+    redirectionLimitReached?: boolean;
     /** Default: `null` */
     responseHeader?: 'raw' | null;
   }
@@ -141,6 +143,8 @@ declare namespace Dispatcher {
     signal?: AbortSignal | EventEmitter | null;
     /** Default: 0 */
     maxRedirections?: number;
+    /** Default: false */
+    redirectionLimitReached?: boolean;
     /** Default: `null` */
     onInfo?: (info: { statusCode: number, headers: Record<string, string | string[]> }) => void;
     /** Default: `null` */
@@ -164,6 +168,8 @@ declare namespace Dispatcher {
     signal?: AbortSignal | EventEmitter | null;
     /** Default: 0 */
     maxRedirections?: number;
+    /** Default: false */
+    redirectionLimitReached?: boolean;
     /** Default: `null` */
     responseHeader?: 'raw' | null;
   }

diff --git a/types/handlers.d.ts b/types/handlers.d.ts
@@ -1,9 +1,15 @@
 import Dispatcher from "./dispatcher";
 
-export declare class RedirectHandler implements Dispatcher.DispatchHandlers{
-  constructor (dispatch: Dispatcher, maxRedirections: number, opts: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandlers)
+export declare class RedirectHandler implements Dispatcher.DispatchHandlers {
+  constructor(
+    dispatch: Dispatcher,
+    maxRedirections: number,
+    opts: Dispatcher.DispatchOptions,
+    handler: Dispatcher.DispatchHandlers,
+    redirectionLimitReached: boolean
+  );
 }
 
-export declare class DecoratorHandler implements Dispatcher.DispatchHandlers{
-  constructor (handler: Dispatcher.DispatchHandlers)
+export declare class DecoratorHandler implements Dispatcher.DispatchHandlers {
+  constructor(handler: Dispatcher.DispatchHandlers);
 }