fix: support for redirects with 200 status. Fix crashing dev server when have socket error. Follow redirects from proxyRes

Author: VitaliyR

src/utils/proxy.ts

@@ -122,10 +122,14 @@ const formatEdgeFunctionError = (errorBuffer, acceptsHtml) => {
   })
 }
 
-function isInternal(url?: string): boolean {
+function isInternalFunctions(url?: string): boolean {
   return url?.startsWith('/.netlify/') ?? false
 }
 
+function isInternal(url?: string): boolean {
+  return url?.startsWith('/') ?? false
+}
+
 function isFunction(functionsPort: boolean | number | undefined, url: string) {
   return functionsPort && url.match(DEFAULT_FUNCTION_URL_EXPRESSION)
 }
@@ -202,7 +206,7 @@ const handleAddonUrl = function ({ addonUrl, req, res }) {
 
 // @ts-expect-error TS(7006) FIXME: Parameter 'match' implicitly has an 'any' type.
 const isRedirect = function (match) {
-  return match.status && match.status >= 300 && match.status <= 400
+  return match.status && match.status >= 300 && match.status < 400
 }
 
 // @ts-expect-error TS(7006) FIXME: Parameter 'publicFolder' implicitly has an 'any' t... Remove this comment to see the full error message
@@ -417,8 +421,8 @@ const serveRedirect = async function ({
     const ct = req.headers['content-type'] ? contentType.parse(req).type : ''
     if (
       req.method === 'POST' &&
-      !isInternal(req.url) &&
-      !isInternal(destURL) &&
+      !isInternalFunctions(req.url) &&
+      !isInternalFunctions(destURL) &&
       (ct.endsWith('/x-www-form-urlencoded') || ct === 'multipart/form-data')
     ) {
       return proxy.web(req, res, { target: options.functionsServer })
@@ -428,14 +432,13 @@ const serveRedirect = async function ({
     const matchingFunction =
       functionsRegistry &&
       (await functionsRegistry.getFunctionForURLPath(destURL, req.method, () => Boolean(destStaticFile)))
-    let statusValue
     if (
       match.force ||
       (!staticFile && ((!options.framework && destStaticFile) || isInternal(destURL) || matchingFunction))
     ) {
+      // 3xx redirects parsed above, here are 2xx meaning just override the url of proxying page and use the status
+      // which comes from that url
       req.url = destStaticFile ? destStaticFile + dest.search : destURL
-      const { status } = match
-      statusValue = status
       console.log(`${NETLIFYDEVLOG} Rewrote URL to`, req.url)
     }
 
@@ -452,15 +455,17 @@ const serveRedirect = async function ({
 
       return proxy.web(req, res, { headers: functionHeaders, target: options.functionsServer })
     }
+
     if (isImageRequest(req)) {
       return imageProxy(req, res)
     }
+
     const addonUrl = getAddonUrl(options.addonsUrls, req)
     if (addonUrl) {
       return handleAddonUrl({ req, res, addonUrl })
     }
 
-    return proxy.web(req, res, { ...options, status: statusValue })
+    return proxy.web(req, res, options)
   }
 
   return proxy.web(req, res, options)
@@ -519,11 +524,14 @@ const initializeProxy = async function ({
     }
   })
 
-  proxy.on('error', (_, req, res) => {
-    // @ts-expect-error TS(2339) FIXME: Property 'writeHead' does not exist on type 'Socke... Remove this comment to see the full error message
-    res.writeHead(500, {
-      'Content-Type': 'text/plain',
-    })
+  proxy.on('error', (err, req, res) => {
+    console.error('Got error from proxy', err)
+
+    if (res instanceof http.ServerResponse) {
+      res.writeHead(500, {
+        'Content-Type': 'text/plain',
+      })
+    }
 
     const message = isEdgeFunctionsRequest(req)
       ? 'There was an error with an Edge Function. Please check the terminal for more details.'
@@ -596,8 +604,7 @@ const initializeProxy = async function ({
       }
     }
 
-    // @ts-expect-error TS(2339) FIXME: Property 'proxyOptions' does not exist on type 'In... Remove this comment to see the full error message
-    if (req.proxyOptions.staticFile && isRedirect({ status: proxyRes.statusCode }) && proxyRes.headers.location) {
+    if (isRedirect({ status: proxyRes.statusCode }) && proxyRes.headers.location) {
       req.url = proxyRes.headers.location
       return serveRedirect({
         // We don't want to match functions at this point because any redirects
@@ -838,7 +845,7 @@ const onRequest = async (
     hasFormSubmissionHandler &&
     functionsServer &&
     req.method === 'POST' &&
-    !isInternal(req.url) &&
+    !isInternalFunctions(req.url) &&
     (ct.endsWith('/x-www-form-urlencoded') || ct === 'multipart/form-data')
   ) {
     return proxy.web(req, res, { target: functionsServer })

tests/integration/__fixtures__/hugo-site/netlify.toml

@@ -2,6 +2,6 @@
 command = "npm run serve"
 framework = "#custom"
 functions = "functions/"
-port = 7000
+port = 7001
 publish = "out"
 targetPort = 1313

tests/integration/__fixtures__/next-app/app/page.js

@@ -5,6 +5,7 @@ export default function Home() {
   return (
     <main className={styles.main}>
       <div className={styles.description}>
+        <p>Local site Dev Server</p>
         <p>
           Get started by editing&nbsp;
           <code className={styles.code}>app/page.js</code>

tests/integration/__fixtures__/next-app/app/test/exists/page.js

@@ -0,0 +1,3 @@
+export default function ExistsPage() {
+  return <p>Exists page</p>
+}

tests/integration/__fixtures__/next-app/netlify.toml

@@ -4,6 +4,16 @@ publish = "public"
 [dev]
 targetPort = 6123
 
+[[redirects]]
+from = "/local/*"
+to = "/:splat"
+status = 200
+
+[[redirects]]
+from = "/test/*"
+to = "https://www.netlify.app/:splat"
+status = 200
+
 [[redirects]]
 from = "*"
 to = "https://www.netlify.app/:splat"

tests/integration/__fixtures__/site-with-redirect/.gitignore

@@ -0,0 +1 @@
+.netlify

tests/integration/__fixtures__/site-with-redirect/index.js

@@ -0,0 +1,29 @@
+const http = require('http')
+
+const server = http.createServer((req, res) => {
+  const pathname = new URL(req.url, 'http://localhost').pathname
+
+  console.log(`Got ${pathname}`)
+
+  if (pathname === '/') {
+    res.write('Root page')
+    res.end()
+  } else if (pathname === '/test/exists') {
+    res.writeHead(302, undefined, { location: '/test/exists/' })
+    res.end()
+  } else if (pathname === '/test/exists/') {
+    res.write('Test exists page')
+    res.end()
+  } else if (pathname === '/test/not-allowed') {
+    res.writeHead(405)
+    res.write('This not allowed')
+    res.end()
+  } else {
+    res.writeHead(404).write('Page is not found')
+    res.end()
+  }
+})
+
+server.listen(6124, () => {
+  console.log('Server is Running')
+})

tests/integration/__fixtures__/site-with-redirect/netlify.toml

@@ -0,0 +1,8 @@
+[dev]
+targetPort = 6124
+command = "npm run dev"
+
+[[redirects]]
+from = "/local/*"
+to = "/:splat"
+status = 200

tests/integration/__fixtures__/site-with-redirect/package.json

@@ -0,0 +1,9 @@
+{
+  "name": "site-with-redirect",
+  "version": "1.0.0",
+  "private": true,
+  "scripts": {
+    "dev": "node index.js"
+  },
+  "dependencies": {}
+}

tests/integration/commands/dev/redirects.test.ts

@@ -6,7 +6,7 @@ import fetch from 'node-fetch'
 describe('redirects', () => {
   setupFixtureTests('dev-server-with-functions', { devServer: true }, () => {
     test<FixtureTestContext>('should send original query params to functions', async ({ devServer }) => {
-      const response = await fetch(`http://localhost:${devServer.port}/with-params?param2=world&other=1`, {})
+      const response = await fetch(`http://localhost:${devServer.port}/with-params?param2=world&other=1`)
 
       expect(response.status).toBe(200)
 
@@ -19,7 +19,7 @@ describe('redirects', () => {
     test<FixtureTestContext>('should send original query params to functions when using duplicate parameters', async ({
       devServer,
     }) => {
-      const response = await fetch(`http://localhost:${devServer.port}/api/echo?param=hello&param=world`, {})
+      const response = await fetch(`http://localhost:${devServer.port}/api/echo?param=hello&param=world`)
 
       expect(response.status).toBe(200)
 
@@ -31,23 +31,83 @@ describe('redirects', () => {
 
   setupFixtureTests('next-app', { devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } } }, () => {
     test<FixtureTestContext>('should prefer local files instead of redirect when not forced', async ({ devServer }) => {
-      const response = await fetch(`http://localhost:${devServer.port}/test.txt`, {})
+      const response = await fetch(`http://localhost:${devServer.port}/test.txt`)
 
       expect(response.status).toBe(200)
 
       const result = await response.text()
       expect(result.trim()).toEqual('hello world')
+      expect(devServer?.output).not.toContain('Proxying to https://www.netlify.app')
     })
 
     test<FixtureTestContext>('should check for the dynamic page existence before doing redirect', async ({
       devServer,
     }) => {
-      const response = await fetch(`http://localhost:${devServer.port}/`, {})
+      const response = await fetch(`http://localhost:${devServer.port}/`)
 
       expect(response.status).toBe(200)
 
       const result = await response.text()
+      expect(result.toLowerCase()).toContain('local site dev server')
       expect(result.toLowerCase()).not.toContain('netlify')
+      expect(devServer?.output).not.toContain('Proxying to https://www.netlify.app')
+    })
+
+    test<FixtureTestContext>('nested route redirect check for the page existence', async ({ devServer }) => {
+      let response = await fetch(`http://localhost:${devServer.port}/test/exists`)
+      expect(response.status).toBe(200)
+
+      let result = await response.text()
+      expect(result.toLowerCase()).toContain('exists page')
+      expect(devServer?.output).not.toContain('Proxying to https://www.netlify.app/exists')
+
+      response = await fetch(`http://localhost:${devServer.port}/test/about`)
+      expect(response.status).toBe(200)
+
+      result = await response.text()
+      expect(result.toLowerCase()).toContain('netlify')
+
+      expect(devServer?.output).toContain('Proxying to https://www.netlify.app/about')
+    })
+
+    test<FixtureTestContext>('should do local redirect', async ({ devServer }) => {
+      const response = await fetch(`http://localhost:${devServer.port}/local/test/exists`)
+
+      expect(response.status).toBe(200)
+
+      const result = await response.text()
+      expect(response.headers.get('location')).toBeNull()
+      expect(response.status).toBe(200)
+      expect(result.toLowerCase()).toContain('exists page')
+      expect(result.toLowerCase()).not.toContain('netlify')
+      expect(devServer?.output).not.toContain('Proxying to https://www.netlify.app/test')
+    })
+  })
+
+  setupFixtureTests('site-with-redirect', { devServer: true }, () => {
+    test<FixtureTestContext>('should do local redirect', async ({ devServer }) => {
+      const response = await fetch(`http://localhost:${devServer.port}/local/test/exists`)
+
+      expect(response.status).toBe(200)
+
+      const result = await response.text()
+      expect(response.url).toEqual(`http://localhost:${devServer.port}/local/test/exists`)
+      expect(response.status).toBe(200)
+      expect(result.toLowerCase()).toContain('exists page')
+      expect(result.toLowerCase()).not.toContain('netlify')
+      expect(devServer?.output).not.toContain('Proxying to https://www.netlify.app')
+    })
+
+    test<FixtureTestContext>('should pass proper status code of the redirected page', async ({ devServer }) => {
+      let response = await fetch(`http://localhost:${devServer.port}/local/test/not-allowed`)
+
+      expect(response.status).toBe(405)
+      const result = await response.text()
+      expect(result.toLowerCase()).toContain('this not allowed')
+
+      response = await fetch(`http://localhost:${devServer.port}/local/test/not-found`)
+
+      expect(response.status).toBe(404)
     })
   })
 })