Harden CI gates, ingestion commits, and SECURITY DEFINER grants#696
Conversation
Re-land of the top-3 audit fixes on current main (supersedes #694, which was based on a now-95-commit-stale base). Folds in the CodeRabbit review from #694. CI gate integrity: - Offline RAG grounding preflight runs for every non-docs change in BOTH CI and local verify:pr-local (previously gated on the rag_eval_changed allowlist, so a new retrieval file could silently skip a clinical-safety gate). - Added the type-scale, icon-scale, brand, and function-grant guards to the always-run static-pr job (previously local-only, no CI/test backstop). - Added a ci-change-scope self-test for 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. (#3b from #694 is dropped: main already fails closed on commit errors.) SECURITY DEFINER grants: - Added scripts/check-function-grants.mjs (+ check:function-grants, wired into static-pr and verify:cheap, with tests). Fails if a SECURITY DEFINER public function is executable by PUBLIC/anon. Incorporates the CodeRabbit review: a revoke only protects when its FROM list includes PUBLIC (revoke-from-anon is not enough), and named + schema-wide GRANTs to PUBLIC/anon are detected. Current schema passes (20 functions); overload-by-exact-signature is a noted follow-up. Verified offline: check:ci-scope, check:function-grants, check:type-scale, check:icon-scale, brand:check, function-grants test (9/9), 63 worker/ingestion tests, verify:pr-local dry-run, lint clean on all changed files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ
📝 WalkthroughWalkthroughThe PR adds a security-definer function privilege checker and CI wiring, runs offline RAG evaluation for all non-docs changes, tests expanded scope classification, and prevents workers from committing generations with neither chunks nor searchable images. ChangesFunction grant enforcement
Offline RAG evaluation gating
Empty index commit protection
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)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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: 75f2ebae07
ℹ️ 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".
| const grantNamedRe = | ||
| /grant\s+(?:execute|all(?:\s+privileges)?)\s+on\s+function\s+(?:public\.)?"?([a-z0-9_]+)"?[^;]*?\bto\b[^;]*?\b(?:public|anon)\b/gi; |
There was a problem hiding this comment.
When a migration revokes a SECURITY DEFINER RPC and later grants EXECUTE to authenticated, this regex does not record the reopening, so check:function-grants reports the function safe. An authenticated user can invoke a SECURITY DEFINER function directly, bypassing RLS; for any such RPC lacking its own caller/tenant enforcement, that recreates the cross-tenant access surface this gate is intended to block. Extend the role match to include authenticated (and add a regression fixture).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 66-68: Update the schema-wide grant detection around
schemaWideGrantRe and its lines.some check to evaluate complete SQL statements
rather than individual physical lines, allowing whitespace and newlines between
SQL tokens. Ensure multiline GRANT ... ON ALL FUNCTIONS ... TO public or anon
statements are detected, and add a fixture covering a multiline grant.
- Around line 114-118: The grant parsing around grantNamedRe must capture every
function listed after ON FUNCTION, not only the first routine, and add each
routine name to grantedToAnon. Update the parsing logic to handle
comma-separated, optionally schema-qualified and argument-bearing function
specifications, then add a regression fixture where a non-definer function
appears first and a previously revoked function receives the anon grant.
- Around line 104-131: Update the ACL parsing around revokeRe and grantNamedRe
to retain each mutation’s statement position and target roles, then evaluate
mutations in SQL order so later REVOKE statements remove earlier PUBLIC/anon
grants from effective access. Use the final effective ACL when deciding
vulnerability in the byName loop, and add a regression test covering GRANT
followed by REVOKE for public and anon.
🪄 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: bc348aeb-5933-4367-97cd-d71a737d9aae
📒 Files selected for processing (7)
.github/workflows/ci.ymlpackage.jsonscripts/check-function-grants.mjsscripts/ci-change-scope.mjsscripts/verify-pr-local.mjstests/function-grants.test.tsworker/main.ts
| const schemaWideGrantRe = | ||
| /grant\s+(?:execute|all(?:\s+privileges)?)\s+on\s+all\s+functions\s+in\s+schema\s+public\s+to\s+[^;]*?\b(?:public|anon)\b/i; | ||
| if (lines.some((line) => schemaWideGrantRe.test(line))) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Parse schema-wide grants as complete SQL statements.
Testing one physical line misses valid multiline GRANT ... ON ALL FUNCTIONS ... TO anon statements. Such a grant can re-open every SECURITY DEFINER function while this guard reports success. Parse statement-level SQL with whitespace-flexible matching and add a multiline-grant fixture.
🤖 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 66 - 68, Update the
schema-wide grant detection around schemaWideGrantRe and its lines.some check to
evaluate complete SQL statements rather than individual physical lines, allowing
whitespace and newlines between SQL tokens. Ensure multiline GRANT ... ON ALL
FUNCTIONS ... TO public or anon statements are detected, and add a fixture
covering a multiline grant.
| const revokeRe = | ||
| /revoke\s+(?:execute|all(?:\s+privileges)?)\s+on\s+function\s+(?:public\.)?"?([a-z0-9_]+)"?[^;]*?\bfrom\b([^;]*)/gi; | ||
| const revoked = new Set(); | ||
| let rm; | ||
| while ((rm = revokeRe.exec(sql))) { | ||
| if (/\bpublic\b/i.test(rm[2])) revoked.add(rm[1].toLowerCase()); | ||
| } | ||
|
|
||
| // A named GRANT of EXECUTE to PUBLIC/anon re-opens that function even after a | ||
| // revoke — this is what a migration adding an anon-callable RPC looks like. | ||
| const grantNamedRe = | ||
| /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 = grantNamedRe.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.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Evaluate ACL mutations in statement order.
revoked and grantedToAnon discard positions, so a grant followed by REVOKE ... FROM public, anon, authenticated is still reported as vulnerable even though the final ACL is safe. Retain statement order and role targets when computing effective access; add the inverse ordering test.
🤖 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 104 - 131, Update the ACL
parsing around revokeRe and grantNamedRe to retain each mutation’s statement
position and target roles, then evaluate mutations in SQL order so later REVOKE
statements remove earlier PUBLIC/anon grants from effective access. Use the
final effective ACL when deciding vulnerability in the byName loop, and add a
regression test covering GRANT followed by REVOKE for public and anon.
| const grantNamedRe = | ||
| /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 = grantNamedRe.exec(sql))) grantedToAnon.add(gm[1].toLowerCase()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Inspect every function in a multi-function GRANT.
The regex captures only the first routine after ON FUNCTION. For example, a grant to public.plain(), public.sensitive(uuid) records only plain; if sensitive was previously revoked, the checker can pass despite its new anon grant. Parse all routine specifications and add a regression fixture with a non-definer first.
🤖 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 114 - 118, The grant parsing
around grantNamedRe must capture every function listed after ON FUNCTION, not
only the first routine, and add each routine name to grantedToAnon. Update the
parsing logic to handle comma-separated, optionally schema-qualified and
argument-bearing function specifications, then add a regression fixture where a
non-definer function appears first and a previously revoked function receives
the anon grant.
2 participants
Summary
Re-land of the three highest-risk audit findings on current
main. Supersedes #694, which was based on a now-95-commit-stale base (main has since independently fixed the worker commit-error path). Folds in the CodeRabbit review from #694.CI gate integrity
eval:rag:offline) now runs for every non-docs change in both CI and localverify:pr-local— previously gated on therag_eval_changedallowlist, so a new retrieval file outside the patterns could silently skip a clinical-safety gate.check:type-scale,check:icon-scale,brand:check, andcheck:function-grantsguards to the always-runstatic-prjob (previously local-only inverify:cheap, with no CI or test backstop).ci-change-scopeself-test for the scope-narrowing safety net.Ingestion data-safety (
worker/main.ts)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 executable by PUBLIC/anon. Incorporates the CodeRabbit review: a revoke only protects when itsFROMlist includesPUBLIC(a revoke fromanononly is not enough), and both named and schema-wideGRANT … TO public/anonare detected. Current schema passes (all 20 SECURITY DEFINER functions revoked); overload-by-exact-signature matching is a noted follow-up in the script header.Verification
Run locally and offline — no OpenAI/Supabase/live-DB/hosted-CI calls, per the provider-confirmation boundary.
Passed:
check:ci-scope(self-test incl. the new case),check:function-grants,check:type-scale,check:icon-scale,brand:checktests/function-grants.test.ts— 9/9 (incl. anon-only-revoke and schema-wide-grant regressions)verify:pr-local --dry-runconfirms the offline RAG gate now lists for a non-docs RAG lib filelintclean on all changed files;prettier --checkcleanNot run (and why):
npm run verify:pr-local(full) — its build/client-bundle scan and fulltypecheckneed devDeps not installed in this container (CI'snpm ciinstalls them). Offline constituents run individually.npm run check:production-readiness— not run, to respect the provider boundary; recommend running in CI given the ingestion surface.Clinical Governance Preflight
Change touches ingestion (worker commit path) and CI/deployment config.
Clinical KB Database— unchangedNotes
🤖 Generated with Claude Code
https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Safety & Validation