From 801d8b21ab5b4ecf2586878619d203378a12eed3 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 12 Jul 2024 11:23:37 +0200 Subject: [PATCH] refactor!: drop maxRedirect --- README.md | 5 - docs/docs/api/Agent.md | 2 - docs/docs/api/EnvHttpProxyAgent.md | 1 - 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/interceptor/redirect.js | 4 +- test/interceptor.js | 198 --------------------------- test/interceptors/redirect.js | 119 ++++++++++++++-- test/redirect-cross-origin-header.js | 52 ------- test/redirect-pipeline.js | 9 +- test/redirect-relative.js | 23 ---- test/redirect-request.js | 17 +-- test/redirect-stream.js | 28 ++-- test/redirect-upgrade.js | 37 ----- 17 files changed, 143 insertions(+), 403 deletions(-) delete mode 100644 test/interceptor.js delete mode 100644 test/redirect-cross-origin-header.js delete mode 100644 test/redirect-relative.js delete mode 100644 test/redirect-upgrade.js diff --git a/README.md b/README.md index 2ac58b6695e..fea71cb4be6 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,6 @@ Arguments: * **options** [`RequestOptions`](./docs/docs/api/Dispatcher.md#parameter-requestoptions) * **dispatcher** `Dispatcher` - Default: [getGlobalDispatcher](#undicigetglobaldispatcher) * **method** `String` - Default: `PUT` if `options.body`, otherwise `GET` - * **maxRedirections** `Integer` - Default: `0` Returns a promise with the result of the `Dispatcher.request` method. @@ -143,7 +142,6 @@ Arguments: * **options** [`StreamOptions`](./docs/docs/api/Dispatcher.md#parameter-streamoptions) * **dispatcher** `Dispatcher` - Default: [getGlobalDispatcher](#undicigetglobaldispatcher) * **method** `String` - Default: `PUT` if `options.body`, otherwise `GET` - * **maxRedirections** `Integer` - Default: `0` * **factory** `Dispatcher.stream.factory` Returns a promise with the result of the `Dispatcher.stream` method. @@ -160,7 +158,6 @@ Arguments: * **options** [`PipelineOptions`](./docs/docs/api/Dispatcher.md#parameter-pipelineoptions) * **dispatcher** `Dispatcher` - Default: [getGlobalDispatcher](#undicigetglobaldispatcher) * **method** `String` - Default: `PUT` if `options.body`, otherwise `GET` - * **maxRedirections** `Integer` - Default: `0` * **handler** `Dispatcher.pipeline.handler` Returns: `stream.Duplex` @@ -178,7 +175,6 @@ Arguments: * **url** `string | URL | UrlObject` * **options** [`ConnectOptions`](./docs/docs/api/Dispatcher.md#parameter-connectoptions) * **dispatcher** `Dispatcher` - Default: [getGlobalDispatcher](#undicigetglobaldispatcher) - * **maxRedirections** `Integer` - Default: `0` * **callback** `(err: Error | null, data: ConnectData | null) => void` (optional) Returns a promise with the result of the `Dispatcher.connect` method. @@ -338,7 +334,6 @@ Arguments: * **url** `string | URL | UrlObject` * **options** [`UpgradeOptions`](./docs/docs/api/Dispatcher.md#parameter-upgradeoptions) * **dispatcher** `Dispatcher` - Default: [getGlobalDispatcher](#undicigetglobaldispatcher) - * **maxRedirections** `Integer` - Default: `0` * **callback** `(error: Error | null, data: UpgradeData) => void` (optional) Returns a promise with the result of the `Dispatcher.upgrade` method. diff --git a/docs/docs/api/Agent.md b/docs/docs/api/Agent.md index 4892874d618..e3e3e26a93b 100644 --- a/docs/docs/api/Agent.md +++ b/docs/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 @@ -50,7 +49,6 @@ Implements [`Dispatcher.dispatch(options, handler)`](Dispatcher.md#dispatcherdis Extends: [`DispatchOptions`](Dispatcher.md#parameter-dispatchoptions) * **origin** `string | URL` -* **maxRedirections** `Integer`. Implements [`Dispatcher.destroy([error, callback])`](Dispatcher.md#dispatcherdestroyerror-callback-promise). diff --git a/docs/docs/api/EnvHttpProxyAgent.md b/docs/docs/api/EnvHttpProxyAgent.md index a4932de8be7..7d431030f67 100644 --- a/docs/docs/api/EnvHttpProxyAgent.md +++ b/docs/docs/api/EnvHttpProxyAgent.md @@ -133,7 +133,6 @@ Implements [`Dispatcher.dispatch(options, handler)`](Dispatcher.md#dispatcherdis Extends: [`DispatchOptions`](Dispatcher.md#parameter-dispatchoptions) * **origin** `string | URL` -* **maxRedirections** `Integer`. Implements [`Dispatcher.destroy([error, callback])`](Dispatcher.md#dispatcherdestroyerror-callback-promise). 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 f6228764eef..1516e5dbd51 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') @@ -241,15 +240,7 @@ class PipelineHandler extends AsyncResource { 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) { return new PassThrough().destroy(err) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 89948275590..b988c2e7496 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -5,7 +5,6 @@ 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/redirect-handler') const { getResolveErrorBodyCallback } = require('./util') class RequestHandler extends AsyncResource { @@ -192,15 +191,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 c1efd77e33c..50f61632365 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -5,7 +5,6 @@ const { finished, PassThrough } = require('node:stream') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors') const util = require('../core/util') -const RedirectHandler = require('../handler/redirect-handler') const { getResolveErrorBodyCallback } = require('./util') 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 49ab3c083d0..6c2076d5ac1 100644 --- a/lib/api/api-upgrade.js +++ b/lib/api/api-upgrade.js @@ -4,7 +4,6 @@ const { InvalidArgumentError, SocketError } = require('../core/errors') const { AsyncResource } = require('node:async_hooks') const assert = require('node:assert') 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/interceptor/redirect.js b/lib/interceptor/redirect.js index 987ab63ab39..55bad59c6ee 100644 --- a/lib/interceptor/redirect.js +++ b/lib/interceptor/redirect.js @@ -7,12 +7,12 @@ function createRedirectInterceptor ({ maxRedirections: defaultMaxRedirections } return function Intercept (opts, handler) { const { maxRedirections = defaultMaxRedirections, ...rest } = opts - if (maxRedirections == null) { + if (maxRedirections == null || maxRedirections === 0) { return dispatch(opts, handler) } - const redirectHandler = new RedirectHandler(dispatch, maxRedirections, opts, handler) const dispatchOpts = { ...rest, maxRedirections: 0 } // Stop sub dispatcher from also redirecting. + const redirectHandler = new RedirectHandler(dispatch, maxRedirections, dispatchOpts, handler) return dispatch(dispatchOpts, redirectHandler) } } 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) - }) -}) diff --git a/test/interceptors/redirect.js b/test/interceptors/redirect.js index efbd01acad7..0d8c333cf7d 100644 --- a/test/interceptors/redirect.js +++ b/test/interceptors/redirect.js @@ -1,7 +1,9 @@ 'use strict' -const { tspl } = require('@matteo.collina/tspl') const { test, after } = require('node:test') +const { createServer } = require('node:http') +const { once } = require('node:events') +const { tspl } = require('@matteo.collina/tspl') const undici = require('../..') const { startRedirectingServer, @@ -10,7 +12,9 @@ const { startRedirectingWithoutLocationServer, startRedirectingWithAuthorization, startRedirectingWithCookie, - startRedirectingWithQueryParams + startRedirectingWithQueryParams, + startServer, + startRedirectingWithRelativePath } = require('../utils/redirecting-servers') const { createReadable, createReadableStream } = require('../utils/stream') @@ -20,7 +24,9 @@ const { for (const factory of [ (server, opts) => - new undici.Agent(opts).compose(redirect({ maxRedirections: opts?.maxRedirections })), + new undici.Agent(opts).compose( + redirect({ maxRedirections: opts?.maxRedirections }) + ), (server, opts) => new undici.Pool(`http://${server}`, opts).compose( redirect({ maxRedirections: opts?.maxRedirections }) @@ -584,7 +590,7 @@ test('should follow redirections when going cross origin', async t => { context: { history } } = await undici.request(`http://${server1}`, { method: 'POST', - maxRedirections: 10 + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })) }) const body = await bodyStream.text() @@ -613,7 +619,9 @@ test('should handle errors (callback)', async t => { undici.request( 'http://localhost:0', { - maxRedirections: 10 + dispatcher: new undici.Agent({}).compose( + redirect({ maxRedirections: 10 }) + ) }, error => { t.match(error.code, /EADDRNOTAVAIL|ECONNREFUSED/) @@ -627,7 +635,11 @@ test('should handle errors (promise)', async t => { t = tspl(t, { plan: 1 }) try { - await undici.request('http://localhost:0', { maxRedirections: 10 }) + await undici.request('http://localhost:0', { + dispatcher: new undici.Agent({}).compose( + redirect({ maxRedirections: 10 }) + ) + }) t.fail('Did not throw') } catch (error) { t.match(error.code, /EADDRNOTAVAIL|ECONNREFUSED/) @@ -641,7 +653,7 @@ test('removes authorization header on third party origin', async t => { const [server1] = await startRedirectingWithAuthorization('secret') const { body: bodyStream } = await undici.request(`http://${server1}`, { - maxRedirections: 10, + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })), headers: { authorization: 'secret' } @@ -658,7 +670,7 @@ test('removes cookie header on third party origin', async t => { t = tspl(t, { plan: 1 }) const [server1] = await startRedirectingWithCookie('a=b') const { body: bodyStream } = await undici.request(`http://${server1}`, { - maxRedirections: 10, + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })), headers: { cookie: 'a=b' } @@ -670,3 +682,94 @@ test('removes cookie header on third party origin', async t => { await t.completed }) + +test('should upgrade the connection when no redirects are present', async t => { + t = tspl(t, { plan: 2 }) + + const server = await startServer((req, res) => { + if (req.url === '/') { + res.statusCode = 301 + res.setHeader('Location', `http://${server}/end`) + res.end('REDIRECT') + return + } + + res.statusCode = 101 + res.setHeader('Connection', 'upgrade') + res.setHeader('Upgrade', req.headers.upgrade) + res.end('') + }) + + const { headers, socket } = await undici.upgrade(`http://${server}/`, { + method: 'GET', + protocol: 'foo/1', + dispatcher: new undici.Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) + }) + + socket.end() + + t.strictEqual(headers.connection, 'upgrade') + t.strictEqual(headers.upgrade, 'foo/1') + + 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 undici.request(`http://${server}`, { + dispatcher: new undici.Client(`http://${server}/`).compose(redirect({ maxRedirections: 3 })) + }) + + const finalPath = await body.text() + + t.strictEqual(statusCode, 200) + t.strictEqual(finalPath, '/absolute/b') +}) + +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 undici.request(`http://localhost:${server2.address().port}`, { + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 1 })), + 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') -}) diff --git a/test/redirect-pipeline.js b/test/redirect-pipeline.js index 98896fae19c..52e4545e0d6 100644 --- a/test/redirect-pipeline.js +++ b/test/redirect-pipeline.js @@ -2,13 +2,14 @@ const { tspl } = require('@matteo.collina/tspl') const { test } = require('node:test') -const { pipeline: undiciPipeline } = require('..') +const { pipeline: undiciPipeline, Client, interceptors } = require('..') const { pipeline: streamPipelineCb } = require('node:stream') const { promisify } = require('node:util') const { createReadable, createWritable } = require('./utils/stream') const { startRedirectingServer } = require('./utils/redirecting-servers') const streamPipeline = promisify(streamPipelineCb) +const redirect = interceptors.redirect test('should not follow redirection by default if not using RedirectAgent', async t => { t = tspl(t, { plan: 3 }) @@ -18,7 +19,9 @@ test('should not follow redirection by default if not using RedirectAgent', asyn await streamPipeline( createReadable('REQUEST'), - undiciPipeline(`http://${serverRoot}/`, {}, ({ statusCode, headers, body }) => { + undiciPipeline(`http://${serverRoot}/`, { + dispatcher: new Client(`http://${serverRoot}/`).compose(redirect({ maxRedirections: null })) + }, ({ statusCode, headers, body }) => { t.strictEqual(statusCode, 302) t.strictEqual(headers.location, `http://${serverRoot}/302/1`) @@ -38,7 +41,7 @@ test('should not follow redirects when using RedirectAgent within pipeline', asy await streamPipeline( createReadable('REQUEST'), - undiciPipeline(`http://${serverRoot}/`, { maxRedirections: 1 }, ({ statusCode, headers, body }) => { + undiciPipeline(`http://${serverRoot}/`, { dispatcher: new Client(`http://${serverRoot}/`).compose(redirect({ maxRedirections: 1 })) }, ({ statusCode, headers, body }) => { t.strictEqual(statusCode, 302) t.strictEqual(headers.location, `http://${serverRoot}/302/1`) diff --git a/test/redirect-relative.js b/test/redirect-relative.js deleted file mode 100644 index e4d45406e75..00000000000 --- a/test/redirect-relative.js +++ /dev/null @@ -1,23 +0,0 @@ -'use strict' - -const { tspl } = require('@matteo.collina/tspl') -const { test } = require('node:test') -const { request } = require('..') -const { - startRedirectingWithRelativePath -} = require('./utils/redirecting-servers') - -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(`http://${server}`, { - maxRedirections: 3 - }) - - const finalPath = await body.text() - - t.strictEqual(statusCode, 200) - t.strictEqual(finalPath, '/absolute/b') -}) diff --git a/test/redirect-request.js b/test/redirect-request.js index fdcea5dd065..c7264bc5faf 100644 --- a/test/redirect-request.js +++ b/test/redirect-request.js @@ -13,11 +13,12 @@ const { startRedirectingWithQueryParams } = require('./utils/redirecting-servers') const { createReadable, createReadableStream } = require('./utils/stream') +const redirect = undici.interceptors.redirect for (const factory of [ - (server, opts) => new undici.Agent(opts), - (server, opts) => new undici.Pool(`http://${server}`, opts), - (server, opts) => new undici.Client(`http://${server}`, opts) + (server, opts) => new undici.Agent(opts).compose(redirect({ maxRedirections: opts?.maxRedirections })), + (server, opts) => new undici.Pool(`http://${server}`, opts).compose(redirect({ maxRedirections: opts?.maxRedirections })), + (server, opts) => new undici.Client(`http://${server}`, opts).compose(redirect({ maxRedirections: opts?.maxRedirections })) ]) { const request = (t, server, opts, ...args) => { const dispatcher = factory(server, opts) @@ -440,7 +441,7 @@ test('should follow redirections when going cross origin', async t => { const { statusCode, headers, body: bodyStream, context: { history } } = await undici.request(`http://${server1}`, { method: 'POST', - maxRedirections: 10 + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })) }) const body = await bodyStream.text() @@ -466,7 +467,7 @@ test('should handle errors (callback)', async t => { undici.request( 'http://localhost:0', { - maxRedirections: 10 + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })) }, error => { t.match(error.code, /EADDRNOTAVAIL|ECONNREFUSED/) @@ -480,7 +481,7 @@ test('should handle errors (promise)', async t => { t = tspl(t, { plan: 1 }) try { - await undici.request('http://localhost:0', { maxRedirections: 10 }) + await undici.request('http://localhost:0', { dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })) }) t.fail('Did not throw') } catch (error) { t.match(error.code, /EADDRNOTAVAIL|ECONNREFUSED/) @@ -494,7 +495,7 @@ test('removes authorization header on third party origin', async t => { const [server1] = await startRedirectingWithAuthorization('secret') const { body: bodyStream } = await undici.request(`http://${server1}`, { - maxRedirections: 10, + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })), headers: { authorization: 'secret' } @@ -511,7 +512,7 @@ test('removes cookie header on third party origin', async t => { t = tspl(t, { plan: 1 }) const [server1] = await startRedirectingWithCookie('a=b') const { body: bodyStream } = await undici.request(`http://${server1}`, { - maxRedirections: 10, + dispatcher: new undici.Agent({}).compose(redirect({ maxRedirections: 10 })), headers: { cookie: 'a=b' } diff --git a/test/redirect-stream.js b/test/redirect-stream.js index 3d03cc946d2..f45456d331e 100644 --- a/test/redirect-stream.js +++ b/test/redirect-stream.js @@ -2,7 +2,7 @@ const { tspl } = require('@matteo.collina/tspl') const { test, describe } = require('node:test') -const { stream } = require('..') +const { stream, Agent, Client, interceptors: { redirect } } = require('..') const { startRedirectingServer, startRedirectingWithBodyServer, @@ -21,7 +21,7 @@ test('should always have a history with the final URL even if no redirections we await stream( `http://${server}/200?key=value`, - { opaque: body, maxRedirections: 10 }, + { opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque, context: { history } }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -42,7 +42,7 @@ test('should not follow redirection by default if max redirect = 0', async t => const body = [] const server = await startRedirectingServer() - await stream(`http://${server}`, { opaque: body }, ({ statusCode, headers, opaque }) => { + await stream(`http://${server}`, { opaque: body, dispatcher: new Agent({}).compose(redirect({ maxRedirections: 0 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 302) t.strictEqual(headers.location, `http://${server}/302/1`) @@ -60,7 +60,7 @@ test('should follow redirection after a HTTP 300', async t => { await stream( `http://${server}/300?key=value`, - { opaque: body, maxRedirections: 10 }, + { opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque, context: { history } }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -88,7 +88,7 @@ test('should follow redirection after a HTTP 301', async t => { await stream( `http://${server}/301`, - { method: 'POST', body: 'REQUEST', opaque: body, maxRedirections: 10 }, + { method: 'POST', body: 'REQUEST', opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -108,7 +108,7 @@ test('should follow redirection after a HTTP 302', async t => { await stream( `http://${server}/302`, - { method: 'PUT', body: Buffer.from('REQUEST'), opaque: body, maxRedirections: 10 }, + { method: 'PUT', body: Buffer.from('REQUEST'), opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -126,7 +126,7 @@ test('should follow redirection after a HTTP 303 changing method to GET', async const body = [] const server = await startRedirectingServer() - await stream(`http://${server}/303`, { opaque: body, maxRedirections: 10 }, ({ statusCode, headers, opaque }) => { + await stream(`http://${server}/303`, { opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -163,7 +163,7 @@ test('should remove Host and request body related headers when following HTTP 30 '4' ], opaque: body, - maxRedirections: 10 + dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) @@ -196,7 +196,7 @@ test('should remove Host and request body related headers when following HTTP 30 'X-Bar': '4' }, opaque: body, - maxRedirections: 10 + dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) @@ -217,7 +217,7 @@ test('should follow redirection after a HTTP 307', async t => { await stream( `http://${server}/307`, - { method: 'DELETE', opaque: body, maxRedirections: 10 }, + { method: 'DELETE', opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -237,7 +237,7 @@ test('should follow redirection after a HTTP 308', async t => { await stream( `http://${server}/308`, - { method: 'OPTIONS', opaque: body, maxRedirections: 10 }, + { method: 'OPTIONS', opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -257,7 +257,7 @@ test('should ignore HTTP 3xx response bodies', async t => { await stream( `http://${server}/`, - { opaque: body, maxRedirections: 10 }, + { opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque, context: { history } }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) @@ -278,7 +278,7 @@ test('should follow a redirect chain up to the allowed number of times', async t await stream( `http://${server}/300`, - { opaque: body, maxRedirections: 2 }, + { opaque: body, dispatcher: new Client(`http://${server}/`).compose(redirect({ maxRedirections: 2 })) }, ({ statusCode, headers, opaque, context: { history } }) => { t.strictEqual(statusCode, 300) t.strictEqual(headers.location, `http://${server}/300/3`) @@ -299,7 +299,7 @@ test('should follow redirections when going cross origin', async t => { await stream( `http://${server1}`, - { method: 'POST', opaque: body, maxRedirections: 10 }, + { method: 'POST', opaque: body, dispatcher: new Agent({}).compose(redirect({ maxRedirections: 10 })) }, ({ statusCode, headers, opaque, context: { history } }) => { t.strictEqual(statusCode, 200) t.strictEqual(headers.location, undefined) diff --git a/test/redirect-upgrade.js b/test/redirect-upgrade.js deleted file mode 100644 index 5e331797379..00000000000 --- a/test/redirect-upgrade.js +++ /dev/null @@ -1,37 +0,0 @@ -'use strict' - -const { tspl } = require('@matteo.collina/tspl') -const { test } = require('node:test') -const { upgrade } = require('..') -const { startServer } = require('./utils/redirecting-servers') - -test('should upgrade the connection when no redirects are present', async t => { - t = tspl(t, { plan: 2 }) - - const server = await startServer((req, res) => { - if (req.url === '/') { - res.statusCode = 301 - res.setHeader('Location', `http://${server}/end`) - res.end('REDIRECT') - return - } - - res.statusCode = 101 - res.setHeader('Connection', 'upgrade') - res.setHeader('Upgrade', req.headers.upgrade) - res.end('') - }) - - const { headers, socket } = await upgrade(`http://${server}/`, { - method: 'GET', - protocol: 'foo/1', - maxRedirections: 10 - }) - - socket.end() - - t.strictEqual(headers.connection, 'upgrade') - t.strictEqual(headers.upgrade, 'foo/1') - - await t.completed -})