fix(proxy): name the reason a control-plane signature was rejected - #3357
Conversation
|
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: 8 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)
📝 WalkthroughWalkthroughControl-plane signature verification now returns typed rejection reasons, emits optional structured warnings, and receives the proxy logger through request handling. Contract tests cover API-style Ed25519 JWS requests, claim validation, rejection diagnostics, and silent non-internal routes. ChangesControl-plane authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Handler
participant CandidateAuth
participant SignatureVerification
participant Logger
Handler->>CandidateAuth: verify control-plane candidate
CandidateAuth->>SignatureVerification: validate token and request claims
SignatureVerification-->>CandidateAuth: success or rejection reason
CandidateAuth->>Logger: warn for rejected internal request
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
a154714 to
3d629b8
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves diagnosability of internal control-plane signature verification in the proxy by emitting a structured rejection reason (only for internal-route candidates), and adds a cross-repo contract test to prevent drift between veryfront-api’s minted JWS claims and the proxy verifier.
Changes:
- Add optional logger plumbing so signature verification can log a specific rejection reason without changing verification behavior.
- Refactor internal signature verification to return a typed rejection reason internally and log it for internal-route candidates.
- Add an API-contract test that mints a JWS in the same shape as
veryfront-apiand validates required request binding claims and replay protection.
Verification (reported in PR description; not run in this review):
deno test src/proxy/(passed)deno lint(changed files, clean)deno check(changed files, clean)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/proxy/handler.ts | Passes the proxy logger into internal control-plane candidate/verification helpers so failures can be logged with a reason. |
| src/proxy/control-plane-signature.ts | Introduces rejection-reason classification and logs structured details (reason, method, pathname, optional audience) via an optional logger. |
| src/proxy/control-plane-signature.api-contract.test.ts | Adds a cross-repo contract test that mints an API-style JWS and asserts acceptance/rejection behavior and logging reasons. |
Suppressed comments (1)
src/proxy/control-plane-signature.api-contract.test.ts:195
- For the non-internal route silence assertion, use the same obviously-test hostname style as other proxy tests (and the RUN_STREAM_URL above) to keep fixtures consistent and avoid introducing additional real-looking domains.
);
});
it("stays silent for ordinary non-internal routes", async () => {
const reasons: string[] = [];
const pageUrl = "http://slug.preview.veryfront.org/";
await isAuthenticInternalControlPlaneCandidate(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 `@src/proxy/control-plane-signature.api-contract.test.ts`:
- Around line 2-3: Update the imports in the control-plane signature contract
test to use the required explicit helper paths: import assertEquals from
`#veryfront/testing/assert.ts` and afterEach, describe, and it from
`#veryfront/testing/bdd.ts`.
- Around line 12-14: Update the contract fixture constants and related claims to
use reserved synthetic values instead of the preview hostname and
fixture-specific identifiers for RUN_STREAM_URL, aud, project_id, and the run
ID. Preserve the existing URL method/path relationships and ensure all related
assertions remain consistent with the replacement values.
🪄 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: 134cc08f-cf7d-4758-acf8-f94eed0df51c
📒 Files selected for processing (3)
src/proxy/control-plane-signature.api-contract.test.tssrc/proxy/control-plane-signature.tssrc/proxy/handler.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/proxy/control-plane-signature.api-contract.test.ts:15
- RUN_STREAM_URL uses what looks like a real preview hostname. Tests should avoid hardcoding real service/project identifiers to reduce accidental leakage and to keep the fixture clearly synthetic. Use a reserved test domain (for example .invalid) and a generic slug.
const RUN_STREAM_URL =
"http://outlook-agent-hvjoe9.preview.veryfront.org/api/control-plane/runs/r_1/stream";
const encoder = new TextEncoder();
src/proxy/control-plane-signature.api-contract.test.ts:58
- The contract fixture hardcodes a specific-looking project slug (aud) and UUID (project_id). Even in tests, prefer obviously synthetic identifiers so the fixture cannot be mistaken for real customer/project data.
aud: "outlook-agent-hvjoe9",
sub: "r_1",
surface: "studio",
project_id: "979f3e04-e951-4807-8aa8-98530d9b8ba1",
request_hash: await sha256Base64url(body),
src/proxy/control-plane-signature.api-contract.test.ts:106
- verifyApiStyleRequest mutates CHANNEL_DISPATCH_SIGNING_PUBLIC_KEY and the surrounding afterEach unconditionally deletes it. With deno test --parallel (as used in this repo), this can leak state across tests and also clobber a pre-existing value set by other suites. Prefer saving the previous value and restoring it in a try/finally local to the helper (or use the same save/restore pattern as control-plane-signature.test.ts).
const body = JSON.stringify({ messages: [{ role: "user", content: "hi" }] });
const { jws, publicKeyPem } = await mintApiStyleJws(body, claimOverrides);
Deno.env.set(PUBLIC_KEY_ENV, publicKeyPem);
const req = new Request(RUN_STREAM_URL, {
src/proxy/control-plane-signature.api-contract.test.ts:157
- reasonFor only sets CHANNEL_DISPATCH_SIGNING_PUBLIC_KEY when the chosen value is truthy. This means the test case that passes publicKeyPem: "" never actually sets the env var to an empty string, so it doesn't cover the intended "configured but empty" scenario (it behaves like the var is unset). Also, the suite-level afterEach deletes the var instead of restoring any pre-existing value; restoring locally in reasonFor avoids cross-test interference.
Deno.env.delete(PUBLIC_KEY_ENV);
if (built.publicKeyPem ?? publicKeyPem) {
Deno.env.set(PUBLIC_KEY_ENV, built.publicKeyPem ?? publicKeyPem);
}
veryfront-code is public. The fixture carried a real project slug and project UUID; replace them with the placeholders the sibling signature test already uses (protected / proj-1), and switch the test-helper imports to the explicit .ts paths the guidelines require. Addresses review feedback on #3357.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/proxy/control-plane-signature.ts:276
missing_x_tokenis returned (and logged) for any request to an internal control-plane route withoutx-token, even when there is no signature header present. Because these endpoints are internet-reachable via the proxy, this can produce high-volume warn logs from routine probes/scans that are not actually attempting a signed internal bypass. Consider only treating this as a reportable internal-rejection when at least one signature header is present; otherwise returnnot_an_internal_route(so it stays silent) while still failing closed.
// The candidate only matters when there is an x-token to use for metadata
// lookup or forward after the resolved project binding succeeds.
if (!req.headers.get("x-token")) return "missing_x_token";
src/proxy/control-plane-signature.api-contract.test.ts:146
- In
reasonFor, theif (built.publicKeyPem ?? publicKeyPem)guard prevents settingCHANNEL_DISPATCH_SIGNING_PUBLIC_KEYwhenpublicKeyPemis an empty string. That makes the "unconfigured verification key" case behave like the env var is unset, and it never exercises the explicit empty-string configuration path (even thoughgetHostEnv/!publicKeyPemtreats both as not configured). Prefer setting the computed key unconditionally (or checking!== undefined) sopublicKeyPem: ""does what it says.
Deno.env.delete(PUBLIC_KEY_ENV);
if (built.publicKeyPem ?? publicKeyPem) {
Deno.env.set(PUBLIC_KEY_ENV, built.publicKeyPem ?? publicKeyPem);
}
Addresses review on #3357. B1 — unauthenticated log amplification. The warn logged url.pathname verbatim. Two admissible route patterns carry an unbounded runId segment, so a client with no headers at all could POST an 8KB path and write one 8KB line per request into log ingest, drowning the signal this feature exists to surface. Now: pathname bounded to 256 code units, and the two 'missing' reasons are logged only when the caller actually presented a signature header. Anonymous traffic is silent; every reason still fires for a real internal caller. B2 — the contract test asserted through the unbound candidate check, which verifies neither aud, project_id nor request_hash. A JWS with a wrong audience and a hash of a different body was accepted. The earlier identifier scrub is the proof: it changed aud and project_id and nothing failed. Now asserts through isVerifiedInternalControlPlaneRequest with a binding, with explicit rejection cases for a drifted aud and project_id, plus body-binding cases through resolveVerifiedControlPlaneBranchBinding that pin request_hash. Also: rename not_an_internal_route -> route_not_admissible, since reserved routes are internal but inadmissible; export the reason union so dashboards do not couple to an unexported type; assert exactly one warn per rejection; drop a redundant undefined-deletion loop; document the logging side effect in the module header.
Adversarial review — round 1: 78/100, two blocking issues. Both now fixed.Behaviour preservation — verified branch by branchThe central question for this file, since it gates the protected-environment auth bypass and Guard order and short-circuit order untouched. Both terminal branches keep a character-identical options literal, and the There is no input for which the new code returns true where the old returned false. The success sentinel is BLOCKING 1 — unauthenticated log amplification (fixed)The warn logged These classify as ~8000× amplification per request into billed log ingest, with no sampling or rate limiting anywhere in Fixed: BLOCKING 2 — the contract test did not pin half the claim set (fixed)It asserted through The earlier identifier scrub is itself the proof: it changed Fixed: asserts through Non-blocking, also fixed
Deliberately deferred
It needs Also deferred: the silent empty-audience rejection (unreachable from all current call sites), and an Verified good
Current state
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/proxy/control-plane-signature.ts:359
shouldLogRejection()usesreq.headers.get(header)in a truthiness check. If a signature header is present but has an empty value (e.g.x-veryfront-control-plane-jws:),get()returns "" and this will incorrectly treat it as absent, suppressing the rejection log even though the caller attempted to authenticate. Usingheaders.has()avoids this edge case without changing the privacy/amplification goal.
function shouldLogRejection(req: Request, rejection: InternalControlPlaneRejection): boolean {
if (rejection === "route_not_admissible") return false;
if (rejection === "verification_key_not_configured") return true;
return INTERNAL_CONTROL_PLANE_SIGNATURE_HEADERS.some((header) => req.headers.get(header));
…Access deno check failed on the test added in the previous commit: with noUncheckedIndexedAccess, pathnames[0] is string | undefined, so pathnames[0].length is TS2532. The test-typecheck ratchet in the ci (lint) chain gates on this. Optional-chain it — the preceding length assertion already guarantees presence. Also: - shouldLogRejection uses headers.has rather than get() truthiness, so a set-but-empty signature header counts as presented and its rejection is logged rather than silently dropped. - Name sub and surface in the test as deliberately unbound at the proxy: the run id is already pinned through request_path, and surface is only checked for membership of CONTROL_PLANE_SURFACES.
Round 2: 84/100 — one new blocking issue, introduced by the round-1 fix itself. Now fixed → 93.B1 and B2 — verified fixed, by attack rather than inspectionThe reviewer re-ran their own exploits against the fix: And re-ran the B2 acceptance proof through the bound entry point the test now uses:
Behaviour preservation re-confirmed at the new tip, not on inspection: every probe case was run twice, once with a logger and once without, and the verdicts compared identically. BLOCKING — the round-1 fix did not typecheck (fixed)The repo sets My own miss: I ran One correction to the review recordThe reviewer reported that the ratchet also fails on a pre-existing entry, Half right, and the conclusion does not follow. That file genuinely has 2 type errors — I confirmed it directly. But it is grandfathered in
Non-blocking, also fixed
Deferrals — all three accepted, one strengthenedThe reviewer withdrew the implication that N1 (splitting N3 and N8 remain cosmetic. Verification at the merged tip
|
Two corrections to my review comment aboveFinal review came back 93/100. Both of these are corrections to my explanations, not to the merged code. 1. I explained the
|
Problem
verifyInternalControlPlaneSignaturereturns a barefalseon every failure path. An unconfigured verification key, a strippedx-token, a missing signature header and a genuinely bad signature all produce byte-identical output, with no log line at any of them.This cost real time on veryfront-issue-inbox#356.
0.1.1189(#3251) added the requiredrequest_method/request_pathclaims; veryfront-api was never updated to mint them, so the signed-internal bypass stopped firing and the proxy 302'd every control-plane run on a protected environment to the sign-in page. The API followed the redirect, got 200 plus ~31KB of HTML, parsed zero SSE events, and failed the run asRUNTIME_TERMINAL_EVENT_MISSING.Nothing in the proxy logs distinguished that from a missing key or config drift. Diagnosis needed a cross-service log correlation plus two source dives.
Changes
Reason codes. Rejections now carry
missing_x_token,verification_key_not_configured,missing_signature_header, orsignature_rejected, logged with method, pathname, and audience. Ordinary non-internal routes stay silent — every page request passes through here.The logger is an optional parameter, so the exported signatures stay source-compatible and the verification logic is unchanged.
Cross-repo contract test.
control-plane-signature.test.tsmints its own JWS that always carriesrequest_method/request_path. It proves the verifier works on a compliant token, but never that veryfront-api produces one — exactly the gap that let #3251 ship a breaking change unnoticed. The new test mints the payload as veryfront-api'screateControlPlaneRequestSignaturedoes and pins both sides together, plus guards the operation binding against replay across routes.Verification
deno test src/proxy/deno lint(changed files)deno check(changed files)Tested with Deno 2.7.7; 2.9.x fails this harness on an unrelated
node:util/typesbrand-check error.No version bump — recent fix PRs leave that to the
release:PRs.Companion
veryfront-api#4255 adds the missing claims. That is the actual fix; this PR makes the next occurrence diagnosable from a single log line, and stops the contract from drifting again.
Refs veryfront-issue-inbox#356
Summary by CodeRabbit
Security
Monitoring
Quality