From 181f38107495aa0e29e34791bcde93485e4c0179 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 15:03:05 +0000 Subject: [PATCH] fix(developer-hub): read the environment strip's document count with a credential that has privileges The hub's document count could never load. `environment-facts.ts` counted `public.documents` through the cookie-bound user client, on the reasoning that the `documents owner read` policy would scope it in the database. It cannot: `schema.sql:5299` revokes all `public` table privileges from `anon` and `authenticated` and grants that table to `service_role` only, migration `20260725000000` re-applies the revoke after every earlier grant, and no later migration restores it -- the only `to authenticated` hits after that date are RLS policies. A policy cannot hand back an SQL SELECT privilege the role does not hold, so the read returned permission denied on every hub load and the strip could only ever render "document count unavailable". It was invisible because the module degrades a failed read to `null` by design and its tests mock the client, so nothing on either side could see it. It surfaced only because the corpus-health panel copied this module as its model and hit the same wall, where Codex caught it on #2504. Same shape as the corpus-health fix that already landed on this branch: the cookie-bound client identifies the caller and reads no table, the caller must carry the administrator claim `DeveloperAreaGate` checks, and the count filters on `owner_id` explicitly. That filter is the whole owner-scoping guarantee now, not an addition to row-level security. A non-administrator still gets their email back rather than a blank strip, since it was already read and does not depend on the count. The source assertion named "reads through the user-session client and never the service-role client" was backwards and is replaced rather than dropped: the tests now assert the issued query carries `owner_id` equal to the caller, and that a non-administrator issues no query at all. Every existing test that pins null-not-zero and the rejection guard is kept unchanged. Proven by mutation: five rules broken on purpose, five reds, no survivors, file restored byte-identical by SHA-256. Reverting the read to the user client is one of them, so the regression that caused this cannot come back silently. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XG7wQurapeZwWRsNhHA1PY --- src/lib/developer-area/environment-facts.ts | 72 ++++++--- tests/developer-hub-environment-facts.test.ts | 140 +++++++++++++----- 2 files changed, 155 insertions(+), 57 deletions(-) diff --git a/src/lib/developer-area/environment-facts.ts b/src/lib/developer-area/environment-facts.ts index 7b421f43e9..ed97e3f468 100644 --- a/src/lib/developer-area/environment-facts.ts +++ b/src/lib/developer-area/environment-facts.ts @@ -1,6 +1,8 @@ import "server-only"; +import { isAdministratorUser } from "@/lib/authorization"; import { isDemoMode } from "@/lib/env"; +import { createAdminClient } from "@/lib/supabase/admin"; import { createSupabaseServerClient } from "@/lib/supabase/server"; export type HubEnvironmentFacts = { @@ -12,32 +14,52 @@ export type HubEnvironmentFacts = { /** * The three facts the developer hub's environment strip cannot read for itself. * - * One Supabase client, one auth call and at most one count query per hub load — - * the gate above this page (`DeveloperAreaGate`) already resolves the same user - * on the same request, so this is deliberately the *second* auth call and not a - * third: email and the document count are gathered together rather than through - * two independent helpers. + * One auth call and at most one count query per hub load — the gate above this + * page (`DeveloperAreaGate`) already resolves the same user on the same request, + * so this is deliberately the *second* auth call and not a third: email and the + * document count are gathered together rather than through two independent + * helpers. * - * **The user-session client, never the service-role admin client.** - * `public.documents` has row-level security enabled with a single select policy, - * `documents owner read` (`owner_id = auth.uid()`), so the database itself scopes - * this count to the caller's own documents. `createAdminClient` bypasses RLS and - * would report every owner's document total to whoever happened to be signed in. + * **Why the count is read with the service-role client, and what scopes it.** + * This module first counted `public.documents` through the cookie-bound user + * client, on the reasoning that the `documents owner read` policy + * (`owner_id = auth.uid()`) would scope the count in the database. It cannot. + * `supabase/schema.sql:5299` revokes all table privileges in `public` from + * `anon` and `authenticated`, and the grant block below it names + * `public.documents` for `service_role` only; + * `supabase/migrations/20260725000000_audit_security_remediation.sql:81` + * re-applies that revoke after every earlier grant, and no later migration + * restores it. The schema says as much in a comment of its own: browser clients + * receive no direct table privileges, signed-in access is mediated by the server + * routes, and the owner policies remain as defence in depth. A policy cannot + * hand back an SQL `SELECT` privilege the role does not hold, so the count + * returned permission denied on every hub load and the strip could only ever + * render "document count unavailable" — silently, because a failed read here + * degrades to `null` by design. Found while building the corpus-health panel, + * which had copied this module as its model (PR #2504). + * + * So the scoping moves up one layer, matching `corpus-health.ts` and + * `src/app/api/ingestion/jobs/route.ts`: the cookie-bound client identifies the + * caller and reads no table, the caller must carry the same administrator claim + * `DeveloperAreaGate` checks, and the count filters on `owner_id` explicitly. + * That filter is the whole of the owner-scoping guarantee now, not an addition + * to row-level security, and `tests/developer-hub-environment-facts.test.ts` + * asserts it on the issued query. Do not "restore" the user client here on the + * strength of the policy: it reads nothing, and it fails by looking healthy. * * **Every failure returns `null`, never `0`.** Zero is a true and meaningful * answer here — an account that has uploaded nothing — so a failed read must not * be able to impersonate it. The strip renders `null` as "document count * unavailable", which is the conservative degradation this repo requires: name * the gap rather than state a number nothing read. The same reasoning is why an - * unauthenticated request skips the query outright instead of reporting the `0` - * rows RLS would correctly return to it. + * unauthenticated request skips the query outright instead of reporting a `0`. */ export async function resolveHubEnvironmentFacts(): Promise { const demoMode = isDemoMode(); const unread: HubEnvironmentFacts = { demoMode, documentCount: null, email: null }; - const supabase = await createSupabaseServerClient(); - if (!supabase) return unread; + const session = await createSupabaseServerClient(); + if (!session) return unread; // Both awaits are wrapped, and a returned `{ error }` is only half of what can // go wrong. An aborted request or one that exhausts its network retries makes @@ -45,18 +67,32 @@ export async function resolveHubEnvironmentFacts(): Promise // rejection here would fail the whole page rather than degrade one line of it // — the opposite of this module's contract, and worst during exactly the // Supabase outage that makes the hub worth opening. `demoMode` survives either - // way, because it never depended on the network. + // way, because it never depended on the network. `createAdminClient()` is + // inside the same guard: it calls `requireServerEnv()` and throws when the + // server env is absent. try { - const { data } = await supabase.auth.getUser(); + const { data } = await session.auth.getUser(); const user = data.user; if (!user) return unread; - const { count, error } = await supabase.from("documents").select("id", { count: "exact", head: true }); + // The email is known at this point and does not depend on the count, so a + // non-administrator still gets a named account rather than a blank strip. + // The `owner_id` filter below would already confine the count to this + // caller's own documents; this check is the same defence in depth the + // corpus-health panel applies. + const email = user.email ?? null; + if (!isAdministratorUser(user)) return { demoMode, documentCount: null, email }; + + const admin = createAdminClient(); + const { count, error } = await admin + .from("documents") + .select("id", { count: "exact", head: true }) + .eq("owner_id", user.id); return { demoMode, documentCount: error ? null : (count ?? null), - email: user.email ?? null, + email, }; } catch { return unread; diff --git a/tests/developer-hub-environment-facts.test.ts b/tests/developer-hub-environment-facts.test.ts index d92f865929..5b8a2e89ed 100644 --- a/tests/developer-hub-environment-facts.test.ts +++ b/tests/developer-hub-environment-facts.test.ts @@ -1,13 +1,19 @@ -import { readFileSync } from "node:fs"; - import { afterEach, describe, expect, it, vi } from "vitest"; // resolveHubEnvironmentFacts() supplies three of the four facts on the developer // hub's environment strip. Two of its rules are the reason it exists rather than -// being inlined into the page: the document count must be scoped to the caller's -// own documents by the database, and a count it could not read must report as -// absent rather than as zero. An empty corpus and a failed query look identical -// on screen if that second rule ever slips. +// being inlined into the page: the document count must be confined to the +// caller's own documents, and a count it could not read must report as absent +// rather than as zero. An empty corpus and a failed query look identical on +// screen if that second rule ever slips. +// +// The first rule used to be delegated to row-level security. It cannot be: the +// `authenticated` role holds no table privilege on `public.documents` +// (`schema.sql:5299`, re-applied by migration `20260725000000`), so the +// owner-read policy sits behind a privilege the role does not have and the read +// returned permission denied on every hub load. The count now goes through the +// service-role client with an explicit `owner_id` filter, which makes that +// filter the whole guarantee — so it is asserted on the issued query below. afterEach(() => { vi.restoreAllMocks(); @@ -15,7 +21,7 @@ afterEach(() => { }); type LoadOptions = { - user?: { id: string; email?: string } | null; + user?: { id: string; email?: string; app_metadata?: Record } | null; count?: number | null; error?: { message: string } | null; demoMode?: boolean; @@ -24,7 +30,9 @@ type LoadOptions = { rejectAuth?: boolean; }; -const selectCalls: { columns: string; options: unknown }[] = []; +const selectCalls: { columns: string; options: unknown; filters: [string, unknown][] }[] = []; + +const ADMINISTRATOR = { site_role: "administrator" }; async function load({ user = null, @@ -37,6 +45,9 @@ async function load({ selectCalls.length = 0; vi.doMock("server-only", () => ({})); vi.doMock("@/lib/env", () => ({ isDemoMode: () => demoMode })); + // The user client identifies the caller and reads no table; the service-role + // client performs the count. Mocked apart, so a read issued through the wrong + // one shows up as a missing call rather than passing silently. vi.doMock("@/lib/supabase/server", () => ({ createSupabaseServerClient: vi.fn(async () => ({ auth: { @@ -45,10 +56,27 @@ async function load({ return { data: { user } }; }), }, + })), + })); + vi.doMock("@/lib/supabase/admin", () => ({ + createAdminClient: vi.fn(() => ({ from: vi.fn(() => ({ select: vi.fn((columns: string, options: unknown) => { - selectCalls.push({ columns, options }); - return rejectCount ? Promise.reject(new Error("fetch failed")) : Promise.resolve({ count, error }); + const call = { columns, options, filters: [] as [string, unknown][] }; + const chain = { + eq(column: string, value: unknown) { + call.filters.push([column, value]); + return chain; + }, + then(resolve: (value: unknown) => unknown, reject: (reason: unknown) => unknown) { + selectCalls.push(call); + return (rejectCount ? Promise.reject(new Error("fetch failed")) : Promise.resolve({ count, error })).then( + resolve, + reject, + ); + }, + }; + return chain; }), })), })), @@ -80,16 +108,16 @@ describe("resolveHubEnvironmentFacts", () => { documentCount: null, email: null, }); - // Not merely "returns null": the query must not run at all. Row-level - // security would correctly return 0 rows to an anonymous caller, and - // rendering that as "0 documents" would state something false about the - // corpus rather than about the session. + // Not merely "returns null": the query must not run at all. There is no + // owner id to filter on, so the count would be a whole-table count across + // every account — and reporting it as "N documents" would state something + // false about this account's corpus. expect(selectCalls).toHaveLength(0); }); it("reports the owner's document count and email for a signed-in user", async () => { const { resolveHubEnvironmentFacts } = await load({ - user: { id: "user-1", email: "clinician@example.com" }, + user: { id: "user-1", email: "clinician@example.com", app_metadata: ADMINISTRATOR }, count: 2851, }); @@ -98,26 +126,38 @@ describe("resolveHubEnvironmentFacts", () => { documentCount: 2851, email: "clinician@example.com", }); - expect(selectCalls).toEqual([{ columns: "id", options: { count: "exact", head: true } }]); + expect(selectCalls).toEqual([ + { columns: "id", options: { count: "exact", head: true }, filters: [["owner_id", "user-1"]] }, + ]); }); it("keeps an empty corpus distinct from a count it could not read", async () => { - const empty = await load({ user: { id: "user-1" }, count: 0 }); + const empty = await load({ user: { id: "user-1", app_metadata: ADMINISTRATOR }, count: 0 }); await expect(empty.resolveHubEnvironmentFacts()).resolves.toMatchObject({ documentCount: 0 }); vi.resetModules(); - const failed = await load({ user: { id: "user-1" }, count: 0, error: { message: "permission denied" } }); + const failed = await load({ + user: { id: "user-1", app_metadata: ADMINISTRATOR }, + count: 0, + error: { message: "permission denied" }, + }); await expect(failed.resolveHubEnvironmentFacts()).resolves.toMatchObject({ documentCount: null }); }); it("reports a missing count as unavailable rather than as zero", async () => { - const { resolveHubEnvironmentFacts } = await load({ user: { id: "user-1" }, count: null }); + const { resolveHubEnvironmentFacts } = await load({ + user: { id: "user-1", app_metadata: ADMINISTRATOR }, + count: null, + }); await expect(resolveHubEnvironmentFacts()).resolves.toMatchObject({ documentCount: null }); }); it("has no email to report for a user record that carries none", async () => { - const { resolveHubEnvironmentFacts } = await load({ user: { id: "user-1" }, count: 3 }); + const { resolveHubEnvironmentFacts } = await load({ + user: { id: "user-1", app_metadata: ADMINISTRATOR }, + count: 3, + }); await expect(resolveHubEnvironmentFacts()).resolves.toMatchObject({ email: null, documentCount: 3 }); }); @@ -131,7 +171,7 @@ describe("resolveHubEnvironmentFacts", () => { */ it("degrades to unavailable when the count read rejects instead of returning an error", async () => { const { resolveHubEnvironmentFacts } = await load({ - user: { id: "user-1", email: "clinician@example.com" }, + user: { id: "user-1", email: "clinician@example.com", app_metadata: ADMINISTRATOR }, rejectCount: true, demoMode: false, }); @@ -157,24 +197,46 @@ describe("resolveHubEnvironmentFacts", () => { }); /** - * The owner-scoping guarantee is structural, not behavioural: it holds because - * this module uses the cookie-bound user client, which row-level security - * scopes to `owner_id = auth.uid()`. The service-role admin client bypasses RLS - * entirely, so importing it here would silently turn one account's count into - * every account's — with no failing assertion anywhere, because the mock in the - * tests above would still answer. A source assertion is the only thing that can - * catch that substitution. + * The owner-scoping guarantee, and the reason this replaced a source assertion + * that the module used the cookie-bound user client and never the admin one. + * + * That assertion was backwards. The user client reads nothing here: + * `schema.sql:5299` revokes all `public` table privileges from + * `authenticated`, migration `20260725000000` re-applies the revoke after + * every earlier grant, and no later migration restores it — so the + * `documents owner read` policy sits behind a privilege the role does not + * hold and the count returned permission denied on every hub load. The read + * goes through the service-role client, which is not subject to that policy, + * which makes the explicit filter the entire guarantee rather than a second + * layer over it. Asserting it on the issued query is what stops the count + * from silently becoming a total across every account. */ - it("reads through the user-session client and never the service-role client", () => { - const source = readFileSync(new URL("../src/lib/developer-area/environment-facts.ts", import.meta.url), "utf8"); - // Import statements only. The module's own comment names `createAdminClient` - // to explain why it is wrong here, so a whole-file substring search for that - // identifier would fail on the documentation rather than on the code. - const imports = source.split("\n").filter((line) => line.startsWith("import ")); - - expect(imports.some((line) => line.includes('"@/lib/supabase/server"'))).toBe(true); - expect(imports.some((line) => line.includes("supabase/admin"))).toBe(false); - // Catches a dynamic import or a re-export that no import line would show. - expect(source).not.toContain("supabase/admin"); + it("filters the count by the caller's own owner id", async () => { + const { resolveHubEnvironmentFacts } = await load({ + user: { id: "user-1", email: "clinician@example.com", app_metadata: ADMINISTRATOR }, + count: 2851, + }); + await resolveHubEnvironmentFacts(); + + expect(selectCalls).toHaveLength(1); + expect(selectCalls[0]!.filters).toContainEqual(["owner_id", "user-1"]); + }); + + it("counts nothing for a signed-in user without the administrator claim", async () => { + const { resolveHubEnvironmentFacts } = await load({ + user: { id: "user-2", email: "someone@example.com", app_metadata: {} }, + count: 2851, + }); + + // The same claim `DeveloperAreaGate` checks. The owner filter would already + // confine the count to this caller's own documents, so this is defence in + // depth — but the email is still reported, because it was already read and + // a named account is more useful than a blank strip. + await expect(resolveHubEnvironmentFacts()).resolves.toEqual({ + demoMode: false, + documentCount: null, + email: "someone@example.com", + }); + expect(selectCalls).toHaveLength(0); }); });