From 7411c3fbdddb3db14a8483e166cf8ba09553cc2e Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Mon, 5 Oct 2026 21:23:34 -0400 Subject: [PATCH] ENG-2368 Revoke write privileges on the my_spaces and my_accounts views --- .../database/features/readOnlyViews.feature | 26 +++++ .../features/step-definitions/stepdefs.ts | 107 ++++++++++++++---- ...120000_read_only_my_spaces_my_accounts.sql | 5 + .../database/supabase/schemas/account.sql | 6 + 4 files changed, 121 insertions(+), 23 deletions(-) create mode 100644 packages/database/features/readOnlyViews.feature create mode 100644 packages/database/supabase/migrations/20261005120000_read_only_my_spaces_my_accounts.sql diff --git a/packages/database/features/readOnlyViews.feature b/packages/database/features/readOnlyViews.feature new file mode 100644 index 0000000000..7d3e561d3d --- /dev/null +++ b/packages/database/features/readOnlyViews.feature @@ -0,0 +1,26 @@ +Feature: Read-only access views + User story: + * As a user of a plugin + * Logged in through a given space's anonymous account + * I can read spaces and accounts through the my_spaces and my_accounts views + * But I cannot write through them, since such writes would bypass the tables' RLS + + Acceptance criteria: + * Inserts, updates and deletes through my_spaces and my_accounts fail with insufficient privilege + * Reads through both views still work + + Background: + Given the database is blank + And the user user1 opens the Roam plugin in space s1 + + Scenario: Clients cannot write through my_spaces, even for their own space + Then a user logged in space s1 cannot insert spaces through my_spaces + And a user logged in space s1 cannot update spaces through my_spaces + And a user logged in space s1 cannot delete spaces through my_spaces + And a user logged in space s1 should see 1 my_spaces in the database + + Scenario: Clients cannot write through my_accounts, even for their own accounts + Then a user logged in space s1 cannot insert accounts through my_accounts + And a user logged in space s1 cannot update accounts through my_accounts + And a user logged in space s1 cannot delete accounts through my_accounts + And a user logged in space s1 should see 2 my_accounts in the database diff --git a/packages/database/features/step-definitions/stepdefs.ts b/packages/database/features/step-definitions/stepdefs.ts index 6de5ef0816..b88b3a3709 100644 --- a/packages/database/features/step-definitions/stepdefs.ts +++ b/packages/database/features/step-definitions/stepdefs.ts @@ -380,33 +380,94 @@ Then( }), ); +type ViewWrites = Record< + "insert" | "update" | "delete", + () => PromiseLike> +>; + +// Each update and delete filter matches visible rows, so a granted write would succeed. +const expectWriteRejected = async ( + operation: string, + writes: ViewWrites, +): Promise => { + if (!Object.hasOwn(writes, operation)) + assert.fail(`unknown operation ${operation}`); + const response = await writes[operation as keyof ViewWrites](); + assert.equal(response.error?.code, INSUFFICIENT_PRIVILEGE); +}; + +const getSpaceClient = async ( + spaceName: string, +): Promise<{ + spaceId: number; + client: Awaited>; +}> => { + const localRefs = (world.localRefs || {}) as LocalRefsType; + const spaceId = localRefs[spaceName]; + if (typeof spaceId !== "number") assert.fail("spaceId not a number"); + return { spaceId, client: await getLoggedinDatabase(spaceId) }; +}; + Then( "a user logged in space {word} cannot {word} concepts through my_concepts", async (spaceName: string, operation: string) => { - const localRefs = (world.localRefs || {}) as LocalRefsType; - const spaceId = localRefs[spaceName]; - if (typeof spaceId !== "number") assert.fail("spaceId not a number"); - const client = await getLoggedinDatabase(spaceId); + const { spaceId, client } = await getSpaceClient(spaceName); const view = client.from("my_concepts"); - let response: PostgrestSingleResponse; - if (operation === "insert") { - const now = new Date().toISOString(); - response = await view.insert({ - name: "written through my_concepts", - space_id: spaceId, - created: now, - last_modified: now, - }); - } else if (operation === "update") { - response = await view - .update({ name: "written through my_concepts" }) - .eq("space_id", spaceId); - } else if (operation === "delete") { - response = await view.delete().eq("space_id", spaceId); - } else { - assert.fail(`unknown operation ${operation}`); - } - assert.equal(response.error?.code, INSUFFICIENT_PRIVILEGE); + const now = new Date().toISOString(); + await expectWriteRejected(operation, { + insert: () => + view.insert({ + name: "written through my_concepts", + space_id: spaceId, + created: now, + last_modified: now, + }), + update: () => + view + .update({ name: "written through my_concepts" }) + .eq("space_id", spaceId), + delete: () => view.delete().eq("space_id", spaceId), + }); + }, +); + +Then( + "a user logged in space {word} cannot {word} spaces through my_spaces", + async (spaceName: string, operation: string) => { + const { spaceId, client } = await getSpaceClient(spaceName); + const view = client.from("my_spaces"); + await expectWriteRejected(operation, { + insert: () => + view.insert({ + name: "written through my_spaces", + url: "https://example.com/written-through-my-spaces", + platform: "Roam", + }), + update: () => + view.update({ name: "written through my_spaces" }).eq("id", spaceId), + delete: () => view.delete().eq("id", spaceId), + }); + }, +); + +Then( + "a user logged in space {word} cannot {word} accounts through my_accounts", + async (spaceName: string, operation: string) => { + const { client } = await getSpaceClient(spaceName); + const view = client.from("my_accounts"); + await expectWriteRejected(operation, { + insert: () => + view.insert({ + name: "written through my_accounts", + account_local_id: "written-through-my-accounts", + platform: "Roam", + }), + update: () => + view + .update({ name: "written through my_accounts" }) + .eq("platform", "Roam"), + delete: () => view.delete().eq("platform", "Roam"), + }); }, ); diff --git a/packages/database/supabase/migrations/20261005120000_read_only_my_spaces_my_accounts.sql b/packages/database/supabase/migrations/20261005120000_read_only_my_spaces_my_accounts.sql new file mode 100644 index 0000000000..998388e85a --- /dev/null +++ b/packages/database/supabase/migrations/20261005120000_read_only_my_spaces_my_accounts.sql @@ -0,0 +1,5 @@ +-- Single-table views are writable, and writes run as the view owner, bypassing Space's RLS. +REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.my_spaces FROM anon, authenticated; + +-- Single-table views are writable, and writes run as the view owner, bypassing PlatformAccount's RLS. +REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.my_accounts FROM anon, authenticated; diff --git a/packages/database/supabase/schemas/account.sql b/packages/database/supabase/schemas/account.sql index bc415f6629..3171a316e0 100644 --- a/packages/database/supabase/schemas/account.sql +++ b/packages/database/supabase/schemas/account.sql @@ -431,6 +431,9 @@ SELECT platform FROM public."Space" WHERE id = any(public.my_space_ids('partial')); +-- Single-table views are writable, and writes run as the view owner, bypassing Space's RLS. +REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.my_spaces FROM anon, authenticated; + -- PlatformAccount: Access to anyone sharing a space with you to create an account, to allow editing authors -- Once the account is claimed by a user, only allow this user to modify it. -- Eventually: Allow platform admin to modify? @@ -458,6 +461,9 @@ WHERE id IN ( SELECT id FROM public."PlatformAccount" WHERE dg_account = auth.uid() ); +-- Single-table views are writable, and writes run as the view owner, bypassing PlatformAccount's RLS. +REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.my_accounts FROM anon, authenticated; + CREATE OR REPLACE VIEW public.my_pseudo_accounts AS SELECT pa.id,