From c409afd688c549c7a59f66b066c5f56b5d745967 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 19 Aug 2020 12:30:33 -0500 Subject: [PATCH] Fix basePath and public folder check ending routes early (#16356) This corrects the basePath being required check for filesystem routes to not consider the public folder catch-all route since it always matches even if the public file isn't present and instead moves the basePath check inside of the public-folder catch-all. Tests already exist that catch this by adding a public folder to the existing `basepath` test suite Fixes: https://github.com/vercel/next.js/issues/16332 Closes: https://github.com/vercel/next.js/pull/16350 --- packages/next/next-server/server/next-server.ts | 8 ++++++++ packages/next/next-server/server/router.ts | 11 ++++++----- test/integration/basepath/public/data.txt | 1 + test/integration/basepath/test/index.test.js | 11 +++++++++++ 4 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 test/integration/basepath/public/data.txt diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index cfb63ee16535f..40b2f62f116b9 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -783,6 +783,14 @@ export default class Server { name: 'public folder catchall', fn: async (req, res, params, parsedUrl) => { const pathParts: string[] = params.path || [] + const { basePath } = this.nextConfig + + // if basePath is defined require it be present + if (basePath) { + if (pathParts[0] !== basePath.substr(1)) return { finished: false } + pathParts.shift() + } + const path = `/${pathParts.join('/')}` if (publicFiles.has(path)) { diff --git a/packages/next/next-server/server/router.ts b/packages/next/next-server/server/router.ts index 4a20a1ff21d17..4bc78f3831b82 100644 --- a/packages/next/next-server/server/router.ts +++ b/packages/next/next-server/server/router.ts @@ -37,6 +37,7 @@ export type PageChecker = (pathname: string) => Promise const customRouteTypes = new Set(['rewrite', 'redirect', 'header']) function replaceBasePath(basePath: string, pathname: string) { + // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/` return pathname!.replace(basePath, '') || '/' } @@ -166,9 +167,10 @@ export default class Router { const originalPathname = currentPathname const requireBasePath = testRoute.requireBasePath !== false const isCustomRoute = customRouteTypes.has(testRoute.type) + const isPublicFolderCatchall = testRoute.name === 'public folder catchall' + const keepBasePath = isCustomRoute || isPublicFolderCatchall - if (!isCustomRoute) { - // If replace ends up replacing the full url it'll be `undefined`, meaning we have to default it to `/` + if (!keepBasePath) { currentPathname = replaceBasePath(this.basePath, currentPathname!) } @@ -178,7 +180,7 @@ export default class Router { if (newParams) { // since we require basePath be present for non-custom-routes we // 404 here when we matched an fs route - if (!isCustomRoute) { + if (!keepBasePath) { if (!originallyHadBasePath && !(req as any)._nextDidRewrite) { if (requireBasePath) { // consider this a non-match so the 404 renders @@ -201,7 +203,7 @@ export default class Router { // since the fs route didn't match we need to re-add the basePath // to continue checking rewrites with the basePath present - if (!isCustomRoute) { + if (!keepBasePath) { parsedUrlUpdated.pathname = originalPathname } @@ -272,7 +274,6 @@ export default class Router { } } } - return false } } diff --git a/test/integration/basepath/public/data.txt b/test/integration/basepath/public/data.txt new file mode 100644 index 0000000000000..95d09f2b10159 --- /dev/null +++ b/test/integration/basepath/public/data.txt @@ -0,0 +1 @@ +hello world \ No newline at end of file diff --git a/test/integration/basepath/test/index.test.js b/test/integration/basepath/test/index.test.js index f2e64a5ae598d..bc12692064d23 100644 --- a/test/integration/basepath/test/index.test.js +++ b/test/integration/basepath/test/index.test.js @@ -135,6 +135,17 @@ const runTests = (context, dev = false) => { }) } + it('should 404 for public file without basePath', async () => { + const res = await fetchViaHTTP(context.appPort, '/data.txt') + expect(res.status).toBe(404) + }) + + it('should serve public file with basePath correctly', async () => { + const res = await fetchViaHTTP(context.appPort, '/docs/data.txt') + expect(res.status).toBe(200) + expect(await res.text()).toBe('hello world') + }) + it('should rewrite with basePath by default', async () => { const html = await renderViaHTTP(context.appPort, '/docs/rewrite-1') expect(html).toContain('getServerSideProps')