Support allowedDevOrigins separately from serverActions.allowedOrigins#337
Conversation
|
/bigbonk review |
commit: |
There was a problem hiding this comment.
Review: Support allowedDevOrigins separately from serverActions.allowedOrigins
This is a clean, well-scoped fix that correctly separates two distinct trust boundaries that were previously conflated. The change matches Next.js's upstream behavior where allowedDevOrigins is a top-level config option (see config-shared.ts in Next.js) and experimental.serverActions.allowedOrigins is a separate CSRF setting.
What the PR does right
-
Config parsing —
allowedDevOriginsis correctly parsed as a top-level field inresolveNextConfig(), withArray.isArray()validation and an empty-array default. The null-config path also correctly initializes it. -
Separation of concerns — The three
index.tscall sites that pass origins tovalidateDevRequest()now correctly usenextConfig?.allowedDevOrigins(dev server cross-origin allowlist), while the RSC entry's CSRF validation for server actions continues to usenextConfig?.serverActionsAllowedOrigins. These are two different security checks with different semantics. -
App Router wiring — The
generateRscEntry()call correctly passesallowedDevOriginsandallowedOriginsas separate fields. In the generated RSC entry code,__allowedDevOrigins(fromgenerateDevOriginCheckCode()) and__allowedOrigins(for__validateCsrfOrigin()) remain distinct variables. -
Prod server is unaffected —
allowedDevOriginsis correctly absent fromprod-server.tsandworker-entry.ts. This is a dev-only setting. -
Test coverage — Good coverage across all layers: config parsing (defaults, separation, null input),
vinext checkdetection, App Router codegen (both unit and integration), and Pages Router dev server integration with a test that verifiesserverActions.allowedOriginsdoes NOT grant dev server access. -
Next.js test porting — The Pages Router integration test correctly references the upstream Next.js test (
test/development/basic/allowed-dev-origins.test.ts) per the project's contribution guidelines.
Minor observations (non-blocking)
- The
AppRouterConfigJSDoc update inapp-dev-server.ts:32is a nice touch — the old comment was misleading ("from serverActionsAllowedOrigins or custom config"). - The
vinext checkchange correctly addsallowedDevOriginsto both theCONFIG_SUPPORTmap and theconfigOptionsscan list.
All tests pass. LGTM.
|
Review submitted. Here's a summary of what I found: Approved — This is a well-implemented fix that correctly separates two distinct security trust boundaries:
Previously, All 8 changed files are consistent, the prod server is correctly unaffected (dev-only setting), and test coverage is thorough across config parsing, App Router codegen, Pages Router integration, and |
Summary
Parse and wire
allowedDevOriginsas its own config surface instead of reusingexperimental.serverActions.allowedOrigins.This fixes the dev-origin enforcement so the dev server allowlist does not widen or narrow trust boundaries based on server action CSRF config.
Changes
allowedDevOriginstoNextConfigandResolvedNextConfigallowedDevOriginsinresolveNextConfig()allowedDevOriginsfor Pages Router dev-origin checksallowedDevOriginsinto the App Router virtual RSC entryexperimental.serverActions.allowedOriginsvinext checkto reportallowedDevOriginsas supportedTests
allowedDevOriginsdefaults and separation fromserverActions.allowedOriginstest/development/basic/allowed-dev-origins.test.tsallowedDevOriginsandallowedOriginsremain distinctvinext checkcoverage forallowedDevOriginsVerification
pnpm test tests/check.test.ts -t "allowedDevOrigins|experimental.serverActions|next.config"pnpm test tests/shims.test.ts -t "allowedDevOrigins|serverActionsAllowedOrigins"pnpm test tests/app-router.test.ts -t "loads allowedDevOrigins from next.config into the virtual RSC entry|keeps allowedDevOrigins separate from allowedOrigins|embeds allowedDevOrigins"pnpm test tests/pages-router.test.ts -t "Pages Router allowedDevOrigins config"pnpm test tests/dev-origin-check.test.ts