fix: add x-action-forwarded guard and E2E tests for server action forwarding loop#1292
Conversation
…ng loops Defensively implements the Next.js server action forwarding loop fix. - Adds ACTION_FORWARDED_HEADER constant to headers.ts - Adds x-action-forwarded to INTERNAL_HEADERS blocklist - Adds guard to handleServerActionRscRequest and handleProgressiveServerActionRequest - Returns 404 + x-nextjs-action-not-found when header is present Ported from Next.js: vercel/next.js@20892dd
- Adds /nextjs-compat/action-forward-loop fixture with middleware rewrite - Adds /nextjs-compat/action-forward-loop-rewrite as rewrite target - Adds error boundary component using unstable_isUnrecognizedActionError - Adds E2E tests: middleware rewrite + action POST, not-found header contract
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 summary
The core server-side guard implementation is clean and correct — the header constant, INTERNAL_HEADERS inclusion, and guard placement in both handleServerActionRscRequest and handleProgressiveServerActionRequest all look good. The unit tests are well-structured and cover the right cases.
However, the E2E tests have structural problems that will cause failures and don't actually exercise the new code or the fixture error boundary.
Issues
-
First E2E test will fail at runtime. The page renders
<p id="action-result"></p>as a static empty element, and nothing populates it with the action's return value. The form usesaction={runAction}directly (notuseActionState), so React invokes the action on submit but the return value"action-ok"is discarded —#action-resultwill always be empty. The test waits for it to contain"action-ok"and will time out. -
Second E2E test doesn't test the forwarding guard. It sends a fetch with
x-rsc-action: "stale-action-id"but withoutx-action-forwarded. This exercises the pre-existing "unknown action ID → 404" path, not the new guard. Furthermore, sincex-action-forwardedis inINTERNAL_HEADERS, it would be stripped from external requests before reaching the action handler — so you can't test the guard via an external fetch frompage.evaluateanyway. -
Error boundary fixture is dead code. Neither E2E test verifies that
#action-not-found-errorrenders. Theerror-boundary.tsxfile is included but never exercised. Compare with the original Next.js test which clicks the button and assertsawait browser.waitForElementByCss('#action-not-found-error')— that's the actual user-facing behavior being tested. -
Misleading test name. "UnrecognizedActionError is caught by client error boundary" describes checking the error boundary rendering, but the test only checks a raw fetch response.
Suggestion
The Next.js test is elegant because middleware naturally triggers the forwarding path. In vinext's single-worker architecture the forwarding loop can't happen organically, but the E2E tests should still meaningfully exercise the guard. Consider:
- For the first test: either use
useActionStateto render the action result, or just test that the form submission doesn't hang (verify the page doesn't time out and the button remains interactive). - For the second test: if you want to test the
x-action-forwardedguard specifically, you'd need the header to survive past theINTERNAL_HEADERSstripping — which only works for internally-forwarded requests. Consider testing theUnrecognizedActionErrorclient contract via the stale-action-id scenario (which is a valid E2E test), but rename it to match what it actually tests.
| Run action | ||
| </button> | ||
| </form> | ||
| <p id="action-result"></p> |
There was a problem hiding this comment.
This <p> will always be empty. The form uses action={runAction} directly, so React calls runAction() on submit but discards the return value — there's no mechanism to render "action-ok" into the DOM.
The E2E test at server-actions.spec.ts:250 waits for #action-result to contain "action-ok" and will time out.
To display the action result, you'd need useActionState:
"use client";
import { useActionState } from "react";
import { ErrorBoundary } from "./error-boundary";
import { runAction } from "./actions";
export default function Page() {
const [result, formAction] = useActionState(runAction, "");
return (
<main>
<h1 id="action-forward-loop-page">Action Forward Loop Test</h1>
<ErrorBoundary>
<form action={formAction}>
<button id="run-action" type="submit">Run action</button>
</form>
<p id="action-result">{result}</p>
</ErrorBoundary>
</main>
);
}(The action signature would also need to change to accept the previous state as the first argument.)
| await expect(async () => { | ||
| const text = await page.locator("#action-result").textContent(); | ||
| expect(text).toContain("action-ok"); | ||
| }).toPass({ timeout: 10_000 }); |
There was a problem hiding this comment.
This assertion will time out because #action-result is always empty — see comment on page.tsx. The page has no client-side state management to render the action's return value.
|
|
||
| // Ported from Next.js: test/e2e/app-dir/action-forward-loop/action-forward-loop.test.ts | ||
| // https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/action-forward-loop/action-forward-loop.test.ts | ||
| test("UnrecognizedActionError is caught by client error boundary", async ({ page }) => { |
There was a problem hiding this comment.
This test name says "UnrecognizedActionError is caught by client error boundary" but the test never checks the error boundary. It only verifies the raw fetch response (404 + header). The error-boundary.tsx fixture and #action-not-found-error element are never exercised.
Also, this doesn't test the new x-action-forwarded guard — the fetch doesn't include that header. It tests the pre-existing "unknown action ID → 404" code path (which already existed before this PR).
Consider either:
- Renaming to reflect what it actually tests (e.g., "stale action ID returns 404 with action-not-found header"), or
- Actually testing the error boundary by triggering a form submit that results in
UnrecognizedActionError, matching the original Next.js test attest/e2e/app-dir/action-forward-loop/action-forward-loop.test.ts
| const res = await fetch(`${base}/nextjs-compat/action-forward-loop`, { | ||
| method: "POST", | ||
| headers: { | ||
| "x-rsc-action": "stale-action-id", |
There was a problem hiding this comment.
Note: x-action-forwarded is in INTERNAL_HEADERS and will be stripped from this external request before it reaches the action handler. So even if you added "x-action-forwarded": "1" here, the guard wouldn't fire — which is correct security behavior (prevents external forgery), but means this approach can't test the guard via E2E.
| @@ -0,0 +1,27 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
This error boundary is well-implemented (matches the Next.js test fixture), but it's currently dead code — no E2E test verifies that #action-not-found-error renders. The Next.js test exercises it by clicking the button and asserting await browser.waitForElementByCss('#action-not-found-error').
In vinext's single-worker model the action is found locally (despite the middleware rewrite), so clicking the button won't trigger UnrecognizedActionError. The error boundary fixture is useful documentation of the API surface, but the tests should acknowledge this architectural difference rather than leaving it unexercised.
| } | ||
|
|
||
| // Defensive guard: prevent infinite forwarding loops. See handleServerActionRscRequest. | ||
| if (options.request.headers.get(ACTION_FORWARDED_HEADER)) { |
There was a problem hiding this comment.
Nit: The guard placement is correct (before CSRF validation — no point validating origin if we're going to 404). One minor observation: in handleServerActionRscRequest below the guard passes options.actionId to createActionNotFoundResponse, but here it passes null because progressive actions don't have an actionId at this point. This is consistent and correct — just noting it for future readers.
|
Review submitted. Here's a summary of the feedback: Core implementation (LGTM): The E2E tests (request changes):
|
- page.tsx: use useActionState so action result renders into #action-result - actions.ts: update runAction signature for useActionState compat - Remove dead error-boundary.tsx (never exercised in single-worker model) - Rename second test to match actual behavior (stale action ID -> 404)
|
/bigbonk review |
There was a problem hiding this comment.
The previous review's feedback has been thoroughly addressed. All CI checks pass. LGTM.
What was fixed since last review:
- Page now uses
useActionStatecorrectly, so the first E2E test works (action result renders into#action-result) - Action signature updated to accept
_prevStateas first arg (matchinguseActionStatecontract) - Dead
error-boundary.tsxfixture removed - Second E2E test renamed from misleading "UnrecognizedActionError is caught by client error boundary" to accurate "stale action ID returns 404 with x-nextjs-action-not-found header"
- Honest comments added about what each E2E test actually exercises vs the architectural limitations
Core implementation is clean — header constant, INTERNAL_HEADERS inclusion for forgery protection, guard placement before CSRF validation in both handleServerActionRscRequest and handleProgressiveServerActionRequest. Unit tests cover the right cases.
One minor nit on a fixture comment below, but not blocking.
| // to /nextjs-compat/action-forward-loop-rewrite so the receiving page does not | ||
| // bundle the action. Without the x-action-forwarded guard, a multi-worker | ||
| // deployment would loop indefinitely. In vinext's single-worker model, the | ||
| // guard still fires defensively when the header is injected. |
There was a problem hiding this comment.
Nit: this comment is slightly misleading — "the guard still fires defensively when the header is injected" implies the middleware or vinext injects x-action-forwarded during this rewrite, which it doesn't. The guard only fires when a request already carries the header (which would come from an inter-worker forward in a multi-worker deployment). In vinext's single-worker model, the guard never fires organically.
Suggestion:
| // guard still fires defensively when the header is injected. | |
| // guard still applies as defense-in-depth if the header is ever present. |
|
Review submitted. Here's a summary: Approved — the previous review's feedback has been thoroughly addressed and all CI checks pass. Key changes since last review:
Core implementation was already solid from the start — the header constant, One minor nit on a middleware fixture comment that's slightly misleading about when the guard fires, but not blocking. |
2 participants
Summary
Defensively implements the Next.js server action forwarding loop fix (vercel/next.js@20892dd), as tracked in #1225.
Background
When middleware rewrites a Server Action POST to a page that does not bundle the action, Next.js' multi-worker architecture can forward the request between workers, creating an infinite loop. Next.js fixed this by:
x-action-forwardedguard that returns 404 +x-nextjs-action-not-found: 1on subsequent forwardsUnrecognizedActionErroris thrownWhat this PR does
1. Adds
x-action-forwardedguardACTION_FORWARDED_HEADERconstant inheaders.tsINTERNAL_HEADERSso it gets stripped from external requests (forgery protection)handleServerActionRscRequest: returns 404 +x-nextjs-action-not-found: 1when header is presenthandleProgressiveServerActionRequest: same behavior for progressive (multipart) actions2. Unit tests (TDD)
app-server-action-execution.test.ts:x-action-forwarded: 1-> 404x-action-forwarded: 1-> 4043. E2E test fixtures
/nextjs-compat/action-forward-looppage with a form + server action/nextjs-compat/action-forward-loop-rewriteas middleware rewrite target (no action)error-boundary.tsxusingunstable_isUnrecognizedActionError4. E2E tests
Why this matters for vinext
Vinext is a single-worker architecture, so the infinite forwarding loop does not apply today. However:
UnrecognizedActionErrorclient contract is part of the public Next.js API surface that vinext reimplementsTest results
Closes
Closes #1225