Disallow basePath: false for internal routes (vercel#15837)

filoblu · Aug 4, 2020 · 9230440 · 9230440
1 parent 35e4a37
commit 9230440
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 39 deletions.
diff --git a/errors/invalid-external-rewrite.md b/errors/invalid-external-rewrite.md
@@ -0,0 +1,13 @@
+# Invalid External Rewrite
+
+#### Why This Error Occurred
+
+A rewrite was defined with both `basePath: false` and an internal `destination`. Rewrites that capture urls outside of the `basePath` must route externally, as they are intended for proxying in the case of incremental adoption of Next.js in a project.
+
+#### Possible Ways to Fix It
+
+Look for any rewrite where `basePath` is `false` and make sure its `destination` starts with `http://` or `https://`.
+
+### Useful Links
+
+- [Rewrites section in Documentation](https://nextjs.org/docs/api-reference/next.config.js/rewrites)
diff --git a/packages/next/lib/load-custom-routes.ts b/packages/next/lib/load-custom-routes.ts
@@ -170,6 +170,23 @@ function checkCustomRoutes(
       continue
     }
 
+    if (
+      type === 'rewrite' &&
+      (route as Rewrite).basePath === false &&
+      !(
+        (route as Rewrite).destination.startsWith('http://') ||
+        (route as Rewrite).destination.startsWith('https://')
+      )
+    ) {
+      console.error(
+        `The route ${
+          (route as Rewrite).source
+        } rewrites urls outside of the basePath. Please use a destination that starts with \`http://\` or \`https://\` https://err.sh/vercel/next.js/invalid-external-rewrite.md`
+      )
+      numInvalidRoutes++
+      continue
+    }
+
     const keys = Object.keys(route)
     const invalidKeys = keys.filter((key) => !allowedKeys.has(key))
     const invalidParts: string[] = []

diff --git a/test/integration/basepath/external/page.html b/test/integration/basepath/external/page.html
@@ -0,0 +1,5 @@
+<html>
+  <body>
+    hello from external
+  </body>
+</html>
diff --git a/test/integration/basepath/next.config.js b/test/integration/basepath/next.config.js
@@ -13,7 +13,7 @@ module.exports = {
       },
       {
         source: '/rewrite-no-basepath',
-        destination: '/gssp',
+        destination: 'http://localhost:__EXTERNAL_APP_PORT__/page.html',
         basePath: false,
       },
       {

diff --git a/test/integration/basepath/test/index.test.js b/test/integration/basepath/test/index.test.js
@@ -1,7 +1,7 @@
 /* eslint-env jest */
 
 import webdriver from 'next-webdriver'
-import { join } from 'path'
+import { join, resolve } from 'path'
 import url from 'url'
 import {
   nextServer,
@@ -21,6 +21,7 @@ import {
   getRedboxSource,
   hasRedbox,
   fetchViaHTTP,
+  startStaticServer,
 } from 'next-test-utils'
 import fs, {
   readFileSync,
@@ -34,6 +35,23 @@ jest.setTimeout(1000 * 60 * 2)
 
 const appDir = join(__dirname, '..')
 
+let externalApp
+
+const nextConfig = new File(resolve(appDir, 'next.config.js'))
+
+beforeAll(async () => {
+  externalApp = await startStaticServer(resolve(__dirname, '../external'))
+  nextConfig.replace(
+    '__EXTERNAL_APP_PORT__',
+    String(externalApp.address().port)
+  )
+})
+
+afterAll(async () => {
+  nextConfig.restore()
+  externalApp.close()
+})
+
 const runTests = (context, dev = false) => {
   if (dev) {
     it('should render error in dev overlay correctly', async () => {
@@ -138,39 +156,7 @@ const runTests = (context, dev = false) => {
 
   it('should rewrite without basePath when set to false', async () => {
     const html = await renderViaHTTP(context.appPort, '/rewrite-no-basePath')
-    expect(html).toContain('getServerSideProps')
-  })
-
-  it('should have correct asPath for rewrite without basePath', async () => {
-    const browser = await webdriver(context.appPort, '/rewrite-no-basePath')
-    expect(await browser.eval(() => window.location.pathname)).toBe(
-      '/rewrite-no-basePath'
-    )
-    expect(await browser.eval(() => window.next.router.asPath)).toBe(
-      '/rewrite-no-basePath'
-    )
-    expect(await browser.eval(() => window.next.router.pathname)).toBe('/gssp')
-  })
-
-  it('should have correct asPath for rewrite without basePath on back()', async () => {
-    const browser = await webdriver(context.appPort, '/rewrite-no-basePath')
-    await browser.eval(() => (window.navigationMarker = true))
-    await browser.eval(() => window.next.router.push('/hello'))
-    await check(
-      () => browser.eval(() => window.location.pathname),
-      '/docs/hello'
-    )
-    await browser.back()
-    await check(
-      () => browser.eval(() => window.location.pathname),
-      '/rewrite-no-basePath'
-    )
-    await check(
-      () => browser.eval(() => window.next.router.asPath),
-      '/rewrite-no-basePath'
-    )
-    expect(await browser.eval(() => window.next.router.pathname)).toBe('/gssp')
-    expect(await browser.eval(() => window.navigationMarker)).toBe(true)
+    expect(html).toContain('hello from external')
   })
 
   it('should redirect with basePath by default', async () => {
@@ -258,15 +244,17 @@ const runTests = (context, dev = false) => {
     )
   })
 
-  it('should navigating back to a non-basepath 404 that starts with basepath', async () => {
+  it('should navigate back to a non-basepath 404 that starts with basepath', async () => {
     const browser = await webdriver(context.appPort, '/docshello')
+    await browser.eval(() => (window.navigationMarker = true))
     await browser.eval(() => window.next.router.push('/hello'))
     await browser.waitForElementByCss('#pathname')
     await browser.back()
     check(() => browser.eval(() => window.location.pathname), '/docshello')
     expect(await browser.eval(() => window.next.router.asPath)).toBe(
       '/docshello'
     )
+    expect(await browser.eval(() => window.navigationMarker)).toBe(true)
   })
 
   it('should update dynamic params after mount correctly', async () => {
@@ -1008,7 +996,11 @@ describe('basePath production', () => {
   let app
 
   beforeAll(async () => {
-    await nextBuild(appDir)
+    await nextBuild(appDir, [], {
+      env: {
+        EXTERNAL_APP: `http://localhost:${externalApp.address().port}`,
+      },
+    })
     app = nextServer({
       dir: join(__dirname, '../'),
       dev: false,
@@ -1028,8 +1020,6 @@ describe('basePath serverless', () => {
   let context = {}
   let app
 
-  const nextConfig = new File(join(appDir, 'next.config.js'))
-
   beforeAll(async () => {
     await nextConfig.replace(
       '// replace me',

diff --git a/test/integration/invalid-custom-routes/test/index.test.js b/test/integration/invalid-custom-routes/test/index.test.js
@@ -155,6 +155,12 @@ const runTests = () => {
           source: '/hello/world/(.*)',
           destination: '/:0',
         },
+        {
+          // basePath with relative destination
+          source: '/hello',
+          destination: '/world',
+          basePath: false,
+        },
         // invalid objects
         null,
         'string',
@@ -205,6 +211,11 @@ const runTests = () => {
     expect(stderr).not.toContain(
       'Valid redirect statusCode values are 301, 302, 303, 307, 308'
     )
+
+    expect(stderr).toContain(
+      `The route /hello rewrites urls outside of the basePath. Please use a destination that starts with \`http://\` or \`https://\` https://err.sh/vercel/next.js/invalid-external-rewrite.md`
+    )
+
     expect(stderr).toContain('Invalid rewrites found')
   })