From da00475ecb1a109d8736eaea285e1212a14c9e98 Mon Sep 17 00:00:00 2001 From: Martin Donadieu Date: Mon, 27 Apr 2026 14:42:56 +0200 Subject: [PATCH] fix(db): restrict manifest mutation access --- AGENTS.md | 37 +++ ...5834_restrict_manifest_mutation_access.sql | 14 + supabase/tests/26_test_rls_policies.sql | 4 +- tests/manifest-rls.test.ts | 257 ++++++++++++++++-- 4 files changed, 289 insertions(+), 23 deletions(-) create mode 100644 supabase/migrations/20260427105834_restrict_manifest_mutation_access.sql diff --git a/AGENTS.md b/AGENTS.md index a537ef1c17..27f30deec1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -451,6 +451,43 @@ CREATE POLICY "Allow select on my_table" ON public.my_table FOR SELECT USING (condition_1 OR condition_2); ``` +**Rule 1.5: Never rely on implicit deny for table operations.** + +If an operation is intentionally forbidden, add an explicit deny policy for that +operation instead of relying on the absence of a policy. + +- Do not use "no INSERT policy means INSERT is blocked" as the final design. +- Do not use "no DELETE policy means DELETE is blocked" as the final design. +- The expected repository style is explicit allow or explicit deny for each + operation you intentionally care about. +- This is especially important for security-sensitive tables and system-managed + tables such as `manifest`. + +When an operation must be impossible for user-facing roles, prefer an explicit +deny policy with a clear name, for example: + +```sql +-- Example: system-managed table, users must never insert rows directly +CREATE POLICY "Deny insert on my_table" +ON public.my_table +AS RESTRICTIVE +FOR INSERT +TO authenticated +WITH CHECK (false); + +-- Example: users must never delete rows directly +CREATE POLICY "Deny delete on my_table" +ON public.my_table +AS RESTRICTIVE +FOR DELETE +TO authenticated +USING (false); +``` + +If API key traffic must also be denied through user-context RLS, make that +intent explicit in the policy design and naming. Do not leave the operation +blocked only because no policy happened to exist. + **Rule 2: Call `auth.uid()` only once using a subquery.** The `auth.uid()` function should never be called multiple times in a policy. Use a `SELECT *` subquery pattern to call it once and reference the result: diff --git a/supabase/migrations/20260427105834_restrict_manifest_mutation_access.sql b/supabase/migrations/20260427105834_restrict_manifest_mutation_access.sql new file mode 100644 index 0000000000..b930423416 --- /dev/null +++ b/supabase/migrations/20260427105834_restrict_manifest_mutation_access.sql @@ -0,0 +1,14 @@ +DROP POLICY IF EXISTS "Allow users to delete manifest entries" ON "public"."manifest"; +DROP POLICY IF EXISTS "Allow users to insert manifest entries" ON "public"."manifest"; + +CREATE POLICY "Prevent users from inserting manifest entries" ON "public"."manifest" +AS RESTRICTIVE +FOR INSERT +TO "authenticated", "anon" +WITH CHECK (false); + +CREATE POLICY "Prevent users from deleting manifest entries" ON "public"."manifest" +AS RESTRICTIVE +FOR DELETE +TO "authenticated", "anon" +USING (false); diff --git a/supabase/tests/26_test_rls_policies.sql b/supabase/tests/26_test_rls_policies.sql index 03769b9e19..9b4e6a195a 100644 --- a/supabase/tests/26_test_rls_policies.sql +++ b/supabase/tests/26_test_rls_policies.sql @@ -276,9 +276,9 @@ SELECT 'public', 'manifest', ARRAY[ - 'Allow users to delete manifest entries', - 'Allow users to insert manifest entries', 'Allow select for auth, api keys (read+)', + 'Prevent users from deleting manifest entries', + 'Prevent users from inserting manifest entries', 'Prevent users from updating manifest entries' ], 'manifest should have correct policies' diff --git a/tests/manifest-rls.test.ts b/tests/manifest-rls.test.ts index 89033eeecc..91a73219ed 100644 --- a/tests/manifest-rls.test.ts +++ b/tests/manifest-rls.test.ts @@ -13,6 +13,7 @@ import { STRIPE_INFO_CUSTOMER_ID_2, USER_EMAIL_NONMEMBER, USER_ID_2, + USER_ID_NONMEMBER, USER_PASSWORD, USER_PASSWORD_NONMEMBER, } from './test-utils.ts' @@ -20,6 +21,10 @@ import { const id = randomUUID() const APP_OWN = `com.demo.manifest-own.${id}` const APP_OTHER = `com.demo.manifest-other.${id}` +const APP_READ_ONLY = `com.demo.manifest-read-only.${id}` +const READ_ONLY_ORG_ID = randomUUID() +const READ_ONLY_STRIPE_CUSTOMER_ID = `cus_manifest_rls_${id.replaceAll('-', '')}` +const READ_ONLY_VISIBLE_FILE = 'read-only-visible.js' const OTHER_USER_EMAIL = 'test2@capgo.app' let authHeadersUser1: Record @@ -27,28 +32,53 @@ let authHeadersUser2: Record let authHeadersNonMember: Record let ownVersionId: number let otherVersionId: number +let readOnlyVersionId: number const restApiKeyHeaders = { 'Content-Type': 'application/json', } -function getRestManifestUrl(appVersionId: number): string { +interface ManifestRow { + id: number + file_name: string + file_hash: string + s3_path: string + app_version_id: number +} + +function getRestManifestUrl(searchParams: Record): string { const supabaseUrl = process.env.SUPABASE_URL if (!supabaseUrl) throw new Error('SUPABASE_URL is missing') const url = new URL('/rest/v1/manifest', supabaseUrl) - url.searchParams.set('select', 'id,file_name,file_hash,s3_path,app_version_id') - url.searchParams.set('app_version_id', `eq.${appVersionId}`) + for (const [key, value] of Object.entries(searchParams)) + url.searchParams.set(key, value) return url.toString() } +async function parseResponseBody(response: Response): Promise { + const text = await response.text() + if (!text) + return null + + try { + return JSON.parse(text) as unknown + } + catch { + return text + } +} + async function fetchManifestRows(headers: Record, appVersionId: number) { const anonKey = process.env.SUPABASE_ANON_KEY if (!anonKey) throw new Error('SUPABASE_ANON_KEY is missing') - const response = await fetchWithRetry(getRestManifestUrl(appVersionId), { + const response = await fetchWithRetry(getRestManifestUrl({ + select: 'id,file_name,file_hash,s3_path,app_version_id', + app_version_id: `eq.${appVersionId}`, + }), { method: 'GET', headers: { ...headers, @@ -56,26 +86,68 @@ async function fetchManifestRows(headers: Record, appVersionId: }, }) - const data = await response.json() as Array<{ - id: number - file_name: string - file_hash: string - s3_path: string - app_version_id: number - }> + const data = await response.json() as ManifestRow[] return { response, data } } +async function insertManifestRow(headers: Record, row: Omit) { + const anonKey = process.env.SUPABASE_ANON_KEY + if (!anonKey) + throw new Error('SUPABASE_ANON_KEY is missing') + + const response = await fetchWithRetry(getRestManifestUrl({ + select: 'id,file_name,file_hash,s3_path,app_version_id', + }), { + method: 'POST', + headers: { + ...headers, + apikey: anonKey, + prefer: 'return=representation', + }, + body: JSON.stringify(row), + }) + + return { + response, + body: await parseResponseBody(response), + } +} + +async function deleteManifestRow(headers: Record, manifestId: number) { + const anonKey = process.env.SUPABASE_ANON_KEY + if (!anonKey) + throw new Error('SUPABASE_ANON_KEY is missing') + + const response = await fetchWithRetry(getRestManifestUrl({ + select: 'id', + id: `eq.${manifestId}`, + }), { + method: 'DELETE', + headers: { + ...headers, + apikey: anonKey, + prefer: 'return=representation', + }, + }) + + return { + response, + body: await parseResponseBody(response), + } +} + beforeAll(async () => { - await Promise.all([ - resetAndSeedAppData(APP_OWN), - resetAndSeedAppData(APP_OTHER, { - orgId: ORG_ID_2, - userId: USER_ID_2, - stripeCustomerId: STRIPE_INFO_CUSTOMER_ID_2, - }), - ]) + await resetAndSeedAppData(APP_OWN) + await resetAndSeedAppData(APP_OTHER, { + orgId: ORG_ID_2, + userId: USER_ID_2, + stripeCustomerId: STRIPE_INFO_CUSTOMER_ID_2, + }) + await resetAndSeedAppData(APP_READ_ONLY, { + orgId: READ_ONLY_ORG_ID, + stripeCustomerId: READ_ONLY_STRIPE_CUSTOMER_ID, + }) authHeadersUser1 = await getAuthHeaders() authHeadersUser2 = await getAuthHeadersForCredentials(OTHER_USER_EMAIL, USER_PASSWORD) @@ -106,6 +178,24 @@ beforeAll(async () => { otherVersionId = otherVersion.id + const { data: readOnlyVersion, error: readOnlyVersionError } = await supabase + .from('app_versions') + .select('id') + .eq('app_id', APP_READ_ONLY) + .eq('name', '1.0.0') + .single() + + if (readOnlyVersionError || !readOnlyVersion) + throw readOnlyVersionError ?? new Error('Missing read-only app version') + + readOnlyVersionId = readOnlyVersion.id + + await supabase.from('org_users').insert({ + org_id: READ_ONLY_ORG_ID, + user_id: USER_ID_NONMEMBER, + user_right: 'read', + }).throwOnError() + await supabase.from('manifest').insert([ { app_version_id: ownVersionId, @@ -121,15 +211,30 @@ beforeAll(async () => { file_hash: 'other-hash', file_size: 102, }, + { + app_version_id: readOnlyVersionId, + file_name: READ_ONLY_VISIBLE_FILE, + s3_path: `orgs/${READ_ONLY_ORG_ID}/apps/${APP_READ_ONLY}/baseline/${READ_ONLY_VISIBLE_FILE}`, + file_hash: 'read-only-visible-hash', + file_size: 103, + }, ]).throwOnError() -}) +}, 120000) afterAll(async () => { + await getSupabaseClient() + .from('org_users') + .delete() + .eq('org_id', READ_ONLY_ORG_ID) + .eq('user_id', USER_ID_NONMEMBER) + .throwOnError() + await Promise.all([ resetAppData(APP_OWN), resetAppData(APP_OTHER), + resetAppData(APP_READ_ONLY), ]) -}) +}, 120000) describe('manifest RLS', () => { it.concurrent('allows an authenticated user to read manifest entries for their own org', async () => { @@ -155,6 +260,14 @@ describe('manifest RLS', () => { expect(data[0]?.file_name).toBe('other-entry.js') }) + it.concurrent('allows a read-only org member to read manifest entries for their own org', async () => { + const { response, data } = await fetchManifestRows(authHeadersNonMember, readOnlyVersionId) + + expect(response.status).toBe(200) + expect(data).toHaveLength(1) + expect(data[0]?.file_name).toBe(READ_ONLY_VISIBLE_FILE) + }) + it.concurrent('allows an API key for the owning org to read manifest entries', async () => { const { response, data } = await fetchManifestRows({ ...restApiKeyHeaders, @@ -175,4 +288,106 @@ describe('manifest RLS', () => { expect(response.status).toBe(200) expect(data).toHaveLength(0) }) + + it('prevents an org owner from inserting manifest entries directly', async () => { + const { response, body } = await insertManifestRow(authHeadersUser1, { + app_version_id: ownVersionId, + file_name: 'blocked-owner.js', + s3_path: `orgs/${READ_ONLY_ORG_ID}/apps/${APP_OWN}/blocked-owner.js`, + file_hash: 'blocked-owner-hash', + }) + + expect(response.ok).toBe(false) + expect([401, 403]).toContain(response.status) + expect(JSON.stringify(body).toLowerCase()).toContain('row-level security') + }) + + it('prevents a read-only org member from inserting manifest entries', async () => { + const { response, body } = await insertManifestRow(authHeadersNonMember, { + app_version_id: readOnlyVersionId, + file_name: 'blocked-readonly.js', + s3_path: `orgs/${READ_ONLY_ORG_ID}/apps/${APP_READ_ONLY}/blocked-readonly.js`, + file_hash: 'blocked-readonly-hash', + }) + + expect(response.ok).toBe(false) + expect([401, 403]).toContain(response.status) + expect(JSON.stringify(body).toLowerCase()).toContain('row-level security') + }) + + it('prevents an org owner from deleting manifest entries directly', async () => { + const { data: createdRow, error: createError } = await getSupabaseClient() + .from('manifest') + .insert({ + app_version_id: ownVersionId, + file_name: 'owner-delete-guard.js', + s3_path: `orgs/${READ_ONLY_ORG_ID}/apps/${APP_OWN}/owner-delete-guard.js`, + file_hash: 'owner-delete-guard-hash', + }) + .select('id') + .single() + + expect(createError).toBeNull() + expect(createdRow?.id).toBeTypeOf('number') + + const createdRowId = createdRow!.id + + const { response: blockedDeleteResponse, body: blockedDeleteBody } = await deleteManifestRow(authHeadersUser1, createdRowId) + + expect(blockedDeleteResponse.status).toBe(200) + expect(blockedDeleteBody).toEqual([]) + + const { data: stillPresent, error: stillPresentError } = await getSupabaseClient() + .from('manifest') + .select('id') + .eq('id', createdRowId) + .maybeSingle() + + expect(stillPresentError).toBeNull() + expect(stillPresent?.id).toBe(createdRowId) + + await getSupabaseClient() + .from('manifest') + .delete() + .eq('id', createdRowId) + .throwOnError() + }) + + it('prevents a read-only org member from deleting manifest entries', async () => { + const { data: createdRow, error: createError } = await getSupabaseClient() + .from('manifest') + .insert({ + app_version_id: readOnlyVersionId, + file_name: 'readonly-delete-guard.js', + s3_path: `orgs/${READ_ONLY_ORG_ID}/apps/${APP_READ_ONLY}/readonly-delete-guard.js`, + file_hash: 'readonly-delete-guard-hash', + }) + .select('id') + .single() + + expect(createError).toBeNull() + expect(createdRow?.id).toBeTypeOf('number') + + const createdRowId = createdRow!.id + + const { response: blockedDeleteResponse, body: blockedDeleteBody } = await deleteManifestRow(authHeadersNonMember, createdRowId) + + expect(blockedDeleteResponse.status).toBe(200) + expect(blockedDeleteBody).toEqual([]) + + const { data: stillPresent, error: stillPresentError } = await getSupabaseClient() + .from('manifest') + .select('id') + .eq('id', createdRowId) + .maybeSingle() + + expect(stillPresentError).toBeNull() + expect(stillPresent?.id).toBe(createdRowId) + + await getSupabaseClient() + .from('manifest') + .delete() + .eq('id', createdRowId) + .throwOnError() + }) })