diff --git a/DECISIONS.md b/DECISIONS.md index 916b658d..73f5e852 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -360,5 +360,6 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-24] Accounted MCP naming is an additive namespace selected with tool_namespace=accounted: internal gnubok_* ids, authorization maps, API-key prefixes, and the gnubok-mcp package remain canonical compatibility surfaces, while new clients advertise accounted_* aliases through accounted-mcp so existing connections never invalidate. [2026-07-24] PR-review pass on bug/invalid-imports: declined CodeRabbit's ask to fill un-posted prior years in asset-note-figures ibAck via the engine when priorPosted.length>0. The note must tie out to the ledger-driven balansrakning, which reflects posted-only accumulated depreciation; estimating a skipped year would over-state ibAck and BREAK the tie-out this module exists to preserve. A posted-history gap is a real books gap that the build-data tie-out warning correctly surfaces (fix = post the missing year, not paper over it). Engine fallback stays gated to priorPosted.length===0 (pre-onboarding, nothing booked). [2026-07-24] Kept the leading-`!` Tailwind important syntax (`!p-0`/`!pl-0`/`!pr-0`) in the transaction tables over CodeRabbit's trailing-`!` (`p-0!`) rewrite: verified against the installed tailwindcss 4.1.18 compiler that `!p-0` alone emits `.\!p-0{padding:… !important}`, so both forms work in v4 and the flush-edge columns are not broken. Declined the 18-site churn. +[2026-07-24] BankID signup email guard = createUser email_exists error mapping, not a profiles.email pre-check: profiles is not a reliable mirror of auth.users (anonymize tombstones scrub it), and the error-based check has no TOCTOU window. Deleted-account emails stay blocked for re-signup by design (tombstone keeps auth.users.email). [2026-07-24] Journey onboarding stack MERGED to main (#1141, #1145 ex-#1142, #1143) after founder preview click-through + "safe to merge". Production flag NEXT_PUBLIC_ONBOARDING_JOURNEY deliberately NOT set at merge time: BankID roles-prefill path is reducer-tested but not yet live-verified, so the flip is an explicit founder step, followed by one BankID smoke and then PR D (wizard deletion, /companies/new mode='add', picker restyle). [2026-07-24] Onboarding journey migration COMPLETE with PR #1150: wizard deleted, /companies/new on journey mode='add', BankID picker = searchable list, flag conditional removed (env var cleaned from Vercel post-merge). Bot-review triage: compliance findings on getUser()/redirect()/ensure_user_team skipped as App Router misreadings or pre-existing patterns; fixed the real ones (stale select_company keys, unused hasExistingCompanies plumbing). diff --git a/app/api/account/delete/__tests__/route.test.ts b/app/api/account/delete/__tests__/route.test.ts index 32433747..43a4595d 100644 --- a/app/api/account/delete/__tests__/route.test.ts +++ b/app/api/account/delete/__tests__/route.test.ts @@ -137,16 +137,17 @@ describe('POST /api/account/delete', () => { }) expect(updateUserById).toHaveBeenCalledWith( 'user-1', - expect.objectContaining({ - user_metadata: {}, - app_metadata: {}, - ban_duration: expect.any(String), - }) + expect.objectContaining({ ban_duration: expect.any(String) }) ) + const updatePayload = updateUserById.mock.calls[0][1] // Email must NOT be scrubbed: retaining it is what blocks re-signup // with the same address. Recovery goes through support instead. - const updatePayload = updateUserById.mock.calls[0][1] expect(updatePayload).not.toHaveProperty('email') + // Metadata is NOT wiped here: GoTrue merges metadata maps, so passing {} + // was a no-op. The anonymize_user_account RPC scrubs auth.users directly + // (migration 20260724150000). + expect(updatePayload).not.toHaveProperty('user_metadata') + expect(updatePayload).not.toHaveProperty('app_metadata') expect(adminSignOut).toHaveBeenCalledWith('user-1', 'global') expect(emitted).toHaveLength(1) expect(emitted[0]).toMatchObject({ userId: 'user-1' }) diff --git a/app/api/account/delete/route.ts b/app/api/account/delete/route.ts index 6481b30a..538a070d 100644 --- a/app/api/account/delete/route.ts +++ b/app/api/account/delete/route.ts @@ -90,8 +90,8 @@ export async function POST(request: Request) { ) } - // Wipe PII in auth.users metadata and ban the tombstone row ~100 years. - // DB functions can't reach supabase.auth.admin, so we do it here. + // Ban the tombstone row ~100 years so login is impossible. The DB function + // can't set the ban (GoTrue-managed), so we do it here. // // Note: auth.users.email is intentionally NOT scrubbed. The original // address is retained as a legitimate-interest tombstone so that: @@ -105,18 +105,18 @@ export async function POST(request: Request) { // after this point: login is impossible (row is banned) and the // profile is anonymized, so no UI ever surfaces it. // - // user_metadata / app_metadata ARE wiped: they may contain display - // name, avatar, or provider info that isn't needed for recovery. - // The admin API replaces (not merges) these, so passing {} clears them. + // user_metadata / app_metadata PII is scrubbed by the RPC itself, NOT + // here: GoTrue's admin update MERGES metadata maps, so the previous + // updateUserById(..., { user_metadata: {}, app_metadata: {} }) call was + // a silent no-op that left the full name on the tombstone (found on + // prod 2026-07-24, repaired by migration 20260724150000). const service = createServiceClient() try { await service.auth.admin.updateUserById(user.id, { - user_metadata: {}, - app_metadata: {}, ban_duration: '876000h', }) } catch (err) { - log.error('Failed to wipe metadata and ban anonymized user', { userId: user.id, err }) + log.error('Failed to ban anonymized user', { userId: user.id, err }) } try { diff --git a/extensions/general/tic/__tests__/bankid-complete.test.ts b/extensions/general/tic/__tests__/bankid-complete.test.ts index 080d930c..4f958243 100644 --- a/extensions/general/tic/__tests__/bankid-complete.test.ts +++ b/extensions/general/tic/__tests__/bankid-complete.test.ts @@ -103,11 +103,17 @@ afterEach(() => { describe('POST /bankid/complete', () => { describe('signup mode: account_exists regression (CWE-287)', () => { it('returns 409 account_exists and performs NO side effects when email is already registered', async () => { + // The guard is createUser's own auth.users uniqueness check, NOT a + // profiles.email pre-check: anonymized tombstones (account deletion) + // have no profiles.email but still hold the address in auth.users. vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin, client } = mockServiceClient([ { data: null }, // bankid_identities pnr lookup → not linked - { data: { id: 'victim-user-uuid' } }, // profiles email lookup → EXISTS ]) + admin.createUser.mockResolvedValueOnce({ + data: { user: null }, + error: { status: 422, code: 'email_exists', message: 'A user with this email address has already been registered' }, + } as never) const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', { method: 'POST', @@ -121,15 +127,37 @@ describe('POST /bankid/complete', () => { expect(body.error).toBe('account_exists') expect(body.data).toBeUndefined() - // Critical: none of the account-mutation or session-issuance calls ran. - expect(admin.createUser).not.toHaveBeenCalled() + // Critical: no account mutation or session issuance happened. expect(admin.updateUserById).not.toHaveBeenCalled() expect(admin.generateLink).not.toHaveBeenCalled() + expect(admin.deleteUser).not.toHaveBeenCalled() - // No insert into bankid_identities. Only two from() calls should have happened - // (the pnr lookup and the profile lookup), neither of which is an insert. + // No insert into bankid_identities: the only from() call is the pnr lookup. const fromCalls = vi.mocked(client.from).mock.calls - expect(fromCalls.map((c) => c[0])).toEqual(['bankid_identities', 'profiles']) + expect(fromCalls.map((c) => c[0])).toEqual(['bankid_identities']) + }) + + it('returns 500 internal_error for createUser failures that are NOT email_exists', async () => { + vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) + const { admin } = mockServiceClient([ + { data: null }, // pnr lookup → not linked + ]) + admin.createUser.mockResolvedValueOnce({ + data: { user: null }, + error: { status: 500, code: 'unexpected_failure', message: 'boom' }, + } as never) + + const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', { + method: 'POST', + body: { sessionId: 'test-session', mode: 'signup', email: 'fresh@example.com' }, + }) + const { status, body } = await parseJsonResponse<{ error?: string }>( + await findCompleteHandler()(req) + ) + + expect(status).toBe(500) + expect(body.error).toBe('internal_error') + expect(admin.generateLink).not.toHaveBeenCalled() }) }) @@ -138,7 +166,6 @@ describe('POST /bankid/complete', () => { vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient([ { data: null }, // pnr lookup → not linked - { data: null }, // email lookup → not taken { error: null }, // bankid_identities insert OK ]) @@ -200,7 +227,6 @@ describe('POST /bankid/complete', () => { vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient([ { data: null }, // pnr lookup → not linked - { data: null }, // email lookup → not taken { error: { message: 'insert boom', code: 'XX000' } }, // identity insert FAILS ]) @@ -223,7 +249,6 @@ describe('POST /bankid/complete', () => { vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient([ { data: null }, // pnr lookup → not linked - { data: null }, // email lookup → not taken { error: null }, // identity insert OK ]) admin.generateLink.mockResolvedValueOnce({ @@ -248,7 +273,6 @@ describe('POST /bankid/complete', () => { vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient([ { data: null }, // pnr lookup → not linked - { data: null }, // email lookup → not taken ]) admin.updateUserById.mockResolvedValueOnce({ data: null, @@ -272,7 +296,6 @@ describe('POST /bankid/complete', () => { it('does NOT delete anything on the happy path', async () => { vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient([ - { data: null }, { data: null }, { error: null }, ]) @@ -354,7 +377,6 @@ describe('POST /bankid/complete', () => { }) const { client } = mockServiceClient([ { data: null }, // pnr lookup → not linked - { data: null }, // email lookup → not taken { error: null }, // bankid_identities insert OK ]) diff --git a/extensions/general/tic/__tests__/bankid-unlink.test.ts b/extensions/general/tic/__tests__/bankid-unlink.test.ts index ea7603e2..4b2aabdf 100644 --- a/extensions/general/tic/__tests__/bankid-unlink.test.ts +++ b/extensions/general/tic/__tests__/bankid-unlink.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { NextResponse } from 'next/server' import { createMockRequest, parseJsonResponse } from '@/tests/helpers' vi.mock('../lib/bankid-client', () => ({ @@ -15,17 +16,41 @@ vi.mock('@/lib/supabase/server', () => ({ createClient: vi.fn(), })) +vi.mock('@/lib/auth/require-auth', () => ({ + requireAuth: vi.fn(), +})) + import { collectBankIdResult } from '../lib/bankid-client' import { createServiceClient } from '@/lib/supabase/server' +import { requireAuth } from '@/lib/auth/require-auth' import { ticExtension } from '../index' -import type { ExtensionContext } from '@/lib/extensions/types' const TEST_KEY = 'a'.repeat(64) -function findHandler(method: string, path: string) { +function findRoute(method: string, path: string) { const route = ticExtension.apiRoutes!.find((r) => r.method === method && r.path === path) if (!route) throw new Error(`${method} ${path} route not found in ticExtension.apiRoutes`) - return route.handler + return route +} + +function findHandler(method: string, path: string) { + return findRoute(method, path).handler +} + +function mockAuthenticated(userId = 'user-1') { + vi.mocked(requireAuth).mockResolvedValue({ + user: { id: userId }, + supabase: {}, + error: null, + } as unknown as Awaited>) +} + +function mockUnauthenticated() { + vi.mocked(requireAuth).mockResolvedValue({ + user: null, + supabase: {}, + error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }), + } as unknown as Awaited>) } type QueuedResult = { data?: unknown; error?: unknown } @@ -73,8 +98,6 @@ function mockServiceClient(fromResults: QueuedResult[], appMetadata: Record { vi.clearAllMocks() vi.stubEnv('BANKID_ENCRYPTION_KEY', TEST_KEY) @@ -84,18 +107,35 @@ afterEach(() => { vi.unstubAllEnvs() }) +describe('route flags', () => { + // These routes are user-level: a zero-company user (fresh BankID signup, + // pre-onboarding) must be able to manage the connection from settings. + // Without skipCompanyContext the dispatcher throws 'No company context'. + it('link and unlink skip company context', () => { + expect(findRoute('POST', '/bankid/link').skipCompanyContext).toBe(true) + expect(findRoute('POST', '/bankid/unlink').skipCompanyContext).toBe(true) + }) + + it('link and unlink still require auth', () => { + expect(findRoute('POST', '/bankid/link').skipAuth).toBeUndefined() + expect(findRoute('POST', '/bankid/unlink').skipAuth).toBeUndefined() + }) +}) + describe('POST /bankid/unlink', () => { - it('returns 401 without an authenticated context', async () => { + it('returns 401 when unauthenticated', async () => { + mockUnauthenticated() mockServiceClient([], {}) const req = createMockRequest('/api/extensions/ext/tic/bankid/unlink', { method: 'POST' }) const { status } = await parseJsonResponse(await findHandler('POST', '/bankid/unlink')(req)) expect(status).toBe(401) }) - it('merges app_metadata instead of replacing it — has_password must survive unlink', async () => { + it('merges app_metadata instead of replacing it: has_password must survive unlink', async () => { // A BankID-only user: has_password false. Wiping it would make // userHasPassword() infer TRUE (bankid_linked false ⇒ password assumed), // hiding the set-password escape hatch from a user with no login method. + mockAuthenticated() const { admin } = mockServiceClient( [{ error: null }], // bankid_identities delete OK { has_password: false, bankid_linked: true, provider: 'email' } @@ -103,7 +143,7 @@ describe('POST /bankid/unlink', () => { const req = createMockRequest('/api/extensions/ext/tic/bankid/unlink', { method: 'POST' }) const { status, body } = await parseJsonResponse<{ data?: { unlinked?: boolean } }>( - await findHandler('POST', '/bankid/unlink')(req, ctx) + await findHandler('POST', '/bankid/unlink')(req) ) expect(status).toBe(200) @@ -114,6 +154,7 @@ describe('POST /bankid/unlink', () => { }) it('returns 500 when the identity delete fails and does not touch app_metadata', async () => { + mockAuthenticated() const { admin } = mockServiceClient( [{ error: { message: 'delete boom', code: 'XX000' } }], { has_password: false, bankid_linked: true } @@ -121,7 +162,7 @@ describe('POST /bankid/unlink', () => { const req = createMockRequest('/api/extensions/ext/tic/bankid/unlink', { method: 'POST' }) const { status } = await parseJsonResponse( - await findHandler('POST', '/bankid/unlink')(req, ctx) + await findHandler('POST', '/bankid/unlink')(req) ) expect(status).toBe(500) @@ -143,7 +184,30 @@ describe('POST /bankid/link', () => { } as unknown as Awaited> } + it('returns 401 when unauthenticated', async () => { + mockUnauthenticated() + mockServiceClient([], {}) + const req = createMockRequest('/api/extensions/ext/tic/bankid/link', { + method: 'POST', + body: { sessionId: 'test-session' }, + }) + const { status } = await parseJsonResponse(await findHandler('POST', '/bankid/link')(req)) + expect(status).toBe(401) + }) + + it('returns 400 when sessionId is missing', async () => { + mockAuthenticated() + mockServiceClient([], {}) + const req = createMockRequest('/api/extensions/ext/tic/bankid/link', { + method: 'POST', + body: {}, + }) + const { status } = await parseJsonResponse(await findHandler('POST', '/bankid/link')(req)) + expect(status).toBe(400) + }) + it('merges app_metadata so an existing has_password: true survives linking', async () => { + mockAuthenticated() vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient( [ @@ -158,7 +222,7 @@ describe('POST /bankid/link', () => { body: { sessionId: 'test-session' }, }) const { status, body } = await parseJsonResponse<{ data?: { linked?: boolean } }>( - await findHandler('POST', '/bankid/link')(req, ctx) + await findHandler('POST', '/bankid/link')(req) ) expect(status).toBe(200) @@ -169,6 +233,7 @@ describe('POST /bankid/link', () => { }) it('returns 409 already_linked when the personnummer belongs to another user', async () => { + mockAuthenticated() vi.mocked(collectBankIdResult).mockResolvedValue(makeSession()) const { admin } = mockServiceClient( [{ data: { user_id: 'someone-else' } }], @@ -180,7 +245,7 @@ describe('POST /bankid/link', () => { body: { sessionId: 'test-session' }, }) const { status, body } = await parseJsonResponse<{ error?: string }>( - await findHandler('POST', '/bankid/link')(req, ctx) + await findHandler('POST', '/bankid/link')(req) ) expect(status).toBe(409) diff --git a/extensions/general/tic/index.ts b/extensions/general/tic/index.ts index 40d34e63..949d937e 100644 --- a/extensions/general/tic/index.ts +++ b/extensions/general/tic/index.ts @@ -28,6 +28,7 @@ import type { TICCompanyProfile, TICFinancialReportSummary } from './lib/tic-typ import type { BankIdCompleteRequest } from './lib/bankid-types' import type { CompanyLookupResult } from '@/lib/company-lookup/types' import { hashPersonalNumber, encryptPersonalNumber } from '@/lib/auth/bankid' +import { requireAuth } from '@/lib/auth/require-auth' import { createServiceClient } from '@/lib/supabase/server' import { createLogger } from '@/lib/logger' import type { SupabaseClient } from '@supabase/supabase-js' @@ -960,30 +961,11 @@ export const ticExtension: Extension = { ) } - // If the email is already registered, refuse signup. Linking BankID to an - // existing account must go through the authenticated /bankid/link route so - // email ownership is proven by password login first. (CWE-287) - const { data: existingByEmail } = await supabase - .from('profiles') - .select('id') - .eq('email', trimmedEmail!) - .single() - - if (existingByEmail) { - log.warn('bankid signup rejected: email already registered', { - sessionId, - pnrHashPrefix: pnrHash.slice(0, 8), - }) - return NextResponse.json( - { - error: 'account_exists', - message: 'An account with this email already exists. Log in and link BankID from settings.', - }, - { status: 409 } - ) - } - - // Create new Supabase user + // Create new Supabase user. Email uniqueness is checked by createUser + // itself against auth.users: do NOT pre-check profiles.email instead. + // The profile mirror can lack the address while the auth row still + // holds it (anonymize_user_account scrubs profiles.email but keeps the + // auth tombstone), which used to fall through to a dead-end 500 here. const randomPassword = crypto.randomBytes(32).toString('base64url') const { data: newUser, error: createError } = await supabase.auth.admin.createUser({ email: trimmedEmail!, @@ -993,7 +975,30 @@ export const ticExtension: Extension = { }) if (createError || !newUser?.user) { - log.error('createUser failed', { email: trimmedEmail, status: createError?.status, code: createError?.code, message: createError?.message }) + // Email already registered (including deleted-account tombstones, + // which keep their email on purpose): refuse signup. Linking BankID + // to an existing account must go through the authenticated + // /bankid/link route so email ownership is proven by password login + // first. (CWE-287) + if (createError?.code === 'email_exists') { + log.warn('bankid signup rejected: email already registered', { + sessionId, + pnrHashPrefix: pnrHash.slice(0, 8), + }) + return NextResponse.json( + { + error: 'account_exists', + message: 'Det finns redan ett konto med den här e-postadressen. Logga in och koppla BankID under Inställningar.', + }, + { status: 409 } + ) + } + log.error('createUser failed', { + emailHashPrefix: crypto.createHash('sha256').update(trimmedEmail!).digest('hex').slice(0, 8), + status: createError?.status, + code: createError?.code, + message: createError?.message, + }) return NextResponse.json( { error: 'internal_error', message: 'Kunde inte skapa kontot. Försök igen.' }, { status: 500 } @@ -1127,13 +1132,24 @@ export const ticExtension: Extension = { { method: 'POST', path: '/bankid/link', - // skipAuth: false, requires existing Supabase session - handler: async (request: Request, ctx?) => { + // Requires a Supabase session but NOT a company: a user who just + // signed up (or hasn't finished onboarding) must be able to manage + // their BankID connection from /settings/account. Without this flag + // the dispatcher's requireCompanyId() throws 'No company context' + // for zero-company users. skipCompanyContext dispatches without ctx, + // so the handler resolves the caller itself via requireAuth() (same + // MFA/AAL2 enforcement the dispatcher applies). + skipCompanyContext: true, + handler: async (request: Request) => { try { + const auth = await requireAuth() + if (auth.error) return auth.error + const userId = auth.user.id + const body = await request.json() const { sessionId } = body - if (!sessionId || !ctx?.userId) { + if (!sessionId) { return NextResponse.json({ error: 'sessionId is required' }, { status: 400 }) } @@ -1157,14 +1173,14 @@ export const ticExtension: Extension = { .eq('personal_number_hash', pnrHash) .single() - if (existing && existing.user_id !== ctx.userId) { + if (existing && existing.user_id !== userId) { return NextResponse.json( { error: 'already_linked', message: 'This BankID is already linked to another account' }, { status: 409 } ) } - if (existing && existing.user_id === ctx.userId) { + if (existing && existing.user_id === userId) { return NextResponse.json({ data: { linked: true, alreadyLinked: true } }) } @@ -1172,7 +1188,7 @@ export const ticExtension: Extension = { const { error: insertError } = await supabase .from('bankid_identities') .insert({ - user_id: ctx.userId, + user_id: userId, personal_number_hash: pnrHash, personal_number_enc: encryptPersonalNumber(personalNumber), given_name: givenName, @@ -1192,9 +1208,9 @@ export const ticExtension: Extension = { // { bankid_linked: true } would wipe has_password for users who // already set one, they'd then be incorrectly shown the // set-password banner on their next session. - const { data: priorUser } = await supabase.auth.admin.getUserById(ctx.userId) + const { data: priorUser } = await supabase.auth.admin.getUserById(userId) const priorMeta = priorUser?.user?.app_metadata ?? {} - await supabase.auth.admin.updateUserById(ctx.userId, { + await supabase.auth.admin.updateUserById(userId, { app_metadata: { ...priorMeta, bankid_linked: true }, }) @@ -1219,12 +1235,15 @@ export const ticExtension: Extension = { { method: 'POST', path: '/bankid/unlink', - // skipAuth: false, requires existing Supabase session - handler: async (_request: Request, ctx?) => { + // Requires a Supabase session but NOT a company: see /bankid/link. + // A brand-new BankID signup (zero companies) must be able to undo + // the connection from /settings/account. + skipCompanyContext: true, + handler: async (_request: Request) => { try { - if (!ctx?.userId) { - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) - } + const auth = await requireAuth() + if (auth.error) return auth.error + const userId = auth.user.id const supabase = createServiceClient() @@ -1232,7 +1251,7 @@ export const ticExtension: Extension = { const { error: deleteError } = await supabase .from('bankid_identities') .delete() - .eq('user_id', ctx.userId) + .eq('user_id', userId) if (deleteError) { log.error('unlink delete failed', { message: deleteError.message, code: deleteError.code }) @@ -1246,9 +1265,9 @@ export const ticExtension: Extension = { // user (has_password: false) would then be inferred as HAVING a // password (lib/auth/has-password.ts) and could strand themselves // with no working login method. - const { data: priorUser } = await supabase.auth.admin.getUserById(ctx.userId) + const { data: priorUser } = await supabase.auth.admin.getUserById(userId) const priorMeta = priorUser?.user?.app_metadata ?? {} - await supabase.auth.admin.updateUserById(ctx.userId, { + await supabase.auth.admin.updateUserById(userId, { app_metadata: { ...priorMeta, bankid_linked: false }, }) diff --git a/supabase/migrations/20260724150000_anonymize_scrub_auth_metadata.sql b/supabase/migrations/20260724150000_anonymize_scrub_auth_metadata.sql new file mode 100644 index 00000000..a34ac285 --- /dev/null +++ b/supabase/migrations/20260724150000_anonymize_scrub_auth_metadata.sql @@ -0,0 +1,95 @@ +-- Scrub auth.users metadata inside anonymize_user_account. +-- +-- WHY +-- --- +-- app/api/account/delete/route.ts tried to wipe user_metadata/app_metadata +-- after the RPC by calling auth.admin.updateUserById(userId, { user_metadata: +-- {}, app_metadata: {} }). GoTrue MERGES metadata maps on admin update, so +-- passing an empty object is a no-op: the tombstone kept the user's full name +-- in raw_user_meta_data on a row we retain ~100 years (verified on production +-- 2026-07-24). Anonymization must actually remove the PII, so the scrub moves +-- into the SECURITY DEFINER function where a direct UPDATE is deterministic +-- and atomic with the profile scrub. +-- +-- raw_user_meta_data is cleared entirely (full_name, avatar, any provider +-- leftovers). raw_app_meta_data only drops our app-specific keys +-- (bankid_linked, has_password): provider/providers stay, GoTrue owns those. + +CREATE OR REPLACE FUNCTION public.anonymize_user_account(target_user_id uuid) + RETURNS void + LANGUAGE plpgsql + SECURITY DEFINER + SET search_path TO 'public' +AS $function$ +DECLARE + blocker_count int; +BEGIN + IF auth.uid() IS DISTINCT FROM target_user_id THEN + RAISE EXCEPTION 'Can only delete your own account'; + END IF; + + -- Reject repeat invocations against an already-anonymized tombstone: the + -- account is gone, re-running would only churn the scrubbed row. + IF EXISTS ( + SELECT 1 FROM public.profiles + WHERE id = target_user_id AND anonymized_at IS NOT NULL + ) THEN + RAISE EXCEPTION 'Account is already deleted' USING ERRCODE = 'P0002'; + END IF; + + SELECT count(*) INTO blocker_count + FROM public.company_members cm + JOIN public.companies c ON c.id = cm.company_id + WHERE cm.user_id = target_user_id + AND cm.role = 'owner' + AND c.archived_at IS NULL; + + IF blocker_count > 0 THEN + RAISE EXCEPTION 'Cannot delete account: user still owns % active compan(y/ies)', blocker_count + USING ERRCODE = 'P0001'; + END IF; + + DELETE FROM public.company_members WHERE user_id = target_user_id; + DELETE FROM public.team_members WHERE user_id = target_user_id; + DELETE FROM public.bankid_identities WHERE user_id = target_user_id; + + DELETE FROM public.user_preferences WHERE user_id = target_user_id; + DELETE FROM public.api_keys WHERE user_id = target_user_id; + + UPDATE public.profiles + SET email = NULL, + full_name = NULL, + avatar_url = NULL, + deleted_at = now(), + anonymized_at = now(), + updated_at = now() + WHERE id = target_user_id; + + -- Scrub PII from the auth tombstone. auth.users.email is intentionally + -- kept (blocks re-signup + lets support verify identity for BFL-retained + -- data recovery; documented legitimate interest, see + -- app/api/account/delete/route.ts). + UPDATE auth.users + SET raw_user_meta_data = '{}'::jsonb, + raw_app_meta_data = coalesce(raw_app_meta_data, '{}'::jsonb) - 'bankid_linked' - 'has_password' + WHERE id = target_user_id; +END; +$function$; + +REVOKE ALL ON FUNCTION public.anonymize_user_account(uuid) FROM PUBLIC, anon; +GRANT EXECUTE ON FUNCTION public.anonymize_user_account(uuid) TO authenticated; + +-- Repair existing tombstones: every already-anonymized profile whose auth row +-- still carries metadata. Guarded by anonymized_at so live users are untouched. +-- The migration runner applies this whole file in a single transaction, so the +-- UPDATE is atomic: it either scrubs all matching rows or none. +UPDATE auth.users u + SET raw_user_meta_data = '{}'::jsonb, + raw_app_meta_data = coalesce(u.raw_app_meta_data, '{}'::jsonb) - 'bankid_linked' - 'has_password' + FROM public.profiles p + WHERE p.id = u.id + AND p.anonymized_at IS NOT NULL + AND (u.raw_user_meta_data <> '{}'::jsonb + OR u.raw_app_meta_data ?| array['bankid_linked', 'has_password']); + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/account-deletion-rpcs.pg.test.ts b/tests/pg/account-deletion-rpcs.pg.test.ts index 9d972b26..5f85f333 100644 --- a/tests/pg/account-deletion-rpcs.pg.test.ts +++ b/tests/pg/account-deletion-rpcs.pg.test.ts @@ -127,4 +127,57 @@ describe('account deletion RPCs (pg)', () => { }) // withUserContext rolls back, so the seeded rows do not leak. }) + + it('scrubs auth.users metadata: user_metadata wiped, app keys dropped, provider kept', async () => { + // Migration 20260724150000: the route-level updateUserById "wipe" was a + // silent no-op (GoTrue merges metadata maps), so the tombstone kept the + // user's full name. The RPC now scrubs auth.users directly. Email must + // survive: it is the documented legitimate-interest tombstone. + const userId = await insertAuthUser() + await getPool().query( + `UPDATE auth.users + SET raw_user_meta_data = '{"full_name": "PG Real Person", "email_verified": true}'::jsonb, + raw_app_meta_data = '{"provider": "email", "providers": ["email"], "bankid_linked": true, "has_password": false}'::jsonb + WHERE id = $1`, + [userId], + ) + + await withUserContext(userId, async (client) => { + await client.query('SELECT public.anonymize_user_account($1)', [userId]) + // The authenticated role has no SELECT on auth.users; drop back to the + // superuser session user to verify. Still inside the same transaction, + // so the rolled-back writes remain visible. + await client.query('RESET ROLE') + const { rows } = await client.query<{ + email: string | null + user_meta: Record + app_meta: Record + }>( + `SELECT email, raw_user_meta_data AS user_meta, raw_app_meta_data AS app_meta + FROM auth.users WHERE id = $1`, + [userId], + ) + expect(rows).toHaveLength(1) + expect(rows[0]!.email).toBe(`pg-real-${userId}@test.invalid`) + expect(rows[0]!.user_meta).toEqual({}) + expect(rows[0]!.app_meta).toEqual({ provider: 'email', providers: ['email'] }) + }) + }) + + it('rejects a repeat invocation against an already-anonymized tombstone', async () => { + const userId = await insertAuthUser() + await getPool().query( + `INSERT INTO public.profiles (id, email, full_name) + VALUES ($1, $2, 'PG Real') + ON CONFLICT (id) DO UPDATE SET email = EXCLUDED.email, full_name = EXCLUDED.full_name`, + [userId, `pg-real-${userId}@test.invalid`], + ) + + await withUserContext(userId, async (client) => { + await client.query('SELECT public.anonymize_user_account($1)', [userId]) + await expect( + client.query('SELECT public.anonymize_user_account($1)', [userId]), + ).rejects.toThrow(/already deleted/i) + }) + }) })