diff --git a/docs/api/Agent.md b/docs/api/Agent.md index 4892874d618..86ce0bb62ee 100644 --- a/docs/api/Agent.md +++ b/docs/api/Agent.md @@ -19,7 +19,6 @@ Returns: `Agent` Extends: [`PoolOptions`](Pool.md#parameter-pooloptions) * **factory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Pool(origin, opts)` -* **maxRedirections** `Integer` - Default: `0`. The number of HTTP redirection to follow unless otherwise specified in `DispatchOptions`. ## Instance Properties diff --git a/lib/agent.js b/lib/agent.js index c8fa0682543..bc9e3d80abf 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -3,7 +3,6 @@ const { InvalidArgumentError } = require('./core/errors') const { kClients, kRunning, kClose, kDestroy, kDispatch } = require('./core/symbols') const DispatcherBase = require('./dispatcher-base') -const RedirectHandler = require('./handler/RedirectHandler') const Pool = require('./pool') const Client = require('./client') const util = require('./core/util') @@ -11,7 +10,6 @@ const util = require('./core/util') const kOnConnect = Symbol('onConnect') const kOnDisconnect = Symbol('onDisconnect') const kOnConnectionError = Symbol('onConnectionError') -const kMaxRedirections = Symbol('maxRedirections') const kOnDrain = Symbol('onDrain') const kFactory = Symbol('factory') const kOptions = Symbol('options') @@ -23,7 +21,7 @@ function defaultFactory (origin, opts) { } class Agent extends DispatcherBase { - constructor ({ factory = defaultFactory, maxRedirections = 0, connect, ...options } = {}) { + constructor ({ factory = defaultFactory, connect, ...options } = {}) { super() if (typeof factory !== 'function') { @@ -34,16 +32,11 @@ class Agent extends DispatcherBase { throw new InvalidArgumentError('connect must be a function or an object') } - if (!Number.isInteger(maxRedirections) || maxRedirections < 0) { - throw new InvalidArgumentError('maxRedirections must be a positive number') - } - if (connect && typeof connect !== 'function') { connect = { ...connect } } this[kOptions] = { ...util.deepClone(options), connect } - this[kMaxRedirections] = maxRedirections this[kFactory] = factory this[kClients] = new Map() @@ -95,18 +88,6 @@ class Agent extends DispatcherBase { this[kClients].set(key, dispatcher) } - if (this[kMaxRedirections] > 0) { - return dispatcher.dispatch( - opts, - new RedirectHandler( - dispatcher.dispatch.bind(dispatcher), - this[kMaxRedirections], - opts, - handler - ) - ) - } - return dispatcher.dispatch(opts, handler) } diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index d19e67f69a2..905e0dff03a 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -3,6 +3,7 @@ const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') const { addSignal, removeSignal } = require('./abort-signal') class ConnectHandler extends AsyncResource { @@ -91,6 +92,16 @@ function connect (opts, callback) { try { const connectHandler = new ConnectHandler(opts, callback) + + if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + if (opts?.maxRedirections > 0) { + RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, connectHandler) + return + } + this.dispatch({ ...opts, method: 'CONNECT' }, connectHandler) } catch (err) { if (typeof callback !== 'function') { diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js index f5112415bf7..534d9a53b45 100644 --- a/lib/api/api-pipeline.js +++ b/lib/api/api-pipeline.js @@ -5,15 +5,16 @@ const { Duplex, PassThrough } = require('node:stream') +const assert = require('node:assert') +const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, InvalidReturnValueError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const { AsyncResource } = require('node:async_hooks') +const RedirectHandler = require('../handler/RedirectHandler') const { addSignal, removeSignal } = require('./abort-signal') -const assert = require('node:assert') const kResume = Symbol('resume') @@ -239,7 +240,17 @@ class PipelineHandler extends AsyncResource { function pipeline (opts, handler) { try { const pipelineHandler = new PipelineHandler(opts, handler) - this.dispatch({ ...opts, body: pipelineHandler.req }, pipelineHandler) + + if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + if (opts?.maxRedirections > 0) { + RedirectHandler.buildDispatch(this, opts.maxRedirections)({ ...opts, body: pipelineHandler.req }, pipelineHandler) + } else { + this.dispatch({ ...opts, body: pipelineHandler.req }, pipelineHandler) + } + return pipelineHandler.ret } catch (err) { return new PassThrough().destroy(err) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 285c76de241..8a3adc7bf95 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -1,13 +1,14 @@ 'use strict' +const { AsyncResource } = require('node:async_hooks') const Readable = require('./readable') const { InvalidArgumentError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') const { getResolveErrorBodyCallback } = require('./util') -const { AsyncResource } = require('node:async_hooks') const { addSignal, removeSignal } = require('./abort-signal') class RequestHandler extends AsyncResource { @@ -166,7 +167,18 @@ function request (opts, callback) { } try { - this.dispatch(opts, new RequestHandler(opts, callback)) + const handler = new RequestHandler(opts, callback) + + if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + if (opts?.maxRedirections > 0) { + RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, handler) + return + } + + this.dispatch(opts, handler) } catch (err) { if (typeof callback !== 'function') { throw err diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js index 0998c9f2da1..2256593f20b 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -7,6 +7,7 @@ const { RequestAbortedError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') const { getResolveErrorBodyCallback } = require('./util') const { AsyncResource } = require('node:async_hooks') const { addSignal, removeSignal } = require('./abort-signal') @@ -207,7 +208,18 @@ function stream (opts, factory, callback) { } try { - this.dispatch(opts, new StreamHandler(opts, factory, callback)) + const handler = new StreamHandler(opts, factory, callback) + + if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + if (opts?.maxRedirections > 0) { + RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, handler) + return + } + + this.dispatch(opts, handler) } catch (err) { if (typeof callback !== 'function') { throw err diff --git a/lib/api/api-upgrade.js b/lib/api/api-upgrade.js index bed946fdca9..00a2147de31 100644 --- a/lib/api/api-upgrade.js +++ b/lib/api/api-upgrade.js @@ -1,10 +1,11 @@ 'use strict' -const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors') +const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') +const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') const { addSignal, removeSignal } = require('./abort-signal') -const assert = require('node:assert') class UpgradeHandler extends AsyncResource { constructor (opts, callback) { @@ -88,11 +89,22 @@ function upgrade (opts, callback) { try { const upgradeHandler = new UpgradeHandler(opts, callback) - this.dispatch({ + const upgradeOpts = { ...opts, method: opts.method || 'GET', upgrade: opts.protocol || 'Websocket' - }, upgradeHandler) + } + + if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + if (opts?.maxRedirections > 0) { + RedirectHandler.buildDispatch(this, opts.maxRedirections)(upgradeOpts, upgradeHandler) + return + } + + this.dispatch(upgradeOpts, upgradeHandler) } catch (err) { if (typeof callback !== 'function') { throw err diff --git a/lib/client.js b/lib/client.js index d6487798491..18a4a382882 100644 --- a/lib/client.js +++ b/lib/client.js @@ -62,7 +62,6 @@ const { kBodyTimeout, kStrictContentLength, kConnector, - kMaxRedirections, kMaxRequests, kCounter, kClose, @@ -137,7 +136,6 @@ class Client extends DispatcherBase { tls, strictContentLength, maxCachedSessions, - maxRedirections, connect, maxRequestsPerClient, localAddress, @@ -206,10 +204,6 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('connect must be a function or an object') } - if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) { - throw new InvalidArgumentError('maxRedirections must be a positive number') - } - if (maxRequestsPerClient != null && (!Number.isInteger(maxRequestsPerClient) || maxRequestsPerClient < 0)) { throw new InvalidArgumentError('maxRequestsPerClient must be a positive number') } @@ -268,7 +262,6 @@ class Client extends DispatcherBase { this[kBodyTimeout] = bodyTimeout != null ? bodyTimeout : 300e3 this[kHeadersTimeout] = headersTimeout != null ? headersTimeout : 300e3 this[kStrictContentLength] = strictContentLength == null ? true : strictContentLength - this[kMaxRedirections] = maxRedirections this[kMaxRequests] = maxRequestsPerClient this[kClosedResolve] = null this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1 diff --git a/lib/handler/RedirectHandler.js b/lib/handler/RedirectHandler.js index 2afbf9427ff..aea13e99e29 100644 --- a/lib/handler/RedirectHandler.js +++ b/lib/handler/RedirectHandler.js @@ -24,6 +24,15 @@ class BodyAsyncIterable { } class RedirectHandler { + static buildDispatch (dispatcher, maxRedirections) { + if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) { + throw new InvalidArgumentError('maxRedirections must be a positive number') + } + + const dispatch = dispatcher.dispatch.bind(dispatcher) + return (opts, originalHandler) => dispatch(opts, new RedirectHandler(dispatch, maxRedirections, opts, originalHandler)) + } + constructor (dispatch, maxRedirections, opts, handler) { if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) { throw new InvalidArgumentError('maxRedirections must be a positive number') diff --git a/test/redirect-request.js b/test/redirect-request.js index e7554b929f6..fdcea5dd065 100644 --- a/test/redirect-request.js +++ b/test/redirect-request.js @@ -90,7 +90,7 @@ for (const factory of [ const server = await startRedirectingServer() - const { statusCode, headers, body: bodyStream, context: { history } } = await request(t, server, { maxRedirections: 10 }, `http://${server}/300?key=value`) + const { statusCode, headers, body: bodyStream, context: { history } } = await request(t, server, undefined, `http://${server}/300?key=value`, { maxRedirections: 10 }) const body = await bodyStream.text() t.strictEqual(statusCode, 200) @@ -380,10 +380,9 @@ for (const factory of [ t = tspl(t, { plan: 1 }) try { - await request(t, 'localhost', { + await request(t, 'localhost', undefined, 'http://localhost', { + method: 'GET', maxRedirections: 'INVALID' - }, 'http://localhost', { - method: 'GET' }) t.fail('Did not throw') diff --git a/test/redirect-stream.js b/test/redirect-stream.js index c50e5033135..3d03cc946d2 100644 --- a/test/redirect-stream.js +++ b/test/redirect-stream.js @@ -36,7 +36,7 @@ test('should always have a history with the final URL even if no redirections we t.strictEqual(body.join(''), `GET /5 key=value :: host@${server} connection@keep-alive`) }) -test('should not follow redirection by default if not using RedirectAgent', async t => { +test('should not follow redirection by default if max redirect = 0', async t => { t = tspl(t, { plan: 3 }) const body = []