From 6921622cd25432ed1327f13650f426d603346487 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:10:41 +0800 Subject: [PATCH] fix(db): fail closed when retrieval_owner_matches gets a NULL owner_filter Close the tenancy defense-in-depth gap where a null owner_filter matched every tenant row. Demo/test/local-no-auth now pass the public sentinel instead of null, and schema/tests encode the fail-closed truth table. Co-authored-by: Cursor --- src/lib/owner-scope.ts | 15 +++-- supabase/drift-allowlist.json | 2 +- supabase/drift-manifest.json | 4 +- ...00_retrieval_owner_matches_fail_closed.sql | 61 +++++++++++++++++++ supabase/schema.sql | 6 +- tests/owner-scope.test.ts | 17 ++++-- tests/retrieval-owner-filter-guard.test.ts | 14 ++--- tests/supabase-schema.test.ts | 2 + 8 files changed, 100 insertions(+), 21 deletions(-) create mode 100644 supabase/migrations/20260708160000_retrieval_owner_matches_fail_closed.sql diff --git a/src/lib/owner-scope.ts b/src/lib/owner-scope.ts index 4eec27540..d126d4253 100644 --- a/src/lib/owner-scope.ts +++ b/src/lib/owner-scope.ts @@ -2,10 +2,17 @@ import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; export const PUBLIC_OWNER_FILTER_SENTINEL = "00000000-0000-0000-0000-000000000000"; -export function requireOwnerScope(ownerId: string | null | undefined): string | undefined { +// Demo / local-no-auth / test have no authenticated owner. These used to return +// `undefined`, which reaches the retrieval RPCs as a NULL owner_filter — and +// retrieval_owner_matches(NULL, …) previously failed OPEN (matched every tenant's +// rows). Return the public sentinel instead, so these modes see only the shared +// public (null-owner) corpus. Combined with the DB fail-closed change (migration +// 20260708160000_retrieval_owner_matches_fail_closed), no legitimate caller ever +// passes NULL. See docs/tenancy-defense-in-depth-review.md §6. +export function requireOwnerScope(ownerId: string | null | undefined): string { if (ownerId) return ownerId; if (isDemoMode() || isLocalNoAuthMode() || process.env.NODE_ENV === "test") { - return undefined; + return PUBLIC_OWNER_FILTER_SENTINEL; } throw new Error( "Owner-scoped retrieval was called without an ownerId; refusing to run to avoid returning another tenant's data.", @@ -16,10 +23,10 @@ export function retrievalOwnerFilter(args: { ownerId?: string | null; documentIds?: string[]; allowGlobalSearch?: boolean; -}): string | null | undefined { +}): string { if (args.ownerId) return requireOwnerScope(args.ownerId); if (isDemoMode() || isLocalNoAuthMode() || process.env.NODE_ENV === "test") { - return undefined; + return PUBLIC_OWNER_FILTER_SENTINEL; } if (args.allowGlobalSearch || args.documentIds?.length) { return PUBLIC_OWNER_FILTER_SENTINEL; diff --git a/supabase/drift-allowlist.json b/supabase/drift-allowlist.json index b73d6df55..47f8e056b 100644 --- a/supabase/drift-allowlist.json +++ b/supabase/drift-allowlist.json @@ -110,7 +110,7 @@ "category": "functions", "key": "public.retrieval_owner_matches(uuid,uuid)", "kind": "mismatch", - "reason": "ACL-only drift: live retains a default PUBLIC execute grant (=X/postgres) that schema.sql (service-role-only) does not. Same posture as search_document_chunks — this is a security-invoker helper, RLS still applies; revoke on live via an approved hardening migration, then remove. The prior search_path half of this drift (live pg_catalog vs schema.sql pg_temp) was reconciled 2026-07-08: schema.sql + drift-manifest def_hash now match live's body (verified read-only via schema_drift_snapshot). This function is a stable boolean owner-scoping helper — NOT one of the raw-SQL retrieval RPCs (match_document_chunks et al.) whose bodies genuinely diverge; do not conflate.", + "reason": "Body+ACL drift until migration 20260708160000 is applied on live: schema.sql now fails CLOSED on NULL owner_filter (tenancy defense-in-depth §6 item 1); live still has the prior fail-open body until that migration runs. Independently, live retains a default PUBLIC execute grant (=X/postgres) that schema.sql (service-role-only) does not — same ACL posture as search_document_chunks; revoke on live via an approved hardening migration, then remove this entry. Security-invoker helper; RLS still applies. Not one of the raw-SQL retrieval RPCs whose bodies genuinely diverge.", "ref": "docs/database-drift-detection.md#reconciliation-backlog" }, { diff --git a/supabase/drift-manifest.json b/supabase/drift-manifest.json index f37e0b762..8405401a2 100644 --- a/supabase/drift-manifest.json +++ b/supabase/drift-manifest.json @@ -2,7 +2,7 @@ "generated_at": "2026-07-08T06:45:01.039Z", "generator": "scripts/generate-drift-manifest.ts", "postgres_image": "supabase/postgres:17.6.1.127", - "schema_sha256": "f7ca50611471d4ced647bfc7731d72f87856bd03890f03eaca8e0ccb8affc6b0", + "schema_sha256": "cb34f64fc531fc042f545475ddaa7cc65edeeae0dfa4df0b8d7d58edb7123adf", "replay_seconds": 13, "snapshot": { "views": [ @@ -5930,7 +5930,7 @@ "postgres=X/postgres", "service_role=X/postgres" ], - "def_hash": "1d88b539bded5aa40393125f672b8cab", + "def_hash": "ca2877374851b432c4ca971e1e15f746", "signature": "public.retrieval_owner_matches(uuid,uuid)" }, { diff --git a/supabase/migrations/20260708160000_retrieval_owner_matches_fail_closed.sql b/supabase/migrations/20260708160000_retrieval_owner_matches_fail_closed.sql new file mode 100644 index 000000000..5851ebccd --- /dev/null +++ b/supabase/migrations/20260708160000_retrieval_owner_matches_fail_closed.sql @@ -0,0 +1,61 @@ +-- Tenancy defense-in-depth (docs/tenancy-defense-in-depth-review.md §6, item 1). +-- +-- retrieval_owner_matches(owner_filter, row_owner_id) previously returned TRUE for +-- EVERY row when owner_filter IS NULL (fail-open). Because RLS is service-role-only, +-- the retrieval RPCs had no database-level tenant floor: a future app-layer regression +-- that passed a NULL owner_filter would silently return every tenant's rows with no +-- alarm. Make the NULL case fail CLOSED (match no rows). +-- +-- This is behaviour-neutral for every real path — no legitimate caller passes NULL: +-- * production authed -> the owner's uuid (exact-owner match) +-- * anon / public / demo / local-no-auth / test -> the public sentinel +-- '00000000-…' (src/lib/owner-scope.ts now returns the sentinel, never null) +-- * production without an owner -> throws before the RPC is called +-- Only the unreachable fail-open branch changes; the sentinel and exact-owner +-- branches are untouched, so the golden retrieval eval (which passes the sentinel) +-- is unaffected. + +set search_path = public, extensions, pg_temp; + +create or replace function public.retrieval_owner_matches(owner_filter uuid, row_owner_id uuid) +returns boolean +language sql +immutable +parallel safe +set search_path = public, pg_catalog +as $$ + select case + when owner_filter is null then false -- fail CLOSED (was: true) — no DB-level global escape hatch + when owner_filter = '00000000-0000-0000-0000-000000000000'::uuid then row_owner_id is null -- public corpus only + else row_owner_id = owner_filter -- exact owner + end; +$$; + +-- Self-verify the truth table so a bad redefinition fails the migration (on live and +-- on Supabase Preview) instead of silently shipping a tenancy regression. +do $verify$ +declare + a uuid := '11111111-1111-1111-1111-111111111111'; + b uuid := '22222222-2222-2222-2222-222222222222'; + sentinel uuid := '00000000-0000-0000-0000-000000000000'; +begin + if public.retrieval_owner_matches(null, a) is not false then + raise exception 'retrieval_owner_matches(NULL, uuid) must be FALSE (fail-closed)'; + end if; + if public.retrieval_owner_matches(null, null) is not false then + raise exception 'retrieval_owner_matches(NULL, NULL) must be FALSE (fail-closed)'; + end if; + if public.retrieval_owner_matches(sentinel, null) is not true then + raise exception 'retrieval_owner_matches(sentinel, NULL) must be TRUE (public row)'; + end if; + if public.retrieval_owner_matches(sentinel, a) is not false then + raise exception 'retrieval_owner_matches(sentinel, owned) must be FALSE (public-only)'; + end if; + if public.retrieval_owner_matches(a, a) is not true then + raise exception 'retrieval_owner_matches(owner, same owner) must be TRUE'; + end if; + if public.retrieval_owner_matches(a, b) is not false then + raise exception 'retrieval_owner_matches(owner, other owner) must be FALSE'; + end if; +end +$verify$; diff --git a/supabase/schema.sql b/supabase/schema.sql index 5c5a5ddea..ab979ae2c 100644 --- a/supabase/schema.sql +++ b/supabase/schema.sql @@ -2050,9 +2050,9 @@ parallel safe set search_path = public, pg_catalog as $$ select case - when owner_filter is null then true - when owner_filter = '00000000-0000-0000-0000-000000000000'::uuid then row_owner_id is null - else row_owner_id = owner_filter + when owner_filter is null then false -- fail CLOSED (was: true) — no DB-level global escape hatch + when owner_filter = '00000000-0000-0000-0000-000000000000'::uuid then row_owner_id is null -- public corpus only + else row_owner_id = owner_filter -- exact owner end; $$; diff --git a/tests/owner-scope.test.ts b/tests/owner-scope.test.ts index 33030b816..cf2025525 100644 --- a/tests/owner-scope.test.ts +++ b/tests/owner-scope.test.ts @@ -12,11 +12,14 @@ describe("requireOwnerScope (fail-closed owner scoping)", () => { expect(requireOwnerScope("owner-1")).toBe("owner-1"); }); - it("stays permissive (undefined) without an owner in demo mode", async () => { + it("returns the public sentinel (never null) without an owner in demo mode", async () => { vi.doMock("@/lib/env", () => ({ isDemoMode: () => true, isLocalNoAuthMode: () => false })); - const { requireOwnerScope } = await import("../src/lib/owner-scope"); - expect(requireOwnerScope(undefined)).toBeUndefined(); - expect(requireOwnerScope(null)).toBeUndefined(); + const { requireOwnerScope, PUBLIC_OWNER_FILTER_SENTINEL } = await import("../src/lib/owner-scope"); + // Must not return undefined/null: that reaches the retrieval RPCs as a NULL + // owner_filter, which now fails closed (migration 20260708160000). The public + // sentinel scopes these modes to the shared public corpus instead. + expect(requireOwnerScope(undefined)).toBe(PUBLIC_OWNER_FILTER_SENTINEL); + expect(requireOwnerScope(null)).toBe(PUBLIC_OWNER_FILTER_SENTINEL); }); it("throws when a real (non-demo, non-local) deployment omits the ownerId", async () => { @@ -35,4 +38,10 @@ describe("retrievalOwnerFilter", () => { const { retrievalOwnerFilter, PUBLIC_OWNER_FILTER_SENTINEL } = await import("../src/lib/owner-scope"); expect(retrievalOwnerFilter({ allowGlobalSearch: true })).toBe(PUBLIC_OWNER_FILTER_SENTINEL); }); + + it("returns the public sentinel (never null) in demo/test mode without an owner", async () => { + vi.doMock("@/lib/env", () => ({ isDemoMode: () => true, isLocalNoAuthMode: () => false })); + const { retrievalOwnerFilter, PUBLIC_OWNER_FILTER_SENTINEL } = await import("../src/lib/owner-scope"); + expect(retrievalOwnerFilter({})).toBe(PUBLIC_OWNER_FILTER_SENTINEL); + }); }); diff --git a/tests/retrieval-owner-filter-guard.test.ts b/tests/retrieval-owner-filter-guard.test.ts index 4f65fb2b4..e282b4da9 100644 --- a/tests/retrieval-owner-filter-guard.test.ts +++ b/tests/retrieval-owner-filter-guard.test.ts @@ -5,13 +5,13 @@ import { describe, expect, it } from "vitest"; // Guard for the retrieval owner-scope boundary (48h-review finding #3). // -// The SQL `retrieval_owner_matches(owner_filter, row_owner_id)` returns TRUE for EVERY row when -// `owner_filter IS NULL` (fails open by design; RLS is service-role-only, so the app filter is the -// sole tenant boundary). Until a migration makes null fail closed — gated on the Supabase Preview -// drift fix so the SQL change can be verified — this test is the tripwire: no `.rpc(...)` call in -// `src/` may pass a *literal* null/undefined `owner_filter`, and every owner_filter value must come -// from the sanctioned scope helpers (which fail closed in production) or the public sentinel — never -// a raw/unaudited value that could reach the RPC as null and silently return another tenant's rows. +// The SQL `retrieval_owner_matches(owner_filter, row_owner_id)` now fails CLOSED when +// `owner_filter IS NULL` (migration 20260708160000_retrieval_owner_matches_fail_closed), and +// src/lib/owner-scope.ts no longer emits null — so the database has a real tenant floor. This +// test remains as defense-in-depth on the app side: no `.rpc(...)` call in `src/` may pass a +// *literal* null/undefined `owner_filter`, and every owner_filter value must come from the +// sanctioned scope helpers (which fail closed in production) or the public sentinel — never a +// raw/unaudited value. const SRC_DIR = join(process.cwd(), "src"); diff --git a/tests/supabase-schema.test.ts b/tests/supabase-schema.test.ts index a10a8fdd9..1347cbe8f 100644 --- a/tests/supabase-schema.test.ts +++ b/tests/supabase-schema.test.ts @@ -564,6 +564,8 @@ describe("Supabase schema Data API grants", () => { expect(schema).toContain( "create or replace function public.retrieval_owner_matches(owner_filter uuid, row_owner_id uuid)", ); + expect(schema).toContain("when owner_filter is null then false"); + expect(schema).not.toContain("when owner_filter is null then true"); expect(schema).toContain( "when owner_filter = '00000000-0000-0000-0000-000000000000'::uuid then row_owner_id is null", );