From 7e8f17467a9d44baebc2cef1c17f1ed2c5cc6422 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 05:08:36 +0000 Subject: [PATCH] Harden CI gates, ingestion commits, and SECURITY DEFINER grants 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 Claude-Session: https://claude.ai/code/session_019UQUcNriJUVSGRhWz794RQ --- .github/workflows/ci.yml | 22 ++++- package.json | 3 +- scripts/check-function-grants.mjs | 157 ++++++++++++++++++++++++++++++ scripts/ci-change-scope.mjs | 13 +++ scripts/verify-pr-local.mjs | 4 +- tests/function-grants.test.ts | 130 +++++++++++++++++++++++++ worker/main.ts | 12 +++ 7 files changed, 337 insertions(+), 4 deletions(-) create mode 100644 scripts/check-function-grants.mjs create mode 100644 tests/function-grants.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 682f7129b..e8b6b582e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,23 @@ jobs: - name: Format check run: npm run format:check + # Design-system guards that were previously only in the local verify:cheap + # chain (offline + fast). Without them a stray type/icon/brand drift merged + # green because no workflow ran them (there is no test backstop either). + - name: Type scale guard + run: npm run check:type-scale + + - name: Icon scale guard + run: npm run check:icon-scale + + - name: Brand asset check + run: npm run brand:check + + # Fails if a SECURITY DEFINER public function is left executable by + # PUBLIC/anon (privilege-escalation / cross-tenant read surface). + - name: Function-grant guard + run: npm run check:function-grants + - name: Lint run: npm run lint @@ -144,7 +161,10 @@ jobs: run: npm run check:codex-autofix-workflow - name: Offline RAG preflight - if: needs.changes.outputs.rag_eval_changed == 'true' + # Runs for every non-docs change (this job's docs_only guard), NOT only + # files matching the rag_eval_changed allowlist. This offline grounding + # gate is cheap and clinical-safety-relevant, so a new retrieval file that + # falls outside the scope patterns must never silently skip it. run: npm run eval:rag:offline coverage: diff --git a/package.json b/package.json index b96c2694f..4b0f6163e 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "test:e2e:chromium": "node scripts/run-playwright.mjs --project=chromium --project=chromium-mockups", "test:e2e:visual": "node scripts/run-playwright.mjs --config=playwright.visual.config.ts", "test:cross-tenant:staging": "node scripts/run-tsx.mjs scripts/test-cross-tenant-staging.ts", - "verify:cheap": "npm run check:runtime && npm run check:github-actions && npm run sitemap:check && npm run brand:check && npm run check:type-scale && npm run check:icon-scale && npm run lint && npm run typecheck && npm run test", + "verify:cheap": "npm run check:runtime && npm run check:github-actions && npm run sitemap:check && npm run brand:check && npm run check:type-scale && npm run check:icon-scale && npm run check:function-grants && npm run lint && npm run typecheck && npm run test", "verify:pr-local": "node scripts/verify-pr-local.mjs", "verify:ui": "npm run check:runtime && npm run test:e2e:chromium", "verify:release": "npm run check:runtime && npm run lint && npm run typecheck && npm run test && npm run build && npm run test:e2e && npm run check:production-readiness && npm run governance:release && npm run eval:quality:release", @@ -91,6 +91,7 @@ "check:july8-live-batch": "node scripts/run-tsx.mjs scripts/check-july8-live-batch.ts", "check:type-scale": "node scripts/check-type-scale.mjs --strict", "check:icon-scale": "node scripts/check-icon-scale.mjs --strict", + "check:function-grants": "node scripts/check-function-grants.mjs", "recover:ingestion": "node scripts/run-tsx.mjs scripts/recover-ingestion-queue.ts", "registry:seed": "node scripts/run-tsx.mjs scripts/seed-registry-records.ts", "registry:embed": "node scripts/run-tsx.mjs scripts/embed-registry-records.ts", diff --git a/scripts/check-function-grants.mjs b/scripts/check-function-grants.mjs new file mode 100644 index 000000000..bcdea2900 --- /dev/null +++ b/scripts/check-function-grants.mjs @@ -0,0 +1,157 @@ +#!/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.: +// - a schema-wide `revoke execute on all functions ... from public` runs AFTER +// its definition, OR it has its own `revoke ... on function ... from` +// a role list that INCLUDES `public` (a revoke FROM anon only is NOT enough — +// PostgreSQL's default PUBLIC grant still applies); AND +// - it is not re-opened by a later `grant execute ... to public/anon`, whether +// named (`on function `) or schema-wide (`on all functions in schema +// public`). +// Otherwise it is reported and CI fails. +// +// Scope notes: SECURITY INVOKER functions run as the caller and stay bound by +// RLS, so they are out of scope. The check runs against the reconciled snapshot +// (kept in sync with the migration chain by the drift check); asserting live +// per-migration ACLs by replaying the whole chain is a deeper follow-up. +// Overloaded functions are matched by bare name (a qualifying revoke on any +// overload covers all overloads of that name); exact per-signature matching is a +// documented 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 fail(message) { + console.error(`check:function-grants: FAIL — ${message}`); + process.exit(1); +} + +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) { + fail( + `no schema-wide "revoke execute on all functions in schema public from ... public" statement found in ` + + `${SCHEMA_PATH}. That baseline revoke strips the default anon EXECUTE grant; its removal is itself the ` + + `regression this guard exists to catch.`, + ); + } + + // A schema-wide GRANT of EXECUTE to PUBLIC/anon re-opens every function and + // defeats the blanket revoke outright — fatal regardless of per-function state. + 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))) { + fail( + `a "grant execute on all functions in schema public to public/anon" re-opens EXECUTE on every function and ` + + `defeats the blanket revoke. Remove it and grant to the intended role (e.g. service_role) instead.`, + ); + } + + // 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); + + // Public-function CREATE sites. CREATE OR REPLACE preserves prior grants, so + // group by name and use the EARLIEST definition to decide blanket 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 isDefiner = /security\s+definer/i.test(lines.slice(current.idx, end).join("\n")); + const prior = byName.get(current.name); + byName.set( + current.name, + prior + ? { earliestIdx: Math.min(prior.earliestIdx, current.idx), isDefiner: prior.isDefiner || isDefiner } + : { earliestIdx: current.idx, isDefiner }, + ); + }); + + // Per-function REVOKE — protective ONLY when the FROM role list includes PUBLIC. + // A `revoke ... from anon` (without public) leaves PostgreSQL's default PUBLIC + // grant intact, so it does not protect the function. Accepts EXECUTE and ALL. + 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; + vulnerable.push({ + name, + idx: info.earliestIdx, + reason: "defined after the blanket revoke with no EXECUTE revoke FROM public", + }); + } + + if (vulnerable.length > 0) { + vulnerable.sort((a, b) => a.idx - b.idx); + 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.() 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.`, + ); + } + + 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) and none are ` + + `re-opened by a grant to PUBLIC/anon.`, + ); +} + +main(); diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index 00708777a..446f9cd96 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -116,6 +116,10 @@ const dbPatterns = [ /^tests\/(supabase|drift|private-rag|private-access|retrieval-owner).*\.test\.ts$/, ]; +// 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 +// both CI (.github/workflows/ci.yml) and local verify:pr-local, so a new +// retrieval file that falls outside these patterns can never silently skip it. const ragEvalPatterns = [ "scripts/fixtures", "src/app/api/answer", @@ -420,6 +424,15 @@ function selfTest() { rag_eval_changed: true, source_changed: true, }); + // A RAG-relevant lib file outside ragEvalPatterns must still be caught as a + // source change (so static-pr and the non-docs safety job / verify:pr-local, + // which run the offline grounding gate, always execute). Guards the "silent + // scope narrowing" gap. + assertScope("rag-lib-outside-allowlist", ["src/lib/hybrid-reranker.ts"], { + source_changed: true, + coverage_changed: true, + docs_only: false, + }); assertScope("database-access", ["src/app/api/documents/route.ts"], { db_changed: true, source_changed: true, diff --git a/scripts/verify-pr-local.mjs b/scripts/verify-pr-local.mjs index 61d95f9a1..29cf24c3b 100644 --- a/scripts/verify-pr-local.mjs +++ b/scripts/verify-pr-local.mjs @@ -67,7 +67,7 @@ function readScope(files) { function selectedScripts(scope, extended) { const scripts = [...baseScripts]; if (scope.build_changed) scripts.push("build"); - if (scope.rag_eval_changed) scripts.push("eval:rag:offline"); + if (!scope.docs_only) scripts.push("eval:rag:offline"); if (extended && scope.ui_changed) scripts.push("verify:ui"); return scripts; } @@ -81,7 +81,7 @@ if (options.dryRun) { console.log("\nPR-local verification plan (dry run):"); for (const script of scripts) console.log(`- npm run ${script}`); if (!scope.build_changed) console.log("- build skipped: no build-affecting changes detected"); - if (!scope.rag_eval_changed) console.log("- offline RAG evaluation skipped: no RAG-affecting changes detected"); + if (scope.docs_only) console.log("- offline RAG evaluation skipped: docs-only change"); if (options.extended && !scope.ui_changed) console.log("- Chromium UI gate skipped: no UI-affecting changes detected"); process.exit(0); diff --git a/tests/function-grants.test.ts b/tests/function-grants.test.ts new file mode 100644 index 000000000..5844f6e4e --- /dev/null +++ b/tests/function-grants.test.ts @@ -0,0 +1,130 @@ +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;"; +const DEFINER = (name: string) => + [ + `create function public.${name}(p_owner uuid)`, + "returns jsonb language plpgsql security definer set search_path = '' as $$", + "begin return '{}'::jsonb; end;", + "$$;", + ].join("\n"); + +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 result = run(fixture("leaky.sql", [BLANKET, DEFINER("leaky")].join("\n"))); + expect(result.code).toBe(1); + expect(result.out).toContain("public.leaky"); + }); + + it("passes when the function is revoked FROM public (revoke all form)", () => { + const result = run( + fixture( + "guarded.sql", + [ + BLANKET, + DEFINER("guarded"), + "revoke all on function public.guarded(uuid) from public, anon, authenticated;", + ].join("\n"), + ), + ); + expect(result.out).toContain("OK"); + expect(result.code).toBe(0); + }); + + it("fails when the revoke omits PUBLIC (revoke from anon only leaves the default PUBLIC grant)", () => { + const result = run( + fixture( + "anon-only-revoke.sql", + [BLANKET, DEFINER("anononly"), "revoke execute on function public.anononly(uuid) from anon;"].join("\n"), + ), + ); + expect(result.code).toBe(1); + expect(result.out).toContain("public.anononly"); + }); + + it("fails a function explicitly granted EXECUTE to anon (even after a revoke)", () => { + const result = run( + fixture( + "reopened.sql", + [ + BLANKET, + DEFINER("reopened"), + "revoke all on function public.reopened(uuid) from public, anon, authenticated;", + "grant execute on function public.reopened(uuid) to anon;", + ].join("\n"), + ), + ); + expect(result.code).toBe(1); + expect(result.out).toContain("public.reopened"); + }); + + it("fails on a schema-wide GRANT ... ON ALL FUNCTIONS ... TO anon that reopens everything", () => { + const result = run( + fixture( + "schema-wide-grant.sql", + [ + BLANKET, + DEFINER("guarded"), + "revoke all on function public.guarded(uuid) from public, anon, authenticated;", + "grant execute on all functions in schema public to anon;", + ].join("\n"), + ), + ); + expect(result.code).toBe(1); + expect(result.out).toContain("all functions"); + }); + + it("ignores SECURITY INVOKER functions (bound by RLS, not an escalation surface)", () => { + const result = run( + fixture( + "invoker.sql", + [BLANKET, "create function public.plain() returns void language sql as $$ select 1 $$;"].join("\n"), + ), + ); + expect(result.code).toBe(0); + }); + + it("treats a function defined before the blanket revoke as covered", () => { + const result = run(fixture("covered.sql", [DEFINER("older"), BLANKET].join("\n"))); + expect(result.code).toBe(0); + }); + + it("fails when the baseline blanket revoke is missing entirely", () => { + const result = run( + fixture("no-blanket.sql", "create function public.f() returns void language sql as $$ select 1 $$;\n"), + ); + expect(result.code).toBe(1); + expect(result.out).toContain("no schema-wide"); + }); +}); diff --git a/worker/main.ts b/worker/main.ts index f0027f40a..b5a9a1c12 100644 --- a/worker/main.ts +++ b/worker/main.ts @@ -1660,6 +1660,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 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,