Repository navigation
fix(actions): authorize server references in fetch action arguments - #3991
Conversation
|
/bigbonk review this PR at head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| loadServerAction, | ||
| validateActionReferences(__argumentActionIds) { | ||
| return __areServerActionsOwnedByRoute( | ||
| __VINEXT_ACTION_OWNERS(), |
There was a problem hiding this comment.
Development still accepts cross-route action references here. index.ts deliberately supplies actionOwners: null for serve mode, and areServerActionsOwnedByRoute(null, ...) returns true, while areServerReferencesRegistered() only proves that the referenced export is a registered action. Thus a fetch action on route A can still receive a registered action owned only by route B in dev, even though Next's createServerModuleMap() filters against the current workStore.page and this PR rejects the same payload in production. Please provide route-aware ownership data in dev (and cover both fetch and progressive paths), rather than treating registration as ownership.
There was a problem hiding this comment.
Declining for this PR: route ownership is not tracked in dev at all today. actionOwners is null in serve mode, so directly invoked actions are not owner-checked or forwarded in dev either. Adding dev ownership would mean new dev-time module-graph tracking for every action path, which is outside this fix. In dev, this PR still rejects every argument reference that is not a registered Server Action, including non-action exports such as a page's default. Cross-route ownership is enforced in production via the owner manifest. I've listed this under Known limitations in the PR description.
|
Review: 1 findings. Posted 1 inline warning. Targeted unit tests pass: 123/123. |
Performance benchmarksCompared 1 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
decodeReply() resolves `$h` argument references through the RSC server manifest, which accepts any export of a server-reference module. A crafted fetch action body could therefore pass a page's default export or another route's Server Action into an action as a callable argument, which Next.js never decodes. Scan the reply body for server references before decoding, resolving their metadata the way React does (including async-iterable entries that only parse inside an iterator result), and reject the request as action-not-found unless every reference is a Server Action owned by the current route (production owner manifest) and loads as a registered Server Action (dev). Apply the same registration check to bound references in progressive `$ACTION_REF_` submissions.
3cc7413 to
bdc24e8
Compare
|
/bigbonk review this PR at head |
|
✅ Approved — No issues found. The scoped server manifest makes every argument reference fail closed when its id is not a Server Action of the current route, and it reuses the owner check that already guards the header action id. The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🔒 Security: ✅ LGTMThe scoped server manifest does what the pull request describes:
📖 AGENTS.md: ✅ LGTMThe change follows the repository guidelines. The fix is checked against Next.js's page-scoped server module map, and the runtime behaviour lives in a 📋 Code Quality: ✅ LGTMThe new logic sits in the typed ⚡ Performance: ✅ LGTMEach action request adds one proxy and one owner check per server-reference lookup. Both run only on the Server Action path, so the common request path is unaffected. 📝 Documentation: ✅ LGTMThe new code comments explain why the manifest is scoped and why a denied lookup throws instead of returning nothing. Reviewed commit: |
| const entries: Iterable<[string, FormDataEntryValue]> = | ||
| typeof body === "string" ? [["0", body]] : body; | ||
| for (const [key, value] of entries) { | ||
| if (!/^\d+$/.test(key)) continue; |
There was a problem hiding this comment.
Actual React stream chunks never have numeric keys: the client encoder appends them under S<streamId> (for example S1), and the server decoder explicitly collects S entries into streamChunks before feeding them to $R/$x. This numeric-only filter therefore skips every real stream entry, so an unauthorized $h nested in a readable stream or async iterable reaches decodeReply() without being checked. The new tests use numeric key 1, which the real decoder does not associate with $R1/$x1; please scan S<id> entries using the React stream framing and update the tests to use the actual wire keys.
There was a problem hiding this comment.
Fixed in 1f558ee by removing the raw-body scanner. React's own decodeReply() now resolves argument references through a route-scoped server manifest, so stream parts and repeated parts go through the owner check wherever React reads them. The e2e covers S1 stream/async-iterable parts and a repeated part.
| if (typeof value !== "string") continue; | ||
| const model = tryParseJson(value); | ||
| if (model) { | ||
| if (isFirstEntry) models.set(key, model.value); |
There was a problem hiding this comment.
The React decoder stores ordinary string parts with Map.set() for every FormData entry, so duplicate keys are last-write-wins. This scanner instead retains the first parsed value in models. An attacker can append safe metadata under key 1 first and unauthorized metadata second: validation resolves $h1 to the safe ID, while decodeReply() resolves it to the final unauthorized export. Please either reject duplicate ordinary parts or mirror the React last-write-wins behavior, with a regression test.
There was a problem hiding this comment.
Fixed in 1f558ee by removing the raw-body scanner. React's own decodeReply() now resolves argument references through a route-scoped server manifest, so stream parts and repeated parts go through the owner check wherever React reads them. The e2e covers S1 stream/async-iterable parts and a repeated part.
|
Since last review: 1 resolved, 0 still open, 2 new. Posted 2 inline blocking findings. |
…anifest Replace the raw reply scanner with a route-scoped server manifest passed to React's decodeReply(), so every server reference React resolves goes through the action-owner check without vinext mirroring the reply wire format.
|
/bigbonk review this PR at head |
| // payload carries it, so that module never loads. | ||
| const serverManifest = new Proxy(options.createServerManifest(), { | ||
| get(target, id, receiver) { | ||
| if (typeof id === "string" && !options.validateActionReferences([id])) return undefined; |
There was a problem hiding this comment.
This can still be bypassed through React's module-id fallback. For metadata id a#owned#default, React first reads manifest["a#owned#default"] (denied), then retries manifest["a#owned"] while retaining default as the export name. Since a#owned is route-owned, this trap returns its module metadata and React loads that module's default export. I confirmed the vendored decoder performs the two accesses in that order (["abc#owned#default","abc#owned"]). Please fail the denied lookup without allowing fallback (for example, throw from the trap), and add a regression using ${app.actionIds.inlineArgument}#default; the current module#default test does not exercise this path.
There was a problem hiding this comment.
Fixed in a5a4e2f: a denied lookup now throws from the manifest trap, so React never retries the module-id fallback. Added a ${app.actionIds.inlineArgument}#default e2e case, which returned 200 on the previous head and is rejected now.
|
Since last review: 2 resolved, 0 still open, 1 new. Posted 1 inline blocking finding. |
|
/bigbonk review this PR at head |
|
Since last review: 1 resolved, 0 still open, 0 new. LGTM! |
1 participant
Summary
Fetch Server Actions (
Next-Actionheader) pass the request body to React'sdecodeReply(). A$hvalue in that body becomes a server reference, and plugin-rsc'sdecodeReply()resolves it through a server manifest that accepts any export of a server-reference module. A crafted body could therefore hand an action a callable argument that is not a Server Action of the current route, for example:defaultexport (any module with an inline"use server"action is a server-reference module).The header action id was already checked against the action-owner manifest. Nested argument references were not.
Next.js resolves argument references through a server module map scoped to the current page (
createServerModuleMap()readsworkStore.page), so neither of these decodes there.Fix
Scope the manifest instead of inspecting the payload. The fetch-action handler now calls React's own
decodeReply(body, serverManifest, options)(from plugin-rsc's vendoredreact-server-dom). TheserverManifestit passes wraps plugin-rsc'screateServerManifest()in a proxy, and the proxy returns no entry for any id thatareServerActionsOwnedByRoute()rejects for the current route. This is the same owner check already used for the header action id and for progressive$ACTION_REF_bound references.React looks up every server reference through this manifest, wherever it sits in the payload: nested arguments, bound args, promises, streams and async iterables. A denied id therefore fails to resolve and its module is never loaded. This fails the same way Next.js's page-scoped map does: React throws while decoding and the action returns its 500 error response, or, for a stream or iterable item, only that argument errors when the action reads it.
vinext no longer depends on React's reply wire format. An earlier revision of this PR scanned the raw multipart body, which broke when React 19.3 changed how it stores reply parts.
Tests
app-action-ownershipfixture): new/ownership/argument-refsroute.ReadableStream/ async-iterable items.defaultexportReadableStreamor async-iterable itemtests/app-server-action-execution.test.ts): owned references resolve through the scoped manifest; a reference from another route fails to resolve and the action never runs.Known limitations
actionOwnersisnullin serve mode). So in dev, argument references are not route-scoped, and directly invoked actions are not owner-checked or forwarded either. Production enforces ownership through the owner manifest.