fix(app-router): authorize the interception source route before rendering it#2733
Open
NathanDrake2406 wants to merge 2 commits into
Open
Conversation
…ring it Interception renders the source route's tree for the request, so one request reaches two routes: the requested target and the claimed interception source. Middleware runs once, for the target's cleanPathname, before the source is known. The source pathname arrives in the `x-vinext-interception-context` client header, so a crafted RSC request naming a middleware-guarded route under the intercepting route causes that route to render and returns its payload, while the guard never sees the path it protects. Applications using middleware path checks as their authorization boundary lose it for any route reachable as an interception source. Run middleware for the claimed source pathname before anything renders from it, and return its response when it denies. The check is skipped when the source resolves to the route already matched for this request, which is also the case where interception does not fire, so ordinary requests and requests without an interception context keep their single middleware run. Only a returned response counts as a denial. A rewritten or normalized pathname means middleware admitted the source and merely routes it elsewhere, which is a routing concern rather than an authorization one. This boundary has no upstream counterpart because the situation does not arise upstream: the generated interception rewrite targets the intercepting route and the client keeps the segments it already holds, so Next.js never renders the source route for this request and has no second route to authorize. It is vinext rendering the source tree that creates the extra boundary.
commit: |
Contributor
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Source-route middleware authorization rebuilt its request from the Server Action request directly. That transferred the body stream and left action dispatch unable to read it. Clone the body branch before changing the source URL so middleware and the action retain independent readable streams.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
This is a high severity security fix. It closes an authorization bypass where a client header causes a middleware-guarded page to render and returns its RSC payload. Same class as #2732, which covered the Route Handler variant; this one covers pages, and it reaches targets that have a direct route, so it does not depend on that PR. Please treat as release-blocking.
Overview
Why
Interception renders the source route's tree for the request. That means one request reaches two routes: the requested target, and the source named by the client. Middleware runs once, against the target's
cleanPathname, before the source is even known.The source pathname arrives in
x-vinext-interception-context, which is a client header and cannot be authenticated (the same is true ofNext-URLupstream). So an RSC request can name any route that satisfies the intercepting route's pattern, including a middleware-guarded one, and that route renders. The guard never sees the path it protects because middleware already ran for a different path.There is no upstream behaviour to mirror, because the situation does not arise upstream. Next.js' generated interception rewrite has a destination fixed at build time to the intercepting route, and
Next-URLappears only as ahasgate. The Next.js client keeps the segments it already holds and applies just the intercepted slot, so the server never renders the source route for this request and there is no second route to authorize. It is vinext rendering the source tree that creates the extra boundary, so vinext owns authorizing it.What changed
Maintainer review path
packages/vinext/src/server/app-rsc-handler.ts, theinterceptionSourceMatchblock. This is the whole change. Note the skip conditioninterceptionSourceMatch.route !== directPreActionMatch?.route: when the source resolves to the route already matched,resolveAppPageInterceptStatereturnsnoneand no interception render happens, so there is no second route and no second run.tests/app-rsc-handler.test.tsfor the denial case and the no-extra-run case.Validation
createAppRscRouteMatcherdriven throughcreateAppRscHandlerthat both reachability paths are closed: a direct-route target (/photos/1) and an interception-only target (/hidden), each with a header naming a guarded/feed/secret, return the middleware denial and never dispatch the page. An unguarded source still renders.app-rsc-handler,app-rsc-route-matching,app-server-action-execution,app-page-request,app-page-dispatch. 359 tests pass.pnpm run checkclean.interception-dynamic-single-segment,interception-dynamic-segment,interception-dynamic-segment-middleware. 18 pass, 3 fail. Those 3 fail identically on a clean checkout of this base with no changes applied, so this PR introduces no new e2e failures. See the note below.Pre-existing e2e failures on main, unrelated to this PR
Three tests in
interception-dynamic-single-segment.spec.tsfail on unmodifiedmainat the base of this branch:preserves a deeply nested dynamic source pagepreserves a consecutive dynamic source pagepreserves a static multi-segment source pageAll three assert that
#childrenshows the descendant source page after an interception navigation; instead it shows the intercepting route's own page. The spec has not changed since #2231, so this looks like a regression from a later commit rather than a broken test. Worth its own issue. Flagging it here because it is the reason the e2e run below is not fully green, and because it is the coverage that would otherwise arbitrate the parity question in Non-goals.Risk / compatibility
The material cost is an extra middleware execution, so it is called out rather than waved past.
Set-Cookie, session rotation, rate-limit counters, logging. Requests with no interception context are unaffected, as is interception that resolves to the already-matched route.Next-URL. It does not need to, because it does not render the source route. Matching upstream here would mean leaving the boundary open.Non-goals
childrensubtree, which would remove this boundary rather than guard it. In the matcher that is a small deletion (it also makesrouteIndexesdead), but it depends on client-side segment retention that vinext does not currently do, and the e2e coverage that would validate it is currently red on main (see above). Left for a follow-up once that coverage is trustworthy.References
generate-interception-routes-rewrites.tsNext-URLonly as ahasgate, source never rendered