fix: middleware redirects accidentally applying search params (#336)

.changeset/large-pillows-try.md

@@ -0,0 +1,7 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+Prevent middleware redirects applying search params.
+
+When a middleware function results in a redirect, the location header specified in the response is the full destination, including any search params, as written by the developer. Previously, we always applied search params to redirects found during the routing process, no matter what. This meant that we accidentally were applying search params to middleware redirects, which would alter the intended final destination. This change prevents that from happening.

packages/next-on-pages/templates/_worker.js/handleRequest.ts

@@ -71,13 +71,17 @@ async function generateResponse(
 	output: VercelBuildOutput
 ): Promise<Response> {
 	// Redirect user to external URL for redirects.
-	if (headers.normal.has('location')) {
-		// Apply the search params to the location header.
-		const location = headers.normal.get('location') ?? '/';
-		const paramsStr = [...searchParams.keys()].length
-			? `?${searchParams.toString()}`
-			: '';
-		headers.normal.set('location', `${location}${paramsStr}`);
+	const locationHeader = headers.normal.get('location');
+	if (locationHeader) {
+		// Apply the search params to the location header if it was not from middleware.
+		// Middleware that returns a redirect will specify the destination, including any search params
+		// that they want to include. Therefore, we should not be appending search params to those.
+		if (locationHeader !== headers.middlewareLocation) {
+			const paramsStr = [...searchParams.keys()].length
+				? `?${searchParams.toString()}`
+				: '';
+			headers.normal.set('location', `${locationHeader ?? '/'}${paramsStr}`);
+		}
 
 		return new Response(null, { status, headers: headers.normal });
 	}

packages/next-on-pages/templates/_worker.js/routes-matcher.ts

@@ -1,5 +1,5 @@
 import { parse } from 'cookie';
-import type { MatchPCREResult } from './utils';
+import type { MatchPCREResult, MatchedSetHeaders } from './utils';
 import { isLocaleTrailingSlashRegex, parseAcceptLanguage } from './utils';
 import {
 	applyHeaders,
@@ -32,7 +32,7 @@ export class RoutesMatcher {
 	/** Status for the response object */
 	public status: number | undefined;
 	/** Headers for the response object */
-	public headers: { normal: Headers; important: Headers };
+	public headers: MatchedSetHeaders;
 	/** Search params for the response object */
 	public searchParams: URLSearchParams;
 	/** Custom response body from middleware */
@@ -184,6 +184,7 @@ export class RoutesMatcher {
 		}
 
 		applyHeaders(this.headers.normal, resp.headers);
+		this.headers.middlewareLocation = resp.headers.get('location');
 	}
 
 	/**

packages/next-on-pages/templates/_worker.js/utils/routing.ts

@@ -6,10 +6,27 @@ import {
 	applySearchParams,
 } from './http';
 
+export type MatchedSetHeaders = {
+	/**
+	 * The headers present on a source route.
+	 * Gets applied to the final response before the response headers from running a function.
+	 */
+	normal: Headers;
+	/**
+	 * The *important* headers - the ones present on a source route that specifies `important: true`.
+	 * Gets applied to the final response after the response headers from running a function.
+	 */
+	important: Headers;
+	/**
+	 * Tracks if a location header is found, and what the value is, after running a middleware function.
+	 */
+	middlewareLocation?: string | null;
+};
+
 export type MatchedSet = {
 	path: string;
 	status: number | undefined;
-	headers: { normal: Headers; important: Headers };
+	headers: MatchedSetHeaders;
 	searchParams: URLSearchParams;
 	body: BodyInit | undefined | null;
 };

packages/next-on-pages/tests/templates/requestTestData/middleware.ts

@@ -105,7 +105,7 @@ export const testSet: TestSet = {
 				status: 307,
 				data: '',
 				headers: {
-					location: 'http://localhost/somewhere-else?redirect=',
+					location: 'http://localhost/somewhere-else',
 				},
 			},
 		},