fix(redirect): handle redirects explicitly (#27)

* fix(redirect): manual retry

* feat(redirect): add rules from node-fetch-npm

* wip(redirect): add tests

* fix(redirect): add test and adjust logic

* fix(redirect): fix test

* fix(redirect): adjust error codes

index.js

@@ -1,6 +1,7 @@
 'use strict'
 
 let Cache
+const url = require('url')
 const CachePolicy = require('http-cache-semantics')
 const fetch = require('node-fetch-npm')
 const pkg = require('./package.json')
@@ -10,6 +11,7 @@ const Stream = require('stream')
 const getAgent = require('./agent')
 const setWarning = require('./warning')
 
+const isURL = /^https?:/
 const USER_AGENT = `${pkg.name}/${pkg.version} (+https://npm.im/${pkg.name})`
 
 const RETRY_ERRORS = [
@@ -303,8 +305,9 @@ function remoteFetch (uri, opts) {
     follow: opts.follow,
     headers: new fetch.Headers(headers),
     method: opts.method,
-    redirect: opts.redirect,
+    redirect: 'manual',
     size: opts.size,
+    counter: opts.counter,
     timeout: opts.timeout
   }
 
@@ -357,7 +360,59 @@ function remoteFetch (uri, opts) {
             return retryHandler(res)
           }
 
-          return res
+          if (!fetch.isRedirect(res.status) || opts.redirect === 'manual') {
+            return res
+          }
+
+          // handle redirects - matches behavior of npm-fetch: https://github.com/bitinn/node-fetch
+          if (opts.redirect === 'error') {
+            const err = new Error(`redirect mode is set to error: ${uri}`)
+            err.code = 'ENOREDIRECT'
+            throw err
+          }
+
+          if (!res.headers.get('location')) {
+            const err = new Error(`redirect location header missing at: ${uri}`)
+            err.code = 'EINVALIDREDIRECT'
+            throw err
+          }
+
+          if (req.counter >= req.follow) {
+            const err = new Error(`maximum redirect reached at: ${uri}`)
+            err.code = 'EMAXREDIRECT'
+            throw err
+          }
+
+          const resolvedUrl = url.resolve(req.url, res.headers.get('location'))
+          let redirectURL = url.parse(resolvedUrl)
+
+          if (isURL.test(res.headers.get('location'))) {
+            redirectURL = url.parse(res.headers.get('location'))
+          }
+
+          // Remove authorization if changing hostnames (but not if just
+          // changing ports or protocols).  This matches the behavior of request:
+          // https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
+          if (url.parse(req.url).hostname !== redirectURL.hostname) {
+            req.headers.delete('authorization')
+          }
+
+          // for POST request with 301/302 response, or any request with 303 response,
+          // use GET when following redirect
+          if (res.status === 303 ||
+            ((res.status === 301 || res.status === 302) && req.method === 'POST')) {
+            opts.method = 'GET'
+            opts.body = null
+            req.headers.delete('content-length')
+          }
+
+          opts.headers = {}
+          req.headers.forEach((value, name) => {
+            opts.headers[name] = value
+          })
+
+          opts.counter = ++req.counter
+          return cachingFetch(resolvedUrl, opts)
         })
         .catch(err => {
           const code = err.code === 'EPROMISERETRY' ? err.retried.code : err.code

test/fetch.js

@@ -7,9 +7,11 @@ const finished = BB.promisify(require('mississippi').finished)
 const test = require('tap').test
 const through = require('mississippi').through
 const tnock = require('./util/tnock')
+const nock = require('nock')
 
 const CONTENT = Buffer.from('hello, world!', 'utf8')
 const HOST = 'https://make-fetch-happen.npm'
+const HTTPHOST = 'http://registry.npm.test.org'
 
 const fetch = require('..')
 
@@ -92,6 +94,135 @@ test('supports following redirects', t => {
   }).then(res => t.equal(res.length, 0, 'empty body'))
 })
 
+test('supports protocol switching on redirect', t => {
+  const httpSrv = tnock(t, HTTPHOST)
+  const srv = tnock(t, HOST)
+
+  httpSrv.get('/redirect').twice().reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+  srv.get('/test').reply(200, CONTENT)
+  return fetch(`${HTTPHOST}/redirect`).then(res => {
+    t.equal(res.status, 200, 'got the final status')
+    return res.buffer()
+  }).then(buf => {
+    t.deepEqual(buf, CONTENT, 'final req gave right body')
+    return fetch(`${HTTPHOST}/redirect`, {
+      redirect: 'manual'
+    })
+  }).then(res => {
+    t.equal(res.status, 301, 'did not follow redirect with manual mode')
+    return res.buffer()
+  }).then(res => t.equal(res.length, 0, 'empty body'))
+})
+
+test('supports manual redirect flag', t => {
+  const srv = tnock(t, HOST)
+
+  srv.get('/redirect').reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+
+  return fetch(`${HOST}/redirect`, {redirect: 'manual'}).then(res => {
+    t.equal(res.status, 301, 'got the final status')
+  })
+})
+
+test('supports error redirect flag', t => {
+  const srv = tnock(t, HOST)
+
+  srv.get('/redirect').reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+
+  return fetch(`${HOST}/redirect`, {redirect: 'error'}).catch(error => {
+    t.equal(error instanceof Error, true)
+    t.equal(error.message, 'redirect mode is set to error: https://make-fetch-happen.npm/redirect')
+  })
+})
+
+test('throws error when redirect location is missing', t => {
+  const srv = tnock(t, HOST)
+
+  srv.get('/redirect').reply(301)
+
+  return fetch(`${HOST}/redirect`).catch(error => {
+    t.equal(error instanceof Error, true)
+    t.equal(error.message, 'redirect location header missing at: https://make-fetch-happen.npm/redirect')
+  })
+})
+
+test('removes authorization header if changing hostnames', t => {
+  const httpSrv = tnock(t, HTTPHOST)
+  const srv = tnock(t, HOST)
+
+  httpSrv.matchHeader('authorization', 'test')
+    .get('/redirect').reply(301, '', {
+      'Location': `${HOST}/test`
+    })
+
+  srv.matchHeader('authorization', 'test')
+    .get('/test').reply(200, CONTENT)
+
+  return fetch(`${HTTPHOST}/redirect`, {
+    headers: { 'authorization': 'test' }
+  })
+  .catch(error => {
+    t.equal(error instanceof Error, true)
+    nock.cleanAll()
+  })
+})
+
+test('supports passthrough of options on redirect', t => {
+  const httpSrv = tnock(t, HTTPHOST)
+  const srv = tnock(t, HOST)
+
+  httpSrv.get('/redirect').reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+  srv.get('/test').matchHeader('x-test', 'test').reply(200, CONTENT)
+
+  return fetch(`${HTTPHOST}/redirect`, {
+    headers: { 'x-test': 'test' }
+  })
+  .then(buf => buf.buffer())
+  .then(buf => {
+    t.deepEqual(buf, CONTENT, 'request succeeded')
+  })
+})
+
+test('supports redirects from POST requests', t => {
+  const srv = tnock(t, HOST)
+
+  srv.get('/test').reply(200, CONTENT)
+  srv.post('/redirect').reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+
+  return fetch(`${HOST}/redirect`, {
+    method: 'POST',
+    body: 'test'
+  }).then(res => {
+    t.equal(res.status, 200)
+    return res.buffer()
+  }).then(buf => {
+    t.deepEqual(buf, CONTENT, 'request succeeded')
+  })
+})
+
+test('throws error if follow is less than request count', t => {
+  const srv = tnock(t, HOST)
+
+  srv.get('/redirect').reply(301, '', {
+    'Location': `${HOST}/test`
+  })
+
+  return fetch(`${HOST}/redirect`, {follow: 0}).catch(error => {
+    t.equal(error instanceof Error, true)
+    t.equal(error.message, 'maximum redirect reached at: https://make-fetch-happen.npm/redirect')
+  })
+})
+
 test('supports streaming content', t => {
   const srv = tnock(t, HOST)
   srv.get('/test').reply(200, CONTENT)