From 38747ae5072ba453ac2295c2b1407a98386d2d6f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 16:00:02 +0000 Subject: [PATCH] Signed-in people can read their wedding again on the hosted project The hosted project had drifted from the migrations: is_wedding_member ran as its caller and, with delete_my_account, could not be executed by authenticated. Every read of a wedding was refused (403, "permission denied for function is_wedding_member") and deleting an account failed. Granted again, the check recursed through the policy it serves ("stack depth limit exceeded"), so it is a security definer again too. Both migrations are applied to the hosted project under the versions it recorded, and change nothing where the functions never drifted. The trigger function is taken from anon and authenticated. A PGlite test models the drift and the fix; the harness grants the auth schema to the API roles, as Supabase does. The self-hosting guide warns against the Security Advisor's remedies for these functions; the review records D8. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HozwaQN87Fzg985EBm6MY1 --- docs/SELF-HOSTING.md | 13 +++++++ .../specs/2026-09-29-database-review.md | 1 + .../lib/documents/history.migrations.test.ts | 34 +++++++++++++++++++ suite/lib/testing/database.ts | 1 + .../20260930155142_member_grants.sql | 21 ++++++++++++ .../20260930155610_member_definer.sql | 12 +++++++ 6 files changed, 82 insertions(+) create mode 100644 supabase/migrations/20260930155142_member_grants.sql create mode 100644 supabase/migrations/20260930155610_member_definer.sql diff --git a/docs/SELF-HOSTING.md b/docs/SELF-HOSTING.md index 6ad77ca3..11658090 100644 --- a/docs/SELF-HOSTING.md +++ b/docs/SELF-HOSTING.md @@ -120,6 +120,19 @@ and supplier links, the planners' library, and the live channel. **The RLS policies are what make one couple unable to read another's wedding**, so applying them is not optional. +Supabase's Security Advisor will then warn that signed-in people can execute +sixteen `security definer` functions, and anyone three. That is the design: + +- every write goes through one of the sixteen, and each checks who is calling; +- the three are the guest-link and supplier-link readers and the supplier's + Confirm, for whoever holds a link. + +**Do not apply the advisor's remedies to them** — revoking `EXECUTE`, or +switching to `SECURITY INVOKER`. Done to `is_wedding_member`, either one stops +every signed-in person reading their own wedding. The hosted project met +exactly that; see D8 in the +[database review](superpowers/specs/2026-09-29-database-review.md). + ## 5. Build and start ```sh diff --git a/docs/superpowers/specs/2026-09-29-database-review.md b/docs/superpowers/specs/2026-09-29-database-review.md index 171e22ff..c802efed 100644 --- a/docs/superpowers/specs/2026-09-29-database-review.md +++ b/docs/superpowers/specs/2026-09-29-database-review.md @@ -64,6 +64,7 @@ Findings are marked as they are elsewhere: | D5 | Smaller foreign keys have no index either: `invites.wedding_id` and `created_by`, a planner's `wedding_members.user_id`, `wedding_documents.updated_by`, and each link's `published_by`. Every one is on a table of a few rows per wedding, so a scan there costs nothing that matters. No action. | Traced | | D6 | The first five migrations build the passphrase schema that the ninth removes, so a fresh install creates and drops it. That is harmless. Squashing them is not worth the risk to existing deployments, which track what they have applied by file name. No action. | Traced | | D7 | **`remove_member` did not refuse a caller with no session.** It decided whether a caller may remove someone with `p_user_id <> auth.uid()`. With nobody signed in, `auth.uid()` is null, that comparison is null, and the `if` it guarded was skipped. Called that way, it removed the member, and with the last one gone, the wedding. In plain Postgres `anon` cannot call it: it is revoked from `public` and granted only to `authenticated`. But a Supabase project also grants new functions to `anon` by default privileges, which `revoke … from public` does not undo. Whether the hosted project was exposed can be checked there with `select has_function_privilege('anon', 'public.remove_member(uuid,uuid)', 'execute')`. **Fixed either way:** the function refuses a caller with no session first. And every function meant for signed-in people is revoked from `anon` by name, proved against a stand-in for Supabase's defaults. | Reproduced | +| D8 | **The hosted project had drifted from the migrations, and no signed-in person could read their wedding.** Found 2026-09-30, with every migration recorded as applied. `is_wedding_member` could not be executed by `authenticated`, and ran as its caller (`security invoker`); `delete_my_account` could not be executed by `authenticated` either. Every migration says otherwise, and none changes them. Those are two of the Security Advisor's own remedies for its warnings about these functions; what applied them is not in the project's logs, which reach back to 2026-09-20. The effect is in them: signing in on 2026-09-29, every read of the wedding came back 403, `permission denied for function is_wedding_member`. With the grant back but still run as its caller, the check asks the policy that asks the check, and reads failed with `stack depth limit exceeded`. **Fixed on the hosted project** by `20260930155142_member_grants` and `20260930155610_member_definer`, which change nothing where the functions never drifted. The first also takes the trigger function `announce_wedding_document` from `anon` and `authenticated`, which Postgres already refused to run outside a trigger. Every other function's definition matches the migrations, byte for byte once the dashboard's `\r\n` line endings are set aside. The self-hosting guide now warns against the advisor's remedies for these functions. | Reproduced on the hosted project (both refusals, the recursion, and each fixed); PGlite test of the drift and the fix | ## The proposal: keep the structure, bound the history diff --git a/suite/lib/documents/history.migrations.test.ts b/suite/lib/documents/history.migrations.test.ts index 7b5fe2e6..b08d1372 100644 --- a/suite/lib/documents/history.migrations.test.ts +++ b/suite/lib/documents/history.migrations.test.ts @@ -7,6 +7,9 @@ import { actAs, everyMigration, userExists } from "@/lib/testing/database"; vi.setConfig({ testTimeout: 60_000, hookTimeout: 60_000 }); +const MEMBER_GRANTS = join(process.cwd(), "..", "supabase", "migrations", "20260930155142_member_grants.sql"); +const MEMBER_DEFINER = join(process.cwd(), "..", "supabase", "migrations", "20260930155610_member_definer.sql"); + let db: PGlite; beforeEach(async () => { db = await everyMigration(); @@ -119,6 +122,7 @@ test("with Supabase's default grants, nobody signed out may call a function mean await db.exec("grant execute on all functions in schema public to anon, authenticated;"); await db.exec(readFileSync(join(process.cwd(), "..", "supabase", "migrations", "20260929000008_signed_in_callers.sql"), "utf8")); await db.exec(readFileSync(join(process.cwd(), "..", "supabase", "migrations", "20260929000007_bounded_history.sql"), "utf8")); + await db.exec(readFileSync(MEMBER_GRANTS, "utf8")); const can = async (role: string, fn: string) => (await db.query<{ ok: boolean }>("select has_function_privilege($1, $2, 'execute') as ok", [role, fn])).rows[0]!.ok; @@ -140,7 +144,37 @@ test("with Supabase's default grants, nobody signed out may call a function mean } expect(await can("authenticated", "public.wedding_role_count(uuid,text)")).toBe(false); expect(await can("authenticated", "public.remove_member(uuid,uuid)")).toBe(true); + // A trigger function is nobody's to call. + expect(await can("anon", "public.announce_wedding_document()")).toBe(false); + expect(await can("authenticated", "public.announce_wedding_document()")).toBe(false); // A link is for anyone who has it. expect(await can("anon", "public.read_share(text)")).toBe(true); expect(await can("anon", "public.confirm_supplier_link(text)")).toBe(true); }); + +test("on a project that had lost them, a member may again read their wedding and delete their account", async () => { + const alex = await userExists(db, "alex@example.com"); + const wedding = await weddingOf(alex); + await save(wedding, "Alex & Sam", 0); + // The hosted project as found on 2026-09-30: both grants to signed-in people + // gone, and the membership check running as its caller. + await db.exec("reset role;"); + await db.exec("revoke all on function public.is_wedding_member(uuid), public.delete_my_account() from authenticated;"); + await db.exec("alter function public.is_wedding_member(uuid) security invoker;"); + await actAs(db, alex); + await expect(db.query("select wedding_id from public.wedding_documents")).rejects.toThrow("permission denied for function is_wedding_member"); + await expect(db.query("select public.delete_my_account()")).rejects.toThrow("permission denied for function delete_my_account"); + + // Granted again but still run as its caller, the check asks the policy that + // asks the check, without end — the hosted project then refused with "stack + // depth limit exceeded". It must run as its owner, which RLS does not bind. + await db.exec("reset role;"); + await db.exec(readFileSync(MEMBER_GRANTS, "utf8")); + await db.exec(readFileSync(MEMBER_DEFINER, "utf8")); + expect((await db.query("select prosecdef from pg_proc where oid = 'public.is_wedding_member(uuid)'::regprocedure")).rows).toEqual([{ prosecdef: true }]); + await actAs(db, alex); + expect((await db.query("select wedding_id from public.wedding_documents")).rows).toEqual([{ wedding_id: wedding }]); + await db.query("select public.delete_my_account()"); + await db.exec("reset role;"); + expect((await db.query("select 1 from public.account_weddings where id = $1", [wedding])).rows).toHaveLength(0); +}); diff --git a/suite/lib/testing/database.ts b/suite/lib/testing/database.ts index aa547174..901d69f1 100644 --- a/suite/lib/testing/database.ts +++ b/suite/lib/testing/database.ts @@ -17,6 +17,7 @@ export async function everyMigration(): Promise { create or replace function auth.uid() returns uuid language sql stable as $$ select nullif(current_setting('request.jwt.claim.sub', true), '')::uuid $$; + grant usage on schema auth to anon, authenticated; create schema if not exists storage; create table if not exists storage.buckets (id text primary key, name text not null, public boolean not null default false); create table if not exists storage.objects ( diff --git a/supabase/migrations/20260930155142_member_grants.sql b/supabase/migrations/20260930155142_member_grants.sql new file mode 100644 index 00000000..d5728aa2 --- /dev/null +++ b/supabase/migrations/20260930155142_member_grants.sql @@ -0,0 +1,21 @@ +-- Two grants the hosted project had lost, and one it never needed. +-- +-- On 2026-09-30 the hosted project's `is_wedding_member(uuid)` and +-- `delete_my_account()` could be executed by `postgres` and `service_role` +-- only. Every version of 20260902000001_accounts.sql grants both to +-- `authenticated`, and no migration revokes them; what did is not in the +-- project's logs. Without the first, every policy that asks it — the wedding, +-- its members, its document, its history, its links, its files — refuses a +-- signed-in reader with "permission denied for function is_wedding_member", +-- which is what signing in on 2026-09-29 met. Without the second, deleting an +-- account fails. Both are granted again. Where they were never lost, this +-- changes nothing. +-- +-- `announce_wedding_document()` is a trigger function: no one calls it, and +-- Postgres refuses it outside a trigger. Supabase's default privileges left it +-- executable by `anon` and `authenticated`, as 20260929000008 describes for +-- the rest; it is revoked from both, so it is plainly no part of the API. + +grant execute on function public.is_wedding_member(uuid) to authenticated; +grant execute on function public.delete_my_account() to authenticated; +revoke all on function public.announce_wedding_document() from anon, authenticated; diff --git a/supabase/migrations/20260930155610_member_definer.sql b/supabase/migrations/20260930155610_member_definer.sql new file mode 100644 index 00000000..9bd08861 --- /dev/null +++ b/supabase/migrations/20260930155610_member_definer.sql @@ -0,0 +1,12 @@ +-- The membership check runs as its owner again. +-- +-- On 2026-09-30 the hosted project's `is_wedding_member(uuid)` was `security +-- invoker`; 20260902000001_accounts.sql creates it `security definer`, and no +-- migration changes that. Run as its caller, its read of `wedding_members` is +-- bound by that table's policy, which asks `is_wedding_member` — which reads +-- `wedding_members` again, without end. Once 20260930155142_member_grants let +-- signed-in people call it, every read of a wedding failed with "stack depth +-- limit exceeded". As its owner, which row-level security does not bind, it +-- reads the table once. The accounts migration explains why it must. + +alter function public.is_wedding_member(uuid) security definer;