fix: stop a boundary crash leaking its message, and formatting one from throwing

Two hazards and four comments that described a mechanism the code does not
have.

The 403 / 401 / 404 catches rendered the thrown value into the response body
in PRODUCTION, so a boundary that crashed leaked its error message to the
client. The 500 path has always drawn that line, because a thrown message is
not author-controlled. Those pages now show the detail in dev and the heading
alone in prod.

Both of those catches, and the dev branch of the default 500 page, also
stringified the thrown value unguarded. String(Object.create(null)) throws on
both runtimes, and all three run inside a catch whose job is to keep the
response alive, so formatting the failure could escape the handler and turn a
handled 500 into an unhandled one. One total formatter covers all three.

The comments claimed the dedup key is the full stack. It is the stage plus
the name, message and construction site, and the same block argues against a
full-stack key three lines later. Two test comments said the same thing, one
of them on the test that would fail under the mechanism it described.

Author: vivek7405

packages/server/src/ssr/render.js

@@ -415,8 +415,30 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
 }
 
 /**
- * A per-request dedup key for a secondary boundary failure: the error's name,
- * message and FULL stack. Identity cannot be used, because a layout that
+ * Render a thrown value as text for a DEV error page, safely.
+ *
+ * `String(err)` is not total: `String(Object.create(null))` throws on both
+ * runtimes, and a getter on a subclass can throw too. Every caller here is
+ * already inside a catch whose job is to keep the response alive, so an
+ * uncaught throw while FORMATTING the failure escapes the handler and takes
+ * the error page down with it, turning a handled 500 into an unhandled one.
+ *
+ * @param {unknown} err
+ * @returns {string}
+ */
+function safeErrorText(err) {
+  try {
+    if (err instanceof Error) return String(err.stack || err.message);
+    return String(err);
+  } catch {
+    return '[unprintable value]';
+  }
+}
+
+/**
+ * A per-request dedup key for a secondary boundary failure: the STAGE it came
+ * from plus the error's name, message and construction site. Identity cannot
+ * be used, because a layout that
  * constructs its error yields a fresh object each time it is re-run, and the
  * whole point is to collapse exactly those repeats while letting a genuinely
  * different failure through.
@@ -443,6 +465,7 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
  * escape `ssrPage` and take the 500 page with it.
  *
  * @param {unknown} err
+ * @param {string} stage  Which attempt produced this throw. Part of the key.
  * @returns {string | null}
  */
 function boundaryErrorKey(err, stage) {
@@ -656,7 +679,13 @@ async function ssrBoundaryHtml(file, heading, opts) {
         }
       }
     } catch (e) {
-      body = `<h1>${heading}</h1><pre>${escapeHtml(String(e))}</pre>`;
+      // Dev shows the failure; prod shows only the heading. The 500 path has
+      // always drawn that line (a thrown error's message is not
+      // author-controlled and must not reach the client), and these pages were
+      // rendering it in production.
+      body = opts.dev
+        ? `<h1>${heading}</h1><pre>${escapeHtml(safeErrorText(e))}</pre>`
+        : `<h1>${heading}</h1>`;
       moduleUrls = [];
     }
   }
@@ -708,7 +737,9 @@ async function ssrNotFoundHtml(notFoundFile, opts) {
         }
       }
     } catch (e) {
-      body = `<h1>404: Not found</h1><pre>${escapeHtml(String(e))}</pre>`;
+      body = opts.dev
+        ? `<h1>404: Not found</h1><pre>${escapeHtml(safeErrorText(e))}</pre>`
+        : '<h1>404: Not found</h1>';
       moduleUrls = [];
     }
   }
@@ -1233,9 +1264,7 @@ export async function ssrPage(route, params, url, opts) {
     // Default: dev shows stack, prod shows a terse message (no stack trace leaks).
     console.error('[webjs] unhandled render error:', err);
     const body = opts.dev
-      ? `<h1>Server error</h1><pre style="white-space:pre-wrap">${escapeHtml(
-          err instanceof Error ? err.stack || err.message : String(err)
-        )}</pre>`
+      ? `<h1>Server error</h1><pre style="white-space:pre-wrap">${escapeHtml(safeErrorText(err))}</pre>`
       : `<h1>Server error</h1><p>Something went wrong. Please try again.</p>`;
     return htmlResponse(
       wrapInDocument(body, { metadata, moduleUrls: [], dev: opts.dev, nonce: errNonce }),

test/ssr/ssr.test.js

@@ -2186,7 +2186,8 @@ test('boundary: one throwing layout is reported ONCE, not once per boundary', as
   // The walk tries each boundary in the chain, and layoutsForBoundary selects
   // by segment ancestry rather than boundary depth, so a root layout that
   // throws fails EVERY attempt. Identity dedup would not help: a layout that
-  // constructs its error yields a fresh object per attempt, so the key is its name, message and stack.
+  // constructs its error yields a fresh object per attempt, so the key is the
+  // STAGE plus its name, message and construction site.
   const { route, appDir } = makeBoundaryApp({
     files: {
       'layout.js': `export default function Root() { throw new Error('root-layout-boom'); }\n`,
@@ -2334,9 +2335,10 @@ test('boundary: a throw whose value cannot be stringified still degrades, never
 });
 
 test('boundary: two boundaries failing through ONE shared helper are both reported', async () => {
-  // The key is the whole stack, not just the frame where the Error was
-  // constructed. Keying on that one frame would make these two collide, and
-  // global-error's own crash would be swallowed by the earlier boundary's.
+  // These two share a construction site, so the error alone cannot separate
+  // them: it is the STAGE in the key (the boundary walk versus the
+  // global-error attempt) that keeps global-error's own crash from being
+  // swallowed by the earlier boundary's.
   const { route, appDir } = makeBoundaryApp({
     files: {
       'boom.js': `export function boom() { throw new Error('shared-helper-boom'); }\n`,
@@ -2357,6 +2359,59 @@ test('boundary: two boundaries failing through ONE shared helper are both report
   assert.equal(hits.length, 2, 'the boundary and global-error each report, despite one construction site');
 });
 
+test('boundary: an unprintable throw degrades in DEV too, and never escapes', async () => {
+  // The prod path was covered; dev formats the error into the page, and that
+  // formatting is itself inside the catch that keeps the response alive.
+  const { route, appDir } = makeBoundaryApp({
+    files: {
+      'page.js': `export default function Page() { throw Object.create(null); }\n`,
+    },
+    page: 'page.js',
+    layouts: [],
+    errors: [],
+  });
+  const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/'), { dev: true, appDir }));
+  assert.equal(resp.status, 500, 'dev degrades to the 500 page rather than throwing out of ssrPage');
+  assert.match(await resp.text(), /unprintable value/, 'and says so instead of crashing while formatting');
+});
+
+test('boundary: a crashing 403 boundary does not leak its error to the client in prod', async () => {
+  // The 500 path has always drawn this line; these pages were rendering the
+  // thrown message into the response body in production.
+  const { route, appDir } = makeBoundaryApp({
+    files: {
+      'admin/forbidden.js': `export default function F() { throw new Error('SECRET_DB_DSN_LEAK'); }\n`,
+      'admin/page.js': `import { forbidden } from ${JSON.stringify(WEBJS_MODULE_URL)};\nexport default function Page() { forbidden(); }\n`,
+    },
+    page: 'admin/page.js',
+    layouts: [],
+    forbiddens: ['admin/forbidden.js'],
+  });
+  const prod = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), { dev: false, appDir }));
+  assert.equal(prod.status, 403);
+  const prodBody = await prod.text();
+  assert.ok(!prodBody.includes('SECRET_DB_DSN_LEAK'), 'prod never renders the thrown message');
+  assert.ok(prodBody.includes('403'), 'but still identifies the status');
+
+  const dev = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), { dev: true, appDir }));
+  assert.match(await dev.text(), /SECRET_DB_DSN_LEAK/, 'dev still shows it, which is the point of dev');
+});
+
+test('boundary: an unprintable throw from a 403 boundary degrades, never escapes', async () => {
+  const { route, appDir } = makeBoundaryApp({
+    files: {
+      'admin/forbidden.js': `export default function F() { throw Object.create(null); }\n`,
+      'admin/page.js': `import { forbidden } from ${JSON.stringify(WEBJS_MODULE_URL)};\nexport default function Page() { forbidden(); }\n`,
+    },
+    page: 'admin/page.js',
+    layouts: [],
+    forbiddens: ['admin/forbidden.js'],
+  });
+  const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), { dev: true, appDir }));
+  assert.equal(resp.status, 403, 'it degrades rather than escaping ssrBoundaryHtml and ssrPage');
+  assert.match(await resp.text(), /unprintable value/);
+});
+
 test('boundary: a boundary response is never storable and never reduced', async () => {
   // Two independent guarantees. The HTML cache refuses a non-200 outright, and
   // the reduced X-Webjs-Have path is structurally unreachable: the boundary