fix: Ensure middleware rewrite status code is properly propagated in cache interceptor (opennextjs#974)

changeset

fix test

fix test

refactor deletion of cache tags header

review

update test

Author: sommeeeer

.changeset/angry-ways-approve.md

@@ -0,0 +1,5 @@
+---
+"@opennextjs/aws": patch
+---
+
+fix: Ensure middleware rewrite status code is properly propagated to cache interceptor

packages/open-next/src/adapters/middleware.ts

@@ -125,7 +125,10 @@ const defaultHandler = async (
         }
       }
 
-      result.headers[INTERNAL_EVENT_REQUEST_ID] = requestId;
+      if (process.env.OPEN_NEXT_REQUEST_ID_HEADER || globalThis.openNextDebug) {
+        result.headers[INTERNAL_EVENT_REQUEST_ID] = requestId;
+      }
+
       debug("Middleware response", result);
       return result;
     },

packages/open-next/src/core/requestHandler.ts

@@ -63,7 +63,7 @@ export async function openNextHandler(
       }
       debug("internalEvent", internalEvent);
 
-      // These 2 will get overwritten by the routing handler if not using an external middleware
+      // These 3 will get overwritten by the routing handler if not using an external middleware
       const internalHeaders = {
         initialPath:
           initialHeaders[INTERNAL_HEADER_INITIAL_URL] ?? internalEvent.rawPath,

packages/open-next/src/core/routing/cacheInterceptor.ts

@@ -1,7 +1,11 @@
 import { createHash } from "node:crypto";
 
 import { NextConfig, PrerenderManifest } from "config/index";
-import type { InternalEvent, InternalResult } from "types/open-next";
+import type {
+  InternalEvent,
+  InternalResult,
+  MiddlewareEvent,
+} from "types/open-next";
 import type { CacheValue } from "types/overrides";
 import { emptyReadableStream, toReadableStream } from "utils/stream";
 
@@ -100,7 +104,7 @@ async function computeCacheControl(
 }
 
 async function generateResult(
-  event: InternalEvent,
+  event: MiddlewareEvent,
   localizedPath: string,
   cachedValue: CacheValue<"cache">,
   lastModified?: number,
@@ -132,8 +136,12 @@ async function generateResult(
   );
   return {
     type: "core",
-    // sometimes other status codes can be cached, like 404. For these cases, we should return the correct status code
-    statusCode: cachedValue.meta?.status ?? 200,
+    // Sometimes other status codes can be cached, like 404. For these cases, we should return the correct status code
+    // Also set the status code to the rewriteStatusCode if defined
+    // This can happen in handleMiddleware in routingHandler.
+    // `NextResponse.rewrite(url, { status: xxx})
+    // The rewrite status code should take precedence over the cached one
+    statusCode: event.rewriteStatusCode ?? cachedValue.meta?.status ?? 200,
     body: toReadableStream(body, false),
     isBase64Encoded: false,
     headers: {
@@ -179,7 +187,7 @@ function decodePathParams(pathname: string): string {
 }
 
 export async function cacheInterceptor(
-  event: InternalEvent,
+  event: MiddlewareEvent,
 ): Promise<InternalEvent | InternalResult> {
   if (
     Boolean(event.headers["next-action"]) ||
@@ -287,7 +295,8 @@ export async function cacheInterceptor(
 
           return {
             type: "core",
-            statusCode: cachedData.value.meta?.status ?? 200,
+            statusCode:
+              event.rewriteStatusCode ?? cachedData.value.meta?.status ?? 200,
             body: toReadableStream(cachedData.value.body, isBinary),
             headers: {
               ...cacheControl,

packages/open-next/src/core/routing/middleware.ts

@@ -6,7 +6,11 @@ import {
   NextConfig,
   PrerenderManifest,
 } from "config/index.js";
-import type { InternalEvent, InternalResult } from "types/open-next.js";
+import type {
+  InternalEvent,
+  InternalResult,
+  MiddlewareEvent,
+} from "types/open-next.js";
 import { emptyReadableStream } from "utils/stream.js";
 
 import { getQueryFromSearchParams } from "../../overrides/converters/utils.js";
@@ -28,12 +32,6 @@ const middleMatch = getMiddlewareMatch(
 
 const REDIRECTS = new Set([301, 302, 303, 307, 308]);
 
-type MiddlewareEvent = InternalEvent & {
-  responseHeaders?: Record<string, string | string[]>;
-  isExternalRewrite?: boolean;
-  rewriteStatusCode?: number;
-};
-
 type Middleware = (request: Request) => Response | Promise<Response>;
 type MiddlewareLoader = () => Promise<{ default: Middleware }>;
 
@@ -198,6 +196,7 @@ export async function handleMiddleware(
     cookies: internalEvent.cookies,
     remoteAddress: internalEvent.remoteAddress,
     isExternalRewrite,
-    rewriteStatusCode: statusCode,
+    rewriteStatusCode:
+      rewriteUrl && !isExternalRewrite ? statusCode : undefined,
   } satisfies MiddlewareEvent;
 }

packages/open-next/src/types/open-next.ts

@@ -32,6 +32,12 @@ export type InternalEvent = {
   readonly remoteAddress: string;
 } & BaseEventOrResult<"core">;
 
+export type MiddlewareEvent = InternalEvent & {
+  responseHeaders?: Record<string, string | string[]>;
+  isExternalRewrite?: boolean;
+  rewriteStatusCode?: number;
+};
+
 export type InternalResult = {
   statusCode: number;
   headers: Record<string, string | string[]>;

packages/open-next/src/utils/cache.ts

@@ -39,7 +39,10 @@ export function getTagsFromValue(value?: CacheValue<"cache">) {
   }
   // The try catch is necessary for older version of next.js that may fail on this
   try {
-    return value.meta?.headers?.["x-next-cache-tags"]?.split(",") ?? [];
+    const cacheTags =
+      value.meta?.headers?.["x-next-cache-tags"]?.split(",") ?? [];
+    delete value.meta?.headers?.["x-next-cache-tags"];
+    return cacheTags;
   } catch (e) {
     return [];
   }

packages/tests-e2e/tests/appRouter/middleware.rewrite.test.ts

@@ -23,21 +23,25 @@ test("Middleware Rewrite", async ({ page }) => {
 });
 
 test("Middleware Rewrite External Image", async ({ page }) => {
-  await page.goto("/rewrite-external");
   page.on("response", async (response) => {
     expect(response.status()).toBe(200);
     expect(response.headers()["content-type"]).toBe("image/png");
     expect(response.headers()["cache-control"]).toBe("max-age=600");
     const bodyBuffer = await response.body();
     expect(validateMd5(bodyBuffer, OPENNEXT_PNG_MD5)).toBe(true);
   });
+  await page.goto("/rewrite-external");
 });
 
 test("Middleware Rewrite Status Code", async ({ page }) => {
+  page.on("response", async (response) => {
+    // Need to set up the event before navigating to the page to avoid missing it
+    // We need to check the URL here also cause there will be multiple responses (i.e the fonts, css, js, etc)
+    if (response.url() === "/rewrite-status-code") {
+      expect(response.status()).toBe(403);
+    }
+  });
   await page.goto("/rewrite-status-code");
   const el = page.getByText("Rewritten Destination", { exact: true });
   await expect(el).toBeVisible();
-  page.on("response", async (response) => {
-    expect(response.status()).toBe(403);
-  });
 });

packages/tests-unit/tests/core/routing/cacheInterceptor.test.ts

@@ -1,7 +1,7 @@
 /* eslint-disable sonarjs/no-duplicate-string */
 import { cacheInterceptor } from "@opennextjs/aws/core/routing/cacheInterceptor.js";
 import { convertFromQueryString } from "@opennextjs/aws/core/routing/util.js";
-import type { InternalEvent } from "@opennextjs/aws/types/open-next.js";
+import type { MiddlewareEvent } from "@opennextjs/aws/types/open-next.js";
 import type { Queue } from "@opennextjs/aws/types/overrides.js";
 import { fromReadableStream } from "@opennextjs/aws/utils/stream.js";
 import { vi } from "vitest";
@@ -26,14 +26,14 @@ vi.mock("@opennextjs/aws/adapters/config/index.js", () => ({
 }));
 
 vi.mock("@opennextjs/aws/core/routing/i18n/index.js", () => ({
-  localizePath: (event: InternalEvent) => event.rawPath,
+  localizePath: (event: MiddlewareEvent) => event.rawPath,
 }));
 
 type PartialEvent = Partial<
-  Omit<InternalEvent, "body" | "rawPath" | "query">
+  Omit<MiddlewareEvent, "body" | "rawPath" | "query">
 > & { body?: string };
 
-function createEvent(event: PartialEvent): InternalEvent {
+function createEvent(event: PartialEvent): MiddlewareEvent {
   const [rawPath, qs] = (event.url ?? "/").split("?", 2);
   return {
     type: "core",
@@ -45,6 +45,7 @@ function createEvent(event: PartialEvent): InternalEvent {
     query: convertFromQueryString(qs ?? ""),
     cookies: event.cookies ?? {},
     remoteAddress: event.remoteAddress ?? "::1",
+    rewriteStatusCode: event.rewriteStatusCode,
   };
 }
 
@@ -469,4 +470,72 @@ describe("cacheInterceptor", () => {
       }),
     );
   });
+
+  it("should return the rewrite status code when there is active cache", async () => {
+    const event = createEvent({
+      url: "/albums",
+      rewriteStatusCode: 403,
+    });
+    incrementalCache.get.mockResolvedValueOnce({
+      value: {
+        type: "app",
+        html: "Hello, world!",
+      },
+    });
+
+    const result = await cacheInterceptor(event);
+    expect(result.statusCode).toBe(403);
+  });
+
+  it("should return the rewriteStatusCode if there is a cached status code", async () => {
+    const event = createEvent({
+      url: "/albums",
+      rewriteStatusCode: 203,
+    });
+    incrementalCache.get.mockResolvedValueOnce({
+      value: {
+        type: "app",
+        html: "Hello, world!",
+        meta: {
+          status: 404,
+        },
+      },
+    });
+
+    const result = await cacheInterceptor(event);
+    expect(result.statusCode).toBe(203);
+  });
+
+  it("should return the cached status code if there is one", async () => {
+    const event = createEvent({
+      url: "/albums",
+    });
+    incrementalCache.get.mockResolvedValueOnce({
+      value: {
+        type: "app",
+        html: "Hello, world!",
+        meta: {
+          status: 405,
+        },
+      },
+    });
+
+    const result = await cacheInterceptor(event);
+    expect(result.statusCode).toBe(405);
+  });
+
+  it("should return 200 if there is no cached status code, nor a rewriteStatusCode", async () => {
+    const event = createEvent({
+      url: "/albums",
+    });
+    incrementalCache.get.mockResolvedValueOnce({
+      value: {
+        type: "app",
+        html: "Hello, world!",
+      },
+    });
+
+    const result = await cacheInterceptor(event);
+    expect(result.statusCode).toBe(200);
+  });
 });