diff --git a/.changeset/fluffy-dingos-tap.md b/.changeset/fluffy-dingos-tap.md new file mode 100644 index 000000000..527670a1c --- /dev/null +++ b/.changeset/fluffy-dingos-tap.md @@ -0,0 +1,11 @@ +--- +"@opennextjs/aws": patch +--- + +fix: Normalize the Location header in redirects + +Normalizes the Location header to either be a relative path or a full URL. +If the Location header is relative to the host, it will return a relative path. +If it is an absolute URL, it will return the full URL. +We want to match the behavior of `next start`. +Query parameters in redirects from Next Config are encoded, but from the middleware they are not touched. \ No newline at end of file diff --git a/packages/open-next/src/core/routing/middleware.ts b/packages/open-next/src/core/routing/middleware.ts index 2d52e6e89..7c5439c12 100644 --- a/packages/open-next/src/core/routing/middleware.ts +++ b/packages/open-next/src/core/routing/middleware.ts @@ -15,6 +15,7 @@ import { convertBodyToReadableStream, getMiddlewareMatch, isExternal, + normalizeLocationHeader, } from "./util.js"; const middlewareManifest = MiddlewareManifest; @@ -25,6 +26,8 @@ const middleMatch = getMiddlewareMatch( functionsConfigManifest, ); +const REDIRECTS = new Set([301, 302, 303, 307, 308]); + type MiddlewareEvent = InternalEvent & { responseHeaders?: Record; isExternalRewrite?: boolean; @@ -132,6 +135,11 @@ export async function handleMiddleware( resHeaders[key] = resHeaders[key] ? [...resHeaders[key], value] : [value]; + } else if ( + REDIRECTS.has(statusCode) && + key.toLowerCase() === "location" + ) { + resHeaders[key] = normalizeLocationHeader(value, internalEvent.url); } else { resHeaders[key] = value; } diff --git a/packages/open-next/src/core/routing/util.ts b/packages/open-next/src/core/routing/util.ts index ad62d8f1f..361db932f 100644 --- a/packages/open-next/src/core/routing/util.ts +++ b/packages/open-next/src/core/routing/util.ts @@ -1,5 +1,6 @@ import crypto from "node:crypto"; import type { OutgoingHttpHeaders } from "node:http"; +import { parse as parseQs, stringify as stringifyQs } from "node:querystring"; import { Readable } from "node:stream"; import { BuildId, HtmlPages, NextConfig } from "config/index.js"; @@ -437,3 +438,42 @@ export async function invalidateCDNOnRequest( ]); } } + +/** + * Normalizes the Location header to either be a relative path or a full URL. + * If the Location header is relative to the origin, it will return a relative path. + * If it is an absolute URL, it will return the full URL. + * Redirects from Next config query parameters are encoded using `stringifyQs` + * Redirects from the middleware the query parameters are not encoded. + * + * @param location The Location header value + * @param baseUrl The base URL to use for relative paths (i.e the original request URL) + * @param encodeQuery Optional flag to indicate if query parameters should be encoded in the Location header + * @returns An absolute or relative Location header value + */ +export function normalizeLocationHeader( + location: string, + baseUrl: string, + encodeQuery = false, +): string { + if (!URL.canParse(location)) { + // If the location is not a valid URL, return it as-is + return location; + } + + const locationURL = new URL(location); + const origin = new URL(baseUrl).origin; + + let search = locationURL.search; + // If encodeQuery is true, we need to encode the query parameters + // We could have used URLSearchParams, but that doesn't match what Next does. + if (encodeQuery && search) { + search = `?${stringifyQs(parseQs(search.slice(1)))}`; + } + const href = `${locationURL.origin}${locationURL.pathname}${search}${locationURL.hash}`; + // The URL is relative if the origin is the same as the base URL's origin + if (locationURL.origin === origin) { + return href.slice(origin.length); + } + return href; +} diff --git a/packages/open-next/src/core/routingHandler.ts b/packages/open-next/src/core/routingHandler.ts index 95502d667..1eb41325c 100644 --- a/packages/open-next/src/core/routingHandler.ts +++ b/packages/open-next/src/core/routingHandler.ts @@ -28,7 +28,7 @@ import { dynamicRouteMatcher, staticRouteMatcher, } from "./routing/routeMatcher"; -import { constructNextUrl } from "./routing/util"; +import { constructNextUrl, normalizeLocationHeader } from "./routing/util"; export const MIDDLEWARE_HEADER_PREFIX = "x-middleware-response-"; export const MIDDLEWARE_HEADER_PREFIX_LEN = MIDDLEWARE_HEADER_PREFIX.length; @@ -109,14 +109,14 @@ export default async function routingHandler( const redirect = handleRedirects(eventOrResult, RoutesManifest.redirects); if (redirect) { // We need to encode the value in the Location header to make sure it is valid according to RFC - // https://stackoverflow.com/a/7654605/16587222 - redirect.headers.Location = new URL( + redirect.headers.Location = normalizeLocationHeader( redirect.headers.Location as string, - ).href; + event.url, + true, + ); debug("redirect", redirect); return redirect; } - const middlewareEventOrResult = await handleMiddleware( eventOrResult, // We need to pass the initial search without any decoding diff --git a/packages/tests-e2e/tests/appRouter/config.redirect.test.ts b/packages/tests-e2e/tests/appRouter/config.redirect.test.ts index 218e83279..906642c90 100644 --- a/packages/tests-e2e/tests/appRouter/config.redirect.test.ts +++ b/packages/tests-e2e/tests/appRouter/config.redirect.test.ts @@ -74,7 +74,6 @@ test.describe("Next Config Redirect", () => { }); test("Should properly encode the Location header for redirects with query params", async ({ page, - baseURL, }) => { await page.goto("/config-redirect"); const responsePromise = page.waitForResponse((response) => { @@ -86,17 +85,14 @@ test.describe("Next Config Redirect", () => { const locationHeader = res.headers().location; expect(locationHeader).toBe( - `${baseURL}/config-redirect/dest?q=%C3%A4%C3%B6%C3%A5%E2%82%AC`, + "/config-redirect/dest?q=%C3%A4%C3%B6%C3%A5%E2%82%AC", ); expect(res.status()).toBe(307); const searchParams = page.getByTestId("searchParams"); await expect(searchParams).toHaveText("q: äöå€"); }); - test("Should respect already encoded query params", async ({ - page, - baseURL, - }) => { + test("Should respect already encoded query params", async ({ page }) => { await page.goto("/config-redirect"); const responsePromise = page.waitForResponse((response) => { return response.status() === 307; @@ -107,7 +103,7 @@ test.describe("Next Config Redirect", () => { const locationHeader = res.headers().location; expect(locationHeader).toBe( - `${baseURL}/config-redirect/dest?q=%C3%A4%C3%B6%C3%A5%E2%82%AC`, + "/config-redirect/dest?q=%C3%A4%C3%B6%C3%A5%E2%82%AC", ); expect(res.status()).toBe(307); diff --git a/packages/tests-unit/tests/core/routing/util.test.ts b/packages/tests-unit/tests/core/routing/util.test.ts index 908057499..bc6b62fd4 100644 --- a/packages/tests-unit/tests/core/routing/util.test.ts +++ b/packages/tests-unit/tests/core/routing/util.test.ts @@ -17,6 +17,7 @@ import { getUrlParts, invalidateCDNOnRequest, isExternal, + normalizeLocationHeader, revalidateIfRequired, unescapeRegex, } from "@opennextjs/aws/core/routing/util.js"; @@ -876,3 +877,66 @@ describe("constructNextUrl", () => { expect(result).toBe("http://localhost/base/path"); }); }); + +describe("normalizeLocationHeader", () => { + it("should normalize relative location header", () => { + const result = normalizeLocationHeader( + "http://localhost:3000/path", + "http://localhost:3000", + ); + expect(result).toBe("/path"); + }); + + it("should not change absolute location header", () => { + const result = normalizeLocationHeader( + "https://opennext.js.org/aws", + "http://localhost:3000", + ); + expect(result).toBe("https://opennext.js.org/aws"); + }); + + it("should normalize relative location with query parameters", () => { + const result = normalizeLocationHeader( + "http://localhost:3000/path?query=1", + "http://localhost:3000", + ); + expect(result).toBe("/path?query=1"); + }); + + it("should normalize and encode special characters in location header", () => { + const result = normalizeLocationHeader( + "http://localhost:3000/about?query=æøå!&", + "http://localhost:3000", + true, + ); + expect(result).toBe("/about?query=%C3%A6%C3%B8%C3%A5!"); + }); + + it("should normalize and respect already encoded characters in location header", () => { + const result = normalizeLocationHeader( + "http://localhost:3000/path?query=test%2F%2F", + "http://localhost:3000", + true, + ); + expect(result).toBe("/path?query=test%2F%2F"); + }); + + it("should preserve multiple query parameters in location header", () => { + const result = normalizeLocationHeader( + "http://localhost:3000/path?query1=value1&query2=value2&query1=value3&query2=value4&random=randomvalue", + "http://localhost:3000", + true, + ); + expect(result).toBe( + "/path?query1=value1&query1=value3&query2=value2&query2=value4&random=randomvalue", + ); + }); + + it("should return absolute URL correctly when the base URL is different", () => { + const result = normalizeLocationHeader( + "https://example.com/path?query=1", + "http://localhost:3000", + ); + expect(result).toBe("https://example.com/path?query=1"); + }); +});