audit fixes (P19): cap anonymous provider spend, bound the proxy claims token, scope eval capture - #2618
Conversation
Per-caller and per-bucket anonymous limits bound one caller, not total spend: answer 30/min + clinical_ask 20/min + speech_transcription 12/min gave 62 paid calls a minute before anything said no, and every caller without a trusted forwarding header shares one unknown-ip bucket, so rotating identities multiplied paid OpenAI calls without limit. Adds one aggregate durable bucket (anon:generation:aggregate, 300 requests per 3600 s) drawn down by every anonymous generation path including streamed document summaries, denied with a distinct 429 code anonymous_generation_ceiling so an operator can tell a single abusive caller from the site hitting its spend ceiling. Authenticated callers never consume or observe it. Trade-offs accepted: the aggregate costs one extra durable-limiter RPC per anonymous generation request (consumed last, so a caller already denied by a narrower limit does not pay for it); the window is deliberately long because the existing per-minute ceilings already bound bursts, making this a sustained-spend cap; and the aggregate row reuses the existing consume_api_subject_rate_limit RPC with a non-bucket key, so no migration and no live database change. Two existing tests pinned the exact anonymous RPC call count; both were updated to assert the new aggregate call as well rather than relaxed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
getProxyAuthSecret() read SUPABASE_SERVICE_ROLE_KEY || INTERNAL_SERVICE_KEY, a secret name declared in no Zod schema, no .env.example, no doc and no env-parity known-name list — an undocumented credential that only tests exercised. It also signed with the database's master credential directly, and the signed envelope carried no expiry, so a header captured from a log or from any future proxy matcher gap would be a bearer credential for the life of the key. Removes the undeclared fallback outright (the preferred option in the finding); derives a dedicated signing key by HKDF-SHA256 of the service-role key under a fixed label, so a signing oracle is no longer an oracle on the database key; and stamps an issued-at into the signed message, verified inside a 60 s window with 5 s of future skew. The signature comparison stays constant-time. Trade-offs accepted: the envelope becomes three parts and the previous two-part form is rejected rather than accepted for compatibility — the proxy and the route handler are the same deployment signing and verifying within one request, so no in-flight header outlives a deploy. A 60 s window is a replay bound, not replay prevention: it does not stop reuse inside that window, which would need a nonce store this design deliberately avoids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
/api/eval-cases stored top_chunk_ids, cited_chunk_ids and top_files on shape alone: UUID-shaped ids and free strings went into rag_query_misses verbatim, while only expected_document_id and expected_chunk_id were ownership-checked. A promoted eval case could therefore name chunks the owner cannot read or that do not exist, and that row is input to the offline eval loader. Filters both id arrays and the file names through the same owner-scoped document check the expected-* fields already use, batched (two reads for the ids, one for the file names, regardless of array length), and counts the removals in the same metadata tallies as malformed ids, adding top_files_rejected alongside them. Ownership validation only: no scoring, ranking, ordering, fixture, harness or src/lib/rag change. Trade-off accepted: the check is owner-scoped exactly like the existing expected-* check, so a capture naming a public-corpus document is recorded as rejected rather than stored — consistent with the rule already in force for the expected document, and visible in the rejected counts rather than silent. RAG impact: no retrieval behaviour change — eval-case capture validates ownership only; no ranking, fixture or harness scoring change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
mobile-webkit and mobile-pwa-standalone are named by no package script and no CI step, so they run only when the release matrix falls back to the whole suite — and docs/testing.md described that matrix as Chromium shards plus Firefox and WebKit, which made the gap invisible to a reader. Chose the documented-decision option over promoting them, on the repository's own timing evidence: each project replays the entire production journey suite on an emulated iPhone 14, the last recorded complete matrix (run 4012) took 38 minutes for three such suites, the job carries a 70-minute timeout, and ci-operations.md records a ~70-minute matrix holding main. Two more full suites is roughly another 25 minutes on a job already at its ceiling, and the blocking PR gate this finding points at runs sharded Chromium only. The phone behaviour they would prove already has a focused blocking owner in verify:phone-chrome. Trade-off accepted and stated in the record: cross-engine phone rendering stays unproven between full-suite matrix runs, since the focused gate is Chromium. The promotion path (the matrix's ordinary-path project list in ci.yml, owned by another package) is named so the decision can be reversed deliberately. Adds tests/playwright-project-cadence.test.ts, which fails closed for any Playwright project selected by no script and recorded in no cadence line, so this kind of silent project cannot recur. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
The L23 ownership filter added an unscoped document_chunks read: the capture body names candidate chunk ids, so the lookup cannot carry owner_id on its own chain. The mechanical tenancy scan correctly refused it as an undeclared derived query. Declared with proof kind parent-document-verified, which the scanner re-checks in the AST rather than accepting on trust: the chunk rows are intersected with a documents read filtered on owner_id in the same function, and only chunk ids whose document appears in that owner-scoped result are returned. tests/retrieval-owner-filter-guard.test.ts: Tests 57 passed (57). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8d59a262-c0c4-4784-81d3-dcdb2da85e8a) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9cb2e75f-ab4f-4096-8f10-085dbf62ae87) |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #15687 (success). That run's conclusion is an aggregate and did not exercise Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Summary
Audit remediation package P19 — Security design items, from
docs/audit/full-repository-audit-2026-09-02.md(PR #2573). Findings fixed:L42,L27,L23,L68. Written test-first in this session; every fix was proved red-then-green before commit.c16572307security: cap total anonymous provider-backed generation (L42)a47648371security: bound and re-key the proxy claims header (L27)68126e54eapi: validate eval-capture chunk and file ownership (L23)984a3e61bdocs: record why the iPhone Playwright projects stay release-only (L68)d6a5eb94ftenancy: declare the eval-capture chunk lookup's ownership proof (L23)L42protects provider spend. The answer, streaming-answer, Clinical Ask and speech-transcription routes accept anonymous callers, limited only per hashed client IP — and every caller without a trusted forwarding header shares oneunknown-ipbucket. Nothing capped total anonymous spend, so a rotating-address caller had no ceiling. There is now one shared durable bucket (anon:generation:aggregate) drawn down by all four routes on top of the existing per-caller limits, failing closed with a distinctanonymous_generation_ceilingcode and a "sign in" message.Ceiling: 300 requests per 3600 s, across all anonymous callers and all generation routes. The existing per-minute limits already bound bursts (62/min combined); this is the sustained-spend cap, roughly 5/min. Documented beside the rate-limit tables in
src/lib/api-rate-limit.tsand operator-side indocs/launch-operator-runbook.md. No migration — it reusesconsume_api_subject_rate_limitwith a non-bucket key.L27removed the undeclaredINTERNAL_SERVICE_KEYfallback (a secret name present in no Zod schema, no.env.example, no doc, and no env-parity list — only in tests), derived a dedicated signing key by HKDF-SHA256 of the service-role key under a fixed label rather than using that key directly, and gave the claims envelope aniatwith a 60 s verification window (5 s future skew). The compare stays constant-time.L23filterstop_chunk_ids,cited_chunk_idsandtop_filesthrough the owner-scoped document check before insert, with rejections counted alongside the existing malformed-id tally. No scoring, fixture or harness change.L68— decided rather than deferred. The iPhone browser projects stay release-only. Promoting them would need an edit toci.yml, which another package owns, and the timing evidence rules it out regardless: each iPhone project replays the whole production suite, the last recorded full matrix took 38 minutes for three such suites against a 70-minute job timeout, anddocs/ci-operations.mdrecords a ~70-minute matrix holdingmain. Phone behaviour already has a focused blocking owner inverify:phone-chrome. Recorded indocs/testing.mdwith a cadence table, the accepted residual risk, and a new fail-closed test so no Playwright project can go silently unused again.Findings in this package not fixed here, with reasons:
L41— pinning the Node andghtarball checksums requires fetching upstreamSHASUMS, a network call this work was not authorised to make.RAG impact: no retrieval behaviour change — eval-case capture validates ownership only; no ranking, ordering, selection, fixture or harness scoring change, and no file under
src/lib/rag/**was touched.Verification
npm run verify:pr-local—- failed: (none)·- not reached: (none)npm run check:diff-integrity—[diff-integrity] PASS — 5 changed test file(s), 23 -> 39 test case(s). No test deleted, skipped or weakened.tests/api-rate-limit-fallback.test.tspinned the anonymous RPC call count; both cases now expect one MORE call (2→3 and 1→2) plus a new assertion that the aggregate bucket is consumed. Verified by reading the diff.L42failed withbucket answer must consume the aggregate ceiling: expected [] to have a length of 1 but got +0, thenTests 6 passed.L27failed withexpected [ …(2) ] to have a length of 3and a stale-headerexpected '…' to be null, thenTest Files 2 passed / Tests 13 passed.L23failed withexpected [ …(2) ] to deeply equal [ Array(1) ], thenTests 9 passed.L68failed withPlaywright project "mobile-webkit" is selected by no npm script and documented in no cadence record, then 2 passed.npx vitest runover all 12 touched suites —Test Files 12 passed (12) / Tests 264 passed (264);tsc --noEmitexit 0;eslint --max-warnings 0exit 0; Prettier clean.npm run check:owner-scope—✓ owner-scope phase 1: 60 src/app/api files clean against 26 owner-scoped tables.·✓ owner-scope phase 2: 87 direct, 18 user-keyed, 30 derived-tier, 1 untiered-table and 1 dynamic-table-dispatch queries scoped on their chain or declarednpm run plan:browser -- --dry-run— levelfull; not run, browser proof left to CI.One point worth a reviewer's attention. The
L23ownership filter must look up chunks by the ids the caller supplies, so that query cannot carry an owner filter on its own chain. The repository's mechanical tenancy scan correctly refused it as an undeclared derived query, failing the gate. It is now declared inscripts/lib/tenancy-scan.mjsunder proof kindparent-document-verified— a kind the scanner re-checks in the AST rather than accepting on trust. The proof: the chunk rows are intersected with adocumentsread filtered on.eq("owner_id", ownerId)in the same function, and only chunk ids whose document appears in that owner-scoped result are returned.Verification not run:
npm run verify:ui— browser proof left to CI; the pinned Chromium is not installed in this container.Verification not run:
npm run verify:release— no release or handoff confidence is claimed.Verification not run: provider-backed gates — nothing here reaches OpenAI, Supabase, Railway or Sentry at run time; all work was offline.
Note:
origin/mainwas merged in (not rebased) after the gate ran, to pick up #2610. The merge was clean, no file overlaps this package's diff, and the three primary suites were re-run afterwards:Test Files 3 passed (3) / Tests 72 passed (72).Risk and rollout
L42can refuse anonymous generation that previously succeeded — that is the intent, and signed-in callers are unaffected.L27changes an internal signing key derivation and adds an expiry, so a stale in-flight header is now rejected after 60 s; the header is stripped unconditionally at the proxy boundary, so there is no external contract to break.L23narrows what an administrator can capture into eval cases.Clinical Governance Preflight
unchanged; no citation, verification or answer-content path touched.
none introduced or expanded.
Clinical KB Database(sjrfecxgysukkwxsowpy)no Supabase env value, migration target or configured project changes; no migration ships here.
tightened: the service-role key is no longer used directly as a signing secret, and its undeclared fallback secret name is removed.
unchanged.
unchanged.
reviewed; no decision-support behaviour changes — these are access, spend and ownership controls.
Notes
.github/workflows/ci.ymlwas deliberately not touched; another package owns it.🤖 Generated with Claude Code
https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Generated by Claude Code
Note
Medium Risk
Changes affect anonymous API availability (new hard spend ceiling), internal auth header verification (stale headers fail), and what admins can persist in eval cases; authenticated core paths are mostly unchanged but misconfiguration could block legitimate anonymous use.
Overview
Audit remediation (P19) tightens anonymous provider spend, internal proxy auth, and eval-case capture tenancy, plus documents Playwright iPhone project cadence.
Anonymous generation ceiling (L42).
api-rate-limit.tsadds a site-wide durable bucket (300 requests / hour) shared by anonymousanswer,clinical_ask,speech_transcription, and streamed document summaries, applied after per-caller limits. Over-limit responses use429with codeanonymous_generation_ceilingand a sign-in message (distinct fromrate_limited). Authenticated traffic is unchanged. The operator runbook now states this is the enforced cap, not spend alerts alone.Proxy claims header (L27).
proxy-auth-crypto.tsderives an HMAC key via HKDF from the service-role key (no direct signing, noINTERNAL_SERVICE_KEYfallback), stampsissued-atin a three-part envelope, and rejects stale or legacy two-part headers within a 60s window.Eval capture ownership (L23).
eval-cases/route.tsfilterssourceChunkIds,citedChunkIds, andsourceFilesthrough batchedownedChunkIdSet/ownedFileNameSetchecks againstdocuments.owner_id; rejections are tallied in metadata (top_files_rejected, existing chunk reject counts). The tenancy scanner registers the chunk lookup underparent-document-verified.Testing docs (L68).
docs/testing.mdadds a Playwright project cadence table and rationale for keepingmobile-webkitandmobile-pwa-standalonerelease-only;playwright-project-cadence.test.tsfails if any project lacks script or doc coverage.Reviewed by Cursor Bugbot for commit 5fb9a68. Configure here.