fix(runtime): stop a project's own middleware blocking its release asset build - #3646
Conversation
…set build
`projectMiddlewareRuntime.execute` wraps the entire handler chain, so a root
`middleware.ts` sits in front of the control plane's signed
`task:release-asset-build` dispatch to
`POST /api/control-plane/runs/{runId}/execute`. A project that merely gates
requests in middleware, the shape the middleware guide itself demonstrates,
answers its own deploy with a 401 and the manifest is never built. The failure
surfaces 120 seconds later as `Release assets were not ready within 120s (last
state: missing)`, naming neither middleware nor the dispatch.
Middleware cannot authorize that dispatch even in principle.
`createApplicationRequest` withholds every `x-veryfront-*` header from project
code, so the signature the receiving handler verifies is invisible to the
middleware that would have to trust it. The gate demands a credential the caller
structurally cannot present, which is the same shape as the CSRF instance fixed
in #3641.
Chosen semantics: a signed control-plane dispatch does not traverse project
middleware. It is not the project's traffic. It addresses a platform handler in
a namespace the project does not serve, and asks the runtime to perform internal
work. Everything else still traverses middleware unchanged, including an
unsigned request to the same path and a project route that merely sits inside
`/api/control-plane/`.
The bypass reuses `isSignedControlPlaneDispatch` from #3641, which requires both
an anchored method/path pair a control-plane handler owns and the signature
header that handler verifies. It concedes nothing: the only routes it can reach
answer 401 without a valid envelope and never fall through to project code.
`veryfront deploy` also stops reporting a silent timeout. A manifest row appears
the moment the runtime begins the build, so never observing one means the
dispatch never reached the builder. That case now names the dispatch, the route,
and the gates that can refuse it, instead of only `last state: missing`. A
manifest that was seen and then vanished, or a poll that kept failing, keeps the
original wording.
Tests cover the signed dispatch reaching the release asset build executor
through the real chain (project middleware, then `CsrfHandler`, then
`ProjectRunExecuteHandler`), all five control-plane surfaces bypassing
middleware, and four adversarial cases that must keep running it: an unsigned
request to the execute path, a project route inside the reserved namespace, a
look-alike prefix, and a registered surface addressed with an unowned method.
|
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 (4)
📝 WalkthroughWalkthroughChangesControl-plane dispatch flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DispatchTest
participant ProjectMiddlewareRuntime
participant ProjectMiddleware
participant ReleaseAssetExecutor
DispatchTest->>ProjectMiddlewareRuntime: Send signed control-plane dispatch
ProjectMiddlewareRuntime->>ReleaseAssetExecutor: Forward authenticated dispatch
ReleaseAssetExecutor-->>DispatchTest: Return 200 response
DispatchTest->>ProjectMiddlewareRuntime: Send unsigned request
ProjectMiddlewareRuntime->>ProjectMiddleware: Execute project middleware
ProjectMiddleware-->>DispatchTest: Return 401 response
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f81dc1af78
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/shared/deployment/deploy-project.ts`:
- Around line 643-645: In the deployment polling flow, add a monotonic
observedTransientFailure flag alongside lastTransientFailure, set it whenever a
retryable control-plane read fails, and use it in the neverStarted calculation.
Continue clearing lastTransientFailure after successful reads for last-failure
messaging, and add a regression covering one retryable error followed by missing
manifests that expects the generic timeout message.
In `@docs/guides/middleware.md`:
- Around line 209-210: The control-plane middleware explanation is too dense. In
the documentation section describing root middleware and control-plane dispatch,
split the routing behavior, signature limitation, narrow bypass condition, and
unsigned/other-path exceptions into short, direct, active present-tense
paragraphs while preserving all existing conditions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3d40733-55f8-46ab-8a1e-a188e86dced5
📒 Files selected for processing (6)
cli/shared/deployment/deploy-project.test.tscli/shared/deployment/deploy-project.tsdocs/guides/middleware.mdsrc/release-assets/build-dispatch-security.test.tssrc/server/runtime-handler/project-middleware.test.tssrc/server/runtime-handler/project-middleware.ts
…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 #3645 would have merged into something brokenBoth this PR and #3645 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
|
… read
`neverStarted` read `lastTransientFailure`, which is cleared after any
successful read. One retryable control-plane failure followed by reads that
returned no manifest row therefore still produced the strong claim that the
`task:release-asset-build` dispatch never reached the builder, when the failed
read had in fact left the build state unknown for that window.
Track `observedTransientFailure` monotonically and key `neverStarted` on it.
`lastTransientFailure` keeps its existing role of naming the most recent
failure in the message, so no existing wording changes.
Also narrow the middleware guide's control-plane paragraph: unsigned
`POST .../stream`, `POST .../resume` and `DELETE .../runs/{runId}` bypass
middleware through `isConfigOptionalControlPlaneRunRequest` regardless of the
signature header, so the blanket claim that unsigned requests still run
middleware was false for those three. Split the section into short paragraphs.
1 participant
The failure
projectMiddlewareRuntime.executewraps the entire handler chain(
src/server/runtime-handler/index.ts:607), so a project-rootmiddleware.tsruns in front of thecontrol plane's signed
task:release-asset-builddispatch toPOST /api/control-plane/runs/{runId}/execute. That dispatch is the only thing that callsbeginReleaseAssetManifestBuild.A project that merely gates requests in middleware, which is the exact shape
docs/guides/middleware.mddemonstrates, therefore answers its own deploy with a 401. Reproduced through the real chain before
the fix:
The user sees none of that. The manifest row is never created, the state stays
missing, and 120seconds later
veryfront deploysaysRelease assets were not ready within 120s (last state: missing), naming neither middleware nor the dispatch. This is the second instance of the bug classfixed in #3641; the first was
security.csrf.The decision, and why
Chosen: (a) a signed control-plane dispatch does not traverse project middleware.
Stated precisely, so it can be argued with:
The reasoning, in the order that decided it:
It is not the project's traffic. No project route is addressed. The request targets a
platform handler, and asks the runtime to perform internal work (build this release's asset
manifest; start, resume, or cancel a run). "Users expect middleware to see their traffic" is
true and is not in tension with this: a platform dispatch was never their traffic.
Middleware cannot authorize it even in principle.
createApplicationRequestwithholds everyx-veryfront-*header from project code (src/security/http/application-request.ts; asserted bythe existing test "exposes application auth while withholding infrastructure headers"). The
signature the receiving handler verifies is invisible to the middleware that would have to
trust it. So middleware that gates on a credential has no branch that lets the dispatch through.
This is the bug class exactly: a gate demanding a credential the caller structurally cannot hold,
here because the platform itself strips it one layer earlier.
The "middleware sees everything" contract was already not true.
executeatproject-middleware.ts:107already bypassed middleware for three of the five control-planesurfaces (stream, resume, cancel) via
isConfigOptionalControlPlaneRunRequest. The contract wasinconsistent, not absolute; this makes it coherent and keyed on something stronger.
(b) traverse but cannot reject was rejected: running project code and discarding its answer is a
contract nobody can reason about. Middleware could still stall the dispatch, mutate the response of
a platform protocol, or run side effects on internal traffic, while its documented ability to gate
becomes a lie.
(c) documentation plus a legible error was rejected as the primary fix: it makes every project
with an auth middleware responsible for maintaining path exclusions against a platform-internal
route list that changes between framework versions. Worse, the advice would have to be "exclude
/api/control-plane/", and that namespace is reserved but not exclusively routed, so the advicewould also tell projects to unprotect their own routes under that prefix. The legible-error half of
(c) is kept, below, as defence in depth.
Not a weakening
The bypass reuses
isSignedControlPlaneDispatchfrom #3641 unchanged: anchored method/path andthe signature header. It exempts nothing that is not authenticated more strongly downstream. The
only routes it can reach (
ProjectRunExecuteHandler,AgentRunResumeHandler,AgentRunCancelHandler, agents list) callverifyControlPlaneRequestand answer 401 without avalid Ed25519 envelope bound to issuer, audience, project id, method, path and body hash. Skipping
project middleware therefore cannot deliver an unauthenticated request to project code; it can only
deliver it to a handler that rejects it. A prefix match would have shipped a bypass a project could
trigger by choosing a path; this cannot.
Making the silence stop
A manifest row appears the moment the runtime begins the build. Never observing one across the
whole polling window is therefore evidence, not a guess, that the dispatch never reached the
builder.
waitForReleaseAssetManifestnow distinguishes that case:A manifest that was seen and later vanished, or a poll that kept failing (
lastTransientFailure),keeps the original wording, because in those cases the build state is genuinely unknown and the
stronger claim would be false.
Tests
Added, all failing first for the right reason:
release-assets/build-dispatch-security.test.tsnow drives the whole chain, project middlewarethen
CsrfHandlerthenProjectRunExecuteHandler, and asserts the release asset build executoris reached while a middleware that 401s everything is installed.
project-middleware.test.ts: all five signed control-plane surfaces reach the route withoutloading middleware.
deploy-project.test.ts: the timeout names the refused dispatch when no manifest row ever appears.Adversarial, proving the gate still runs (these passed before and after; a fix that only proves the
happy path is how a bypass ships):
POST /api/control-plane/runs/{runId}/execute, at both the unit and thefull-chain level;
POST /api/control-plane/checkout, a project route inside the reserved namespace;POST /api/control-plane-mirror/runs/run_1/execute, a look-alike prefix;PUT /api/control-plane/runs/run_1/execute, a registered surface with an unowned method;No existing test was weakened, changed, or deleted. The pre-existing
"keeps project middleware enabled for control-plane run execution" test asserts an unsigned
execute request is still answered by middleware, so it keeps its assertions untouched and now serves
as the adversarial regression; only an explanatory comment was added above it.
Public contract change
docs/guides/middleware.mdstates the new rule. Reviewers who prefer (b) or (c) should say so onthis PR: the semantics are the decision here, the diff is small.
Summary by CodeRabbit
Bug Fixes
Documentation
Reliability