fix: Normalize the Location header in redirects (#941)

* add normalizeLocationHeader

add comment in middleware

check status code

fix e2e

review

add unit test for normalizeLocationHeader

refactor to match Next behavior

update unit test

refactor comment

refactor changeset

* short circuit

Author: sommeeeer

.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.

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<string, string | string[]>;
   isExternalRewrite?: boolean;
@@ -133,6 +136,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;
       }

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";
@@ -438,3 +439,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;
+}

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;
@@ -110,14 +110,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

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);
 

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");
+  });
+});