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;