fix(app-router): action redirects#910
fix(app-router): action redirects#910NathanDrake2406 wants to merge 5 commits intocloudflare:mainfrom
Conversation
commit: |
|
@james-elicx Sorry for the PR dump. I switched from Claude to Codex and lost all restraint. The code reads pretty good to me, curious about your impression on the code quality of these pr |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice extraction — splitting the progressive-enhancement path into its own helper (server/app-server-action-execution.ts) keeps the generated entry thin, which matches the project guidance for entries.
The core behavior changes match Next.js:
- Progressive enhancement classification follows
getServerActionRequestMetadata(POST + multipart-or-urlencoded-or-action-header). - Action route-handler 303 override matches
AppRouteRouteModule.do()whereactionStore.isAction ? RedirectStatusCode.SeeOther : getRedirectStatusCodeFromError(err). - Pending cookies + draftMode cookie on route-handler redirect throws now match
appendMutableCookies(headers, requestStore.mutableCookies)in the same file.
A few notes worth addressing — most importantly the silent-success return path (the body has already been consumed when decodeAction returned a non-action or the action threw a non-redirect that we re-raise into the outer null-return path). Inline comments below.
One thing missing from the test coverage: there's no test for the case where decodeAction returns a non-function (the “could be any POST” path). Right now the helper returns null, and the request falls through to the normal page render — but the body has already been consumed. Worth at least a unit test asserting we return null in that case, and a deliberate decision/comment about whether the post-decode page render is acceptable (Next.js's MPA path only consumes the body once it has confirmed the request is an action via areAllActionIdsValid / decodeAction, so they don't have this problem).
| const action = await options.decodeAction(body); | ||
| if (typeof action !== "function") { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When decodeAction returns a non-function we return null, which causes _handleRequest to fall through to normal rendering. By this point we have already consumed the request body via readFormDataWithLimit, so any downstream code that touches request.formData() / request.body will fail.
Next.js avoids this by validating areAllActionIdsValid(formData, serverModuleMap) against its server module map before calling decodeAction, and only enters the decode path when the FormData looks like an action payload (reference). The rest of their non-action multipart POSTs never have their body read here.
At minimum, please add a test that exercises the typeof action !== 'function' branch and document the body-already-consumed implication. A follow-up to gate decoding on a cheap pre-check (analogous to areAllActionIdsValid) would be valuable.
| } | ||
|
|
||
| if (!actionRedirect) { | ||
| return null; |
There was a problem hiding this comment.
Same body-consumed issue applies here: when an MPA action runs successfully without calling redirect(), we return null and fall through to rendering the page. Next.js handles this by re-rendering the source page with the action's form-state (reference — search for decodeFormState). Today this PR's fall-through path renders the page without any action-result wiring, and formData is already consumed.
For in-scope (redirect-only) progressive enhancement the behavior is fine, but please add a comment explaining the parity gap so it doesn't get silently regressed.
| } catch (error) { | ||
| actionRedirect = getActionRedirect(error); | ||
| if (!actionRedirect) { | ||
| throw error; |
There was a problem hiding this comment.
When the action throws a non-redirect error, we re-throw out of the inner block and end up in the outer catch at line 151, which always returns 500. Two parity issues:
notFound()/forbidden()/unauthorized()from inside an MPA action: Next.js'shandleActionreturns{ type: 'not-found' }forisHTTPAccessFallbackError(err)so the appropriate HTML response is rendered. Today this PR responds 500 with “Server action failed: NEXT_NOT_FOUND” in dev.- Generic user errors: Next.js re-throws into the page render path. We unconditionally surface 500.
If full MPA parity is out of scope (per the PR description), please at least:
- Detect
NEXT_NOT_FOUND/NEXT_HTTP_ERROR_FALLBACK;<status>digests and return aResponse(null, { status })instead of 500. - Add an inline comment noting the deliberate gap so reviewers know it's intentional.
| contentType: string, | ||
| actionId: string | null, | ||
| ): boolean { | ||
| return request.method === "POST" && contentType.startsWith("multipart/form-data") && !actionId; |
There was a problem hiding this comment.
Method comparison here is request.method === "POST" (case-sensitive), but isPossibleAppRouteActionRequest (added in this PR) uses .toUpperCase(). The Fetch spec normalises method to uppercase so it should always be POST in practice, but the inconsistency between the two helpers is easy to trip over.
| return request.method === "POST" && contentType.startsWith("multipart/form-data") && !actionId; | |
| return request.method.toUpperCase() === "POST" && contentType.startsWith("multipart/form-data") && !actionId; |
| return csrfResponse; | ||
| } | ||
|
|
||
| const contentLength = parseInt(options.request.headers.get("content-length") || "0", 10); |
There was a problem hiding this comment.
Minor: the body-size fast path orders Content-Length before validateServerActionPayload, but the CSRF check happens before the Content-Length check. Next.js does CSRF first, then size, then decode (matches this ordering), so that part is fine.
One subtle inconsistency with the existing fetch-action path in app-rsc-entry.ts: that path runs validateCsrfOrigin first, then Content-Length, then stream-limit, then validateServerActionPayload. This helper does CSRF, Content-Length, stream-limit, payload-validate — same order. Good.
No change requested, just confirming the ordering is intentional.
| return ( | ||
| request.headers.has("x-rsc-action") || | ||
| request.headers.has("next-action") || | ||
| contentType === "application/x-www-form-urlencoded" || |
There was a problem hiding this comment.
Strict equality on application/x-www-form-urlencoded matches Next.js exactly (contentType === 'application/x-www-form-urlencoded'), but it means a browser that sends the (uncommon but spec-legal) application/x-www-form-urlencoded; charset=UTF-8 content-type will not be classified as an action.
Next.js has the same gap, so this is parity-correct. Worth a short comment so a future contributor doesn't "fix" the strict equality and quietly break parity.
|
|
||
| // Handle server action POST requests | ||
| const actionId = request.headers.get("x-rsc-action"); | ||
| const actionId = request.headers.get("x-rsc-action") ?? request.headers.get("next-action"); |
There was a problem hiding this comment.
Adding next-action as a fallback header: vinext's client always sends x-rsc-action (see app-browser-state.ts:125), so this fallback only helps for hand-crafted requests or third-party libraries that mimic the Next.js convention. That's fine, but please add a unit test (or an entry-template assertion) so we don't accidentally drop one of the two header names in a future refactor — right now nothing in the test suite exercises the next-action-only path.
| getAndClearPendingCookies() { | ||
| return ["session=1; Path=/"]; | ||
| }, | ||
| getDraftModeCookieHeader() { |
There was a problem hiding this comment.
response.headers.getSetCookie?.() will silently return undefined if the runtime under test doesn't expose getSetCookie, and the toEqual([...]) will fail with a confusing error. Vitest's vite-plus runtime does expose it, so this is fine in practice, but consider asserting it more directly:
| getDraftModeCookieHeader() { | |
| expect(response?.headers.getSetCookie()).toEqual(["session=1; Path=/", "draft=1; Path=/"]); |
If getSetCookie becomes unavailable for any reason, this fails loudly instead of equating undefined with the expected array.
| options.clearRequestContext(); | ||
|
|
||
| const redirectHeaders = new Headers({ | ||
| Location: new URL(actionRedirect.url, options.request.url).toString(), |
There was a problem hiding this comment.
Defense-in-depth thought: actionRedirect.url originates from user code calling redirect(...), then is resolved against request.url and put directly into a Location header. If a user passes attacker-controlled input to redirect() (e.g. redirect(formData.get('next'))), new URL("//evil.com", "https://example.com/") resolves to https://evil.com/, producing an open-redirect.
Next.js has the same surface, so this is parity-correct. The existing fetch-action code path in app-rsc-entry.ts also doesn't guard against this. Worth filing as a follow-up: route through isOpenRedirectShaped (or similar) for any Location produced from a NEXT_REDIRECT digest, both here and in the existing fetch-action path.
| const pendingCookies = options.getAndClearPendingCookies(); | ||
| const draftCookie = options.getDraftModeCookieHeader(); | ||
| const specialError = resolveAppRouteHandlerSpecialError(error, options.request.url, { | ||
| isAction: isPossibleAppRouteActionRequest(options.request), |
There was a problem hiding this comment.
isPossibleAppRouteActionRequest(options.request) is recomputed on every error path, but the caller already knows the method/headers when it dispatches into this function. It's cheap, but if you ever add a non-redirect error path that also wants the flag (e.g. for action error logging), it's worth threading isAction through ExecuteAppRouteHandlerOptions so the classification happens once at request entry. Not blocking.
|
Review posted successfully: #910 (review) Summary of feedbackPosted a single batched review with the following key points: Positive observations:
Issues raised inline:
|
|
Addressed review feedback in ff42478. What changed:
Left unchanged deliberately:
|
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for the thorough follow-up — every actionable item from the previous review has been addressed:
- Body consumption:
request.clone()beforereadFormDataWithLimitlets non-action multipart POSTs and successful non-redirect MPA actions fall through cleanly. Both paths now have unit coverage (returns null for non-action multipart posts without consuming the original body,falls through after successful non-redirect actions without consuming the original body). - HTTP fallback errors:
getActionControlResponsenow decodesNEXT_NOT_FOUNDandNEXT_HTTP_ERROR_FALLBACK;<status>into typed status responses, and the corresponding test iterates over both 404 and 403 cases. - Method casing:
isProgressiveServerActionRequestnow normalises with.toUpperCase(), matchingisPossibleAppRouteActionRequest. Test exercises lowercasepostto lock that behaviour in. - Parity comments: Strict
application/x-www-form-urlencodedequality and the deferred form-state re-render path are both now annotated as deliberate parity gaps — that's exactly the right call. - Test cleanup:
getSetCookie()no longer uses optional chaining, and thenext-actionfallback header is now exercised inapp-route-handler-policy.test.ts.
Leaving the open-redirect defense-in-depth note as a follow-up that should cover both the progressive helper and the pre-existing fetch-action path is also the right choice — patching only one site would create exactly the kind of silent dev/prod parity drift AGENTS.md warns against.
Two non-blocking observations on the latest revision below — neither needs to land in this PR.
LGTM as soon as CI is green.
| } | ||
|
|
||
| if (digest === "NEXT_NOT_FOUND" || digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;")) { | ||
| const statusCode = digest === "NEXT_NOT_FOUND" ? 404 : parseInt(digest.split(";")[1], 10); |
There was a problem hiding this comment.
Minor: parseInt(digest.split(";")[1], 10) returns NaN for a malformed NEXT_HTTP_ERROR_FALLBACK; (no status, or non-numeric). The follow-up Number.isInteger(statusCode) guard catches that and returns null, which then propagates back to the outer catch and turns into a 500 — i.e. malformed fallback digests behave identically to non-special errors. That's a defensible choice (a malformed digest is a programming error, not a user-facing case), but worth a one-line comment to make the intent explicit, or alternatively factor this digest-parsing code out and share it with resolveAppRouteHandlerSpecialError in app-route-handler-policy.ts, which has the exact same shape. Not blocking.
| // Progressive submissions can still fall through to a regular page render when | ||
| // the multipart body is not an action payload. Read a clone so that fallback | ||
| // code can still consume the original request body. | ||
| body = await options.readFormDataWithLimit( |
There was a problem hiding this comment.
Subtle reliability note for follow-up: request.clone() followed by readFormDataWithLimit(clone, …) doubles the in-memory peak for a near-limit multipart payload, because both the original and the clone retain their own buffered copy of the body until one is GC'd. For the current __MAX_ACTION_BODY_SIZE default this is fine, but if the limit is raised significantly (or applied per-route) on Workers it becomes meaningful.
A cheaper alternative would be to gate the clone behind a quick areAllActionIdsValid-style check on the parsed FormData keys (matching Next.js's action-handler.ts flow), but that requires plumbing the server module map through to this helper. Worth filing as a follow-up if Cloudflare-deployed apps start running into memory pressure on multipart POSTs.
|
Review posted: #910 (review) SummaryThe previous review's actionable items have all been addressed in commit ff42478:
Posted two non-blocking follow-up notes (malformed digest handling could be commented or factored to share with |
NathanDrake2406
changed the title
What changed
decodeAction, and return a real HTTP 303 redirect when the action callsredirect().redirect()andpermanentRedirect()return 303 instead of replay-prone 307/308 semantics.cookies()/ draft-mode cookies when a route handler throws a redirect, matching Next.js app-route behavior.Why
Browser form submissions without JavaScript send
multipart/form-dataand no RSC action header. The previous implementation only handled POSTs withx-rsc-action, so progressive-enhancement server action forms fell through to a normal render and the action was silently skipped.For route handlers,
redirect()encodes 307/308 in the digest, but Next.js overrides action-like POST redirects to 303 so the browser follows with GET instead of resubmitting the original POST.Next.js references
getServerActionRequestMetadata(): marks POSTmultipart/form-data, POSTapplication/x-www-form-urlencoded, and POST action-header requests as possible server actions.action-handler.ts: multipart non-fetch actions calldecodeAction(formData, serverModuleMap)and return through the full-page render path for progressive enhancement.app-route/module.ts: app route redirects returnRedirectStatusCode.SeeOtherwhenactionStore.isActionis true, and append mutable cookies to the redirect response.app-action-progressive-enhancement.test.ts: verifies formData + redirect without JavaScript returns 303 and navigates.app-action.test.ts: verifies POST route-handlerredirect()/permanentRedirect()flows use 303.Validation
vp checkvp test run tests/app-route-handler-policy.test.tsvp test run tests/nextjs-compat/app-routes.test.tsvp test run tests/app-router.test.ts -t "server action"vp test run tests/entry-templates.test.tsvp run vinext#buildPLAYWRIGHT_PROJECT=app-router vp exec playwright test tests/e2e/app-router/server-actions.spec.tsNote: the local pre-commit hook failed in this worktree during Vitest suite initialization before running assertions (
Cannot read properties of undefined (reading 'config')). The same snapshot update/test command passes when run directly throughvp, and the updated snapshot is committed.