diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1ea554c7a..9b57884e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -91,6 +91,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 @@ -141,7 +158,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 42e168f9d..077478cf0 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..df748511e --- /dev/null +++ b/scripts/check-function-grants.mjs @@ -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 +// ... 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()); + + // 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; + 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.() 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(); diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index 4128368b0..e8fb0172c 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -112,6 +112,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 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. const ragEvalPatterns = [ "scripts/fixtures", "src/app/api/answer", @@ -407,6 +411,14 @@ 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 + the non-docs safety job, which runs 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/tests/function-grants.test.ts b/tests/function-grants.test.ts new file mode 100644 index 000000000..8c4102eb9 --- /dev/null +++ b/tests/function-grants.test.ts @@ -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"); + }); +}); diff --git a/worker/main.ts b/worker/main.ts index 837800fa5..4d0de92a9 100644 --- a/worker/main.ts +++ b/worker/main.ts @@ -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. throw supabaseStageError("commit_document_index_generation", error); await upsertIndexQuality(args.quality); await deleteStaleIndexGenerationRows(args.documentId, args.indexGenerationId); @@ -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,