feat(fetch): allow manual redirect handling (nodejs#1210)

It doesn't make sense to return opaqueredirect on server side because it is not manually possible to follow an HTTP redirect. Hence, this aligns fetch with the other implementations in Deno and Cloudflare Workers. Fixes: nodejs#1193 Signed-off-by: Darshan Sen <[email protected]>
metcoder95 · Dec 26, 2022 · 872fb86 · 872fb86
1 parent 2168162
commit 872fb86
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 6 deletions.
diff --git a/README.md b/README.md
@@ -332,6 +332,15 @@ aborted.
 * Refs: https://tools.ietf.org/html/rfc2616#section-8.1.2.2
 * Refs: https://tools.ietf.org/html/rfc7230#section-6.3.2
 
+### Manual Redirect
+
+Since it is not possible to manually follow an HTTP redirect on server-side,
+Undici returns the actual response instead of an `opaqueredirect` filtered one
+when invoked with a `manual` redirect. This aligns `fetch()` with the other
+implementations in Deno and Cloudflare Workers.
+
+Refs: https://fetch.spec.whatwg.org/#atomic-http-redirect-handling
+
 ## Collaborators
 
 * [__Daniele Belardi__](https://github.com/dnlup), <https://www.npmjs.com/~dnlup>

diff --git a/lib/fetch/index.js b/lib/fetch/index.js
@@ -1044,7 +1044,10 @@ async function httpFetch (fetchParams) {
     } else if (request.redirect === 'manual') {
       // Set response to an opaque-redirect filtered response whose internal
       // response is actualResponse.
-      response = filterResponse(actualResponse, 'opaqueredirect')
+      // NOTE(spec): On the web this would return an `opaqueredirect` response,
+      // but that doesn't make sense server side.
+      // See https://github.com/nodejs/undici/issues/1193.
+      response = actualResponse
     } else if (request.redirect === 'follow') {
       // Set response to the result of running HTTP-redirect fetch given
       // fetchParams and response.

diff --git a/test/node-fetch/main.js b/test/node-fetch/main.js
@@ -416,7 +416,7 @@ describe('node-fetch', () => {
     }
     return fetch(url, options).then(res => {
       expect(res.url).to.equal(url)
-      expect(res.status).to.equal(0)
+      expect(res.status).to.equal(301)
       expect(res.headers.get('location')).to.be.null
     })
   })
@@ -1475,12 +1475,11 @@ describe('node-fetch', () => {
     ).not.to.timeout
   })
 
-  it('should allow get all responses of a header', () => {
+  it('should not allow getting any cookies from the response header', () => {
     const url = `${base}cookie`
     return fetch(url).then(res => {
-      const expected = 'a=1, b=1'
-      expect(res.headers.get('set-cookie')).to.equal(expected)
-      expect(res.headers.get('Set-Cookie')).to.equal(expected)
+      expect(res.headers.get('set-cookie')).to.equal(null)
+      expect(res.headers.get('Set-Cookie')).to.equal(null)
     })
   })
 
@@ -1627,4 +1626,19 @@ describe('node-fetch', () => {
     const res = await fetch(url)
     expect(res.url).to.equal(`${base}m%C3%B6bius`)
   })
+
+  it('should allow manual redirect handling', function () {
+    this.timeout(5000)
+    const url = 'https://httpbin.org/status/302'
+    const options = {
+      redirect: 'manual'
+    }
+    return fetch(url, options).then(res => {
+      expect(res.status).to.equal(302)
+      expect(res.url).to.equal(url)
+      expect(res.type).to.equal('basic')
+      expect(res.headers.get('Location')).to.equal('/redirect/1')
+      expect(res.ok).to.be.false
+    })
+  })
 })