docs(security): document hosted identity rollout order and close #3290 review gaps - #3335
Conversation
…ew gaps Address the remaining findings from the 85/100 critical review of #3290: - Document the safe deploy order for the hosted identity hardening (set VERYFRONT_TRUST_FORWARDED_HEADERS=1 first, proxy tier before runtime tier) in src/security/README.md and .env.example, and point the bootstrap fail-closed error at that runbook. - Document the purely topological proxy-to-runtime trust boundary and the mTLS / per-hop-secret follow-up; no warn-only compatibility mode is added because it would let unbound identity select tenant data. - Verify the canonical POST method for control-plane branch-binding signatures instead of the raw request casing. - Dispose the uncached filesystem adapter when initialize() fails so repeated failures cannot leak watchers or sockets, with a regression test. - Coerce non-Error abort reasons in the project environment cache into the typed CACHE_ERROR contract. - Explain why release/content-source/path headers are read outside the identity-trust gate in extractRequestHeaders to prevent drift. The residual host-token fallback in agent-stream.handler.ts flagged by the review was already removed on this branch; existing regression tests pin the request-scoped credential behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reached
Next review available in: 17 seconds 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 (8)
Comment |
|
Independent exact-head gate complete at 2e64571 against main f56afcd. The full 8-file +114/-4 delta has no correctness, security, dependency, or production-quality blocker; no hacks or fallback behavior were found. Focused coverage passes (11 tests / 204 steps), verify:quick passes, the adversarial non-Error abort-reason probe preserves the typed CACHE_ERROR contract, and the synthetic merge tree is byte-identical to the head tree. Review threads: 0. Code confidence: 97%. Merge remains gated only on terminal hosted CI and merge-queue combined checks. |
|
Merge confidence: 97%. Reasoning:
|
There was a problem hiding this comment.
Pull request overview
Documents and operationalizes the hosted identity hardening rollout by adding a security runbook (including trust boundary notes and safe upgrade/rollback ordering) and by making runtime failures more actionable, alongside a few small security/robustness fixes in proxy/runtime code paths.
Changes:
- Adds hosted identity rollout guidance and trust-boundary documentation in
src/security/README.md. - Improves proxy-mode bootstrap messaging and expands
.env.exampleguidance forVERYFRONT_TRUST_FORWARDED_HEADERS. - Hardens a few implementation details: canonicalizes signed method verification, disposes adapters on failed initialization (with a regression test), and ensures abort reasons in project-env cache are always
Errorinstances.
Verification:
- Not run in this review environment (rely on CI as stated in the PR).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/server/runtime-handler/project-resolution.ts | Adds a drift-guard comment clarifying intentional asymmetric header trust gating. |
| src/server/project-env/cache.ts | Ensures abort paths always surface an Error (typed fallback for non-Error abort reasons). |
| src/server/bootstrap.ts | Makes proxy-mode crash-loop guidance more explicit, pointing to the rollout runbook. |
| src/security/README.md | Adds trust boundary/residual risk and a detailed rollout/rollback ordering runbook. |
| src/proxy/control-plane-signature.ts | Canonicalizes verified request method to POST for signature verification. |
| src/platform/adapters/fs/veryfront/proxy-manager.ts | Best-effort adapter.dispose() when initialize() fails to avoid resource leaks. |
| src/platform/adapters/fs/veryfront/proxy-manager.test.ts | Adds a regression test asserting disposal on initialization failure and no caching. |
| .env.example | Expands operator guidance for VERYFRONT_TRUST_FORWARDED_HEADERS and rollout ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Exact-head follow-up at c90136e: replaced the non-ASCII dash in .env.example with sentence punctuation required by AGENTS.md. The prior exact-head 11-test/204-step gate and synthetic-main gate were green; the final one-line documentation delta passes git diff --check and verify:quick. The Copilot thread was replied to and resolved only after the fix was pushed. Merge confidence remains 97%, pending fresh hosted checks. |
|
Merge confidence: 94%. Reasoning: exact head Risk notes: this is mostly docs plus narrow hardening around hosted proxy trust gating and cleanup on adapter init failure. I reproduced the broader Scheduling for merge with exact-head protection. |
Summary
PR #3290 merged while its review-fix pass was still running. This follow-up ports the remaining review fixes from the stale PR branch (commit 0a0345e, cherry-picked onto main):
src/security/README.md: safe deploy order for the hosted identity binding (setVERYFRONT_TRUST_FORWARDED_HEADERS=1on the old runtime first → deploy proxy tier → deploy runtime tier; rollback in reverse), plus rationale for not adding a warn-only old-proxy fallback (would break fail-closed)..env.examplecomment expanded.POSTinresolveVerifiedControlPlaneBranchBinding; best-effortadapter.dispose()wheninitialize()fails inproxy-manager.ts(+ regression test); coerce non-Error abort reasons to typedCACHE_ERRORinsrc/server/project-env/cache.ts; drift-guard comment on the intentional asymmetric header gating inextractRequestHeaders.Complements #3332 (which ports the SSR transport commits); no overlap.
Test plan
🤖 Generated with Claude Code