fix: correctly allow redirects to external URLs (#5005)

* fix: fix redirects to external URLs * chore: fix nit from review Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
netlify · Sep 9, 2022 · d62118e · d62118e
1 parent 36170ef
commit d62118e
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 390 deletions.
diff --git a/src/utils/proxy.js b/src/utils/proxy.js
@@ -72,7 +72,7 @@ const proxyToExternalUrl = function ({ dest, destURL, req, res }) {
     pathRewrite: () => destURL,
     ...(Buffer.isBuffer(req.originalBody) && { buffer: toReadableStream(req.originalBody) }),
   })
-  return handler(req, res, {})
+  return handler(req, res, () => {})
 }
 
 const handleAddonUrl = function ({ addonUrl, req, res }) {
@@ -187,7 +187,7 @@ const serveRedirect = async function ({ match, options, proxy, req, res }) {
 
   const staticFile = await getStatic(decodeURIComponent(reqUrl.pathname), options.publicFolder)
   if (staticFile) {
-    req.url = encodeURIComponent(staticFile) + reqUrl.search
+    req.url = encodeURI(staticFile) + reqUrl.search
     // if there is an existing static file and it is not a forced redirect, return the file
     if (!match.force) {
       return proxy.web(req, res, { ...options, staticFile })
@@ -216,18 +216,25 @@ const serveRedirect = async function ({ match, options, proxy, req, res }) {
       })
     }
 
-    const destURL = stripOrigin(dest)
+    let destURL = stripOrigin(dest)
 
     if (isExternal(match)) {
-      return proxyToExternalUrl({ req, res, dest, destURL })
+      if (isRedirect(match)) {
+        // This is a redirect, so we set the complete external URL as destination
+        destURL = `${dest}`
+      } else {
+        return proxyToExternalUrl({ req, res, dest, destURL })
+      }
     }
 
     if (isRedirect(match)) {
+      console.log(`${NETLIFYDEVLOG} Redirecting ${req.url} to ${destURL}`)
       res.writeHead(match.status, {
         Location: destURL,
         'Cache-Control': 'no-cache',
       })
       res.end(`Redirecting to ${destURL}`)
+
       return
     }
 

diff --git a/tests/integration/0.command.dev.test.js b/tests/integration/0.command.dev.test.js
@@ -1,6 +1,5 @@
 // Handlers are meant to be async outside tests
 const { promises: fs } = require('fs')
-const http = require('http')
 const { join } = require('path')
 
 // eslint-disable-next-line ava/use-test
@@ -166,7 +165,7 @@ test('should source redirects file from publish directory', async (t) => {
   })
 })
 
-test('should redirect requests to an external server', async (t) => {
+test('should rewrite requests to an external server', async (t) => {
   await withSiteBuilder('site-redirects-file-to-external', async (builder) => {
     const externalServer = startExternalServer()
     const { port } = externalServer.address()
@@ -186,6 +185,7 @@ test('should redirect requests to an external server', async (t) => {
             'Content-Type': 'application/x-www-form-urlencoded',
           },
           body: 'param=value',
+          followRedirect: false,
         })
         .json()
       t.deepEqual(postResponse, { body: { param: 'value' }, method: 'POST', url: '/ping' })
@@ -206,15 +206,18 @@ test('should follow 301 redirect to an external server', async (t) => {
     await builder.buildAsync()
 
     await withDevServer({ cwd: builder.directory }, async (server) => {
-      const response = await got(`${server.url}/api/ping`).json()
-      t.deepEqual(response, { body: {}, method: 'GET', url: '/ping' })
+      const response = await got(`${server.url}/api/ping`, { followRedirect: false })
+      t.is(response.headers.location, `http://localhost:${port}/ping`)
+
+      const body = await got(`${server.url}/api/ping`).json()
+      t.deepEqual(body, { body: {}, method: 'GET', url: '/ping' })
     })
 
     externalServer.close()
   })
 })
 
-test('should redirect POST request if content-type is missing', async (t) => {
+test('should rewrite POST request if content-type is missing and not crash dev server', async (t) => {
   await withSiteBuilder('site-with-post-no-content-type', async (builder) => {
     builder.withNetlifyToml({
       config: {
@@ -226,27 +229,16 @@ test('should redirect POST request if content-type is missing', async (t) => {
     await builder.buildAsync()
 
     await withDevServer({ cwd: builder.directory }, async (server) => {
-      const options = {
-        host: server.host,
-        port: server.port,
-        path: '/api/echo',
-        method: 'POST',
-      }
-      let data = ''
-      await new Promise((resolve) => {
-        const callback = (response) => {
-          response.on('data', (chunk) => {
-            data += chunk
-          })
-          response.on('end', resolve)
-        }
-        const req = http.request(options, callback)
-        req.write('param=value')
-        req.end()
-      })
-
-      // we're testing Netlify Dev didn't crash
-      t.is(data, 'Method Not Allowed')
+      const error = await t.throwsAsync(
+        async () =>
+          await got.post(`${server.url}/api/echo`, {
+            body: 'param=value',
+            followRedirect: false,
+          }),
+      )
+
+      // Method Not Allowed
+      t.is(error.response.statusCode, 405)
     })
   })
 })