fix: we can redirect disturbed request body if it's not going to be used (#3873)

* fix: we can redirect disturbed request body if it's not going to be used

* fixup

* fixup

* fixup

* fixup

Author: ronag

lib/handler/redirect-handler.js

@@ -49,7 +49,6 @@ class RedirectHandler {
     this.maxRedirections = maxRedirections
     this.handler = handler
     this.history = []
-    this.redirectionLimitReached = false
 
     if (util.isStream(this.opts.body)) {
       // TODO (fix): Provide some way for the user to cache the file to e.g. /tmp
@@ -99,27 +98,42 @@ class RedirectHandler {
     this.handler.onError(error)
   }
 
-  onHeaders (statusCode, headers, resume, statusText) {
-    this.location = this.history.length >= this.maxRedirections || util.isDisturbed(this.opts.body)
-      ? null
-      : parseLocation(statusCode, headers)
-
+  onHeaders (statusCode, rawHeaders, resume, statusText) {
     if (this.opts.throwOnMaxRedirect && this.history.length >= this.maxRedirections) {
-      if (this.request) {
-        this.request.abort(new Error('max redirects'))
+      throw new Error('max redirects')
+    }
+
+    // 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 === 301 || statusCode === 302) && this.opts.method === 'POST') {
+      this.opts.method = 'GET'
+      if (util.isStream(this.opts.body)) {
+        util.destroy(this.opts.body.on('error', noop))
       }
+      this.opts.body = null
+    }
 
-      this.redirectionLimitReached = true
-      this.abort(new Error('max redirects'))
-      return
+    // 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') {
+      this.opts.method = 'GET'
+      if (util.isStream(this.opts.body)) {
+        util.destroy(this.opts.body.on('error', noop))
+      }
+      this.opts.body = null
     }
 
+    this.location = this.history.length >= this.maxRedirections || util.isDisturbed(this.opts.body)
+      ? null
+      : parseLocation(statusCode, rawHeaders)
+
     if (this.opts.origin) {
       this.history.push(new URL(this.opts.path, this.opts.origin))
     }
 
     if (!this.location) {
-      return this.handler.onHeaders(statusCode, headers, resume, statusText)
+      return this.handler.onHeaders(statusCode, rawHeaders, resume, statusText)
     }
 
     const { origin, pathname, search } = util.parseURL(new URL(this.location, this.opts.origin && new URL(this.opts.path, this.opts.origin)))
@@ -133,23 +147,6 @@ class RedirectHandler {
     this.opts.origin = origin
     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 === 301 || statusCode === 302) && this.opts.method === 'POST') {
-      this.opts.method = 'GET'
-      if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop))
-      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') {
-      this.opts.method = 'GET'
-      if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop))
-      this.opts.body = null
-    }
   }
 
   onData (chunk) {
@@ -203,14 +200,14 @@ class RedirectHandler {
   }
 }
 
-function parseLocation (statusCode, headers) {
+function parseLocation (statusCode, rawHeaders) {
   if (redirectableStatusCodes.indexOf(statusCode) === -1) {
     return null
   }
 
-  for (let i = 0; i < headers.length; i += 2) {
-    if (headers[i].length === 8 && util.headerNameToString(headers[i]) === 'location') {
-      return headers[i + 1]
+  for (let i = 0; i < rawHeaders.length; i += 2) {
+    if (rawHeaders[i].length === 8 && util.headerNameToString(rawHeaders[i]) === 'location') {
+      return rawHeaders[i + 1]
     }
   }
 }

test/interceptors/redirect.js

@@ -564,7 +564,7 @@ for (const factory of [
       headers,
       body: bodyStream
     } = await request(t, server, undefined, `http://${server}/301`, {
-      method: 'POST',
+      method: 'PUT',
       body: createReadable('REQUEST'),
       maxRedirections: 10
     })
@@ -576,6 +576,26 @@ for (const factory of [
     t.strictEqual(body.length, 0)
     await t.completed
   })
+
+  test('should follow redirects when using Readable request bodies w/ POST 101', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    const {
+      statusCode,
+      body: bodyStream
+    } = await request(t, server, undefined, `http://${server}/301`, {
+      method: 'POST',
+      body: createReadable('REQUEST'),
+      maxRedirections: 10
+    })
+
+    await bodyStream.text()
+
+    t.strictEqual(statusCode, 200)
+    await t.completed
+  })
 }
 
 test('should follow redirections when going cross origin', async t => {

test/redirect-request.js

@@ -505,7 +505,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: createReadable('REQUEST'),
       maxRedirections: 10
     })
@@ -517,6 +517,23 @@ for (const factory of [
     t.strictEqual(body.length, 0)
     await t.completed
   })
+
+  test('should follow redirects when using Readable request bodies for POST 301', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    const { statusCode, body: bodyStream } = await request(t, server, undefined, `http://${server}/301`, {
+      method: 'POST',
+      body: createReadable('REQUEST'),
+      maxRedirections: 10
+    })
+
+    await bodyStream.text()
+
+    t.strictEqual(statusCode, 200)
+    await t.completed
+  })
 }
 
 test('should follow redirections when going cross origin', async t => {