fix: add ForbiddenBoundary/UnauthorizedBoundary for HTTP access fallback recovery (#764)#945
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: ForbiddenBoundary/UnauthorizedBoundary for HTTP access fallback recovery
The overall approach is correct and follows the existing NotFoundBoundary pattern consistently. The routing discovery, codegen pipeline, boundary components, and wiring all line up. A few issues worth addressing:
Issues
-
Tests don't exercise the actual boundary classes — The new
ForbiddenBoundary/UnauthorizedBoundarytests only test string operations on digest values. They don't import or callgetDerivedStateFromErroron the actual boundary classes (unlike the existingErrorBoundaryInnertests above them in the same file). A typo in the digest check inside the boundary class would not be caught. -
Route-level boundary nesting order (pre-existing, worsened here) — At the route level,
ErrorBoundaryis innermost while the access fallback boundaries wrap it from outside. In Next.js,ErrorBoundaryis always outermost. This was already wrong forNotFoundBoundarybefore this PR, but adding two more boundaries outside ErrorBoundary amplifies the divergence. Not a blocker, but worth a follow-up. -
Minor:
forbiddenPaths/unauthorizedPathsproduce empty arrays vs null-padded — When no forbidden/unauthorized files exist, the generatedforbiddens: []is empty, whilenotFounds: [null]has one null per layout. Safe at runtime but inconsistent. Comes from the fields being optional onAppRoute. -
Inaccurate ported-from comment — The test file header references
cache-components.test.tswhich is unrelated.
| expect(digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;")).toBe(true); | ||
| // The ForbiddenBoundary checks for digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;403") | ||
| expect(digest).toBe("NEXT_HTTP_ERROR_FALLBACK;403"); | ||
| }); |
There was a problem hiding this comment.
These tests only verify string operations — they don't actually import or call ForbiddenBoundaryInner.getDerivedStateFromError. Compare with the ErrorBoundary digest classification (actual class) tests above (line 120), which import the real class and call getDerivedStateFromError directly.
As written, a bug in the boundary's digest matching (e.g. checking startsWith("NEXT_HTTP_ERROR_FALLBACK;40") instead of 403) would not be caught.
Please add tests that import the actual ForbiddenBoundaryInner and UnauthorizedBoundaryInner classes and test their getDerivedStateFromError static methods, verifying:
- 403 digest → returns
{ forbidden: true } - 401 digest → re-throws
- 404 digest → re-throws
- No-digest error → re-throws
(And the symmetric tests for UnauthorizedBoundaryInner.)
| * | ||
| * These test the same digest-based error routing that Next.js uses | ||
| * to distinguish between notFound(), redirect(), forbidden(), and | ||
| * genuine application errors. | ||
| * Ported from Next.js: test/e2e/app-dir/cache-components/cache-components.test.ts | ||
| * https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/cache-components/cache-components.test.ts |
There was a problem hiding this comment.
This Ported from comment references cache-components.test.ts, which has nothing to do with error boundary digest classification. The original comment ("These test the same digest-based error routing that Next.js uses...") was more accurate. Please fix or remove the ported-from attribution.
| * | |
| * These test the same digest-based error routing that Next.js uses | |
| * to distinguish between notFound(), redirect(), forbidden(), and | |
| * genuine application errors. | |
| * Ported from Next.js: test/e2e/app-dir/cache-components/cache-components.test.ts | |
| * https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/cache-components/cache-components.test.ts | |
| * Ported from Next.js error boundary patterns. | |
| * See: packages/next/src/client/components/http-access-fallback/error-boundary.tsx |
| static getDerivedStateFromError(error: Error): Partial<ForbiddenBoundaryState> { | ||
| if (error && typeof error === "object" && "digest" in error) { | ||
| const digest = String(error.digest); | ||
| if (digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;403")) { |
There was a problem hiding this comment.
Using startsWith means NEXT_HTTP_ERROR_FALLBACK;4030 (or any string with a 403 prefix followed by more characters) would also match as a forbidden error. Since Next.js currently only uses exact status codes, this isn't a real issue today — but an exact equality check would be more defensive:
| if (digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;403")) { | |
| if (digest === "NEXT_HTTP_ERROR_FALLBACK;403") { |
Same applies to the UnauthorizedBoundary on line 268. Note that NotFoundBoundary (line 135) has the same pattern with startsWith, so this is consistent with the existing code — but all three could benefit from exact matching.
Non-blocking.
| const forbiddenComponent = | ||
| getDefaultExport(options.route.forbidden) ?? getDefaultExport(options.rootForbiddenModule); | ||
| if (forbiddenComponent) { | ||
| const ForbiddenComponent = forbiddenComponent; | ||
| routeChildren = ( | ||
| <ForbiddenBoundary fallback={<ForbiddenComponent />}>{routeChildren}</ForbiddenBoundary> | ||
| ); | ||
| } | ||
|
|
||
| const unauthorizedComponent = | ||
| getDefaultExport(options.route.unauthorized) ?? | ||
| getDefaultExport(options.rootUnauthorizedModule); | ||
| if (unauthorizedComponent) { | ||
| const UnauthorizedComponent = unauthorizedComponent; | ||
| routeChildren = ( | ||
| <UnauthorizedBoundary fallback={<UnauthorizedComponent />}> | ||
| {routeChildren} | ||
| </UnauthorizedBoundary> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Pre-existing concern, not a blocker for this PR: The route-level nesting puts ErrorBoundary (line 573) inside the access fallback boundaries, yielding:
Unauthorized > Forbidden > NotFound > ErrorBoundary > Loading > children
But in Next.js, ErrorBoundary is always outermost at every level:
ErrorBoundary > Loading > HTTPAccessFallbackBoundary(404+403+401) > children
The per-layout order (line 646) is correct. This mismatch was pre-existing for NotFoundBoundary, but adding two more boundaries outside ErrorBoundary amplifies the divergence. In practice each boundary only catches its own specific digest so cross-contamination is unlikely — but worth a follow-up to align.
| * Forbidden component paths per layout level (aligned with layouts array). | ||
| * Each entry is the forbidden.tsx at that layout's directory, or null. | ||
| * Used to create per-layout ForbiddenBoundary. Optional to simplify test route construction. | ||
| */ |
There was a problem hiding this comment.
Minor inconsistency: notFoundPaths (line 106) is non-optional and always produces [null, null, ...] aligned with layouts. These new fields are optional, producing undefined at the AppRoute level and [] in codegen (via route.forbiddenPaths || []). The runtime handles this safely via ?.[index] ?? null, but making these non-optional like notFoundPaths would be more consistent and would produce forbiddens: [null] instead of forbiddens: [] in the generated code.
Non-blocking — functionally correct as-is.
|
The review has been posted successfully. Here's a summary of what I found: Review Summary for PR #945The PR correctly adds
|
cloudflare#945) - Export ForbiddenBoundaryInner and UnauthorizedBoundaryInner for testing - Strengthen tests to import and call getDerivedStateFromError directly - Use exact equality instead of startsWith for status code digest matching - Make forbiddenPaths and unauthorizedPaths non-optional on AppRoute (matching notFoundPaths) - Fix inaccurate ported-from attribution in test header
|
/bigbonk review |
There was a problem hiding this comment.
Follow-up review
Good work addressing the previous round of feedback. The tests now exercise the actual boundary classes with getDerivedStateFromError, forbiddenPaths/unauthorizedPaths are non-optional, and the digest matching uses exact === equality. CI is all green.
Blocking
- Duplicate
forbiddenPaths/unauthorizedPathsiteration in import registration — Inapp-rsc-entry.ts, lines 185-190 iterate both arrays, then the pre-existing code at lines 192-198 iterates them again.getImportVaris idempotent so no incorrect output, but this is dead code that should be removed. Delete lines 185-190 (the newly added block before theif (route.forbiddenPath)line).
Non-blocking
- Stale nesting order comment —
app-page-route-wiring.tsx:618still saysLayout > Template > Error > NotFound > childrenbut the actual per-layout order is nowLayout > Template > Error > Unauthorized > Forbidden > NotFound > children. Not in the diff so I can't leave an inline suggestion, but worth updating.
| for (const fp of route.forbiddenPaths) { | ||
| if (fp) getImportVar(fp); | ||
| } | ||
| for (const up of route.unauthorizedPaths) { | ||
| if (up) getImportVar(up); | ||
| } |
There was a problem hiding this comment.
These six lines are duplicates. The pre-existing loops just below (lines 192-198) already iterate route.forbiddenPaths and route.unauthorizedPaths for import registration. Since getImportVar is idempotent this doesn't cause incorrect output, but it's dead code.
Delete this block. The only difference between the new and old loops is || [] — which is no longer needed now that both fields are non-optional on AppRoute. You can also remove the || [] from lines 192 and 196 as a cleanup.
| for (const fp of route.forbiddenPaths) { | |
| if (fp) getImportVar(fp); | |
| } | |
| for (const up of route.unauthorizedPaths) { | |
| if (up) getImportVar(up); | |
| } |
|
The review was posted successfully. Here's a summary of what I found: Review Summary for PR #945 (follow-up)The author addressed the key feedback from the first review round:
Blocking issue found: Duplicate iteration of Non-blocking: The nesting order comment at |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Round 3
All CI checks pass. The prior review feedback has been addressed well — boundary tests now exercise the actual getDerivedStateFromError methods, forbiddenPaths/unauthorizedPaths are non-optional on AppRoute, digest matching uses exact ===, and the duplicate iteration issue from round 2 doesn't exist in this commit (looks like it was from a stale diff). The implementation is correct and follows the established NotFoundBoundary pattern.
Two remaining issues (non-blocking) and one nit.
Non-blocking: NotFoundBoundary digest matching inconsistency (pre-existing)
NotFoundBoundary (error-boundary.tsx:135) still uses startsWith("NEXT_HTTP_ERROR_FALLBACK;404") while the new ForbiddenBoundary and UnauthorizedBoundary use exact === matching. This means NotFoundBoundary would match NEXT_HTTP_ERROR_FALLBACK;4040 while the other two boundaries would correctly reject 4030/4010. Not caused by this PR, but now that all three boundaries are side-by-side in the same file, the inconsistency is visible. Worth a follow-up to align NotFoundBoundary to exact matching.
Non-blocking: Test fixture data doesn't exercise per-layout forbidden/unauthorized
See inline comment on tests/entry-templates.test.ts.
Nit: Property ordering inconsistency
See inline comment on app-rsc-entry.ts.
| layoutErrorPaths: [null], | ||
| notFoundPath: null, | ||
| notFoundPaths: [null], | ||
| forbiddenPaths: [], |
There was a problem hiding this comment.
Non-blocking: Test fixture data has forbiddenPaths: [] and unauthorizedPaths: [] while notFoundPaths: [null] (one null per layout). In real routing, discoverBoundaryFilePerLayout always returns layouts.map(...), so for a route with one layout it would produce [null], not [].
This means the test data doesn't fully exercise the per-layout forbidden/unauthorized boundary wiring in createAppPageLayoutEntries — the empty array means no forbiddenModule/unauthorizedModule entries are ever created in layout entries, so the per-layout boundary wrapping code is only covered through notFound.
Worth adding at least one test fixture route with forbiddenPaths: [null] / unauthorizedPaths: [null] (matching the layout count) to verify the codegen produces the expected null-padded output. Could be a follow-up.
| notFound: ${route.notFoundPath ? getImportVar(route.notFoundPath) : "null"}, | ||
| notFounds: [${notFoundVars.join(", ")}], | ||
| forbidden: ${route.forbiddenPath ? getImportVar(route.forbiddenPath) : "null"}, | ||
| forbiddens: [${forbiddenVars.join(", ")}], |
There was a problem hiding this comment.
Nit: Property ordering is now notFound → notFounds (singular first) but forbiddens → forbidden and unauthorizeds → unauthorized (plural first). For consistency, consider matching the notFound/notFounds ordering for all three. Not a functional issue.
|
The review has been posted successfully. Here's a summary: Review Summary for PR #945 (Round 3)All CI checks pass (all 19 checks green). The author has addressed the feedback from rounds 1 and 2 well:
I left three non-blocking items:
No blocking issues — this PR looks ready to merge. |
…ack recovery (cloudflare#764) Per-layout forbidden/unauthorized boundary files (forbidden.tsx, unauthorized.tsx) are now discovered by the router and wired into the App Router component tree via dedicated ForbiddenBoundary and UnauthorizedBoundary client components. Previously, only NotFoundBoundary existed, causing 403/401 errors thrown during RSC rendering (e.g. inside Suspense boundaries) to escape into the generic error shell — this could produce mismatched Flight/HTML data and client-side 'Connection closed' failures. Next.js upstream: vercel/next.js#92231 vercel/next.js#92231
cloudflare#945) - Export ForbiddenBoundaryInner and UnauthorizedBoundaryInner for testing - Strengthen tests to import and call getDerivedStateFromError directly - Use exact equality instead of startsWith for status code digest matching - Make forbiddenPaths and unauthorizedPaths non-optional on AppRoute (matching notFoundPaths) - Fix inaccurate ported-from attribution in test header
…e comment Removes dead code in app-rsc-entry.ts (lines 185-190) that duplicated the existing import registration loops. Also updates the stale nesting order comment in app-page-route-wiring.tsx to include Unauthorized and Forbidden boundaries.
…Unauthorized patterns - Use exact === instead of startsWith in NotFoundBoundary digest check - Add forbiddenPaths/unauthorizedPaths per-layout test fixtures - Fix property ordering: singular before plural for all boundary types
- Use buildAppRscManifestCode for manifest generation (matching main) - Remove redundant || [] guards for non-optional forbiddenPaths/unauthorizedPaths - Fix test fixture indices to match new module import count
Divkix
force-pushed
the
fix/764-prerender-http-error-boundaries
branch
from
8b2efb1 to
9b01172
Compare
…aths in manifest TypeError: route.notFoundPaths is not iterable when routes lack per-layout boundary files. CI was failing on app-router.test.ts with the error. Added null guards in both registerRouteModules() and buildRouteEntries() to handle undefined arrays.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Round 4
All 19 CI checks pass. All feedback from rounds 1-3 has been addressed:
- Tests exercise actual boundary classes via
getDerivedStateFromError(round 1) forbiddenPaths/unauthorizedPathsare non-optional onAppRoute, producing consistent null-padded arrays matchingnotFoundPaths(round 1, round 3)- Digest matching uses exact
===for all three boundaries, including theNotFoundBoundaryfix fromstartsWith(rounds 1, 3 — the round 3 reviewer missed that this PR already fixes theNotFoundBoundaryinconsistency) - No duplicate iteration in
registerRouteModules(round 2) - Per-layout nesting comment updated to reflect the new boundary order (round 2)
- Test fixtures now include forbidden/unauthorized paths for the dashboard route (round 3)
The implementation correctly mirrors the NotFoundBoundary pattern across all layers: routing discovery (discoverBoundaryFilePerLayout), codegen (app-rsc-manifest.ts), entry generation (app-rsc-entry.ts), and component tree construction (app-page-route-wiring.tsx). The boundary components themselves are structurally identical to NotFoundBoundaryInner with the appropriate digest and state field names.
One minor observation (not blocking): the entry-templates.test.ts metadata route assertion at line 275 was changed from checking a specific mod_11 to a regex match (/module: mod_\d+/). This is a reasonable adjustment since adding new imports shifts module numbering, but it does make the assertion slightly weaker — it no longer validates the exact import slot. Fine for now since the snapshot tests provide full coverage.
LGTM.
|
The review has been posted successfully. Here's a summary: Review Summary for PR #945 (Round 4) — ApprovedAll 19 CI checks pass. All prior feedback from rounds 1-3 has been addressed:
The implementation correctly mirrors the |
Move ErrorBoundary after NotFoundBoundary/ForbiddenBoundary/ UnauthorizedBoundary to match Next.js nesting order (Error > Unauthorized > Forbidden > NotFound). Resolves merge conflict with bc70e1c.
|
Merge conflicts resolved, should be good. |
2 participants
Fixes #764
Summary
ForbiddenBoundaryandUnauthorizedBoundaryclient components (matchingNotFoundBoundary) that catchNEXT_HTTP_ERROR_FALLBACK;403andNEXT_HTTP_ERROR_FALLBACK;401digests during RSC renderingbuildAppPageElementsforbiddenPaths/unauthorizedPathsdiscovery in the router and codegen pipelineUpstream
Next.js PR: vercel/next.js#92231
Test plan