From dc939d503c0f9eded300e37c9e26499bd84980b0 Mon Sep 17 00:00:00 2001 From: Kazim Ali Date: Mon, 17 Aug 2026 23:12:38 +1000 Subject: [PATCH 1/2] fix: render custom Pages Router 404 for getStaticProps/getServerSideProps notFound results Root cause: opennextjs-cloudflare builds a bare `NextServer` (via @opennextjs/aws's dist/core/util.js) and calls its request handler directly, without ever running a Next.js "router-server" process. Next.js's Pages Router route handler (next/dist/server/route-modules/pages/pages-handler.js) falls back to a hardcoded `"This page could not be found"` body whenever `routerServerContext.render404` is unavailable. `routerServerContext` is read from a global registry (routerServerGlobal[RouterServerContextSymbol][relativeProjectDir]) that NextNodeServer only self-populates lazily, inside `handleCatchallRenderRequest` - i.e. only once a genuinely unmatched path is hit. Any request that matches a real page whose getStaticProps/getServerSideProps returns `{ notFound: true }` can be the first request handled in a Worker isolate, before that lazy registration ever runs, so the app's actual pages/404/pages/_error is never rendered. Fix: patch NextServer#makeRequestHandler() (next/dist/server/next-server.js) to perform the same self-registration unconditionally, before the request handler is returned and therefore before any request - matched or not - can reach the route module. This reuses `this.render404`, the same method that already correctly renders the custom 404 page for genuinely unmatched paths. The registration key is hardcoded to "" (relativeProjectDir's build-time constant value) since OpenNext always constructs NextServer with dir: "". This mirrors the fix Cloudflare's vinext project shipped for the equivalent Pages Router bug: rerouting `notFound` results to the app's actual 404/error page instead of a built-in fallback. See cloudflare/vinext#1737 and cloudflare/vinext#2773 (header preservation follow-up). Adds an e2e regression test (ssr-not-found) asserting the actual 404/error page body is served, not just a 404 status code. --- .changeset/tame-pandas-render.md | 16 ++++ .../pages-router/e2e/ssr-not-found.test.ts | 23 ++++++ .../src/pages/ssr-not-found/index.tsx | 18 +++++ .../build/patches/plugins/next-server.spec.ts | 70 ++++++++++++++++ .../cli/build/patches/plugins/next-server.ts | 81 +++++++++++++++++++ 5 files changed, 208 insertions(+) create mode 100644 .changeset/tame-pandas-render.md create mode 100644 examples/e2e/pages-router/e2e/ssr-not-found.test.ts create mode 100644 examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx diff --git a/.changeset/tame-pandas-render.md b/.changeset/tame-pandas-render.md new file mode 100644 index 000000000..afe0111b3 --- /dev/null +++ b/.changeset/tame-pandas-render.md @@ -0,0 +1,16 @@ +--- +"@opennextjs/cloudflare": patch +--- + +fix: render the app's custom 404 page for Pages Router `notFound: true` results + +Register Next.js's `routerServerContext` (which provides `render404`) unconditionally +before the first request is handled, instead of relying on Next.js's own lazy +self-registration inside `handleCatchallRenderRequest`. + +Previously, when a Pages Router page's `getStaticProps`/`getServerSideProps` returned +`{ notFound: true }`, `routerServerContext.render404` was undefined for any request that +matched a real page (as opposed to a genuinely unknown path), so Next.js fell back to the +bare hardcoded `"This page could not be found"` body instead of rendering the app's actual +`pages/404`/`pages/_error`. This mirrors the same class of bug fixed for Pages Router in +Cloudflare's `vinext` project (cloudflare/vinext#1737, cloudflare/vinext#2773). diff --git a/examples/e2e/pages-router/e2e/ssr-not-found.test.ts b/examples/e2e/pages-router/e2e/ssr-not-found.test.ts new file mode 100644 index 000000000..248a45bea --- /dev/null +++ b/examples/e2e/pages-router/e2e/ssr-not-found.test.ts @@ -0,0 +1,23 @@ +import { expect, test } from "@playwright/test"; + +test("should render the app's 404 page for a getServerSideProps `notFound` result, not a bare fallback body", async ({ + page, +}) => { + // `/ssr-not-found` always returns `{ notFound: true }` from `getServerSideProps`, which runs on + // every request (unlike a `getStaticProps` page with `fallback: false`, whose `notFound` paths are + // resolved at build time). This means it can be the very first request a fresh Worker isolate + // handles - unlike a route that matches no page at all, which goes through Next's catch-all + // handling. If the router server context (and its `render404`) isn't registered before that first + // request, Next.js falls back to a bare, unstyled `"This page could not be found"` string instead + // of actually rendering the app's 404/error page - see next-server.ts's + // `registerRouterServerContextRule`. + const result = await page.goto("/ssr-not-found"); + expect(result).toBeDefined(); + expect(result?.status()).toBe(404); + + const body = await result?.text(); + // The bare fallback body is the literal, unwrapped string "This page could not be found" with no + // HTML document around it. A real render produces a full HTML document. + expect(body).toContain(""); + expect(body).not.toBe("This page could not be found"); +}); diff --git a/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx b/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx new file mode 100644 index 000000000..d4e0928f7 --- /dev/null +++ b/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx @@ -0,0 +1,18 @@ +import type { InferGetServerSidePropsType } from "next"; + +/** + * `getServerSideProps` runs on every request (unlike `getStaticProps` with `fallback: false`, + * which resolves `notFound` at build time). This makes it possible for this route to be the + * very first request handled by a fresh Worker isolate, which is what regresses if + * `routerServerContext.render404` isn't registered before the first request. + * + * See e2e/ssr-not-found.test.ts's "should render the app's 404 page for a getServerSideProps + * `notFound` result, not a bare fallback body" test. + */ +export async function getServerSideProps() { + return { notFound: true }; +} + +export default function Page({}: InferGetServerSidePropsType) { + return null; +} diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts index a86594666..0b5f336fe 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts @@ -7,6 +7,7 @@ import { createCacheHandlerRule, createComposableCacheHandlersRule, disableNodeMiddlewareRule, + registerRouterServerContextRule, } from "./next-server.js"; describe("Next Server", () => { @@ -363,6 +364,75 @@ class NextNodeServer extends _baseserver.default { `); }); + // Note: the leading single-line comments before `this.prepare().catch(...)` (as found in real + // Next.js 16.2.11 builds) are intentional - a previous version of `registerRouterServerContextRule` + // captured/reprinted the whole method body via a `$$$BODY` meta-variable, which collapsed these + // comments onto a single line and turned the rest of the method into a comment, corrupting it. + const makeRequestHandlerCode = ` +class NextNodeServer extends _baseserver.default { + // ... + makeRequestHandler() { + // This is just optimization to fire prepare as soon as possible. It will be + // properly awaited later. We add the catch here to ensure that it does not + // cause an unhandled promise rejection. The promise rejection will be + // handled later on via the \`await\` when the request handler is called. + this.prepare().catch((err)=>{ + console.error('Failed to prepare server', err); + }); + const handler = super.getRequestHandler(); + return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + } + // ... +} +`; + + test("register router server context", () => { + expect(computePatchDiff("next-server.js", makeRequestHandlerCode, registerRouterServerContextRule)) + .toMatchInlineSnapshot(` + "Index: next-server.js + =================================================================== + --- next-server.js + +++ next-server.js + @@ -1,5 +1,4 @@ + - + class NextNodeServer extends _baseserver.default { + // ... + makeRequestHandler() { + // This is just optimization to fire prepare as soon as possible. It will be + @@ -9,8 +8,28 @@ + this.prepare().catch((err)=>{ + console.error('Failed to prepare server', err); + }); + const handler = super.getRequestHandler(); + - return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + + if (!_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol]) { + + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol] = {}; + +} + +// Note: this is hardcoded to "" rather than computed via \`_path.relative(process.cwd(), this.dir)\` + +// (which is what Next.js's own self-registration in \`handleCatchallRenderRequest\` does) because + +// \`process.cwd()\` at request-handling time is not guaranteed to match \`process.cwd()\` at + +// \`NextNodeServer\` construction time in this runtime (observed to differ by one directory level, + +// e.g. yielding ".." here). The read side, \`RouteModule#getRouterServerContext\`, falls back to + +// \`this.relativeProjectDir\`, which every route module in the OpenNext build has hardcoded to "" + +// (OpenNext always constructs \`NextServer\` with \`dir: ""\`). Using "" here keeps the write side in + +// sync with that build-time constant instead of a runtime-computed value that can drift from it. + +const relativeProjectDir = ""; + +const existingServerContext = _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir]; + +if (!existingServerContext) { + + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir] = { + + render404: this.render404.bind(this) + + }; + +} + +_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; + +_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].isWrappedByNextServer = true; + +return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + } + // ... + } + " + `); + }); + test("attachRequestMeta", () => { expect(computePatchDiff("next-server.js", next15ServerCode, attachRequestMetaRule)) .toMatchInlineSnapshot(` diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts index 15e27b4d4..1f5e82a46 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts @@ -44,6 +44,8 @@ export function patchNextServer(updater: ContentUpdater, buildOpts: BuildOptions contents = patchCode(contents, attachRequestMetaRule); + contents = patchCode(contents, registerRouterServerContextRule); + return contents; }, }, @@ -145,6 +147,85 @@ fix: |- * Callstack: handleRequest-> handleRequestImpl -> attachRequestMeta * */ +/** + * Registers a `routerServerContext` (with a working `render404`) before any request is handled, + * instead of relying on Next.js's own lazy self-registration. + * + * Next.js's Pages Router falls back to a bare, hardcoded `"This page could not be found"` body + * (see `next/dist/server/route-modules/pages/pages-handler.js`) whenever a page's + * `getStaticProps`/`getServerSideProps` returns `{ notFound: true }` and no `routerServerContext.render404` + * is available - instead of rendering the app's actual `pages/404`/`pages/_error`. + * + * `routerServerContext` is read from a well-known global registry + * (`routerServerGlobal[RouterServerContextSymbol][relativeProjectDir]`, see + * `next/dist/server/lib/router-utils/router-server-context.js`) that a real `next start` router-server + * process populates upfront. `NextNodeServer` (`next/dist/server/next-server.js`) *does* also + * self-register into that same registry, but only lazily, inside `handleCatchallRenderRequest` + * (i.e. only once an unmatched/catch-all path is hit). + * + * OpenNext never runs a router-server process and constructs a bare `NextServer` directly + * (see `@opennextjs/aws`'s `dist/core/util.js`), calling `getRequestHandler()`/`makeRequestHandler()` + * once at startup. Any request that matches a real page - i.e. it never goes through + * `handleCatchallRenderRequest` - can therefore be the very first request handled in a Worker + * isolate, before Next.js's lazy self-registration has ever run. If that page's data method returns + * `notFound: true`, `routerServerContext` is `undefined` in `RouteModule#prepare()`, `render404` is + * unavailable, and Next.js falls back to the bare hardcoded body instead of the designed 404 page. + * + * We fix this by performing the same self-registration Next.js already does in + * `handleCatchallRenderRequest` (reusing `this.render404`, which correctly renders `pages/404`/ + * `pages/_error` - it's the same method that already powers 404s for genuinely unmatched paths), + * but unconditionally in `makeRequestHandler()`, which always runs before the request handler is + * returned and therefore before any request - matched or not - can reach the route module. + * + * Prior art: this mirrors the fix Cloudflare's `vinext` project shipped for the equivalent Pages + * Router bug - rerouting `notFound` results to the app's actual 404/error page instead of a + * built-in fallback - see https://github.com/cloudflare/vinext/pull/1737 and + * https://github.com/cloudflare/vinext/pull/2773 (header preservation follow-up). + */ +// Note: this deliberately does NOT capture the whole `makeRequestHandler` body via a `$$$BODY` +// meta-variable (e.g. `context: "class { makeRequestHandler() { $$$BODY } }"`). Doing so requires +// ast-grep to re-serialize the captured statements, and at least in Next.js 16.2.11's +// `next-server.js`, `makeRequestHandler`'s first statement (`this.prepare().catch(...)`) is preceded +// by several consecutive single-line (`//`) comments. Re-serializing them collapses them onto one +// line, which turns everything after the first `//` - including `this.prepare().catch(...)` itself - +// into a comment, corrupting the method and breaking the build with a syntax error. +// +// Matching only the `return (req, res, parsedUrl) => ...` statement and inserting before it avoids +// capturing/reprinting any of the method's other statements (and their leading comments) entirely. +export const registerRouterServerContextRule = ` +rule: + kind: return_statement + pattern: return $EXPR; + inside: + kind: method_definition + has: + field: name + regex: ^makeRequestHandler$ + stopBy: end +fix: |- + if (!_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol]) { + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol] = {}; + } + // Note: this is hardcoded to "" rather than computed via \`_path.relative(process.cwd(), this.dir)\` + // (which is what Next.js's own self-registration in \`handleCatchallRenderRequest\` does) because + // \`process.cwd()\` at request-handling time is not guaranteed to match \`process.cwd()\` at + // \`NextNodeServer\` construction time in this runtime (observed to differ by one directory level, + // e.g. yielding ".." here). The read side, \`RouteModule#getRouterServerContext\`, falls back to + // \`this.relativeProjectDir\`, which every route module in the OpenNext build has hardcoded to "" + // (OpenNext always constructs \`NextServer\` with \`dir: ""\`). Using "" here keeps the write side in + // sync with that build-time constant instead of a runtime-computed value that can drift from it. + const relativeProjectDir = ""; + const existingServerContext = _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir]; + if (!existingServerContext) { + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir] = { + render404: this.render404.bind(this) + }; + } + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].isWrappedByNextServer = true; + return $EXPR; +`; + export const attachRequestMetaRule = ` rule: kind: identifier From 9ac06a9f9ed4e36f432e308b1d19b437b12faf1a Mon Sep 17 00:00:00 2001 From: Kazim Ali Date: Mon, 17 Aug 2026 23:24:58 +1000 Subject: [PATCH 2/2] fix: don't set isWrappedByNextServer when registering routerServerContext Setting isWrappedByNextServer flips RouteModule#prepare() (route-module.js) from serverUtils.normalizeQueryParams(...) to serverUtils.filterInternalQuery(...), which is only correct when an upstream router-server process has already normalized the query. OpenNext has no such process, so this deleted nxtP-/nxti-prefixed routing params instead of decoding them into real page params on every request. Only render404 (and nextConfig, used elsewhere in RouteModule) is needed for the Pages Router notFound fix, so drop the isWrappedByNextServer assignment. Verified manually: /api/dynamic/[slug] resolved params correctly with the fix removed, and the ssr-not-found e2e regression test still passes. Found by Devin Review on PR #1346. --- .../src/cli/build/patches/plugins/next-server.spec.ts | 3 +-- .../cloudflare/src/cli/build/patches/plugins/next-server.ts | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts index 0b5f336fe..793d6bff4 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts @@ -399,7 +399,7 @@ class NextNodeServer extends _baseserver.default { // ... makeRequestHandler() { // This is just optimization to fire prepare as soon as possible. It will be - @@ -9,8 +8,28 @@ + @@ -9,8 +8,27 @@ this.prepare().catch((err)=>{ console.error('Failed to prepare server', err); }); @@ -424,7 +424,6 @@ class NextNodeServer extends _baseserver.default { + }; +} +_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; - +_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].isWrappedByNextServer = true; +return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); } // ... diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts index 1f5e82a46..4aa41375f 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts @@ -222,7 +222,6 @@ fix: |- }; } _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; - _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].isWrappedByNextServer = true; return $EXPR; `;