Harden CI gates, ingestion commits, and SECURITY DEFINER grants #694
+315
−2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| #!/usr/bin/env node | ||
| // Guards the function-privilege convention against silent regressions. | ||
| // | ||
| // A SECURITY DEFINER function runs with its owner's privileges and bypasses RLS. | ||
| // If such a function in the `public` schema is executable by PUBLIC/anon, an | ||
| // unauthenticated caller can invoke it directly — e.g. a retrieval RPC that takes | ||
| // an `owner_filter` argument — and read across tenants. Postgres grants EXECUTE to | ||
| // PUBLIC by DEFAULT on every new function, so protection must be explicit. Today | ||
| // that protection is manual (a schema-wide blanket revoke plus per-function | ||
| // revokes), with nothing stopping a new SECURITY DEFINER RPC from shipping | ||
| // anon-callable. This check is that stop. | ||
| // | ||
| // Invariant enforced against the reconciled snapshot supabase/schema.sql: | ||
| // Every SECURITY DEFINER function in `public` must be non-executable by PUBLIC — | ||
| // i.e. either a schema-wide `revoke execute on all functions ... from public` | ||
| // runs AFTER its definition, or it has its own `revoke execute on function | ||
| // <name> ... from public`. Otherwise it is reported and CI fails. | ||
| // | ||
| // Scope note: SECURITY INVOKER functions run as the caller and stay bound by RLS, | ||
| // so they are out of scope here; this guard targets the escalation surface. It | ||
| // asserts the invariant against the reconciled snapshot (schema.sql is kept in | ||
| // sync with the migration chain by the drift check) and also flags any explicit | ||
| // GRANT of EXECUTE to PUBLIC/anon; asserting live per-migration ACLs by replaying | ||
| // the whole chain is a deeper follow-up. | ||
| import { readFileSync } from "node:fs"; | ||
|
|
||
| // Defaults to the committed snapshot; an explicit path is accepted for tests. | ||
| const SCHEMA_PATH = process.argv[2] ?? "supabase/schema.sql"; | ||
|
|
||
| // SECURITY DEFINER functions intentionally left without a dedicated revoke. | ||
| // Add an entry ONLY with a concrete reason (prefer fixing over allowlisting). | ||
| const ALLOWLIST = new Map([ | ||
| // ["function_name", "why this is safe / follow-up ticket"], | ||
| ]); | ||
|
|
||
| function main() { | ||
| const sql = readFileSync(SCHEMA_PATH, "utf8"); | ||
| const lines = sql.split("\n"); | ||
|
|
||
| const blanketRe = /revoke\s+execute\s+on\s+all\s+functions\s+in\s+schema\s+public\s+from\s+[^;]*\bpublic\b/i; | ||
| const blanketIdxs = []; | ||
| lines.forEach((line, idx) => { | ||
| if (blanketRe.test(line)) blanketIdxs.push(idx); | ||
| }); | ||
|
|
||
| if (blanketIdxs.length === 0) { | ||
| console.error( | ||
| `check:function-grants: FAIL — no schema-wide "revoke execute on all functions in schema public from ... public" ` + | ||
| `statement found in ${SCHEMA_PATH}. That baseline revoke is what strips the default anon EXECUTE grant; its ` + | ||
| `removal is itself the regression this guard exists to catch.`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // A blanket revoke only affects functions that already exist when it runs, so a | ||
| // function is "covered" only if a blanket revoke appears AFTER its definition. | ||
| const coveredByBlanket = (createIdx) => blanketIdxs.some((b) => b > createIdx); | ||
|
|
||
| // Collect every public-function CREATE site. CREATE OR REPLACE preserves prior | ||
| // grants, so group by name and use the EARLIEST definition to decide coverage. | ||
| const createRe = /^\s*create\s+(?:or\s+replace\s+)?function\s+(?:public\.)?"?([a-z0-9_]+)"?\s*\(/i; | ||
| const creates = []; | ||
| lines.forEach((line, idx) => { | ||
| const m = createRe.exec(line); | ||
| if (m) creates.push({ name: m[1].toLowerCase(), idx }); | ||
| }); | ||
|
|
||
| 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()); | ||
|
Comment on lines
+68
to
+92
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Track function overloads by full signature, not bare name.
Also applies to: 104-115 🤖 Prompt for AI Agents
Comment on lines
+84
to
+92
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Only treat revokes from
🤖 Prompt for AI Agents |
||
|
|
||
| // 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; | ||
|
Comment on lines
+94
to
+115
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Detect schema-wide grants that reopen every function. A later 🤖 Prompt for AI Agents |
||
| vulnerable.push({ | ||
| name, | ||
| idx: info.earliestIdx, | ||
| reason: "defined after the blanket revoke with no explicit per-function revoke", | ||
| }); | ||
| } | ||
|
|
||
| if (vulnerable.length > 0) { | ||
| vulnerable.sort((a, b) => a.idx - b.idx); | ||
| console.error( | ||
| `check:function-grants: FAIL — ${vulnerable.length} SECURITY DEFINER function(s) in ${SCHEMA_PATH} are executable ` + | ||
| `by PUBLIC/anon:\n` + | ||
| vulnerable.map((v) => ` - public.${v.name} (schema.sql:${v.idx + 1}) — ${v.reason}`).join("\n") + | ||
| `\n\nFix: add \`revoke execute on function public.<name>(<args>) from public, anon, authenticated;\` (and grant ` + | ||
| `execute only to the intended role, e.g. service_role) in the migration that defines it, then reconcile ` + | ||
| `schema.sql. If genuinely safe, allowlist it with a reason in scripts/check-function-grants.mjs.`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.log( | ||
| `check:function-grants: OK — all ${definerCount} SECURITY DEFINER public function(s) are revoked from PUBLIC ` + | ||
| `(blanket revoke at schema.sql:${Math.max(...blanketIdxs) + 1} or an explicit per-function revoke).`, | ||
| ); | ||
| } | ||
|
|
||
| main(); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import { execFileSync } from "node:child_process"; | ||
| import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { afterAll, describe, expect, it } from "vitest"; | ||
|
|
||
| const SCRIPT = "scripts/check-function-grants.mjs"; | ||
| const workdir = mkdtempSync(join(tmpdir(), "fn-grants-")); | ||
|
|
||
| afterAll(() => rmSync(workdir, { recursive: true, force: true })); | ||
|
|
||
| function run(schemaPath: string): { code: number; out: string } { | ||
| try { | ||
| const stdout = execFileSync("node", [SCRIPT, schemaPath], { encoding: "utf8" }); | ||
| return { code: 0, out: stdout }; | ||
| } catch (error) { | ||
| const err = error as { status?: number; stdout?: string; stderr?: string }; | ||
| return { code: err.status ?? 1, out: `${err.stdout ?? ""}${err.stderr ?? ""}` }; | ||
| } | ||
| } | ||
|
|
||
| function fixture(name: string, sql: string): string { | ||
| const file = join(workdir, name); | ||
| writeFileSync(file, sql); | ||
| return file; | ||
| } | ||
|
|
||
| const BLANKET = "revoke execute on all functions in schema public from public, anon, authenticated;"; | ||
|
|
||
| describe("check:function-grants", () => { | ||
| it("passes against the committed schema.sql", () => { | ||
| const result = run("supabase/schema.sql"); | ||
| expect(result.out).toContain("OK"); | ||
| expect(result.code).toBe(0); | ||
| }); | ||
|
|
||
| it("fails a SECURITY DEFINER function left anon-executable after the blanket revoke", () => { | ||
| const file = fixture( | ||
| "leaky.sql", | ||
| [ | ||
| BLANKET, | ||
| "create function public.leaky(p_owner uuid)", | ||
| "returns jsonb language plpgsql security definer set search_path = '' as $$", | ||
| "begin return '{}'::jsonb; end;", | ||
| "$$;", | ||
| ].join("\n"), | ||
| ); | ||
| const result = run(file); | ||
| expect(result.code).toBe(1); | ||
| expect(result.out).toContain("public.leaky"); | ||
| }); | ||
|
|
||
| it("passes when the SECURITY DEFINER function is explicitly revoked (revoke all form)", () => { | ||
| const file = fixture( | ||
| "guarded.sql", | ||
| [ | ||
| BLANKET, | ||
| "create function public.guarded(p_owner uuid)", | ||
| "returns jsonb language plpgsql security definer set search_path = '' as $$", | ||
| "begin return '{}'::jsonb; end;", | ||
| "$$;", | ||
| "revoke all on function public.guarded(uuid) from public, anon, authenticated;", | ||
| "grant execute on function public.guarded(uuid) to service_role;", | ||
| ].join("\n"), | ||
| ); | ||
| const result = run(file); | ||
| expect(result.out).toContain("OK"); | ||
| expect(result.code).toBe(0); | ||
| }); | ||
|
|
||
| it("fails a SECURITY DEFINER function explicitly granted EXECUTE to anon (even after a revoke)", () => { | ||
| const file = fixture( | ||
| "reopened.sql", | ||
| [ | ||
| BLANKET, | ||
| "create function public.reopened(p_owner uuid)", | ||
| "returns jsonb language plpgsql security definer set search_path = '' as $$", | ||
| "begin return '{}'::jsonb; end;", | ||
| "$$;", | ||
| "revoke all on function public.reopened(uuid) from public, anon, authenticated;", | ||
| "grant execute on function public.reopened(uuid) to anon;", | ||
| ].join("\n"), | ||
| ); | ||
| const result = run(file); | ||
| expect(result.code).toBe(1); | ||
| expect(result.out).toContain("public.reopened"); | ||
| }); | ||
|
|
||
| it("ignores SECURITY INVOKER functions (bound by RLS, not an escalation surface)", () => { | ||
| const file = fixture( | ||
| "invoker.sql", | ||
| [BLANKET, "create function public.plain() returns void language sql as $$ select 1 $$;"].join("\n"), | ||
| ); | ||
| const result = run(file); | ||
| expect(result.code).toBe(0); | ||
| }); | ||
|
|
||
| it("treats a function defined before the blanket revoke as covered", () => { | ||
| const file = fixture( | ||
| "covered.sql", | ||
| [ | ||
| "create function public.older(p_owner uuid)", | ||
| "returns jsonb language plpgsql security definer set search_path = '' as $$", | ||
| "begin return '{}'::jsonb; end;", | ||
| "$$;", | ||
| BLANKET, | ||
| ].join("\n"), | ||
| ); | ||
| const result = run(file); | ||
| expect(result.code).toBe(0); | ||
| }); | ||
|
|
||
| it("fails when the baseline blanket revoke is missing entirely", () => { | ||
| const file = fixture("no-blanket.sql", "create function public.f() returns void language sql as $$ select 1 $$;\n"); | ||
| const result = run(file); | ||
| expect(result.code).toBe(1); | ||
| expect(result.out).toContain("no schema-wide"); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -542,6 +542,13 @@ async function commitDocumentIndexGeneration(args: { | |
| p_quality: sanitizeJsonbRecord(args.quality), | ||
| }); | ||
| if (!error) return; | ||
| // 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. | ||
|
Comment on lines
+545
to
+551
Contributor
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 🤖 Prompt for AI Agents |
||
| throw supabaseStageError("commit_document_index_generation", error); | ||
| await upsertIndexQuality(args.quality); | ||
| await deleteStaleIndexGenerationRows(args.documentId, args.indexGenerationId); | ||
|
Comment on lines
553
to
554
There was a problem hiding this comment.
When a fresh/preview environment lacks Useful? React with 👍 / 👎.
Owner
Author
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 Generated by Claude Code |
||
|
|
@@ -1736,6 +1743,18 @@ async function processJob(job: JobRow) { | |
| embedding_model: env.OPENAI_EMBEDDING_MODEL, | ||
| ...metrics, | ||
| }; | ||
| // Data-safety gate: never commit an empty generation. In the atomic-reindex | ||
| // path (no pre-reset) this would otherwise swap a previously-good index for | ||
| // nothing — e.g. an image-only PDF that OCR could not read. With 0 chunks and | ||
| // 0 searchable images there is nothing to retrieve, so fail the job: the prior | ||
| // committed generation stays live and the document is surfaced to eval | ||
| // governance instead of being silently blanked. | ||
| if (chunks.length === 0 && imageCount === 0) { | ||
| throw new Error( | ||
| `Refusing to commit an empty index generation for document ${job.document_id}: ` + | ||
| "extraction/OCR produced 0 chunks and 0 searchable images.", | ||
| ); | ||
| } | ||
| await commitDocumentIndexGeneration({ | ||
| jobId: job.id, | ||
| documentId: job.document_id, | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard reads only the reconciled
schema.sql, although the documented deployment path appliessupabase/migrationsand 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 toanon/PUBLICwould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 9d338c6. Added explicit anon/PUBLIC grant detection: any
GRANT EXECUTE … TO public/anonon a SECURITY DEFINER function now fails the check regardless of an earlier revoke (with a regression test). The guard asserts against the reconciledschema.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