fix(security): stop a project's auth policy 401ing the control plane's own dispatch - #3645
Conversation
…s own dispatch Second instance of the bug class #3641 fixed for CSRF: a gate demands a credential a legitimate caller structurally cannot hold, and sits in front of that caller's handler. `AuthHandler` runs at priority 0 with `patterns: []`, and the registry never consults `metadata.patterns` -- it calls every handler in priority order -- so the gate stands in front of every control-plane surface, ahead of `CsrfHandler` and `ProjectRunExecuteHandler`. It exempted only OPTIONS and `isCspReportRequest`. The platform cannot satisfy it. The control plane sends `Authorization: Bearer <platform-minted per-run service JWT>` alongside the signed operation envelope: `checkBasicAuth` can never match a `Bearer` header, and `checkBearerAuth` compares it against a secret the project authored. So any project that set `security.auth` -- or `VERYFRONT_BASIC_USER`/`VERYFRONT_BEARER_TOKEN` -- answered its own `task:release-asset-build` dispatch with 401. That status is not retryable (the client retries only on >=500 or 429), so the run died before the manifest row existed, the state stayed `missing`, and `veryfront deploy` failed at its 120s deadline naming neither auth nor config. `POST /api/control-plane/agents/list` is the same failure on a second surface, and worse shaped: `runtime-agent-client` sends no `Authorization` header at all, so Studio's agent listing 401d for any project with `security.auth` set. The fix reuses the predicate #3641 introduced. `isSignedControlPlaneDispatch` requires both conditions: an anchored method/path pair that a control-plane handler owns, and the signature header that handler verifies through `verifyControlPlaneRequest`, which binds the Ed25519 signature to issuer, audience, project id, request method, request path and body hash with expiry/skew bounds. It is not a weakening -- it exempts nothing that is not authenticated more strongly downstream. It deliberately does not key on the prefix. `/api/control-plane/` is reserved but not exclusively routed: the run handlers register the prefix and `return this.continue()` when their anchored regex misses, and `ApiHandlerWrapper` declares no patterns and sits after them, so `/api/control-plane/checkout` falls through to project code. A `startsWith` match would ship a bypass any project could trigger by choosing a path. The check sits ahead of `resolveAuth`, like the CSP report exemption: an unresolvable auth config still fails closed for every browser request, but a project's config typo must not brick the platform's own dispatch. Tests cover both directions: - `auth.test.ts` -- all five surfaces pass for basic, bearer and env-configured auth; a look-alike path, seven project routes inside the namespace, every surface with no signature header, and a surface with an empty header all still get 401. - `build-dispatch-security.test.ts` -- the real chain (`AuthHandler` -> `CsrfHandler` -> `ProjectRunExecuteHandler`) with a genuine signed envelope reaches the release asset build executor under basic auth, bearer auth, and both gates at once. - `internal-agents-list.handler.test.ts` -- the real chain (`AuthHandler` -> `InternalAgentsListHandler`) lists agents for a signed dispatch behind basic and bearer auth, and still challenges the same surface without the signature and a project route inside the namespace. Before the fix, those cases answered `401: Unauthorized` with the executor never reached.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesControl-plane authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ions bag
The auth gate and the project-middleware gate are being exempted on two
branches, and both widen `dispatchReleaseAssetBuild`. Grown as positional
parameters they merge into a signature that still compiles while every
existing call binds its argument to the wrong slot: a `{ projectMiddleware }`
argument lands in the `auth` parameter, no middleware is installed, and the
test that proves the middleware bypass works keeps passing without ever
exercising it.
Take one named options bag instead, and assemble all three gates the way the
runtime does — project middleware outermost, then the security handlers, then
the run executor — so a fix for one gate is exercised with the others
standing.
Pushed: this branch and #3646 would have merged into something brokenBoth this PR and #3646 widen Merging them produces a two-line conflict in the parameter list and auto-merges everything else. That is the trap: the conflict looks trivial, so the natural resolution is to keep both parameters. It compiles. And then: The FixOne named options bag, identical on both branches: interface DispatchOptions {
readonly auth?: SecurityConfig["auth"];
readonly projectMiddleware?: MiddlewareFunction[];
readonly unsigned?: boolean;
}
async function dispatchReleaseAssetBuild(
csrf: CsrfSetting | undefined,
options: DispatchOptions = {},
): Promise<DispatchOutcome>A named field cannot mis-merge the way a positional slot can. The harness also now assembles all three gates in the order the runtime does — project middleware outermost, then the security handlers, then the run executor — so a fix for one gate is exercised with the others standing. Each branch keeps only its own tests. Verified by merging, not by reasoning
|
… string>
The inline array literal widened to a union in which each member carried the
other's keys as `?: undefined`, so neither was assignable to createEnvCtx's
`Record<string, string>` parameter:
TS2345 ... Property 'VERYFRONT_BEARER_TOKEN' is incompatible with index
signature. Type 'undefined' is not assignable to type 'string'.
at src/security/http/auth.test.ts:547:22
Hoisting the literal into an annotated `Record<string, string>[]` contextually
types both members and drops the synthetic optional keys. No behaviour change.
|
1 participant
Second instance of the bug class #3641 fixed for CSRF: a gate demands a credential that a legitimate caller structurally cannot hold, and is positioned to run before that caller's handler.
The hole
AuthHandler(src/security/http/auth.ts) declarespriority: 0andpatterns: []. The registry never consultsmetadata.patterns—registry.tscalls every handler in priority order — so a handler must self-limit, andAuthHandler.handlehad no path check. It ran on every request, ahead ofCsrfHandler(priority 5) andProjectRunExecuteHandler, exempting onlyOPTIONSandisCspReportRequest.The platform cannot satisfy it:
Authorization: Bearer <platform-minted per-run service JWT>plus the JWS envelope header (veryfront-apiruntime-project-run-client.ts:120-158).checkBasicAuthcan never match aBearerheader;checkBearerAuthcompares it against a secret the project authored.security.auth— orVERYFRONT_BASIC_USER/VERYFRONT_BEARER_TOKEN— answered its owntask:release-asset-builddispatch with 401.>=500or429), so the run died before the manifest row existed, the state stayedmissing, andveryfront deployfailed at its 120s deadline reportingRelease assets were not ready within 120s (last state: missing)— naming neither auth nor config.Second surface:
POST /api/control-plane/agents/listfails the same way and worse —runtime-agent-client.ts:253-269sends noAuthorizationheader at all, so Studio's agent listing 401s for any project withsecurity.authset.The fix
Reuses the predicate #3641 introduced, in
AuthHandler.handlealongside the OPTIONS and CSP-report exemptions:isSignedControlPlaneDispatchreturns true only when both hold:isControlPlaneSurfaceRoutematches one of five anchored method/path shapes, andx-veryfront-control-plane-jws.Not a weakening: it exempts nothing that is not authenticated more strongly downstream. Every one of those five surfaces calls
verifyControlPlaneRequest, andverifyControlPlaneJwsbinds the Ed25519 signature to issuer, audience, project id, request method, request path and body hash, with expiry/skew bounds.Why not a prefix match.
/api/control-plane/is reserved but not exclusively routed. The run handlers register only the prefix andreturn this.continue()when their anchored regex misses;ApiHandlerWrapperdeclares no patterns and sits after them, so/api/control-plane/checkoutfalls through to project code. Anything keyed onstartsWith/includeswould ship a bypass a project could trigger by choosing a path.Why ahead of
resolveAuth. Same placement as the CSP-report exemption. An unresolvable auth config still fails closed for every browser request, but a project's config typo must not brick the platform's own dispatch, which authenticates itself downstream. A test pins both halves of that with one config.Tests (written first, confirmed red)
Red, before the production change:
Coverage:
src/security/http/auth.test.ts— all five surfaces pass for basic, bearer and env-configured auth. Adversarial: a look-alike path (/api/control-plane-mirror/...), seven project routes inside the namespace with the signature header attached, every registered surface with no signature header, and a surface with an empty signature header — all still 401.src/release-assets/build-dispatch-security.test.ts— drives the real chain (AuthHandler→CsrfHandler→ProjectRunExecuteHandler) with a genuine signed Ed25519 envelope and asserts the release asset build executor is reached, under basic auth, bearer auth, and both gates at once. The four pre-existing CSRF cases are unchanged and still pass withAuthHandleradded to the chain.src/server/handlers/request/internal-agents-list.handler.test.ts— real chain (AuthHandler→InternalAgentsListHandler) for the second surface, sending exactly whatruntime-agent-clientsends (noAuthorizationheader).No existing test was weakened or deleted.
Verification
deno test src/security/ src/release-assets/ src/channels/ src/server/handlers/(repo test flags):ok | 216 passed (2435 steps) | 0 faileddeno task lint:module-boundaries— exit 0; the import mirrors the onecsrf-handler.tsalready makes. (The "debt decreased by 1" note it prints is pre-existing on cleanorigin/main.)deno lint/deno fmt --checkon the changed files,deno check src/security/http/auth.tsdeno task docsregenerated the one shifted line pin indocs/api-reference/veryfront/security.mdDeno 2.7.7 throughout.
🤖 Generated with Claude Code
Summary by CodeRabbit
Security Improvements
Documentation
AuthHandlersource reference in the API documentation.