From 51e86d7572a9788b8c935836c66e48049374b718 Mon Sep 17 00:00:00 2001 From: Magnus Dahl Eide Date: Wed, 3 Sep 2025 19:38:38 +0200 Subject: [PATCH] fix: Ensure middleware rewrite status code is properly propagated in cache interceptor changeset fix test fix test refactor deletion of cache tags header review update test --- .changeset/angry-ways-approve.md | 5 ++ packages/open-next/src/adapters/middleware.ts | 5 +- packages/open-next/src/core/requestHandler.ts | 2 +- .../src/core/routing/cacheInterceptor.ts | 21 +++-- .../open-next/src/core/routing/middleware.ts | 15 ++-- packages/open-next/src/types/open-next.ts | 6 ++ packages/open-next/src/utils/cache.ts | 5 +- .../appRouter/middleware.rewrite.test.ts | 12 ++- .../core/routing/cacheInterceptor.test.ts | 77 ++++++++++++++++++- 9 files changed, 123 insertions(+), 25 deletions(-) create mode 100644 .changeset/angry-ways-approve.md diff --git a/.changeset/angry-ways-approve.md b/.changeset/angry-ways-approve.md new file mode 100644 index 000000000..7395419ff --- /dev/null +++ b/.changeset/angry-ways-approve.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/aws": patch +--- + +fix: Ensure middleware rewrite status code is properly propagated to cache interceptor diff --git a/packages/open-next/src/adapters/middleware.ts b/packages/open-next/src/adapters/middleware.ts index 8d247d303..68956eb27 100644 --- a/packages/open-next/src/adapters/middleware.ts +++ b/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; }, diff --git a/packages/open-next/src/core/requestHandler.ts b/packages/open-next/src/core/requestHandler.ts index 51becb872..b66eda577 100644 --- a/packages/open-next/src/core/requestHandler.ts +++ b/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, diff --git a/packages/open-next/src/core/routing/cacheInterceptor.ts b/packages/open-next/src/core/routing/cacheInterceptor.ts index 69a3498d2..e4ef5cf85 100644 --- a/packages/open-next/src/core/routing/cacheInterceptor.ts +++ b/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 { 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, diff --git a/packages/open-next/src/core/routing/middleware.ts b/packages/open-next/src/core/routing/middleware.ts index 94fb3a0f2..4b42bd912 100644 --- a/packages/open-next/src/core/routing/middleware.ts +++ b/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; - isExternalRewrite?: boolean; - rewriteStatusCode?: number; -}; - type Middleware = (request: Request) => Response | Promise; 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; } diff --git a/packages/open-next/src/types/open-next.ts b/packages/open-next/src/types/open-next.ts index 53023d5ae..5c277ddf4 100644 --- a/packages/open-next/src/types/open-next.ts +++ b/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; + isExternalRewrite?: boolean; + rewriteStatusCode?: number; +}; + export type InternalResult = { statusCode: number; headers: Record; diff --git a/packages/open-next/src/utils/cache.ts b/packages/open-next/src/utils/cache.ts index bfc2fd781..f4e6ca70f 100644 --- a/packages/open-next/src/utils/cache.ts +++ b/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 []; } diff --git a/packages/tests-e2e/tests/appRouter/middleware.rewrite.test.ts b/packages/tests-e2e/tests/appRouter/middleware.rewrite.test.ts index 7a56ac49b..07a227666 100644 --- a/packages/tests-e2e/tests/appRouter/middleware.rewrite.test.ts +++ b/packages/tests-e2e/tests/appRouter/middleware.rewrite.test.ts @@ -23,7 +23,6 @@ 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"); @@ -31,13 +30,18 @@ test("Middleware Rewrite External Image", async ({ page }) => { 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); - }); }); diff --git a/packages/tests-unit/tests/core/routing/cacheInterceptor.test.ts b/packages/tests-unit/tests/core/routing/cacheInterceptor.test.ts index 8e9ef20e4..2320edf80 100644 --- a/packages/tests-unit/tests/core/routing/cacheInterceptor.test.ts +++ b/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 + Omit > & { 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, }; } @@ -452,4 +453,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); + }); });