From a7024ed3cad6bbd745366d7f35519b9ef45ed3e7 Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 17:14:19 +0100 Subject: [PATCH 1/8] fix: 301 and 302 change method to GET --- lib/handler/redirect-handler.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index 2f1995e6a92..a13b87f34eb 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -132,6 +132,14 @@ class RedirectHandler { this.opts.maxRedirections = 0 this.opts.query = null + // https://tools.ietf.org/html/rfc7231#section-6.4.2 + // https://fetch.spec.whatwg.org/#http-redirect-fetch + // In case of HTTP 301 or 302 with POST, change the method to GET + if ((statusCode === 302 || statusCode === 301) && this.opts.method === 'POST') { + this.opts.method = 'GET' + this.opts.body = null + } + // https://tools.ietf.org/html/rfc7231#section-6.4.4 // In case of HTTP 303, always replace method to be either HEAD or GET if (statusCode === 303 && this.opts.method !== 'HEAD') { From a070253cc036980773a700a8d312a5cf941a8053 Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 18:15:20 +0100 Subject: [PATCH 2/8] tests: update tests to reflect changes --- lib/handler/redirect-handler.js | 2 +- test/interceptors/redirect.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index a13b87f34eb..c00cfddd68c 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -135,7 +135,7 @@ class RedirectHandler { // https://tools.ietf.org/html/rfc7231#section-6.4.2 // https://fetch.spec.whatwg.org/#http-redirect-fetch // In case of HTTP 301 or 302 with POST, change the method to GET - if ((statusCode === 302 || statusCode === 301) && this.opts.method === 'POST') { + if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' this.opts.body = null } diff --git a/test/interceptors/redirect.js b/test/interceptors/redirect.js index 0d8c333cf7d..fe4cc86256c 100644 --- a/test/interceptors/redirect.js +++ b/test/interceptors/redirect.js @@ -167,7 +167,7 @@ for (const factory of [ await t.completed }) - test('should follow redirection after a HTTP 301', async t => { + test('should follow redirection after a HTTP 301 changing method to GET', async t => { t = tspl(t, { plan: 3 }) const server = await startRedirectingServer() @@ -188,7 +188,7 @@ for (const factory of [ t.ok(!headers.location) t.strictEqual( body, - `POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST` + `GET /5 :: host@${server} connection@keep-alive` ) }) @@ -540,7 +540,7 @@ for (const factory of [ headers, body: bodyStream } = await request(t, server, undefined, `http://${server}/301`, { - method: 'POST', + method: 'PUT', body: createReadableStream('REQUEST'), maxRedirections: 10 }) @@ -608,7 +608,7 @@ test('should follow redirections when going cross origin', async t => { `http://${server1}/end` ] ) - t.strictEqual(body, 'POST') + t.strictEqual(body, 'GET') await t.completed }) From bc4cbac60d7e86dd561b9a8c5adabc8784a1a90a Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 18:40:00 +0100 Subject: [PATCH 3/8] tests: update other tests too --- test/redirect-request.js | 6 +++--- test/redirect-stream.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/redirect-request.js b/test/redirect-request.js index 4b7d114319f..55583d7ac05 100644 --- a/test/redirect-request.js +++ b/test/redirect-request.js @@ -125,7 +125,7 @@ for (const factory of [ t.strictEqual(statusCode, 200) t.ok(!headers.location) - t.strictEqual(body, `POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST`) + t.strictEqual(body, `GET /5 :: host@${server} connection@keep-alive`) }) test('should follow redirection after a HTTP 302', async t => { @@ -485,7 +485,7 @@ for (const factory of [ const server = await startRedirectingServer() const { statusCode, headers, body: bodyStream } = await request(t, server, undefined, `http://${server}/301`, { - method: 'POST', + method: 'PUT', body: createReadableStream('REQUEST'), maxRedirections: 10 }) @@ -541,7 +541,7 @@ test('should follow redirections when going cross origin', async t => { `http://${server3}/end`, `http://${server1}/end` ]) - t.strictEqual(body, 'POST') + t.strictEqual(body, 'GET') await t.completed }) diff --git a/test/redirect-stream.js b/test/redirect-stream.js index f45456d331e..d6e15259556 100644 --- a/test/redirect-stream.js +++ b/test/redirect-stream.js @@ -80,7 +80,7 @@ test('should follow redirection after a HTTP 300', async t => { t.strictEqual(body.join(''), `GET /5 key=value :: host@${server} connection@keep-alive`) }) -test('should follow redirection after a HTTP 301', async t => { +test('should follow redirection after a HTTP 301 changing method to GET', async t => { t = tspl(t, { plan: 3 }) const body = [] @@ -97,7 +97,7 @@ test('should follow redirection after a HTTP 301', async t => { } ) - t.strictEqual(body.join(''), `POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST`) + t.strictEqual(body.join(''), `GET /5 :: host@${server} connection@keep-alive`) }) test('should follow redirection after a HTTP 302', async t => { @@ -316,7 +316,7 @@ test('should follow redirections when going cross origin', async t => { } ) - t.strictEqual(body.join(''), 'POST') + t.strictEqual(body.join(''), 'GET') }) describe('when a Location response header is NOT present', async () => { From 3772d501b723dd02938f1ae6eaa4bf299868cadc Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 19:32:30 +0100 Subject: [PATCH 4/8] fix: destroy body --- lib/handler/redirect-handler.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index c00cfddd68c..e92fa66bab8 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -137,6 +137,7 @@ class RedirectHandler { // In case of HTTP 301 or 302 with POST, change the method to GET if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' + util.destroy(this.opts.body) this.opts.body = null } @@ -144,6 +145,7 @@ class RedirectHandler { // In case of HTTP 303, always replace method to be either HEAD or GET if (statusCode === 303 && this.opts.method !== 'HEAD') { this.opts.method = 'GET' + util.destroy(this.opts.body) this.opts.body = null } } From 6a78c2ad69982e7676436a8b10c2432277549906 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 21 Nov 2024 19:34:54 +0100 Subject: [PATCH 5/8] Apply suggestions from code review --- lib/handler/redirect-handler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index e92fa66bab8..fefd4d979dc 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -137,7 +137,7 @@ class RedirectHandler { // In case of HTTP 301 or 302 with POST, change the method to GET if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' - util.destroy(this.opts.body) + util.destroy(this.opts.body.on?.('error', () => {}) this.opts.body = null } @@ -145,7 +145,7 @@ class RedirectHandler { // In case of HTTP 303, always replace method to be either HEAD or GET if (statusCode === 303 && this.opts.method !== 'HEAD') { this.opts.method = 'GET' - util.destroy(this.opts.body) + util.destroy(this.opts.body.on?.('error', () => {})) this.opts.body = null } } From e5b5894bfb6abdb6e6a7b7ce8ea6ef0cc5c5140c Mon Sep 17 00:00:00 2001 From: D Trombett Date: Thu, 21 Nov 2024 19:46:15 +0100 Subject: [PATCH 6/8] fix: missing ) --- lib/handler/redirect-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index fefd4d979dc..d30b9d5229d 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -137,7 +137,7 @@ class RedirectHandler { // In case of HTTP 301 or 302 with POST, change the method to GET if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' - util.destroy(this.opts.body.on?.('error', () => {}) + util.destroy(this.opts.body.on?.('error', () => {})) this.opts.body = null } From be844b670c8c5b82a00bd4dcc7ced4874f2e13a6 Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 19:56:43 +0100 Subject: [PATCH 7/8] fix: check if stream is valid --- lib/handler/redirect-handler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index d30b9d5229d..85572f4eea9 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -137,7 +137,7 @@ class RedirectHandler { // In case of HTTP 301 or 302 with POST, change the method to GET if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' - util.destroy(this.opts.body.on?.('error', () => {})) + if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', () => {})) this.opts.body = null } @@ -145,7 +145,7 @@ class RedirectHandler { // In case of HTTP 303, always replace method to be either HEAD or GET if (statusCode === 303 && this.opts.method !== 'HEAD') { this.opts.method = 'GET' - util.destroy(this.opts.body.on?.('error', () => {})) + if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', () => {})) this.opts.body = null } } From 8840c9ed822224a57d5db6517e4d0dca58840563 Mon Sep 17 00:00:00 2001 From: DTrombett Date: Thu, 21 Nov 2024 20:32:37 +0100 Subject: [PATCH 8/8] refactor: extract noop function --- lib/handler/redirect-handler.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/handler/redirect-handler.js b/lib/handler/redirect-handler.js index 85572f4eea9..40adeb52415 100644 --- a/lib/handler/redirect-handler.js +++ b/lib/handler/redirect-handler.js @@ -10,6 +10,8 @@ const redirectableStatusCodes = [300, 301, 302, 303, 307, 308] const kBody = Symbol('body') +const noop = () => {} + class BodyAsyncIterable { constructor (body) { this[kBody] = body @@ -137,7 +139,7 @@ class RedirectHandler { // In case of HTTP 301 or 302 with POST, change the method to GET if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') { this.opts.method = 'GET' - if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', () => {})) + if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop)) this.opts.body = null } @@ -145,7 +147,7 @@ class RedirectHandler { // In case of HTTP 303, always replace method to be either HEAD or GET if (statusCode === 303 && this.opts.method !== 'HEAD') { this.opts.method = 'GET' - if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', () => {})) + if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop)) this.opts.body = null } }