fix(db): NULL-safe tenant guards via caller_is_company_member + mechanical sweep (#881)

The house guard 'p_company_id NOT IN (SELECT public.user_company_ids())' skips the deny branch on UNKNOWN (NULL on either side). Not exploitable today (company_members.company_id is NOT NULL) but a NULL p_company_id passes the guard, and the shape fails silently under change. 9 live functions carried it (link_*_to_voucher, reserve/release_voucher_range, mark_entry_as_opening_balance, retag_line_dimensions, ensure_company_dimensions, company_has_capability, rotate_company_inbox). - caller_is_company_member(uuid): NULL-safe membership predicate (NULL -> false, always). - Mechanical rewrite: every public function carrying the raw pattern is re-created via pg_get_functiondef with the guard swapped — deliberate over hand-copying 9 bodies (the stale-copy hazard behind the 07-03 constraint clobber). Probe-validated locally: pattern swapped, NULL denied. - pg-real ratchet: after full replay no public function may contain the raw pattern (also blocks future reintroduction); detector self-test; helper semantics (member/foreigner/NULL). Existing tenant-guard suites re-assert deny semantics on the rewritten functions in CI. Part of dev_docs/mcp_optimization_plan.md (P2-3 follow-up, PR #872 review). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-03 11:48:09 +02:00
parent ea236cbcdf
commit b27f6cdb04
2 changed files with 147 additions and 0 deletions
@@ -0,0 +1,71 @@
+-- NULL-safe tenant guards across all SECURITY DEFINER company-scoped
+-- functions (mcp_optimization_plan P2-3 follow-up from PR #872 review).
+--
+-- The house guard pattern `p_company_id NOT IN (SELECT public.user_company_ids())`
+-- evaluates to UNKNOWN — and the deny branch silently does not fire — when
+-- either side yields NULL. Not exploitable today (company_members.company_id
+-- is NOT NULL so the set never contains NULLs), but a NULL p_company_id
+-- skips the guard, and the shape breaks silently if the membership helper
+-- ever changes. 9 live functions carried the pattern at authoring time.
+--
+-- Fix in two parts:
+--   1. caller_is_company_member(uuid) — the NULL-safe membership predicate
+--      (NULL company → false, always).
+--   2. A mechanical rewrite: every public function whose source contains the
+--      raw pattern is re-created via pg_get_functiondef with the guard line
+--      swapped to the helper. Mechanical-over-hand-copied is deliberate —
+--      reproducing 9 function bodies by hand is the same stale-copy hazard
+--      that caused the 2026-07-03 constraint clobber. The rewrite is
+--      validated three ways: the pg-real ratchet test asserts no function
+--      retains the raw pattern after full migration replay, the existing
+--      tenant-guard suites re-assert deny semantics on these exact
+--      functions, and prod is verified 9 → 0 after apply.
+--
+-- pg-test: tests/pg/null-safe-tenant-guards.pg.test.ts
+
+CREATE OR REPLACE FUNCTION public.caller_is_company_member(p_company_id uuid)
+RETURNS boolean
+LANGUAGE sql
+STABLE
+SET search_path TO 'public'
+AS $$
+  SELECT p_company_id IS NOT NULL AND EXISTS (
+    SELECT 1 FROM public.user_company_ids() AS c(id) WHERE c.id = p_company_id
+  )
+$$;
+
+REVOKE ALL ON FUNCTION public.caller_is_company_member(uuid) FROM PUBLIC, anon;
+GRANT EXECUTE ON FUNCTION public.caller_is_company_member(uuid) TO authenticated, service_role;
+
+DO $$
+DECLARE
+  fn record;
+  def text;
+  new_def text;
+  rewritten integer := 0;
+BEGIN
+  FOR fn IN
+    SELECT p.oid, p.proname
+    FROM pg_proc p
+    JOIN pg_namespace n ON n.oid = p.pronamespace
+    WHERE n.nspname = 'public'
+      AND p.prosrc LIKE '%NOT IN (SELECT public.user_company_ids())%'
+  LOOP
+    def := pg_get_functiondef(fn.oid);
+    new_def := regexp_replace(
+      def,
+      '([a-zA-Z_][a-zA-Z0-9_.]*)\s+NOT IN \(SELECT public\.user_company_ids\(\)\)',
+      'NOT public.caller_is_company_member(\1)',
+      'g'
+    );
+    IF new_def <> def THEN
+      EXECUTE new_def;
+      rewritten := rewritten + 1;
+      RAISE NOTICE 'null_safe_tenant_guards: rewrote %', fn.proname;
+    END IF;
+  END LOOP;
+  RAISE NOTICE 'null_safe_tenant_guards: rewrote % function(s) total', rewritten;
+END;
+$$;
+
+NOTIFY pgrst, 'reload schema';
@@ -0,0 +1,76 @@
+import { afterAll, describe, expect, it } from 'vitest'
+import { getPool, withUserContext } from './setup'
+import { seedCompany, insertAuthUser } from './fixtures'
+
+/**
+ * NULL-safe tenant guards (mcp_optimization_plan P2-3 / PR #872 review):
+ * `x NOT IN (SELECT public.user_company_ids())` skips the deny branch on
+ * UNKNOWN. Migration 20260703180000 introduces caller_is_company_member()
+ * and mechanically rewrites every public function carrying the raw pattern.
+ *
+ * The ratchet below runs after full migration replay in CI, so it both
+ * proves the rewrite worked and permanently blocks the pattern from
+ * returning in future migrations.
+ */
+
+const RAW_PATTERN = '%NOT IN (SELECT public.user_company_ids())%'
+
+async function functionsWithRawPattern(): Promise<string[]> {
+  const { rows } = await getPool().query<{ proname: string }>(
+    `SELECT p.proname
+     FROM pg_proc p
+     JOIN pg_namespace n ON n.oid = p.pronamespace
+     WHERE n.nspname = 'public' AND p.prosrc LIKE $1
+     ORDER BY p.proname`,
+    [RAW_PATTERN],
+  )
+  return rows.map((r) => r.proname)
+}
+
+describe('NULL-safe tenant guards', () => {
+  afterAll(async () => {
+    await getPool().query('DROP FUNCTION IF EXISTS public._ratchet_probe_raw_guard(uuid)')
+  })
+
+  it('ratchet: no public function carries the raw NOT IN guard pattern', async () => {
+    const offenders = await functionsWithRawPattern()
+    expect(
+      offenders,
+      `Functions still using the NULL-unsafe guard — use public.caller_is_company_member() instead:\n${offenders.join('\n')}`,
+    ).toEqual([])
+  })
+
+  it('the ratchet detector actually catches the pattern (test the test)', async () => {
+    await getPool().query(`
+      CREATE OR REPLACE FUNCTION public._ratchet_probe_raw_guard(p_company_id uuid)
+      RETURNS boolean LANGUAGE plpgsql AS $$
+      BEGIN
+        IF p_company_id NOT IN (SELECT public.user_company_ids()) THEN
+          RETURN false;
+        END IF;
+        RETURN true;
+      END;
+      $$`)
+    const offenders = await functionsWithRawPattern()
+    expect(offenders).toContain('_ratchet_probe_raw_guard')
+    await getPool().query('DROP FUNCTION public._ratchet_probe_raw_guard(uuid)')
+  })
+
+  it('caller_is_company_member: member true, foreigner false, NULL always false', async () => {
+    const { userId, companyId } = await seedCompany()
+    const strangerId = await insertAuthUser()
+
+    const asUser = async (uid: string, company: string | null) =>
+      withUserContext(uid, async (client) => {
+        const { rows } = await client.query<{ ok: boolean }>(
+          `SELECT public.caller_is_company_member($1) AS ok`,
+          [company],
+        )
+        return rows[0].ok
+      })
+
+    expect(await asUser(userId, companyId)).toBe(true)
+    expect(await asUser(strangerId, companyId)).toBe(false)
+    expect(await asUser(userId, null)).toBe(false)
+  })
+})