fix: next.config.js URL rewrites unintentionally redirecting instea…

…d of rewriting (#394) * fix: `next.config.js` rewrites unintentionally redirecting instead of rewriting * fix fetch not using original method * mock fetch
cloudflare · Jul 24, 2023 · f08f2b9 · f08f2b9
1 parent ee6b7dc
commit f08f2b9
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 27 deletions.
diff --git a/.changeset/gorgeous-carrots-breathe.md b/.changeset/gorgeous-carrots-breathe.md
@@ -0,0 +1,5 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+Fix `next.config.js` rewrites unintentionally redirecting the user instead of rewriting the request.
diff --git a/packages/next-on-pages/templates/_worker.js/handleRequest.ts b/packages/next-on-pages/templates/_worker.js/handleRequest.ts
@@ -1,5 +1,5 @@
 import type { MatchedSet } from './utils';
-import { applyHeaders, runOrFetchBuildOutputItem } from './utils';
+import { applyHeaders, isUrl, runOrFetchBuildOutputItem } from './utils';
 import { RoutesMatcher } from './routes-matcher';
 import type { RequestContext } from '../../src/utils/requestContext';
 
@@ -86,16 +86,23 @@ async function generateResponse(
 		return new Response(null, { status, headers: headers.normal });
 	}
 
-	let resp =
-		body !== undefined
-			? // If we have a response body from matching, use it instead.
-			  new Response(body, { status })
-			: await runOrFetchBuildOutputItem(output[path], reqCtx, {
-					path,
-					status,
-					headers,
-					searchParams,
-			  });
+	let resp: Response;
+
+	if (body !== undefined) {
+		// If we have a response body from matching, use it instead.
+		resp = new Response(body, { status });
+	} else if (isUrl(path)) {
+		// If the path is an URL from matching, that means it was rewritten to a full URL.
+		resp = await fetch(new URL(path), reqCtx.request);
+	} else {
+		// Otherwise, we need to serve a file from the Vercel build output.
+		resp = await runOrFetchBuildOutputItem(output[path], reqCtx, {
+			path,
+			status,
+			headers,
+			searchParams,
+		});
+	}
 
 	const newHeaders = headers.normal;
 	applyHeaders(newHeaders, resp.headers);

diff --git a/packages/next-on-pages/templates/_worker.js/routes-matcher.ts b/packages/next-on-pages/templates/_worker.js/routes-matcher.ts
@@ -593,11 +593,6 @@ export class RoutesMatcher {
 		this.checkPhaseCounter = 0;
 		const result = await this.checkPhase(phase);
 
-		// Check if path is an external URL.
-		if (isUrl(this.path)) {
-			this.headers.normal.set('location', this.path);
-		}
-
 		// Update status to redirect user to external URL.
 		if (
 			this.headers.normal.has('location') &&

diff --git a/packages/next-on-pages/tests/_helpers/index.ts b/packages/next-on-pages/tests/_helpers/index.ts
@@ -29,8 +29,9 @@ export type TestCase = {
 	method?: string;
 	expected: {
 		status: number;
-		data: string;
+		data: string | RegExp;
 		headers?: Record<string, string>;
+		ignoreHeaders?: boolean;
 		reqHeaders?: Record<string, string>;
 		mockConsole?: { log?: string[]; error?: (string | Error)[] };
 	};

diff --git a/packages/next-on-pages/tests/templates/handleRequest.test.ts b/packages/next-on-pages/tests/templates/handleRequest.test.ts
@@ -63,10 +63,17 @@ function runTestCase(
 			);
 
 			expect(res.status).toEqual(expected.status);
-			await expect(res.text()).resolves.toEqual(expected.data);
-			expect(Object.fromEntries(res.headers.entries())).toEqual(
-				expected.headers || {},
-			);
+			const textContent = await res.text();
+			if (expected.data instanceof RegExp) {
+				expect(textContent).toMatch(expected.data);
+			} else {
+				expect(textContent).toEqual(expected.data);
+			}
+			if (!expected.ignoreHeaders) {
+				expect(Object.fromEntries(res.headers.entries())).toEqual(
+					expected.headers || {},
+				);
+			}
 			if (expected.reqHeaders) {
 				expect(Object.fromEntries(req.headers.entries())).toEqual(
 					expected.reqHeaders,
@@ -87,6 +94,18 @@ function runTestCase(
  * @param testSet Test set to run.
  */
 async function runTestSet({ config, files, testCases }: TestSet) {
+	const originalFetch = globalThis.fetch;
+	globalThis.fetch = async (...args: Parameters<typeof originalFetch>) => {
+		const req = new Request(...args);
+		const url = new URL(req.url);
+
+		if (url.hostname === 'external-test-url.com') {
+			return new Response('external test url response');
+		}
+
+		return originalFetch(...args);
+	};
+
 	const { vercelConfig, buildOutput, assetsFetcher, restoreMocks } =
 		await createRouterTestData(config, files);
 
@@ -99,7 +118,10 @@ async function runTestSet({ config, files, testCases }: TestSet) {
 		runTestCase(reqCtx, vercelConfig, buildOutput, testCase),
 	);
 
-	afterAll(() => restoreMocks());
+	afterAll(() => {
+		globalThis.fetch = originalFetch;
+		restoreMocks();
+	});
 }
 
 vi.mock('esbuild', async () => {

diff --git a/packages/next-on-pages/tests/templates/requestTestData/configRewritesRedirectsHeaders.ts b/packages/next-on-pages/tests/templates/requestTestData/configRewritesRedirectsHeaders.ts
@@ -60,7 +60,7 @@ const rawVercelConfig: VercelConfig = {
 		{ handle: 'resource' },
 		{
 			src: '^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))?(?:/)?$',
-			dest: 'https://my-old-site.com/$1',
+			dest: 'https://external-test-url.com/$1',
 			check: true,
 		},
 		{ src: '/.*', status: 404 },
@@ -165,11 +165,13 @@ export const testSet: TestSet = {
 		},
 		{
 			name: 'rewrites - `fallback`: rewrites on any request that has not been matched',
-			paths: ['/invalid-route'],
+			paths: ['/plain'],
 			expected: {
-				status: 307,
-				data: '',
-				headers: { location: 'https://my-old-site.com/invalid-route' },
+				status: 200,
+				data: 'external test url response',
+				headers: {
+					'content-type': 'text/plain;charset=UTF-8',
+				},
 			},
 		},
 		{