fix(app-router): reject cyclic Flight payloads in server actions#798
Conversation
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Review
The security fix and dependency bumps look correct and well-tested. A few observations:
What's good:
- The version bump to
react/react-dom/react-server-dom-webpack19.2.5and@vitejs/plugin-rsc0.5.23correctly enforces the upstream CVE-2026-23869 decoder fix as a dependency floor - The
validateServerActionPayloadguard runs beforedecodeReply, which is exactly where it needs to be to prevent the PoC from triggering expensive deserialization - The vendored decoder regression tests in
init.test.ts(expectConsumedBeforeInitialization) are a smart belt-and-suspenders approach — they'll catch if a futureplugin-rscupdate accidentally ships without the React fix - The cycle detection algorithm (standard DFS with gray/black coloring via
stack/visitedsets) is correct - Non-numeric FormData keys are properly skipped, so legitimate user form fields won't trigger false positives
- The generated entry wiring test verifies ordering (
validateServerActionPayloadbeforedecodeReply) at the template level - The integration test in
app-router.test.tsusesAbortSignal.timeout(2_000)which is good defensive practice — if the guard somehow fails, the test won't hang - Dev/prod parity is maintained since the App Router prod server delegates to the built RSC entry
One minor observation (non-blocking):
collectRefs unconditionally adds entries to fieldRefs even when a numeric field contains zero container references (empty Set). This means a FormData payload with numeric fields containing no $Q/$W/$i patterns (e.g., body.set("0", '"hello"')) will still pass through the missing-reference and cycle-detection checks unnecessarily. It's harmless — empty sets have no refs to fail either check — but adding a if (refs.size > 0) guard before fieldRefs.set(fieldKey, refs) would skip the validation entirely for payloads that have no container references, matching the early fieldRefs.size === 0 bail-out more precisely. Not a correctness issue, just a minor clarity/performance point.
LGTM — good work catching that the upstream version bumps alone weren't sufficient for vinext's action path.
|
Review posted and PR approved. Here's a summary of my findings: Overall: LGTM — this is a well-structured security fix. The PR does three things:
I noted one minor non-blocking observation about |
Summary
react/react-dom/react-server-dom-webpack19.2.5and@vitejs/plugin-rsc0.5.23, matching the upstream CVE-2026-23869 decoder updatesdecodeReply, since vinext's App Router action path still reproduces the PoC without an early guard even after the upstream version bumpsTesting
vp test run tests/request-pipeline.test.tsvp test run tests/init.test.tsvp test run tests/app-router.test.ts -t "payload validation"vp test run tests/app-router.test.ts -t "cyclic multipart server action payloads"References