Harden CI gates, ingestion commits, and SECURITY DEFINER grants#694
Harden CI gates, ingestion commits, and SECURITY DEFINER grants#694BigSimmo wants to merge 2 commits into
Conversation
Addresses the three highest-risk audit findings — silent CI regressions, a blanked/failed clinical index, and anon-callable privileged RPCs. CI gate integrity: - The offline RAG grounding preflight no longer depends on the rag_eval_changed allowlist (a new retrieval file outside the patterns could silently skip it); it now runs for every non-docs change. - Add the type-scale, icon-scale, and brand guards to the always-run static-pr job — they were only in the local verify:cheap chain with no CI or test backstop, so a drift could merge green. - Add a ci-change-scope self-test documenting the scope-narrowing safety net. Ingestion data-safety (worker/main.ts): - Refuse to commit an empty index generation (0 chunks + 0 searchable images), so an atomic reindex can no longer swap a previously-good index for nothing (e.g. an image-only PDF OCR could not read). The job fails and the prior generation stays live. - Restore the missing-schema commit fallback that was stranded after an unconditional throw, guarded by isMissingSchemaError so real commit errors still fail loudly (only fresh/preview envs without the RPC fall back). SECURITY DEFINER grants: - Add scripts/check-function-grants.mjs, which fails if a SECURITY DEFINER public function is left executable by PUBLIC/anon (privilege-escalation / cross-tenant read surface). Wired into static-pr and verify:cheap with a regression test. The current schema passes (all 20 functions revoked); the guard prevents new anon-callable RPCs from shipping unnoticed. Verified offline (no provider/DB/CI calls): check:ci-scope, check:function-grants, check:type-scale, check:icon-scale, brand:check, the new vitest test, lint, and 66 worker/ingestion/reindex tests all pass. Typecheck is clean on every changed file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds CI design-system and function-grant guards, broadens offline RAG preflight execution, adds SQL ACL validation with tests, and prevents document index commits when extraction yields no searchable content. ChangesFunction grant validation
Offline RAG preflight scope
Document index commit safety
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![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: cde21d257f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await upsertIndexQuality(args.quality); | ||
| await deleteStaleIndexGenerationRows(args.documentId, args.indexGenerationId); |
There was a problem hiding this comment.
When a fresh/preview environment lacks commit_document_index_generation, this fallback only writes index quality and deletes stale artifacts. The normal RPC also marks documents.status indexed, updates counts/error state, and replaces document_pages (supabase/schema.sql:1382-1415); the subsequent worker code only patches metadata before completing the job. Thus a successful fallback ingestion leaves the document in processing with no pages (and consequently unavailable to status-gated consumers) while its job is marked completed. Implement those commit operations in the fallback, or continue failing closed until the RPC exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d338c6 — reverted to failing closed. You're right the client-side fallback is incomplete (it doesn't flip documents.status, update counts, or replace document_pages), so a partial commit would strand the document in processing with no pages while the job reports completed. Any commit error now surfaces loudly, including a missing RPC in a fresh/preview env that hasn't applied migrations. The empty-generation gate is unchanged; a complete client-side fallback is deferred to a separate change.
Generated by Claude Code
| // Defaults to the committed snapshot; an explicit path is accepted for tests. | ||
| const SCHEMA_PATH = process.argv[2] ?? "supabase/schema.sql"; |
There was a problem hiding this comment.
This guard reads only the reconciled schema.sql, although the documented deployment path applies supabase/migrations and treats the snapshot as a reference mirror (README.md:50-54). Because the snapshot contains a final blanket revoke, a SECURITY DEFINER function is considered covered whenever that later snapshot line exists; an actual later migration that explicitly grants EXECUTE to anon/PUBLIC would still pass this check even though it runs after the deployed blanket revoke. Inspect migrations in execution order (including explicit GRANT statements), or replay them and assert the resulting function ACLs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9d338c6. Added explicit anon/PUBLIC grant detection: any GRANT EXECUTE … TO public/anon on a SECURITY DEFINER function now fails the check regardless of an earlier revoke (with a regression test). The guard asserts against the reconciled schema.sql, which the drift check keeps in sync with the migration chain, so a migration adding an anon grant surfaces there. Full per-migration ACL replay (execution-order assertion) is noted as a deeper follow-up in the script header.
Generated by Claude Code
…rants P1 (worker/main.ts): the restored client-side commit fallback was incomplete — it wrote index quality and purged stale rows but did not flip documents.status to indexed, update counts, or replace document_pages, so a fallback ingestion would leave the document in `processing` with no pages while its job reported completed. Revert to failing closed: any commit error (including a missing RPC in a fresh/preview env that has not applied migrations) surfaces loudly instead of committing a half-indexed document. The empty-generation gate is unchanged. P2 (check-function-grants.mjs): the guard read only revokes, so a later explicit GRANT of EXECUTE to PUBLIC/anon (which re-opens a function after a revoke) would pass. Detect and fail on any such grant to a SECURITY DEFINER function, and document that the check runs against the reconciled snapshot (full per-migration ACL replay is a deeper follow-up). Added a regression test for the grant case. Verified offline: check:function-grants OK (real schema, 20 functions), the function-grants test (7/7), 32 worker/ingestion tests, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@scripts/check-function-grants.mjs`:
- Around line 84-92: The revoke detection in the script currently accepts any
role-specific revoke; update revokeRe and its parsing logic to match complete
revoke statements and add a function to revoked only when the FROM role list
includes PUBLIC. Preserve support for both EXECUTE and ALL PRIVILEGES forms, and
add a regression fixture covering an anon-only revoke that must not satisfy the
guard.
- Around line 94-115: Update the grant parsing and evaluation in the checker
around grantAnonRe and the byName loop to detect GRANT EXECUTE or ALL ON ALL
FUNCTIONS IN SCHEMA statements targeting PUBLIC or anon, preserve statement
order, and treat affected SECURITY DEFINER functions as explicitly granted
regardless of earlier blanket or per-function revokes. Add a fixture covering
blanket revoke followed by a schema-wide anon grant and verify those functions
are reported as vulnerable.
- Around line 68-92: The grant-checking logic currently collapses overloaded
functions by bare name. Update byName, revoked, and grantedToAnon handling to
parse and normalize each function’s full argument-type signature, including
schema-qualified names, and key all maps/sets by that normalized signature;
ensure revoke and grant matching use the same normalization so
public.lookup(uuid) is evaluated independently from public.lookup(text).
In `@scripts/ci-change-scope.mjs`:
- Around line 115-118: Update the local verification flow in
scripts/verify-pr-local.mjs so eval:rag:offline runs for every non-docs change,
matching the CI condition rather than depending solely on rag_eval_changed.
Ensure the related test for src/lib/hybrid-reranker.ts exercises this case by
asserting the gate runs, or alternatively add that path to ragEvalPatterns and
set rag_eval_changed: true.
In `@worker/main.ts`:
- Around line 545-551: Remove the unreachable post-commit calls to
upsertIndexQuality and deleteStaleIndexGenerationRows, or relocate them before
the successful commit return if they are required after the RPC. Keep the
failure path throwing for commit errors, and ensure no fallback calls remain
after the return/throw branches.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f870eae-a232-4b92-a444-52f70be92751
📒 Files selected for processing (6)
.github/workflows/ci.ymlpackage.jsonscripts/check-function-grants.mjsscripts/ci-change-scope.mjstests/function-grants.test.tsworker/main.ts
| const byName = new Map(); // name -> { earliestIdx, isDefiner } | ||
| creates.forEach((current, i) => { | ||
| const end = i + 1 < creates.length ? creates[i + 1].idx : lines.length; | ||
| const body = lines.slice(current.idx, end).join("\n"); | ||
| const isDefiner = /security\s+definer/i.test(body); | ||
| const prior = byName.get(current.name); | ||
| if (!prior) { | ||
| byName.set(current.name, { earliestIdx: current.idx, isDefiner }); | ||
| } else { | ||
| byName.set(current.name, { | ||
| earliestIdx: Math.min(prior.earliestIdx, current.idx), | ||
| isDefiner: prior.isDefiner || isDefiner, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Names with an explicit per-function execute revoke anywhere in the file. | ||
| // Accepts both `revoke execute on function` and `revoke all [privileges] on | ||
| // function` (both strip the default PUBLIC EXECUTE; the schema uses each form). | ||
| // Lenient name-level match (ignores the exact argument signature) so a correctly | ||
| // revoked function never trips the guard. | ||
| const revokeRe = /revoke\s+(?:execute|all(?:\s+privileges)?)\s+on\s+function\s+(?:public\.)?"?([a-z0-9_]+)"?/gi; | ||
| const revoked = new Set(); | ||
| let rm; | ||
| while ((rm = revokeRe.exec(sql))) revoked.add(rm[1].toLowerCase()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Track function overloads by full signature, not bare name.
byName, revoked, and grantedToAnon collapse every overload into one entry. Revoking public.lookup(uuid) therefore causes an unrevoked public.lookup(text) SECURITY DEFINER overload to pass. Parse and normalize the argument-type signature, then evaluate each overload independently.
Also applies to: 104-115
🤖 Prompt for 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.
In `@scripts/check-function-grants.mjs` around lines 68 - 92, The grant-checking
logic currently collapses overloaded functions by bare name. Update byName,
revoked, and grantedToAnon handling to parse and normalize each function’s full
argument-type signature, including schema-qualified names, and key all maps/sets
by that normalized signature; ensure revoke and grant matching use the same
normalization so public.lookup(uuid) is evaluated independently from
public.lookup(text).
| // Names with an explicit per-function execute revoke anywhere in the file. | ||
| // Accepts both `revoke execute on function` and `revoke all [privileges] on | ||
| // function` (both strip the default PUBLIC EXECUTE; the schema uses each form). | ||
| // Lenient name-level match (ignores the exact argument signature) so a correctly | ||
| // revoked function never trips the guard. | ||
| const revokeRe = /revoke\s+(?:execute|all(?:\s+privileges)?)\s+on\s+function\s+(?:public\.)?"?([a-z0-9_]+)"?/gi; | ||
| const revoked = new Set(); | ||
| let rm; | ||
| while ((rm = revokeRe.exec(sql))) revoked.add(rm[1].toLowerCase()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Only treat revokes from PUBLIC as protective.
revokeRe ignores the FROM roles. A function created after the blanket revoke can use REVOKE EXECUTE ... FROM anon, remain executable through PostgreSQL’s default PUBLIC grant, and still pass this check. Parse the complete statement and require PUBLIC among the revoked roles; add a regression fixture for an anon-only revoke.
🤖 Prompt for 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.
In `@scripts/check-function-grants.mjs` around lines 84 - 92, The revoke detection
in the script currently accepts any role-specific revoke; update revokeRe and
its parsing logic to match complete revoke statements and add a function to
revoked only when the FROM role list includes PUBLIC. Preserve support for both
EXECUTE and ALL PRIVILEGES forms, and add a regression fixture covering an
anon-only revoke that must not satisfy the guard.
| // A later explicit GRANT of EXECUTE to PUBLIC/anon re-opens the function even | ||
| // after a blanket or per-function revoke — this is what a migration adding an | ||
| // anon-callable RPC looks like in the reconciled snapshot. Any such grant on a | ||
| // SECURITY DEFINER function is a violation regardless of earlier revokes. | ||
| const grantAnonRe = | ||
| /grant\s+(?:execute|all(?:\s+privileges)?)\s+on\s+function\s+(?:public\.)?"?([a-z0-9_]+)"?[^;]*?\bto\b[^;]*?\b(?:public|anon)\b/gi; | ||
| const grantedToAnon = new Set(); | ||
| let gm; | ||
| while ((gm = grantAnonRe.exec(sql))) grantedToAnon.add(gm[1].toLowerCase()); | ||
|
|
||
| let definerCount = 0; | ||
| const vulnerable = []; | ||
| for (const [name, info] of byName) { | ||
| if (!info.isDefiner) continue; | ||
| definerCount += 1; | ||
| if (ALLOWLIST.has(name)) continue; | ||
| if (grantedToAnon.has(name)) { | ||
| vulnerable.push({ name, idx: info.earliestIdx, reason: "explicitly grants EXECUTE to PUBLIC/anon" }); | ||
| continue; | ||
| } | ||
| if (coveredByBlanket(info.earliestIdx)) continue; | ||
| if (revoked.has(name)) continue; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Detect schema-wide grants that reopen every function.
A later GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA public TO anon reverses the blanket revoke, but grantAnonRe only recognizes named ON FUNCTION grants. The checker consequently reports covered functions as safe. Track schema-wide grants in statement order and add a failing fixture for this sequence.
🤖 Prompt for 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.
In `@scripts/check-function-grants.mjs` around lines 94 - 115, Update the grant
parsing and evaluation in the checker around grantAnonRe and the byName loop to
detect GRANT EXECUTE or ALL ON ALL FUNCTIONS IN SCHEMA statements targeting
PUBLIC or anon, preserve statement order, and treat affected SECURITY DEFINER
functions as explicitly granted regardless of earlier blanket or per-function
revokes. Add a fixture covering blanket revoke followed by a schema-wide anon
grant and verify those functions are reported as vulnerable.
| // NOTE: rag_eval_changed is an ADVISORY narrowing signal only. The clinical | ||
| // offline-grounding gate (eval:rag:offline) runs for every non-docs change in CI | ||
| // regardless of this flag (see .github/workflows/ci.yml), so a new retrieval file | ||
| // that falls outside these patterns can never silently skip that gate. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Cover the local verifier’s RAG gate as well.
This guarantee is true for .github/workflows/ci.yml, but scripts/verify-pr-local.mjs still runs eval:rag:offline only when rag_eval_changed is true. For src/lib/hybrid-reranker.ts outside ragEvalPatterns, local verification can therefore skip the preflight, and this test does not detect it because it omits rag_eval_changed.
Either make local verification use the same non-docs condition as CI, or include this path in ragEvalPatterns and assert rag_eval_changed: true here.
Also applies to: 414-421
🤖 Prompt for 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.
In `@scripts/ci-change-scope.mjs` around lines 115 - 118, Update the local
verification flow in scripts/verify-pr-local.mjs so eval:rag:offline runs for
every non-docs change, matching the CI condition rather than depending solely on
rag_eval_changed. Ensure the related test for src/lib/hybrid-reranker.ts
exercises this case by asserting the gate runs, or alternatively add that path
to ragEvalPatterns and set rag_eval_changed: true.
| // Fail CLOSED on any commit error, including a missing commit RPC (a | ||
| // fresh/preview env that has not applied migrations). The client-side path | ||
| // below cannot fully reproduce the RPC — which also flips documents.status to | ||
| // indexed, updates counts, and replaces document_pages — so running it as a | ||
| // fallback would leave the document in `processing` with no pages while its | ||
| // job reports completed. Surfacing the error is safer than a half-indexed | ||
| // commit; a complete client-side fallback is tracked separately. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove or relocate the unreachable post-commit calls.
The success path returns at Line 544, and the new error path throws at Line 552, so upsertIndexQuality and deleteStaleIndexGenerationRows at Lines 553-555 can never execute. If they are still required after a successful RPC, move them before the success return; otherwise remove the obsolete fallback remnants.
🤖 Prompt for 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.
In `@worker/main.ts` around lines 545 - 551, Remove the unreachable post-commit
calls to upsertIndexQuality and deleteStaleIndexGenerationRows, or relocate them
before the successful commit return if they are required after the RPC. Keep the
failure path throwing for commit errors, and ensure no fallback calls remain
after the return/throw branches.
|
Superseded by #696 and closing in its favor. This branch was based on a main that is now ~95 commits stale, and main has since independently fixed the worker commit-error path (it fails closed on any commit error), so this branch no longer merges cleanly. The still-relevant changes have been re-landed on current
Please review #696 instead. Generated by Claude Code |
2 participants
Summary
Implements the three highest-risk findings from the repository audit — the ones that can actually cause harm (silent CI regressions, a blanked/failed clinical index, and anon-callable privileged RPCs). Scoped to these three; the remaining audit findings are deferred to follow-up PRs.
CI gate integrity
eval:rag:offline) no longer depends on therag_eval_changedallowlist — a new retrieval file outside the scope patterns could silently skip a clinical-safety gate. It now runs for every non-docs change (thesafetyjob already runs then, and the check is offline + cheap).check:type-scale,check:icon-scale, andbrand:checkguards to the always-runstatic-prjob. They previously lived only in the localverify:cheapchain with no CI or unit-test backstop, so a drift could merge green.ci-change-scopeself-test documenting the scope-narrowing safety net.Ingestion data-safety (
worker/main.ts)throw, guarded by the existingisMissingSchemaErrorhelper so real commit errors still fail loudly; only fresh/preview envs without the RPC take the fallback (production has the RPC, so the healthy path is unchanged).SECURITY DEFINER grants
scripts/check-function-grants.mjs(+check:function-grants, wired intostatic-prandverify:cheap, withtests/function-grants.test.ts). It fails if a SECURITY DEFINERpublicfunction is left executable by PUBLIC/anon — the privilege-escalation / cross-tenant-read surface. The current schema passes (all 20 SECURITY DEFINER functions are revoked); the guard stops a new anon-callable RPC from shipping unnoticed.Verification
Everything below was run locally and offline — no OpenAI, Supabase, live-DB, or hosted-CI calls, per the repo's provider-confirmation boundary.
Passed:
check:ci-scope(self-test incl. the new case)check:function-grants— OK, all 20 SECURITY DEFINER functions revokedcheck:type-scale,check:icon-scale,brand:check(the newly CI-wired guards — green on the current tree)tests/function-grants.test.ts— 6/6linton all changed files;typecheckclean on every changed fileNot run (and why):
npm run verify:pr-local— its build/client-bundle scan and fulltypecheckneed devDeps not installed in this container (@testing-library/react,@axe-core/playwrightare declared but absent; CI'snpm ciinstalls them). The offline constituents above were run individually.npm run eval:retrieval:quality— not applicable: no retrieval/ranking/selection/chunking/scoring behavior changed (the RAG change is CI scheduling only).npm run check:production-readiness/check:deployment-readiness— not run, to respect the provider boundary; recommend running in CI / with approval before merge given the ingestion + deployment-config surface.Clinical Governance Preflight
Change touches ingestion (worker commit path) and CI/deployment config.
Clinical KB Database— unchangedNotes
worker/main.tsmissing-schema fallback restores documented intent; the ingestion-worker owner should confirm the client-side fallback is complete for a truly RPC-missing env (it only runs in non-production preview/fresh envs).record_source_reviewis correctly protected (viarevoke all on function).🤖 Generated with Claude Code
https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Security
Tests