From c838237d5581f54c26d8ddf4055a92f074f4abba Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Sun, 18 Feb 2024 15:48:04 +0100 Subject: [PATCH 1/4] refactor(#2722)!: Drop Interceptors support (#2754) --- docs/docs/api/Agent.md | 2 - docs/docs/api/Client.md | 2 - docs/docs/api/DispatchInterceptor.md | 60 -------- docs/docs/api/Pool.md | 1 - lib/api/api-connect.js | 14 +- lib/api/api-pipeline.js | 17 ++- lib/api/api-request.js | 20 ++- lib/api/api-stream.js | 14 +- lib/api/api-upgrade.js | 20 ++- lib/core/symbols.js | 1 - lib/dispatcher/agent.js | 22 +-- lib/dispatcher/balanced-pool.js | 7 +- lib/dispatcher/dispatcher-base.js | 36 +---- lib/dispatcher/pool.js | 8 +- lib/dispatcher/proxy-agent.js | 5 +- lib/handler/redirect-handler.js | 9 ++ test/jest/interceptor.test.js | 197 --------------------------- test/node-test/agent.js | 5 +- test/redirect-request.js | 7 +- test/redirect-stream.js | 2 +- test/types/client.test-d.ts | 12 -- test/types/interceptor.test-d.ts | 5 - types/agent.d.ts | 2 - types/client.d.ts | 5 - types/index.d.ts | 1 - types/pool.d.ts | 2 - 26 files changed, 97 insertions(+), 379 deletions(-) delete mode 100644 docs/docs/api/DispatchInterceptor.md delete mode 100644 test/jest/interceptor.test.js delete mode 100644 test/types/interceptor.test-d.ts diff --git a/docs/docs/api/Agent.md b/docs/docs/api/Agent.md index dd5d99bc1e1..86ce0bb62ee 100644 --- a/docs/docs/api/Agent.md +++ b/docs/docs/api/Agent.md @@ -19,8 +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`. -* **interceptors** `{ Agent: DispatchInterceptor[] }` - Default: `[RedirectInterceptor]` - A list of interceptors that are applied to the dispatch method. Additional logic can be applied (such as, but not limited to: 302 status code handling, authentication, cookies, compression and caching). Note that the behavior of interceptors is Experimental and might change at any given time. ## Instance Properties diff --git a/docs/docs/api/Client.md b/docs/docs/api/Client.md index 03342f59959..4b193bda508 100644 --- a/docs/docs/api/Client.md +++ b/docs/docs/api/Client.md @@ -29,8 +29,6 @@ Returns: `Client` * **pipelining** `number | null` (optional) - Default: `1` - The amount of concurrent requests to be sent over the single TCP/TLS connection according to [RFC7230](https://tools.ietf.org/html/rfc7230#section-6.3.2). Carefully consider your workload and environment before enabling concurrent requests as pipelining may reduce performance if used incorrectly. Pipelining is sensitive to network stack settings as well as head of line blocking caused by e.g. long running requests. Set to `0` to disable keep-alive connections. * **connect** `ConnectOptions | Function | null` (optional) - Default: `null`. * **strictContentLength** `Boolean` (optional) - Default: `true` - Whether to treat request content length mismatches as errors. If true, an error is thrown when the request content-length header doesn't match the length of the request body. - -* **interceptors** `{ Client: DispatchInterceptor[] }` - Default: `[RedirectInterceptor]` - A list of interceptors that are applied to the dispatch method. Additional logic can be applied (such as, but not limited to: 302 status code handling, authentication, cookies, compression and caching). Note that the behavior of interceptors is Experimental and might change at any given time. **Note: this is deprecated in favor of [Dispatcher#compose](./Dispatcher.md#dispatcher). Support will be droped in next major.** * **autoSelectFamily**: `boolean` (optional) - Default: depends on local Node version, on Node 18.13.0 and above is `false`. Enables a family autodetection algorithm that loosely implements section 5 of [RFC 8305](https://tools.ietf.org/html/rfc8305#section-5). See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. This option is ignored if not supported by the current Node version. * **autoSelectFamilyAttemptTimeout**: `number` - Default: depends on local Node version, on Node 18.13.0 and above is `250`. The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. * **allowH2**: `boolean` - Default: `false`. Enables support for H2 if the server has assigned bigger priority to it through ALPN negotiation. diff --git a/docs/docs/api/DispatchInterceptor.md b/docs/docs/api/DispatchInterceptor.md deleted file mode 100644 index 7dfc260e32a..00000000000 --- a/docs/docs/api/DispatchInterceptor.md +++ /dev/null @@ -1,60 +0,0 @@ -# Interface: DispatchInterceptor - -Extends: `Function` - -A function that can be applied to the `Dispatcher.Dispatch` function before it is invoked with a dispatch request. - -This allows one to write logic to intercept both the outgoing request, and the incoming response. - -### Parameter: `Dispatcher.Dispatch` - -The base dispatch function you are decorating. - -### ReturnType: `Dispatcher.Dispatch` - -A dispatch function that has been altered to provide additional logic - -### Basic Example - -Here is an example of an interceptor being used to provide a JWT bearer token - -```js -'use strict' - -const insertHeaderInterceptor = dispatch => { - return function InterceptedDispatch(opts, handler){ - opts.headers.push('Authorization', 'Bearer [Some token]') - return dispatch(opts, handler) - } -} - -const client = new Client('https://localhost:3000', { - interceptors: { Client: [insertHeaderInterceptor] } -}) - -``` - -### Basic Example 2 - -Here is a contrived example of an interceptor stripping the headers from a response. - -```js -'use strict' - -const clearHeadersInterceptor = dispatch => { - const { DecoratorHandler } = require('undici') - class ResultInterceptor extends DecoratorHandler { - onHeaders (statusCode, headers, resume) { - return super.onHeaders(statusCode, [], resume) - } - } - return function InterceptedDispatch(opts, handler){ - return dispatch(opts, new ResultInterceptor(handler)) - } -} - -const client = new Client('https://localhost:3000', { - interceptors: { Client: [clearHeadersInterceptor] } -}) - -``` diff --git a/docs/docs/api/Pool.md b/docs/docs/api/Pool.md index 8fcabac3154..6b08294b61c 100644 --- a/docs/docs/api/Pool.md +++ b/docs/docs/api/Pool.md @@ -19,7 +19,6 @@ Extends: [`ClientOptions`](Client.md#parameter-clientoptions) * **factory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Client(origin, opts)` * **connections** `number | null` (optional) - Default: `null` - The number of `Client` instances to create. When set to `null`, the `Pool` instance will create an unlimited amount of `Client` instances. -* **interceptors** `{ Pool: DispatchInterceptor[] } }` - Default: `{ Pool: [] }` - A list of interceptors that are applied to the dispatch method. Additional logic can be applied (such as, but not limited to: 302 status code handling, authentication, cookies, compression and caching). ## Instance Properties diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index f9dbbf64fe8..1f1f5e6aaa4 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -4,6 +4,7 @@ const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, SocketError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') const { addSignal, removeSignal } = require('./abort-signal') class ConnectHandler extends AsyncResource { @@ -95,7 +96,18 @@ function connect (opts, callback) { try { const connectHandler = new ConnectHandler(opts, callback) - this.dispatch({ ...opts, method: 'CONNECT' }, connectHandler) + const connectOptions = { ...opts, method: 'CONNECT' } + + 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)(connectOptions, connectHandler) + return + } + + this.dispatch(connectOptions, connectHandler) } catch (err) { if (typeof callback !== 'function') { throw err diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js index 322334e7848..7ebda7e54fd 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') @@ -240,7 +241,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 31cc3e5cd00..937cc58e528 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -1,11 +1,14 @@ 'use strict' +const { AsyncResource } = require('node:async_hooks') const assert = require('node:assert') -const { Readable } = require('./readable') + const { InvalidArgumentError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') +const RedirectHandler = require('../handler/RedirectHandler') + +const { Readable } = require('./readable') const { getResolveErrorBodyCallback } = require('./util') -const { AsyncResource } = require('node:async_hooks') class RequestHandler extends AsyncResource { constructor (opts, callback) { @@ -189,7 +192,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 fba2266dd7d..cb1a8de6320 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -4,6 +4,7 @@ const assert = require('node:assert') const { finished, PassThrough } = require('node:stream') const { InvalidArgumentError, InvalidReturnValueError } = 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 019fe1efa2d..10d32b7dc83 100644 --- a/lib/api/api-upgrade.js +++ b/lib/api/api-upgrade.js @@ -1,10 +1,11 @@ 'use strict' -const { InvalidArgumentError, SocketError } = require('../core/errors') +const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') +const { InvalidArgumentError, 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) { @@ -91,11 +92,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/core/symbols.js b/lib/core/symbols.js index c8ba5dd8ec5..1c3732620b1 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -52,7 +52,6 @@ module.exports = { kMaxRequests: Symbol('maxRequestsPerClient'), kProxy: Symbol('proxy agent options'), kCounter: Symbol('socket request counter'), - kInterceptors: Symbol('dispatch interceptors'), kMaxResponseSize: Symbol('max response size'), kHTTP2Session: Symbol('http2Session'), kHTTP2SessionState: Symbol('http2Session state'), diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index 98f1486cac0..bc9e3d80abf 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -1,17 +1,15 @@ 'use strict' -const { InvalidArgumentError } = require('../core/errors') -const { kClients, kRunning, kClose, kDestroy, kDispatch, kInterceptors } = require('../core/symbols') +const { InvalidArgumentError } = require('./core/errors') +const { kClients, kRunning, kClose, kDestroy, kDispatch } = require('./core/symbols') const DispatcherBase = require('./dispatcher-base') const Pool = require('./pool') const Client = require('./client') -const util = require('../core/util') -const createRedirectInterceptor = require('../interceptor/redirect-interceptor') +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,23 +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[kInterceptors] = options.interceptors?.Agent && Array.isArray(options.interceptors.Agent) - ? options.interceptors.Agent - : [createRedirectInterceptor({ maxRedirections })] - this[kOptions] = { ...util.deepClone(options), connect } - this[kOptions].interceptors = options.interceptors - ? { ...options.interceptors } - : undefined - this[kMaxRedirections] = maxRedirections this[kFactory] = factory this[kClients] = new Map() diff --git a/lib/dispatcher/balanced-pool.js b/lib/dispatcher/balanced-pool.js index 15a7e7b5879..40bf75d1622 100644 --- a/lib/dispatcher/balanced-pool.js +++ b/lib/dispatcher/balanced-pool.js @@ -13,8 +13,8 @@ const { kGetDispatcher } = require('./pool-base') const Pool = require('./pool') -const { kUrl, kInterceptors } = require('../core/symbols') -const { parseOrigin } = require('../core/util') +const { kUrl } = require('./core/symbols') +const { parseOrigin } = require('./core/util') const kFactory = Symbol('factory') const kOptions = Symbol('options') @@ -53,9 +53,6 @@ class BalancedPool extends PoolBase { throw new InvalidArgumentError('factory must be a function.') } - this[kInterceptors] = opts.interceptors?.BalancedPool && Array.isArray(opts.interceptors.BalancedPool) - ? opts.interceptors.BalancedPool - : [] this[kFactory] = factory for (const upstream of upstreams) { diff --git a/lib/dispatcher/dispatcher-base.js b/lib/dispatcher/dispatcher-base.js index bd860acdcf4..afe4e9086db 100644 --- a/lib/dispatcher/dispatcher-base.js +++ b/lib/dispatcher/dispatcher-base.js @@ -6,11 +6,10 @@ const { ClientClosedError, InvalidArgumentError } = require('../core/errors') -const { kDestroy, kClose, kClosed, kDestroyed, kDispatch, kInterceptors } = require('../core/symbols') +const { kDestroy, kClose, kClosed, kDestroyed, kDispatch } = require('../core/symbols') const kOnDestroyed = Symbol('onDestroyed') const kOnClosed = Symbol('onClosed') -const kInterceptedDispatch = Symbol('Intercepted Dispatch') class DispatcherBase extends Dispatcher { constructor () { @@ -30,23 +29,6 @@ class DispatcherBase extends Dispatcher { return this[kClosed] } - get interceptors () { - return this[kInterceptors] - } - - set interceptors (newInterceptors) { - if (newInterceptors) { - for (let i = newInterceptors.length - 1; i >= 0; i--) { - const interceptor = this[kInterceptors][i] - if (typeof interceptor !== 'function') { - throw new InvalidArgumentError('interceptor must be an function') - } - } - } - - this[kInterceptors] = newInterceptors - } - close (callback) { if (callback === undefined) { return new Promise((resolve, reject) => { @@ -142,20 +124,6 @@ class DispatcherBase extends Dispatcher { }) } - [kInterceptedDispatch] (opts, handler) { - if (!this[kInterceptors] || this[kInterceptors].length === 0) { - this[kInterceptedDispatch] = this[kDispatch] - return this[kDispatch](opts, handler) - } - - let dispatch = this[kDispatch].bind(this) - for (let i = this[kInterceptors].length - 1; i >= 0; i--) { - dispatch = this[kInterceptors][i](dispatch) - } - this[kInterceptedDispatch] = dispatch - return dispatch(opts, handler) - } - dispatch (opts, handler) { if (!handler || typeof handler !== 'object') { throw new InvalidArgumentError('handler must be an object') @@ -174,7 +142,7 @@ class DispatcherBase extends Dispatcher { throw new ClientClosedError() } - return this[kInterceptedDispatch](opts, handler) + return this[kDispatch](opts, handler) } catch (err) { if (typeof handler.onError !== 'function') { throw new InvalidArgumentError('invalid onError method') diff --git a/lib/dispatcher/pool.js b/lib/dispatcher/pool.js index 2d84cd96488..87f14aef3c0 100644 --- a/lib/dispatcher/pool.js +++ b/lib/dispatcher/pool.js @@ -12,7 +12,7 @@ const { InvalidArgumentError } = require('../core/errors') const util = require('../core/util') -const { kUrl, kInterceptors } = require('../core/symbols') +const { kUrl } = require('../core/symbols') const buildConnector = require('../core/connect') const kOptions = Symbol('options') @@ -63,15 +63,9 @@ class Pool extends PoolBase { }) } - this[kInterceptors] = options.interceptors?.Pool && Array.isArray(options.interceptors.Pool) - ? options.interceptors.Pool - : [] this[kConnections] = connections || null this[kUrl] = util.parseOrigin(origin) this[kOptions] = { ...util.deepClone(options), connect, allowH2 } - this[kOptions].interceptors = options.interceptors - ? { ...options.interceptors } - : undefined this[kFactory] = factory } diff --git a/lib/dispatcher/proxy-agent.js b/lib/dispatcher/proxy-agent.js index 226b67846da..1924bcc896d 100644 --- a/lib/dispatcher/proxy-agent.js +++ b/lib/dispatcher/proxy-agent.js @@ -1,6 +1,6 @@ 'use strict' -const { kProxy, kClose, kDestroy, kInterceptors } = require('../core/symbols') +const { kProxy, kClose, kDestroy } = require('../core/symbols') const { URL } = require('node:url') const Agent = require('./agent') const Pool = require('./pool') @@ -40,9 +40,6 @@ class ProxyAgent extends DispatcherBase { const { href, origin, port, protocol, username, password, hostname: proxyHostname } = url this[kProxy] = { uri: href, protocol } - this[kInterceptors] = opts.interceptors?.ProxyAgent && Array.isArray(opts.interceptors.ProxyAgent) - ? opts.interceptors.ProxyAgent - : [] this[kRequestTls] = opts.requestTls this[kProxyTls] = opts.proxyTls this[kProxyHeaders] = opts.headers || {} diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index 16a7b2150a9..3db7a476d45 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.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/jest/interceptor.test.js b/test/jest/interceptor.test.js deleted file mode 100644 index 84e6210fb8d..00000000000 --- a/test/jest/interceptor.test.js +++ /dev/null @@ -1,197 +0,0 @@ -'use strict' - -const { createServer } = require('node:http') -const { Agent, request } = require('../../index') -const DecoratorHandler = require('../../lib/handler/decorator-handler') -/* global expect */ - -const defaultOpts = { keepAliveTimeout: 10, keepAliveMaxTimeout: 10 } - -describe('interceptors', () => { - let server - beforeEach(async () => { - server = createServer((req, res) => { - res.setHeader('Content-Type', 'text/plain') - res.end('hello') - }) - await new Promise((resolve) => { server.listen(0, resolve) }) - }) - afterEach(async () => { - await new Promise((resolve) => server.close(resolve)) - }) - - test('interceptors are applied on client from an agent', async () => { - const interceptors = [] - const buildInterceptor = dispatch => { - const interceptorContext = { requestCount: 0 } - interceptors.push(interceptorContext) - return (opts, handler) => { - interceptorContext.requestCount++ - return dispatch(opts, handler) - } - } - - const opts = { interceptors: { Client: [buildInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await Promise.all([ - request(origin, { dispatcher: agent }), - request(origin, { dispatcher: agent }) - ]) - - // Assert that the requests are run on different interceptors (different Clients) - const requestCounts = interceptors.map(x => x.requestCount) - expect(requestCounts).toEqual([1, 1]) - }) - - test('interceptors are applied in the correct order', async () => { - const setHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - opts.headers.push('foo', 'bar') - return dispatch(opts, handler) - } - } - - const assertHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - expect(opts.headers).toEqual(['foo', 'bar']) - return dispatch(opts, handler) - } - } - - const opts = { interceptors: { Pool: [setHeaderInterceptor, assertHeaderInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await request(origin, { dispatcher: agent, headers: [] }) - }) - - test('interceptors handlers are called in reverse order', async () => { - const clearResponseHeadersInterceptor = (dispatch) => { - return (opts, handler) => { - class ResultInterceptor extends DecoratorHandler { - onHeaders (statusCode, headers, resume) { - return super.onHeaders(statusCode, [], resume) - } - } - - return dispatch(opts, new ResultInterceptor(handler)) - } - } - - const assertHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - class ResultInterceptor extends DecoratorHandler { - onHeaders (statusCode, headers, resume) { - expect(headers).toEqual([]) - return super.onHeaders(statusCode, headers, resume) - } - } - - return dispatch(opts, new ResultInterceptor(handler)) - } - } - - const opts = { interceptors: { Agent: [assertHeaderInterceptor, clearResponseHeadersInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await request(origin, { dispatcher: agent, headers: [] }) - }) -}) - -describe('interceptors with NtlmRequestHandler', () => { - class FakeNtlmRequestHandler { - constructor (dispatch, opts, handler) { - this.dispatch = dispatch - this.opts = opts - this.handler = handler - this.requestCount = 0 - } - - onConnect (...args) { - return this.handler.onConnect(...args) - } - - onError (...args) { - return this.handler.onError(...args) - } - - onUpgrade (...args) { - return this.handler.onUpgrade(...args) - } - - onHeaders (statusCode, headers, resume, statusText) { - this.requestCount++ - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onHeaders(statusCode, headers, resume, statusText) - } - } - - onData (...args) { - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onData(...args) - } - } - - onComplete (...args) { - if (this.requestCount < 2) { - this.dispatch(this.opts, this) - } else { - return this.handler.onComplete(...args) - } - } - - onBodySent (...args) { - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onBodySent(...args) - } - } - } - let server - - beforeEach(async () => { - // This Test is important because NTLM and Negotiate require several - // http requests in sequence to run on the same keepAlive socket - - const socketRequestCountSymbol = Symbol('Socket Request Count') - server = createServer((req, res) => { - if (req.socket[socketRequestCountSymbol] === undefined) { - req.socket[socketRequestCountSymbol] = 0 - } - req.socket[socketRequestCountSymbol]++ - res.setHeader('Content-Type', 'text/plain') - - // Simulate NTLM/Negotiate logic, by returning 200 - // on the second request of each socket - if (req.socket[socketRequestCountSymbol] >= 2) { - res.statusCode = 200 - res.end() - } else { - res.statusCode = 401 - res.end() - } - }) - await new Promise((resolve) => { server.listen(0, resolve) }) - }) - afterEach(async () => { - await new Promise((resolve) => server.close(resolve)) - }) - - test('Retry interceptor on Client will use the same socket', async () => { - const interceptor = dispatch => { - return (opts, handler) => { - return dispatch(opts, new FakeNtlmRequestHandler(dispatch, opts, handler)) - } - } - const opts = { interceptors: { Client: [interceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - const { statusCode } = await request(origin, { dispatcher: agent, headers: [] }) - expect(statusCode).toEqual(200) - }) -}) diff --git a/test/node-test/agent.js b/test/node-test/agent.js index 74a3ae26e77..c5238d7b570 100644 --- a/test/node-test/agent.js +++ b/test/node-test/agent.js @@ -662,11 +662,8 @@ test('stream: fails with invalid onInfo', async (t) => { }) test('constructor validations', t => { - const p = tspl(t, { plan: 4 }) + const p = tspl(t, { plan: 1 }) p.throws(() => new Agent({ factory: 'ASD' }), errors.InvalidArgumentError, 'throws on invalid opts argument') - p.throws(() => new Agent({ maxRedirections: 'ASD' }), errors.InvalidArgumentError, 'throws on invalid opts argument') - p.throws(() => new Agent({ maxRedirections: -1 }), errors.InvalidArgumentError, 'throws on invalid opts argument') - p.throws(() => new Agent({ maxRedirections: null }), errors.InvalidArgumentError, 'throws on invalid opts argument') }) test('dispatch validations', async t => { 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 = [] diff --git a/test/types/client.test-d.ts b/test/types/client.test-d.ts index c416d779bec..d9c1e447813 100644 --- a/test/types/client.test-d.ts +++ b/test/types/client.test-d.ts @@ -68,18 +68,6 @@ expectAssignable(new Client(new URL('http://localhost'), {})) expectAssignable(new Client('', { autoSelectFamilyAttemptTimeout: 300e3 })) - expectAssignable(new Client('', { - interceptors: { - Client: [(dispatcher) => { - expectAssignable(dispatcher); - return (opts, handlers) => { - expectAssignable(opts); - expectAssignable(handlers); - return dispatcher(opts, handlers) - } - }] - } - })) } { diff --git a/test/types/interceptor.test-d.ts b/test/types/interceptor.test-d.ts deleted file mode 100644 index ba242bfcadb..00000000000 --- a/test/types/interceptor.test-d.ts +++ /dev/null @@ -1,5 +0,0 @@ -import {expectAssignable} from "tsd"; -import Undici from "../.."; -import Dispatcher from "../../types/dispatcher"; - -expectAssignable(Undici.createRedirectInterceptor({ maxRedirections: 3 })) diff --git a/types/agent.d.ts b/types/agent.d.ts index 58081ce9372..4ff6baccd86 100644 --- a/types/agent.d.ts +++ b/types/agent.d.ts @@ -20,8 +20,6 @@ declare namespace Agent { factory?(origin: string | URL, opts: Object): Dispatcher; /** Integer. Default: `0` */ maxRedirections?: number; - - interceptors?: { Agent?: readonly Dispatcher.DispatchInterceptor[] } & Pool.Options["interceptors"] } export interface DispatchOptions extends Dispatcher.DispatchOptions { diff --git a/types/client.d.ts b/types/client.d.ts index d0a5379f33c..be07a47d351 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -28,12 +28,7 @@ export class Client extends Dispatcher { } export declare namespace Client { - export interface OptionsInterceptors { - Client: readonly Dispatcher.DispatchInterceptor[]; - } export interface Options { - /** TODO */ - interceptors?: OptionsInterceptors; /** The maximum length of request headers in bytes. Default: Node.js' `--max-http-header-size` or `16384` (16KiB). */ maxHeaderSize?: number; /** The amount of time, in milliseconds, the parser will wait to receive the complete HTTP headers (Node 14 and above only). Default: `300e3` milliseconds (300s). */ diff --git a/types/index.d.ts b/types/index.d.ts index f6be35cd9bf..25d6526f633 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -42,7 +42,6 @@ declare namespace Undici { var RedirectHandler: typeof import ('./handlers').RedirectHandler var DecoratorHandler: typeof import ('./handlers').DecoratorHandler var RetryHandler: typeof import ('./retry-handler').default - var createRedirectInterceptor: typeof import ('./interceptors').default.createRedirectInterceptor var BalancedPool: typeof import('./balanced-pool').default; var Client: typeof import('./client').default; var buildConnector: typeof import('./connector').default; diff --git a/types/pool.d.ts b/types/pool.d.ts index bad5ba0308e..d24cfd06870 100644 --- a/types/pool.d.ts +++ b/types/pool.d.ts @@ -33,7 +33,5 @@ declare namespace Pool { factory?(origin: URL, opts: object): Dispatcher; /** The max number of clients to create. `null` if no limit. Default `null`. */ connections?: number | null; - - interceptors?: { Pool?: readonly Dispatcher.DispatchInterceptor[] } & Client.Options["interceptors"] } } From 2955897ffa8bf9ea53b130807f34342af848d489 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 3 Jul 2024 12:41:36 +0200 Subject: [PATCH 2/4] fix: tweaks --- lib/api/api-connect.js | 2 +- lib/api/api-pipeline.js | 2 +- lib/api/api-request.js | 2 +- lib/api/api-stream.js | 2 +- lib/api/api-upgrade.js | 2 +- lib/dispatcher/agent.js | 6 +- lib/dispatcher/balanced-pool.js | 4 +- test/interceptor.js | 198 -------------------------------- 8 files changed, 10 insertions(+), 208 deletions(-) delete mode 100644 test/interceptor.js diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index 1f1f5e6aaa4..ee9300cd6ae 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -4,7 +4,7 @@ const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, SocketError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/RedirectHandler') +const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') class ConnectHandler extends AsyncResource { diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js index 7ebda7e54fd..74fd63346cf 100644 --- a/lib/api/api-pipeline.js +++ b/lib/api/api-pipeline.js @@ -13,7 +13,7 @@ const { RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/RedirectHandler') +const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') const kResume = Symbol('resume') diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 937cc58e528..668f1a47d60 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -5,7 +5,7 @@ const assert = require('node:assert') const { InvalidArgumentError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/RedirectHandler') +const RedirectHandler = require('../handler/redirect-handler') const { Readable } = require('./readable') const { getResolveErrorBodyCallback } = require('./util') diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js index cb1a8de6320..25f8731bef7 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -4,7 +4,7 @@ const assert = require('node:assert') const { finished, PassThrough } = require('node:stream') const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/RedirectHandler') +const RedirectHandler = require('../handler/redirect-handler') const { getResolveErrorBodyCallback } = require('./util') const { AsyncResource } = require('node:async_hooks') const { addSignal, removeSignal } = require('./abort-signal') diff --git a/lib/api/api-upgrade.js b/lib/api/api-upgrade.js index 10d32b7dc83..b2292c861b6 100644 --- a/lib/api/api-upgrade.js +++ b/lib/api/api-upgrade.js @@ -4,7 +4,7 @@ const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, SocketError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/RedirectHandler') +const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') class UpgradeHandler extends AsyncResource { diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index bc9e3d80abf..9e362fa0682 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -1,11 +1,11 @@ 'use strict' -const { InvalidArgumentError } = require('./core/errors') -const { kClients, kRunning, kClose, kDestroy, kDispatch } = require('./core/symbols') +const { InvalidArgumentError } = require('../core/errors') +const { kClients, kRunning, kClose, kDestroy, kDispatch } = require('../core/symbols') const DispatcherBase = require('./dispatcher-base') const Pool = require('./pool') const Client = require('./client') -const util = require('./core/util') +const util = require('../core/util') const kOnConnect = Symbol('onConnect') const kOnDisconnect = Symbol('onDisconnect') diff --git a/lib/dispatcher/balanced-pool.js b/lib/dispatcher/balanced-pool.js index 40bf75d1622..0b1decf3014 100644 --- a/lib/dispatcher/balanced-pool.js +++ b/lib/dispatcher/balanced-pool.js @@ -13,8 +13,8 @@ const { kGetDispatcher } = require('./pool-base') const Pool = require('./pool') -const { kUrl } = require('./core/symbols') -const { parseOrigin } = require('./core/util') +const { kUrl } = require('../core/symbols') +const { parseOrigin } = require('../core/util') const kFactory = Symbol('factory') const kOptions = Symbol('options') diff --git a/test/interceptor.js b/test/interceptor.js deleted file mode 100644 index 55a657120ea..00000000000 --- a/test/interceptor.js +++ /dev/null @@ -1,198 +0,0 @@ -'use strict' - -const { strictEqual, deepStrictEqual } = require('node:assert') -const { describe, beforeEach, afterEach, test } = require('node:test') -const { createServer } = require('node:http') -const { Agent, request } = require('../index') -const DecoratorHandler = require('../lib/handler/decorator-handler') - -const defaultOpts = { keepAliveTimeout: 10, keepAliveMaxTimeout: 10 } - -describe('interceptors', () => { - let server - beforeEach(async () => { - server = createServer((req, res) => { - res.setHeader('Content-Type', 'text/plain') - res.end('hello') - }) - await new Promise((resolve) => { server.listen(0, resolve) }) - }) - afterEach(async () => { - await new Promise((resolve) => server.close(resolve)) - }) - - test('interceptors are applied on client from an agent', async () => { - const interceptors = [] - const buildInterceptor = dispatch => { - const interceptorContext = { requestCount: 0 } - interceptors.push(interceptorContext) - return (opts, handler) => { - interceptorContext.requestCount++ - return dispatch(opts, handler) - } - } - - const opts = { interceptors: { Client: [buildInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await Promise.all([ - request(origin, { dispatcher: agent }), - request(origin, { dispatcher: agent }) - ]) - - // Assert that the requests are run on different interceptors (different Clients) - const requestCounts = interceptors.map(x => x.requestCount) - deepStrictEqual(requestCounts, [1, 1]) - }) - - test('interceptors are applied in the correct order', async () => { - const setHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - opts.headers.push('foo', 'bar') - return dispatch(opts, handler) - } - } - - const assertHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - deepStrictEqual(opts.headers, ['foo', 'bar']) - return dispatch(opts, handler) - } - } - - const opts = { interceptors: { Pool: [setHeaderInterceptor, assertHeaderInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await request(origin, { dispatcher: agent, headers: [] }) - }) - - test('interceptors handlers are called in reverse order', async () => { - const clearResponseHeadersInterceptor = (dispatch) => { - return (opts, handler) => { - class ResultInterceptor extends DecoratorHandler { - onHeaders (statusCode, headers, resume) { - return super.onHeaders(statusCode, [], resume) - } - } - - return dispatch(opts, new ResultInterceptor(handler)) - } - } - - const assertHeaderInterceptor = (dispatch) => { - return (opts, handler) => { - class ResultInterceptor extends DecoratorHandler { - onHeaders (statusCode, headers, resume) { - deepStrictEqual(headers, []) - return super.onHeaders(statusCode, headers, resume) - } - } - - return dispatch(opts, new ResultInterceptor(handler)) - } - } - - const opts = { interceptors: { Agent: [assertHeaderInterceptor, clearResponseHeadersInterceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - await request(origin, { dispatcher: agent, headers: [] }) - }) -}) - -describe('interceptors with NtlmRequestHandler', () => { - class FakeNtlmRequestHandler { - constructor (dispatch, opts, handler) { - this.dispatch = dispatch - this.opts = opts - this.handler = handler - this.requestCount = 0 - } - - onConnect (...args) { - return this.handler.onConnect(...args) - } - - onError (...args) { - return this.handler.onError(...args) - } - - onUpgrade (...args) { - return this.handler.onUpgrade(...args) - } - - onHeaders (statusCode, headers, resume, statusText) { - this.requestCount++ - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onHeaders(statusCode, headers, resume, statusText) - } - } - - onData (...args) { - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onData(...args) - } - } - - onComplete (...args) { - if (this.requestCount < 2) { - this.dispatch(this.opts, this) - } else { - return this.handler.onComplete(...args) - } - } - - onBodySent (...args) { - if (this.requestCount < 2) { - // Do nothing - } else { - return this.handler.onBodySent(...args) - } - } - } - let server - - beforeEach(async () => { - // This Test is important because NTLM and Negotiate require several - // http requests in sequence to run on the same keepAlive socket - - const socketRequestCountSymbol = Symbol('Socket Request Count') - server = createServer((req, res) => { - if (req.socket[socketRequestCountSymbol] === undefined) { - req.socket[socketRequestCountSymbol] = 0 - } - req.socket[socketRequestCountSymbol]++ - res.setHeader('Content-Type', 'text/plain') - - // Simulate NTLM/Negotiate logic, by returning 200 - // on the second request of each socket - if (req.socket[socketRequestCountSymbol] >= 2) { - res.statusCode = 200 - res.end() - } else { - res.statusCode = 401 - res.end() - } - }) - await new Promise((resolve) => { server.listen(0, resolve) }) - }) - afterEach(async () => { - await new Promise((resolve) => server.close(resolve)) - }) - - test('Retry interceptor on Client will use the same socket', async () => { - const interceptor = dispatch => { - return (opts, handler) => { - return dispatch(opts, new FakeNtlmRequestHandler(dispatch, opts, handler)) - } - } - const opts = { interceptors: { Client: [interceptor] }, ...defaultOpts } - const agent = new Agent(opts) - const origin = new URL(`http://localhost:${server.address().port}`) - const { statusCode } = await request(origin, { dispatcher: agent, headers: [] }) - strictEqual(statusCode, 200) - }) -}) From 1f88d583a7c032ccdd50e8c70dace4457aac89e6 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 5 Jul 2024 10:34:06 +0200 Subject: [PATCH 3/4] refactor: cleanup, WIP --- lib/api/api-connect.js | 10 ------ lib/api/api-pipeline.js | 11 +----- lib/api/api-request.js | 10 ------ lib/api/api-stream.js | 10 ------ lib/api/api-upgrade.js | 10 ------ lib/dispatcher/client.js | 17 --------- test/interceptors/redirect.js | 52 ++++++++++++++++++++++++++++ test/redirect-cross-origin-header.js | 52 ---------------------------- 8 files changed, 53 insertions(+), 119 deletions(-) delete mode 100644 test/redirect-cross-origin-header.js diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index ee9300cd6ae..c8b86dd7d53 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -4,7 +4,6 @@ const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, SocketError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') class ConnectHandler extends AsyncResource { @@ -98,15 +97,6 @@ function connect (opts, callback) { const connectHandler = new ConnectHandler(opts, callback) const connectOptions = { ...opts, method: 'CONNECT' } - 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)(connectOptions, connectHandler) - return - } - this.dispatch(connectOptions, connectHandler) } catch (err) { if (typeof callback !== 'function') { diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js index 74fd63346cf..3adb6be15dd 100644 --- a/lib/api/api-pipeline.js +++ b/lib/api/api-pipeline.js @@ -13,7 +13,6 @@ const { RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') const kResume = Symbol('resume') @@ -242,15 +241,7 @@ function pipeline (opts, handler) { try { const pipelineHandler = new PipelineHandler(opts, handler) - 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) - } + this.dispatch({ ...opts, body: pipelineHandler.req }, pipelineHandler) return pipelineHandler.ret } catch (err) { diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 668f1a47d60..8cc63be2e97 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -5,7 +5,6 @@ const assert = require('node:assert') const { InvalidArgumentError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { Readable } = require('./readable') const { getResolveErrorBodyCallback } = require('./util') @@ -194,15 +193,6 @@ function request (opts, callback) { try { 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') { diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js index 25f8731bef7..c004ec79433 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -4,7 +4,6 @@ const assert = require('node:assert') const { finished, PassThrough } = require('node:stream') const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { getResolveErrorBodyCallback } = require('./util') const { AsyncResource } = require('node:async_hooks') const { addSignal, removeSignal } = require('./abort-signal') @@ -210,15 +209,6 @@ function stream (opts, factory, callback) { try { 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') { diff --git a/lib/api/api-upgrade.js b/lib/api/api-upgrade.js index b2292c861b6..1965ecd4430 100644 --- a/lib/api/api-upgrade.js +++ b/lib/api/api-upgrade.js @@ -4,7 +4,6 @@ const assert = require('node:assert') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, SocketError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { addSignal, removeSignal } = require('./abort-signal') class UpgradeHandler extends AsyncResource { @@ -98,15 +97,6 @@ function upgrade (opts, callback) { upgrade: opts.protocol || 'Websocket' } - 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') { diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index cb61206b1ed..d88227c4e33 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -49,7 +49,6 @@ const { kClose, kDestroy, kDispatch, - kInterceptors, kLocalAddress, kMaxResponseSize, kOnError, @@ -59,7 +58,6 @@ const { } = require('../core/symbols.js') const connectH1 = require('./client-h1.js') const connectH2 = require('./client-h2.js') -let deprecatedInterceptorWarned = false const kClosedResolve = Symbol('kClosedResolve') @@ -77,7 +75,6 @@ class Client extends DispatcherBase { * @param {import('../../types/client.js').Client.Options} options */ constructor (url, { - interceptors, maxHeaderSize, headersTimeout, socketTimeout, @@ -208,18 +205,6 @@ class Client extends DispatcherBase { }) } - if (interceptors?.Client && Array.isArray(interceptors.Client)) { - this[kInterceptors] = interceptors.Client - if (!deprecatedInterceptorWarned) { - deprecatedInterceptorWarned = true - process.emitWarning('Client.Options#interceptor is deprecated. Use Dispatcher#compose instead.', { - code: 'UNDICI-CLIENT-INTERCEPTOR-DEPRECATED' - }) - } - } else { - this[kInterceptors] = [createRedirectInterceptor({ maxRedirections })] - } - this[kUrl] = util.parseOrigin(url) this[kConnector] = connect this[kPipelining] = pipelining != null ? pipelining : 1 @@ -362,8 +347,6 @@ class Client extends DispatcherBase { } } -const createRedirectInterceptor = require('../interceptor/redirect-interceptor.js') - function onError (client, err) { if ( client[kRunning] === 0 && diff --git a/test/interceptors/redirect.js b/test/interceptors/redirect.js index 239c9597568..1685c765a19 100644 --- a/test/interceptors/redirect.js +++ b/test/interceptors/redirect.js @@ -1,6 +1,8 @@ 'use strict' const { tspl } = require('@matteo.collina/tspl') +const { createServer } = require('node:http') +const { once } = require('node:events') const { test, after } = require('node:test') const undici = require('../..') const { @@ -670,3 +672,53 @@ test('removes cookie header on third party origin', async t => { await t.completed }) + +test('Cross-origin redirects clear forbidden headers', { only: true }, async (t) => { + const { strictEqual } = tspl(t, { plan: 6 }) + + const server1 = createServer((req, res) => { + strictEqual(req.headers.cookie, undefined) + strictEqual(req.headers.authorization, undefined) + strictEqual(req.headers['proxy-authorization'], undefined) + res.end('redirected') + }).listen(0) + + const server2 = createServer((req, res) => { + strictEqual(req.headers.authorization, 'test') + strictEqual(req.headers.cookie, 'ddd=dddd') + + res.writeHead(302, { + ...req.headers, + location: `http://localhost:${server1.address().port}` + }) + res.end() + }).listen(0) + + t.after(() => { + server1.close() + server2.close() + }) + + await Promise.all([ + once(server1, 'listening'), + once(server2, 'listening') + ]) + + console.log(server1.address().port) + console.log(server2.address().port) + + const client = new undici.Client(`http://localhost:${server2.address().port}`).compose(redirect({ maxRedirections: 1 })) + const res = await client.request({ + maxRedirections: 1, + path: '/', + method: 'GET', + headers: { + Authorization: 'test', + Cookie: 'ddd=dddd', + 'Proxy-Authorization': 'test' + } + }) + + const text = await res.body.text() + strictEqual(text, 'redirected') +}) diff --git a/test/redirect-cross-origin-header.js b/test/redirect-cross-origin-header.js deleted file mode 100644 index 451563cadc5..00000000000 --- a/test/redirect-cross-origin-header.js +++ /dev/null @@ -1,52 +0,0 @@ -'use strict' - -const { test } = require('node:test') -const { tspl } = require('@matteo.collina/tspl') -const { createServer } = require('node:http') -const { once } = require('node:events') -const { request } = require('..') - -test('Cross-origin redirects clear forbidden headers', async (t) => { - const { strictEqual } = tspl(t, { plan: 6 }) - - const server1 = createServer((req, res) => { - strictEqual(req.headers.cookie, undefined) - strictEqual(req.headers.authorization, undefined) - strictEqual(req.headers['proxy-authorization'], undefined) - - res.end('redirected') - }).listen(0) - - const server2 = createServer((req, res) => { - strictEqual(req.headers.authorization, 'test') - strictEqual(req.headers.cookie, 'ddd=dddd') - - res.writeHead(302, { - ...req.headers, - Location: `http://localhost:${server1.address().port}` - }) - res.end() - }).listen(0) - - t.after(() => { - server1.close() - server2.close() - }) - - await Promise.all([ - once(server1, 'listening'), - once(server2, 'listening') - ]) - - const res = await request(`http://localhost:${server2.address().port}`, { - maxRedirections: 1, - headers: { - Authorization: 'test', - Cookie: 'ddd=dddd', - 'Proxy-Authorization': 'test' - } - }) - - const text = await res.body.text() - strictEqual(text, 'redirected') -}) From b9574f25dd1234fd1f23b9f0d6910310c034d772 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 5 Jul 2024 12:39:46 +0200 Subject: [PATCH 4/4] test: WIP --- lib/handler/redirect-handler.js | 9 --- test/interceptors/redirect.js | 110 +++++++++++++++++--------------- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index 3db7a476d45..16a7b2150a9 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -24,15 +24,6 @@ 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/interceptors/redirect.js b/test/interceptors/redirect.js index 1685c765a19..996313754dc 100644 --- a/test/interceptors/redirect.js +++ b/test/interceptors/redirect.js @@ -12,7 +12,8 @@ const { startRedirectingWithoutLocationServer, startRedirectingWithAuthorization, startRedirectingWithCookie, - startRedirectingWithQueryParams + startRedirectingWithQueryParams, + startRedirectingWithRelativePath } = require('../utils/redirecting-servers') const { createReadable, createReadableStream } = require('../utils/stream') @@ -572,6 +573,63 @@ for (const factory of [ t.strictEqual(body.length, 0) await t.completed }) + + test('should redirect to relative URL according to RFC 7231', async t => { + t = tspl(t, { plan: 2 }) + + const server = await startRedirectingWithRelativePath() + + const { statusCode, body } = await request( + t, + server, + undefined, + `http://${server}`, + { + maxRedirections: 3 + } + ) + + const finalPath = await body.text() + + t.strictEqual(statusCode, 200) + t.strictEqual(finalPath, '/absolute/b') + }) + + // test('should follow redirections when going cross origin', { only: true }, async t => { + // t = tspl(t, { plan: 4 }) + + // const [server1, server2, server3] = await startRedirectingChainServers() + + // const { + // statusCode, + // headers, + // body: bodyStream, + // context: { history } + // } = await request(t, server1, undefined, `http://${server1}`, { + // method: 'POST', + // maxRedirections: 10 + // }) + + // const body = await bodyStream.text() + + // // console.log('history') + // t.strictEqual(statusCode, 200) + // t.ok(!headers.location) + // t.deepStrictEqual( + // history.map(x => x.toString()), + // [ + // `http://${server1}/`, + // `http://${server2}/`, + // `http://${server3}/`, + // `http://${server2}/end`, + // `http://${server3}/end`, + // `http://${server1}/end` + // ] + // ) + // t.strictEqual(body, 'POST') + + // await t.completed + // }) } test('should follow redirections when going cross origin', async t => { @@ -672,53 +730,3 @@ test('removes cookie header on third party origin', async t => { await t.completed }) - -test('Cross-origin redirects clear forbidden headers', { only: true }, async (t) => { - const { strictEqual } = tspl(t, { plan: 6 }) - - const server1 = createServer((req, res) => { - strictEqual(req.headers.cookie, undefined) - strictEqual(req.headers.authorization, undefined) - strictEqual(req.headers['proxy-authorization'], undefined) - res.end('redirected') - }).listen(0) - - const server2 = createServer((req, res) => { - strictEqual(req.headers.authorization, 'test') - strictEqual(req.headers.cookie, 'ddd=dddd') - - res.writeHead(302, { - ...req.headers, - location: `http://localhost:${server1.address().port}` - }) - res.end() - }).listen(0) - - t.after(() => { - server1.close() - server2.close() - }) - - await Promise.all([ - once(server1, 'listening'), - once(server2, 'listening') - ]) - - console.log(server1.address().port) - console.log(server2.address().port) - - const client = new undici.Client(`http://localhost:${server2.address().port}`).compose(redirect({ maxRedirections: 1 })) - const res = await client.request({ - maxRedirections: 1, - path: '/', - method: 'GET', - headers: { - Authorization: 'test', - Cookie: 'ddd=dddd', - 'Proxy-Authorization': 'test' - } - }) - - const text = await res.body.text() - strictEqual(text, 'redirected') -})