Skip to content

fix(security): stop a project's CSRF policy blocking its own release asset build - #3641

Merged
kojiwakayama merged 2 commits into
mainfrom
fix/csrf-blocks-release-asset-build
Aug 12, 2026
Merged

fix(security): stop a project's CSRF policy blocking its own release asset build#3641
kojiwakayama merged 2 commits into
mainfrom
fix/csrf-blocks-release-asset-build

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

The failure

A customer demo project set

security: { csrf: { excludePaths: ["/api/ag-ui"] } }

to keep CSRF enforced everywhere except the agent endpoint its chat client
posts to. Deploy then failed with

Release assets were not ready within 120s (last state: missing)

The developer compared manifests across releases — four consecutive releases
carrying a nested csrf object have no manifest, releases before and after
report state=ready — and shipped csrf: false on a sign-in-protected
customer demo because it was the only setting that deployed.

Mechanism

The manifest is built by the project runtime, not the CLI. The control plane
POSTs a signed operation envelope to
/api/control-plane/runs/{runId}/execute with
target: "task:release-asset-build"; that dispatch is the only caller of
beginReleaseAssetManifestBuild (src/release-assets/build-executor.ts:2297).
Until it lands, no manifest row exists and the state reads missing.

CsrfHandler (src/security/http/csrf/csrf-handler.ts:63) registers at
priority 5 with patterns: [], so it runs in front of
ProjectRunExecuteHandler for that POST
(src/server/runtime-handler/index.ts:138-159,
src/routing/registry/registry.ts:86-101). The control plane is not a browser:
it carries no __Host-vf_csrf cookie and authorizes from the JWS the handler
verifies. So the gate answered the project's own build dispatch with
403 Forbidden – invalid or missing CSRF token, the run failed before the
manifest existed, and the deploy surfaced 120 seconds later naming neither
CSRF nor config.

The boundary is not the nested object. csrf: true fails identically —
only an absent or false setting ever built a manifest. Absent is the common
case because the task rail dispatches to the main-branch runtime, which
resolves as preview, so the production csrf default never applies there
(src/security/http/config.ts:271). That is why manifests build for everyone
who never wrote the key.

Fix

isControlPlaneSurfaceRequest (src/channels/control-plane.ts) exempts the
signed control-plane prefix from the CSRF gate — the same shape as the existing
isCspReportRequest exemption, and for the same reason: the gate expects a
browser credential the caller cannot hold. Every handler behind that prefix
authenticates its envelope through verifyControlPlaneRequest before acting
(runs execute/resume/cancel/stream, agents list), so the exemption removes no
authorization. The prefix is matched against URL.pathname, which resolves dot
segments, and a path that merely starts alike
(/api/control-plane-mirror/...) stays gated.

Tests

src/release-assets/build-dispatch-security.test.ts drives the real chain —
CsrfHandler then ProjectRunExecuteHandler, with a real signed envelope —
across all four csrf shapes and asserts the release asset build executor is
reached.

Red, before the fix:

release assets: control-plane build dispatch ... builds a manifest when the project enables csrf with a boolean
error: AssertionError: Values are not equal: release asset build never started; runtime answered 403: Forbidden – invalid or missing CSRF token
-   false
+   true

release assets: control-plane build dispatch ... builds a manifest when the project excludes a path from csrf
error: AssertionError: Values are not equal: release asset build never started; runtime answered 403: Forbidden – invalid or missing CSRF token
-   false
+   true

FAILED | 0 passed (2 steps) | 1 failed (2 steps)

Green, after:

release assets: control-plane build dispatch ...
  builds a manifest when the project leaves csrf unset ... ok
  builds a manifest when the project disables csrf ... ok
  builds a manifest when the project enables csrf with a boolean ... ok
  builds a manifest when the project excludes a path from csrf ... ok
ok | 1 passed (4 steps) | 0 failed

Plus two cases in csrf-handler.test.ts: a run dispatch passes for every
enabled csrf shape, and a look-alike prefix is still rejected. No existing test
was changed or weakened.

Not fixed here, worth a follow-up

  • AuthHandler has the same hole. It runs at priority 0, exempts only
    isCspReportRequest, and reads security.auth
    (src/security/http/auth.ts:156-183). A project that configures basic or
    bearer auth in veryfront.config.ts should be expected to 401 its own
    control-plane dispatches the same way. Not changed here because it needs its
    own reproduction rather than a symmetric guess.
  • Legibility. This fix removes one instance; the shape stays. A config
    value can still stop a manifest with the failure surfacing 120 s later as a
    timeout that names no cause. What would close that: have deploy read the
    release-asset build run's terminal state and error alongside the manifest
    state, so last state: missing can say why the build never began.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability for signed control-plane operations by allowing valid requests to proceed without requiring a CSRF token.
    • Restricted this exception to recognized paths, supported methods, and valid signatures, preserving protection for look-alike or unrelated requests.
    • Ensured release-asset build dispatch works consistently across different CSRF configuration settings.
  • Tests

    • Added coverage for successful dispatch, supported request variations, and rejection of invalid paths, methods, and signatures.

…asset build

A release asset manifest is built by the project runtime, not the CLI: the
control plane POSTs a signed operation envelope to
`/api/control-plane/runs/{runId}/execute` with `target:
"task:release-asset-build"`, and only that dispatch calls
`beginReleaseAssetManifestBuild`. `CsrfHandler` runs at priority 5 with an
empty pattern list, so it sits in front of that POST, and the control plane
holds no `__Host-vf_csrf` cookie to echo -- it authorizes with a signature the
receiving handler verifies. Any project that set `security.csrf` to anything
truthy therefore answered its own build dispatch with
`403 Forbidden - invalid or missing CSRF token`.

Nothing downstream could see it. The run failed before the manifest row
existed, so the state stayed `missing` and `veryfront deploy` reported
`Release assets were not ready within 120s (last state: missing)` -- naming
neither CSRF nor config. A customer demo lost the safe option
(`csrf: { excludePaths: ["/api/ag-ui"] }`) and shipped with CSRF off entirely.
The report attributed this to the nested object shape; the boundary is
narrower and worse than that: `csrf: true` fails identically, and only an
absent or `false` setting ever built a manifest.

`isControlPlaneSurfaceRequest` exempts the signed control-plane prefix, the
same shape as the existing `isCspReportRequest` exemption and for the same
reason: the gate expects a browser credential the caller cannot hold, and
every handler behind that prefix authenticates its envelope through
`verifyControlPlaneRequest` before acting. The prefix is matched against
`URL.pathname`, which resolves dot segments, and a path that merely starts
alike (`/api/control-plane-mirror/...`) stays gated.

Covered by a dispatch test that drives the real chain -- `CsrfHandler` then
`ProjectRunExecuteHandler`, with a signed envelope -- across all four csrf
shapes and asserts the release asset build executor is reached.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 658a17d4-3c9d-4dce-9926-ee76d11c9e5f

📥 Commits

Reviewing files that changed from the base of the PR and between 6c9efa1 and 6b5c5de.

📒 Files selected for processing (1)
  • src/channels/control-plane.ts

📝 Walkthrough

Walkthrough

The change adds shared control-plane route classification and signed dispatch detection. The CSRF handler bypasses validation for registered signed dispatches. Tests cover route rejection and release-asset manifest execution across CSRF configurations.

Changes

Control-plane CSRF dispatch

Layer / File(s) Summary
Define shared control-plane route classification
src/channels/control-plane.ts, src/proxy/control-plane-signature.ts
Adds method-specific route matching and signed dispatch detection. The proxy uses the shared route predicate.
Bypass CSRF for signed dispatch
src/security/http/csrf/csrf-handler.ts, src/security/http/csrf/csrf-handler.test.ts, docs/api-reference/veryfront/security.md
The CSRF handler bypasses validation for signed registered control-plane requests. Tests cover supported routes, missing signatures, unsupported methods, and look-alike paths.
Validate release-asset dispatch
src/release-assets/build-dispatch-security.test.ts
Runs signed control-plane requests through the CSRF and project-run handlers. Tests cover unset, disabled, enabled, and excluded-path CSRF configurations.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: kwakayama, ariskemper

Sequence Diagram(s)

sequenceDiagram
  participant ControlPlaneRequest
  participant CsrfHandler
  participant ProjectRunExecuteHandler
  participant ManifestBuilder
  ControlPlaneRequest->>CsrfHandler: Submit signed execution request
  CsrfHandler->>ProjectRunExecuteHandler: Continue without CSRF token validation
  ProjectRunExecuteHandler->>ManifestBuilder: Begin manifest building
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main security fix: preventing project CSRF policy from blocking its own release asset build.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/csrf-blocks-release-asset-build

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e055307f43

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/security/http/csrf/csrf-handler.ts Outdated
Comment thread src/release-assets/build-dispatch-security.test.ts Outdated
Comment thread src/channels/control-plane.ts Outdated
@kwakayama kwakayama added the needs-human-input Maintainer action required label Aug 12, 2026
…patch

The exemption matched on path prefix alone, so any request under
`/api/control-plane/` skipped the CSRF gate. That namespace is reserved
but not exclusively routed: only five method/path shapes reach a handler
that verifies a signed envelope, and everything else under the prefix
falls through to `ApiHandlerWrapper`. A project App or Pages API route
placed under the prefix in a custom runtime is cookie authenticated, so
the prefix match let a project turn CSRF off on its own state-changing
routes by choosing a path.

`isSignedControlPlaneDispatch` now requires both conditions: the method
and path must address a registered control-plane surface, and the request
must carry the control-plane signature header the receiving handler
verifies. The signature covers the request method and path, so an
envelope cannot be replayed against another surface, and a browser cannot
attach that header cross-origin without a preflight the runtime does not
grant.

The route table moves into `channels/control-plane.ts` as
`isControlPlaneSurfaceRoute`, and the proxy classifier consumes it
instead of keeping a second copy of the same patterns.

Tests cover a project route inside the namespace, a signature header on
an unrecognized path, and a registered surface with no signature: all
three still enforce CSRF.

Also drops a customer hostname from the regression test in favour of
`example.test`, and removes em dashes from the new exported JSDoc.
@kojiwakayama
kojiwakayama force-pushed the fix/csrf-blocks-release-asset-build branch from 6c9efa1 to 6b5c5de Compare August 12, 2026 16:21
@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 3858001 Aug 12, 2026
33 checks passed
@kojiwakayama
kojiwakayama deleted the fix/csrf-blocks-release-asset-build branch August 12, 2026 16:44
kojiwakayama added a commit that referenced this pull request Aug 12, 2026
#3641's docstring, repeated verbatim by #3647, argued the exemption was safe
because "a browser cannot attach the signature header to a cross-origin request
without a preflight the runtime does not grant". The runtime does grant it: with
no configured `allowedHeaders`, `resolveNormalizedCORSPreflightPolicy` reflects
whatever `Access-Control-Request-Headers` asked for, so any project whose CORS
policy admits an origin advertises the signature header to it. The proxy also
forwards an unverified `x-veryfront-*-jws` from a public request rather than
stripping it.

State the actual basis instead: the exemption skips only the browser-credential
gate, authority still comes from the downstream signature verification that an
attacker cannot forge, and every admitted route terminates at a handler doing
that verification ahead of ApiHandlerWrapper. Same correction to the three gate
comments that said the exemption is keyed on "the request being a real
dispatch": no predicate can know that from a header.
@kojiwakayama kojiwakayama mentioned this pull request Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-input Maintainer action required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants