fix(proxy): harden request, routing, and cache boundaries - #3251
Conversation
![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: 6edb42bbce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
55d5780 to
7063645
Compare
|
@codex review Please review exact head |
There was a problem hiding this comment.
Pull request overview
This PR hardens Veryfront’s proxy and control-plane boundaries by tightening request validation, method/path binding for JWS verification, and making the token-cache contract explicitly extension-activated (fail-closed) rather than implicitly Redis-activated or memory-fallback driven.
Changes:
- Bind control-plane JWS trust to the exact uppercase HTTP method and canonical
URL.pathname, and add regression tests for replay across methods/paths. - Replace implicit Redis cache selection with an explicit
CACHE_TYPE=extensioncontract-driven token-cache model, including explicit standalone CLI activation/teardown. - Retire the
/_vf_modules/_batchendpoint with a non-cacheable410 Gone, restrict module-serving namespaces toGET/HEAD, and harden proxy header/body handling (cache-control parsing, hop-by-hop stripping, request draining, retries).
Verification (not run in this review environment):
deno task test -- tests/docs/deno test --no-check --allow-all(or the repo’s full test command used in CI)
Reviewed changes
Copilot reviewed 95 out of 95 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/project-identity.test.ts | Adds tests for project slug/opaque id normalization and bounds. |
| src/utils/permit-semaphore.ts | Introduces a bounded FIFO permit semaphore with abort/timeout handling. |
| src/utils/permit-semaphore.test.ts | Tests semaphore capacity validation, timeout validation, FIFO ordering. |
| src/utils/lru-wrapper.ts | Adds generic option typing and supports estimateSizeOf for byte eviction. |
| src/utils/lru-wrapper.test.ts | Tests byte-bound eviction using a custom size estimator. |
| src/utils/abort.ts | Adds abort-reason normalization and abortable awaiting helpers. |
| src/utils/abort.test.ts | Tests abort error normalization and cancellation winning behavior. |
| src/server/handlers/utils/dependency-snapshot-protocol.ts | Ensures HEAD conflict responses have no body. |
| src/server/handlers/utils/dependency-snapshot-protocol.test.ts | Adds a HEAD response-body regression test. |
| src/server/handlers/request/project-run-execute.handler.test.ts | Updates control-plane signature construction to include method/path binding. |
| src/server/handlers/request/module/module.handler.ts | Restricts module namespaces to GET/HEAD; wires removed batch endpoint handler. |
| src/server/handlers/request/module/module.handler.test.ts | Tests method restriction and removed batch endpoint tombstone behavior. |
| src/server/handlers/request/module/batch-module-handler.ts | Implements 410 Gone tombstone and method restriction for batch endpoint. |
| src/server/handlers/request/lib-modules.handler.ts | Adds method restriction and snapshot-protocol based dependency pin enforcement. |
| src/server/handlers/request/internal-agents-list.handler.test.ts | Forces control-plane signatures in tests to include request bindings. |
| src/server/handlers/request/internal-agent-run.test-helpers.ts | Extends signature helper to include request method/path claims. |
| src/server/handlers/request/agent-stream.handler.test.ts | Updates resume signature tests with method/path binding. |
| src/server/handlers/request/agent-run-resume.handler.test.ts | Updates helper wrapper to supply method/path defaults for resume. |
| src/server/handlers/request/agent-run-cancel.handler.test.ts | Adds replay regression test (resume signature vs DELETE cancel) and updates bindings. |
| src/proxy/upstream-error-response.test.ts | Tightens error response headers (content-type charset, no-store). |
| src/proxy/token-priority.test.ts | Updates metadata expectations (protected: false). |
| src/proxy/timing.ts | Adds portable max timer delay constant for proxy timers. |
| src/proxy/server-resolver.test.ts | Adds extensive resolver hardening tests: expiry, inflight bounds, body limits, unsafe inputs. |
| src/proxy/retry.ts | Hardens retry classification against hostile errors and bounds retry count. |
| src/proxy/retry.test.ts | Adds hostile accessor/proxy tests and retry-count validation tests. |
| src/proxy/response-headers.ts | Replaces naive cache-control parsing with a bounded, strict parser. |
| src/proxy/response-headers.test.ts | Tests malformed/ambiguous cache-control handling and quoted values. |
| src/proxy/response-body.ts | Splits “fire-and-forget” cancel from “settled” drain, and tightens cancellation semantics. |
| src/proxy/request-path.ts | Adds origin-form path normalization (single leading slash). |
| src/proxy/request-lifecycle.ts | Uses ensureError for safe span errors on hostile throws. |
| src/proxy/request-lifecycle.test.ts | Tests hostile non-Error throws are contained while preserving original rejection. |
| src/proxy/request-init.ts | Adds duplex tagging for streamed request bodies for Node fetch portability. |
| src/proxy/request-init.test.ts | Tests duplex tagging and ownership preservation for ReadableStream bodies. |
| src/proxy/request-host.ts | Adds Host header canonicalization and validation for routing/token identity. |
| src/proxy/request-host.test.ts | Tests host normalization, fallback behavior, and invalid authority rejection. |
| src/proxy/request-drain.ts | Hardens drain timing validation and adds non-cacheable draining response headers. |
| src/proxy/request-drain.test.ts | Tests new timing validation and server-close bounding behavior. |
| src/proxy/proxy-token-resolution.ts | Hardens auth cookie parsing, passes abort signals to token waiters, typed missing-project classification. |
| src/proxy/proxy-token-resolution.test.ts | Adds tests for bounded cookie parsing and cancellation propagation. |
| src/proxy/oauth-client.ts | Hardens OAuth token fetch: URL/timeout validation, bounded body reads, caller abort propagation, sanitized errors. |
| src/proxy/oauth-client.test.ts | Adds tests for malformed responses, oversized body, sanitized errors, and invalid policies. |
| src/proxy/mode-parity.test.ts | Adds parity tests for cancellation propagation and streaming body forwarding. |
| src/proxy/main.ts | Switches to explicit extension cache activation acquisition and renderer router creation helper. |
| src/proxy/local-project-resolver.ts | Hardens local project discovery inputs, bounds config, isolates discovery caches, adds disable switch. |
| src/proxy/local-project-resolver.test.ts | Adds tests for traversal slugs, inherited entries, operational fs failures, per-resolver cache isolation. |
| src/proxy/hop-by-hop-headers.ts | Introduces strict hop-by-hop and Connection-owned header stripping. |
| src/proxy/hop-by-hop-headers.test.ts | Tests hop-by-hop stripping behavior. |
| src/proxy/error-response.ts | Makes proxy error responses non-cacheable, adds nosniff, and tightens redirect headers. |
| src/proxy/error-response.test.ts | Tests new cache/no-sniff/redirect header behavior. |
| src/proxy/control-plane-signature.ts | Adds method/path-aware route classification and uses shared signature verifiers with audience/project binding. |
| src/proxy/control-plane-signature.test.ts | Adds extensive tests for binding, route classification, and replay rejection. |
| src/proxy/cache/validation.ts | Adds strict cache boundary validation/snapshotting utilities for hostile contracts. |
| src/proxy/cache/types.ts | Changes cache type contract to memory or extension; removes redis inline options. |
| src/proxy/cache/tracing-cache.ts | Snapshots cache ops, validates inputs/outputs, adds close ownership controls. |
| src/proxy/cache/tracing-cache.test.ts | Adds tests for snapshotting, accessor rejection, stat/entry validation, close semantics. |
| src/proxy/cache/memory-cache.ts | Hardens memory cache: validates options, enforces bounds, LRU recency refresh, idempotent close. |
| src/proxy/cache/memory-cache.test.ts | Adds tests for immutable snapshots, accessor rejection, LRU behavior, close behavior, policy validation. |
| src/proxy/cache/index.ts | Implements explicit extension-vs-memory cache selection and fail-closed extension mode. |
| src/proxy/cache/index.test.ts | Adds tests for fail-closed extension mode and ownership semantics. |
| src/proxy/cache/extension-store.ts | Adds explicit extension store acquisition gated by CACHE_TYPE=extension. |
| src/proxy/cache/extension-store.test.ts | Tests acquisition gating, fail-closed behavior, and hostile inputs. |
| src/internal-agents/control-plane-auth.ts | Adds request method/path binding to downstream JWS verification options. |
| src/internal-agents/control-plane-auth.test.ts | Updates tests for canonical path binding and method inclusion. |
| src/extensions/cache/token-cache-store.ts | Updates TokenCacheStats type to memory or extension. |
| src/channels/control-plane.test.ts | Adds method/path binding tests and introduces signature-only verifier tests. |
| src/cache/verified-api-credential-context.test.ts | Updates control-plane request tests to include explicit method. |
| src/cache/backend.test.ts | Updates control-plane request tests to include explicit method. |
| extensions/ext-cache-redis/src/redis-cache.ts | Updates reported cache type to extension. |
| extensions/ext-cache-redis/src/index.ts | Makes missing Redis config a setup error; updates env reading behavior. |
| extensions/ext-cache-redis/src/index.test.ts | Adds manifest parity test and updates behavior to fail-closed on missing config. |
| extensions/ext-cache-redis/README.md | Updates docs for explicit activation and contract details. |
| extensions/ext-cache-redis/deno.json | Marks extension activation as explicit in manifest metadata. |
| docs/guides/extensions.md | Updates extension usage examples and contract family naming. |
| cli/commands/serve/split-mode.ts | Removes implicit Redis env requirements/static env for split mode. |
| cli/commands/serve/split-mode.test.ts | Asserts split mode does not force CACHE_TYPE. |
| cli/commands/serve/proxy-extension-composition.ts | Adds explicit activation composition for standalone proxy cache extension. |
| cli/commands/serve/proxy-extension-composition.test.ts | Tests explicit activation and borrowed store semantics. |
| cli/commands/serve/command.ts | Activates selected proxy cache extension before importing proxy runtime; ensures teardown on startup failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: 706364513b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 98 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/utils/permit-semaphore.ts:107
- The abort-path promise rejection uses
signal?.reasondirectly, which can be any value (including non-Error). To keep error handling consistent for callers, reject with a normalized AbortError/Error instance instead of an arbitrary value.
src/utils/permit-semaphore.ts:71 - When an AbortSignal is already aborted, this throws
signal.reasondirectly. Abort reasons are allowed to be non-Error values, so callers may end up catching a non-Error rejection/throw. Normalize the abort reason to an Error instance (consistent with other abort helpers in the codebase).
This issue also appears on line 105 of the same file.
src/utils/abort.ts:17
createAbortError()/throwIfAborted()appear to duplicate the existing implementations in src/agent/runtime/error-utils.ts. Keeping two copies risks behavioral drift; consider consolidating behind a single shared utility or re-exporting one from the other.
a77f9f8 to
8b6e630
Compare
|
@codex review Please review exact rebased head |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 98 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/utils/permit-semaphore.ts:107
- The queued-abort path can also reject with a non-Error when
AbortSignal.reasonis a string/primitive. Reject with a normalizedErrorso downstream error handling is consistent.
src/utils/permit-semaphore.ts:71 tryAcquire()can throw a non-Error whenAbortSignal.reasonis a string/primitive (because it throwssignal.reasondirectly). That makes abort handling inconsistent and can break callers that assume anErrorinstance. Normalize the abort reason to anErrorbefore throwing.
This issue also appears on line 105 of the same file.
|
@codex review Please review exact corrected head |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 98 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/proxy/request-host.ts:56
normalizeProxyRequestHost()returnsparsed.hostname, which drops IPv6 brackets (for[::1]:8080it returns::1). The accompanying test expects[::1], and preserving brackets is also important if this value is later used as an HTTP Host authority (IPv6 literals require brackets to avoid colon ambiguity).
|
@codex review Please review exact head |
430c487 to
63fe3f1
Compare
|
@codex review Please review exact rebased head |
8ddfab8 to
26eacdd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 103 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/proxy/oauth-client.ts:194
- On caller-driven aborts, a primitive abort reason (like a string) is currently converted into a generic "OAuth token request was aborted" DOMException, dropping the original cancellation message. Preserve a non-empty string reason in the thrown AbortError message (or normalize via
createAbortError(config.signal?.reason)).
src/proxy/oauth-client.ts:119 - When the abort signal carries a primitive reason (e.g. a string), this branch replaces it with a generic DOMException message. That loses caller-provided cancellation context and is inconsistent with
createAbortError()behavior used elsewhere in the proxy. Preserve a non-empty string reason in the thrown AbortError message (or usecreateAbortError(signal.reason)).
This issue also appears on line 189 of the same file.
CodeQL's abstract interpreter does not model hexadecimal BigInt literals and reports them as undefined arithmetic operands (js/implicit-operand-conversion). Match the decimal spelling already used by the FNV-1a constants in src/cache/keys/utils.ts, which the same query accepts. Values and hash outputs are unchanged.
|
Collapsed/suppressed review audit is complete on exact head
Local exact-head validation: focused OAuth/abort 6 tests / 30 steps; proxy 46 files / 455 steps; agent runtime 317 tests / 594 steps; Redis plus CLI composition 3 files / 31 steps; server module/snapshot plus control-plane auth 7 files / 133 steps; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 104 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/proxy/oauth-client.ts:176
- The OAuth success-path content-type check is case-sensitive (
contentType !== "application/json"). HTTP media types are case-insensitive, and other proxy codepaths (for examplesrc/proxy/project-metadata-client.ts) normalize to lowercase before comparing. As written, a valid response likeContent-Type: Application/JSON; charset=utf-8would be rejected and the body would be canceled.
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@kwakayama #3251 is ready for independent code-owner approval at exact head |
Every failure path in verifyInternalControlPlaneSignature returned a bare `false`. An unconfigured verification key, a stripped x-token, a missing signature header and a genuinely bad signature were indistinguishable, with no log line at any of them. That cost real time on veryfront-issue-inbox#356. veryfront-api was not minting the request_method / request_path claims this repo began requiring in 0.1.1189 (#3251), so the bypass never fired and protected environments 302'd every control-plane run to the sign-in page. Nothing in the logs distinguished that from a missing key or a config drift. Failures now carry a reason: missing_x_token, verification_key_not_configured, missing_signature_header or signature_rejected. Ordinary non-internal routes stay silent. Also adds a cross-repo contract test. control-plane-signature.test.ts mints its own compliant JWS, so it proves the verifier works on a good token but never that veryfront-api produces one — which is exactly the gap that let #3251 ship. The new test mints the payload as veryfront-api does and pins both sides together. Refs veryfront-issue-inbox#356
Summary
410 Gone, restrict module-serving namespaces toGETandHEAD, and enforce dependency snapshot consistency for self-hosted library modulesURL.pathname, including a two-phase authenticity then project-binding flow for custom domainsBreaking changes
CACHE_TYPE=redisis no longer accepted. Core supportsmemoryorextension; selectingextensionrequires an already activatedTokenCacheStoreprovider.REDIS_URLalone never activates a provider./_vf_modules/_batchnow returns410 Gone; its broken generator is no longer reachable.GETandHEAD.request_methodandrequest_path. The method must be an uppercase HTTP token. The path must equal the exact canonicalURL.pathnamereceived at verification. Current control-plane routes are root-scoped. Host, scheme, and query are deliberately excluded. Legacy signatures missing either claim fail closed.Security regression coverage
DELETEcancellation while leaving the run activeConnection-owned headers in both proxy deployment modes and retains concurrency permits through hostile body cancellationValidation
deno checkanddeno lint; all 103 changed files:deno fmt --checkdeno task typecheckand consumer typecheck against the built npm declarationsdeno task lint:core-deps, dependency/module boundaries, extension contracts/capabilities, and architecture validationsrc/discovery/auto-discovery.integration.test.tsdeno task verify:quickExisting mainline gate debt
deno task testscansscripts/build/build-npm-extension-packages.ts, whose#dntalias currently exists only inscripts/test.deno.json; this branch does not modify the script or either config