fix(security): bind hosted source and environment identity - #3290
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Hardens hosted multi-tenant identity boundaries by ensuring branch/source/environment identity, proxy trust, cache scoping, module admission, and internal-agent dispatch are derived only from verified control-plane or operator-trusted context (and not from spoofable transport headers or query params).
Changes:
- Makes proxy trust an operator-owned topology decision (
VERYFRONT_TRUST_FORWARDED_HEADERS=1) and removes dispatch-JWS as a generic proxy-trust signal. - Binds hosted branch/environment identity to verified control-plane inputs, fails closed on ambiguous identity, and scopes env var / adapter / middleware caches to canonical project and credential identity.
- Tightens browser module and HMR admission (manifest-based production browser-module boundary, protected-path blocking, and no preview/HMR unlocking via Host/forwarded/query).
Verification
- Not run in this review environment.
- Safest next step: run
deno test --no-check --allow-all --parallel(or the repo’s documented verify task) and ensure hosted/security + integration suites remain green.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/vfs-proxy-mode-e2e.test.ts | Enables forwarded-header trust for proxy-mode e2e coverage. |
| tests/integration/server/production-server.test.ts | Ensures shared middleware loads after trusted request context resolution. |
| tests/integration/server/modules/hmr-handler.test.ts | Updates HMR tests to reject Host/forwarded/query-based preview inference. |
| src/types/server.ts | Adds canonical branch/default-branch fields to handler context. |
| src/server/utils/proxy-trust.ts | Defines proxy trust as operator-topology trust only. |
| src/server/utils/proxy-trust.test.ts | Updates expectations to ensure dispatch JWS does not grant proxy trust. |
| src/server/runtime-handler/timeout-manager.ts | Preserves registered HTTP semantics at outer request boundary for VeryfrontErrors. |
| src/server/runtime-handler/timeout-manager.test.ts | Tests that registered permission errors surface with problem+json semantics. |
| src/server/runtime-handler/project-runtime-context.ts | Propagates trusted identity through request context; tightens proxy guard and env var cache scoping. |
| src/server/runtime-handler/project-resolution.ts | Stops trusting query/header environment identity unless operator-trusted; adds env-name/default-branch identity. |
| src/server/runtime-handler/project-resolution.test.ts | Adds coverage for trusted-only identity header extraction. |
| src/server/runtime-handler/project-middleware.ts | Restricts middleware caching to immutable production release identity and canonical project ID in shared mode. |
| src/server/runtime-handler/project-middleware.test.ts | Updates cache behavior tests for preview/no-cache and shared ID partitioning. |
| src/server/runtime-handler/index.ts | Moves proxy-identity rejection earlier; blocks websocket query tenant selection; scopes env var cache fetcher. |
| src/server/runtime-handler/index.test.ts | Tests early rejection avoids isolation/tracking side effects; enforces trusted topology for proxy context. |
| src/server/runtime-handler/handler-context-builder.ts | Wires canonical branch/default-branch identifiers into handler context. |
| src/server/runtime-handler/adapter-factory.ts | Removes dispatch-JWS-based authorization for x-project-path; uses operator topology trust only. |
| src/server/runtime-handler/adapter-factory.test.ts | Updates adapter-factory tests to require trusted topology and reject dispatch-JWS path override. |
| src/server/project-env/production-environment-resolver.ts | Adds bounded, cached environment identity resolver with typed error mapping. |
| src/server/project-env/production-environment-resolver.test.ts | Tests environment identity resolution, binding, size limits, and abort behavior. |
| src/server/project-env/index.ts | Exposes updated cache scope/types and new environment identity resolvers. |
| src/server/project-env/fetcher.ts | Adds bounded response reading, redirect-safe fetches, typed auth errors, and strict internal-endpoint behavior. |
| src/server/project-env/fetcher.test.ts | Expands coverage for redirects, authorization gating, timeouts, malformed payloads, and size bounds. |
| src/server/project-env/cache.ts | Reworks env var caching to be scope- and credential-principal keyed with inflight limits and fail-closed behavior. |
| src/server/project-env/cache.test.ts | Tests scope isolation, invalidation epochs, inflight admission limits, and timeout recovery. |
| src/server/handlers/request/module/module-server-handler.ts | Restricts SSR module mode to explicitly admitted local projects. |
| src/server/handlers/request/internal-agents-list.handler.test.ts | Ensures host token is not forwarded when request token is absent. |
| src/server/handlers/request/agent-stream.handler.ts | Revalidates agent source/target binding and environment identity before loading secrets; returns typed errors. |
| src/server/handlers/request/agent-stream.handler.test-helpers.ts | Extends test helpers with branch/default-branch context to match new binding. |
| src/server/handlers/preview/hmr.handler.ts | Removes Host/forwarded/query-based preview/HMR unlocking; relies on resolved request context/local project. |
| src/server/handlers/preview/hmr.handler.test.ts | Updates HMR handler tests to reflect stricter admission rules. |
| src/server/context/request-context.ts | Adds allowHostTokenFallback to prevent combining host token with attacker-selected tenant identity. |
| src/server/context/request-context.test.ts | Tests disabling host-token fallback for shared proxy admission. |
| src/server/bootstrap.ts | Requires explicit operator-trusted proxy topology for hosted proxy mode. |
| src/server/bootstrap.test.ts | Adds bootstrap validation coverage for trusted-topology requirement. |
| src/security/README.md | Documents shared proxy identity requirements and fail-closed env/secret-fetch behavior. |
| src/security/http/base-handler.ts | Removes host-token fallback from proxy-context helper to avoid confused-deputy behavior. |
| src/security/http/base-handler.test.ts | Ensures host token is not treated as a request credential for contextual adapters. |
| src/release-assets/manifest-cache.ts | Adds a manifest accessor for browser-module admission that bypasses rollout-flag gating. |
| src/proxy/split-forward-request.test.ts | Extends proxy forwarding parity to include canonical environment name. |
| src/proxy/mode-parity.test.ts | Ensures proxy header injection strips/sets new identity headers and default-branch identity. |
| src/proxy/handler.ts | Adds env-name + default-branch/preview-branch identity propagation and rejects partial identities. |
| src/proxy/handler.test.ts | Tests body-hash-bound control-plane branch binding and header injection into runtime identity. |
| src/proxy/control-plane-signature.ts | Adds body-bound verification to extract trusted branch/default-branch identity from signed control-plane requests. |
| src/platform/adapters/veryfront-api-transport.ts | Adds redirect policy + diagnostic body inclusion controls; improves error logging context safety. |
| src/platform/adapters/fs/veryfront/schemas/proxy-manager.schema.ts | Tightens projectId validation to require non-empty string when present. |
| src/platform/adapters/fs/veryfront/proxy-manager.ts | Partitions adapter identity by canonical project ID + credential principal; removes mutable token reuse on cached adapters. |
| src/platform/adapters/fs/veryfront/proxy-manager.test.ts | Adds isolation tests for slug reassignment, credential rotation, and corrupted cached context eviction. |
| src/modules/server/module-server.ts | Enforces protected browser-module paths, server-only boundary checks, and production manifest-based admission. |
| src/modules/server/module-server.test.ts | Adds security regression coverage for protected paths, server-only sources, SSR spoofing, and manifest admission. |
| src/modules/server/browser-module-admission.ts | Defines protected project roots/metadata paths that are never browser module entrypoints. |
| src/modules/server/browser-module-admission.test.ts | Tests protected-path detection and configured root handling. |
| src/internal-agents/schema.ts | Adds explicit runtime target kind + IDs and validates source/target binding at schema layer. |
| src/internal-agents/schema.test.ts | Updates schema tests for new target fields and rejects mismatched target/source combinations. |
| src/channels/control-plane.ts | Adds body-hash-bound control-plane verifier suitable for proxy pre-validation. |
| src/channels/control-plane.test.ts | Tests that the proxy-safe verifier binds to the exact request body. |
| src/cache/keys/builders/render.ts | Extends proxy cache key to include canonical project ID and credential principal. |
| src/cache/keys.test.ts | Tests proxy cache key partitioning by canonical project and credential principal. |
| src/agent/runtime/agent-invocation-contract.ts | Exposes target selection types and validates source/target binding for runtime invocations. |
| src/agent/runtime/agent-invocation-contract.test.ts | Adds coverage for environment targets and rejects incompatible source/target selections. |
| docs/architecture/11-control-plane-channels.md | Documents signed target/source binding and branch identity propagation behavior. |
| .env.example | Documents required proxy-mode env vars and internal env-var endpoint constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/server/runtime-handler/project-runtime-context.ts:412
createProxyGuard()treats several identity headers as invalid when they appear outside the trusted topology, but it currently omitsx-default-branch-namefrom that list. Since this header becomes part of the canonical hosted source identity, it should be rejected on untrusted requests for the same reasons asx-branch-id/x-branch-name.
src/server/runtime-handler/project-runtime-context.ts:420- In proxy mode, the guard fails closed for partial environment identity (
x-environment-idwithoutx-environment-name), but it does not apply equivalent pairing/ambiguity checks for branch identity. With the introduction ofbranchId/branchNameanddefaultBranchNameon the handler context, admitting a request with (a) only one ofx-branch-id/x-branch-name, or (b) both preview-branch identity andx-default-branch-name, can leave downstream code with an inconsistent source identity.
Consider rejecting these cases at admission the same way the environment identity pair is handled.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/runtime-handler/project-runtime-context.ts:439
- In proxy mode,
createProxyGuard()rejects incomplete environment identity (x-environment-id/name) but does not reject incomplete or ambiguous branch identity. IfidentityHeadersTrustedis true, a request can supply onlyx-branch-id(or onlyx-branch-name), or supply both preview-branch identity andx-default-branch-name, and still pass this guard. For consistency with the environment pairing rule and to fail closed on ambiguous hosted identity, add explicit branch completeness checks here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/server/project-env/fetcher.ts:145
fetchEnvironmentVariables()merges...headersafter settingAuthorizationandAccept, which allows a caller-provided headers object to override those required values. This is brittle for a security-sensitive helper and also doesn't correctly merge whenheadersis aHeadersinstance. Build aHeadersobject fromheadersand setAuthorization/Acceptafter merging so they cannot be overridden.
src/security/http/base-handler.test.ts:223- This test sets
VERYFRONT_API_TOKENviasetEnv()but never restores it. That leaks process state into subsequent tests and can cause order-dependent failures. Capture the original value and restore/delete it in afinallyblock.
it("does not treat the host token as a request credential", async () => {
setEnv("VERYFRONT_API_TOKEN", "vf_test_token");
const handler = new TestHandler();
let called = false;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/project-env/cache.ts:134
abortReason()can return a non-Error value whenAbortSignal.reasonis set to something other than anError(for example a string). That would causeEnvironmentVariableCacheto reject/throw non-Error values, which breaks the typed error contract used elsewhere in this PR (most callers expectError/VeryfrontError). Coerce non-Error abort reasons into aCACHE_ERROR(and preserve the original reason ascause).
94ed872 to
a245a1b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 71 out of 71 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/project-env/production-environment-resolver.ts:187
- Environment matching is currently case-sensitive (
name === environmentName). Elsewhere in the codebase environment names can be title-cased (for example "Production"), so this can fail closed unexpectedly when the environments endpoint returns a different casing for the same logical environment. Normalize both sides (for example, compare lowercased strings) before deciding the identity is missing/ambiguous.
a245a1b to
ecb982a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/runtime-handler/project-resolution.ts:123
x-branch-nameis decoded but not trimmed/normalized, whilex-default-branch-nameis trimmed. Control-plane branch binding (requireBranchName) enforces canonical (trimmed) names, so leaving whitespace here can cause false mismatches (for example, signed source vs. trusted branchName) and inconsistent cache identity. Trim the decoded branch name the same way as the default branch name.
|
Closed the remaining RSC entry-admission gap in
Fresh verification:
The only local full-file failure is the existing unrelated HTTP-mock test at the late same-origin absolute-module case; it bypasses its test mock and times out against the external fixture address. The changed steps complete successfully before it. |
Encoded branch identity can reintroduce surrounding whitespace after the Fetch headers layer has already normalized raw header OWS. Normalize the decoded preview branch name the same way as the default branch name so signed source matching and cache identity use the canonical branch string. Constraint: Latest exact-head Copilot review found branchName/defaultBranchName normalization drift in hosted identity extraction Rejected: Change header encoding globally | ByteString preservation is intentional for existing ASCII branch names Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/server/runtime-handler/project-resolution.test.ts src/proxy/handler.test.ts src/utils/header-identity.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: npx --yes deno@2.7.7 lint src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: npx --yes deno@2.7.7 check src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: git diff --check
|
Addressed the latest branch identity normalization finding at exact head 7dc18cb. What changed:
Local evidence on the rebased head:
The normal pre-push hook previously reached 3720 passed / 26830 steps and failed on an unrelated |
|
Follow-up Fresh evidence:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/platform/adapters/veryfront-api-transport.ts:189
- When
outboundPolicyis enabled, the transport silently overrides the caller-selectedredirectpolicy to"error"in theguardedOutboundFetch()path. SinceTransportRequestInit.redirectis now part of the public request contract (and tests validate explicitfollow/manual), this silent override is surprising and can mask misconfiguration. Prefer to fail fast by rejecting non-"error"redirect policies whenoutboundPolicyis enabled (or explicitly document/enforce this invariant).
const res = config.outboundPolicy
? await guardedOutboundFetch(url, { ...requestInit, redirect: "error" }, {
authorizeUrl: config.outboundPolicy.authorizeUrl,
})
: await fetch(url, requestInit);
|
Merge confidence: 93% for exact head d373087. Reasoning: this exact head has a fresh no-comment Copilot review, 0 unresolved review threads, and all applicable hosted checks green. The branch has had several security-contract follow-ups, and the latest head specifically keeps browser-boundary violations mapped to the historical module-server 404 while preserving the legacy RSC endpoint's sanitized 500 behavior. The review history also includes an independent architect gate at 97% after the RSC entry-admission fix, and the subsequent commits are narrow regression/contract fixes with focused evidence: endpoint-router plus browser-module-bundler passed 129/129 steps, Residual risk is moderate because this PR has a broad changed-file surface around hosted identity, runtime environment binding, browser module admission, and control-plane transport behavior. The risk is offset by exact-head CI, no unresolved comments, fresh automated rereview, and repeated focused regression coverage for the security-sensitive boundaries. This clears the strictly-above-90% merge threshold for the current queued head. |
|
|
Updated the branch ref Addressed review/pre-push blockers:
Verification:
Push note: the normal local pre-push hook ran format, lint, typecheck, then hit one parallel unit-suite failure in |
Summary
Hardens hosted multi-tenant identity boundaries so branch/source selection, project environment resolution, module admission, cache identity, and internal-agent dispatch remain bound to verified project context.
Security behavior
maindefault branches and preview branch IDs without trusting client-selected branch headersCompatibility
This intentionally rejects ambiguous or mismatched hosted identity inputs that were previously accepted. Local project behavior remains explicit and separately tested. No third-party dependency was added to framework core.
Verification
Current head:
ecb982a6106d24b550183026e1c5307687a1dad1, rebased ontod4dc53b3b(#3299). Fresh independent exact-head security review is required before merge.deno task verify:quick: passeddeno task lint:test-typecheck: passed with 0 new errorsgit diff --check: passedAll review threads are resolved.
Final browser-bundle resource hardening
The exact rebased head now also bounds public request-triggered RSC client compilation:
Verification on the rebased head:
An independent exact-head review and GitHub CI are required before merge.
Deployment order (required for existing hosted deployments)
This change fails closed at two points, so mixed-version rollouts must follow this order:
VERYFRONT_TRUST_FORWARDED_HEADERS=1(and ensureCHANNEL_DISPATCH_SIGNING_PUBLIC_KEYis set) on the runtime environment while it still runs the previous version. Earlier runtimes already honor the variable as an explicit operator trust opt-in, so this is behavior-preserving inside the required private topology. A new runtime booted without it crash-loops intentionally (validateProductionEnvironment).x-default-branch-nameheader, and thevf-utf8:branch-name encoding only applies to values an earlier proxy could not forward at all.PERMISSION_DENIED, because branch identity now comes only from the verified control-plane body binding.Roll back in reverse order (runtime first, then proxy); the trust variable can stay set. There is deliberately no warn-only compatibility mode: unbound identity must never select tenant data. Full runbook:
src/security/README.md, "Rollout ordering for hosted identity changes".