Read the hub document count with a credential that has privileges

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,51 +14,85 @@ 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<HubEnvironmentFacts> {
   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
   // the client *reject* rather than resolve with an error, and an unhandled
   // 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;

tests/developer-hub-environment-facts.test.ts

@@ -1,21 +1,27 @@
-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();
   vi.resetModules();
 });
 
 type LoadOptions = {
-  user?: { id: string; email?: string } | null;
+  user?: { id: string; email?: string; app_metadata?: Record<string, unknown> } | 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);
   });
 });