Bug/tic unlink (#1153)

* fix(tic): allow BankID link/unlink without a company context

/bankid/link and /bankid/unlink are user-level actions, but the extension
dispatcher resolved an active company for them, so a zero-company user
(fresh BankID signup, pre-onboarding) got a 500 'No company context' when
managing the connection from /settings/account. Mark both routes
skipCompanyContext and resolve the caller in-handler via requireAuth(),
which preserves the dispatcher's MFA/AAL2 enforcement.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(tic): return 409 account_exists instead of 500 on BankID signup with taken email

The signup guard pre-checked profiles.email, but the authoritative store
is auth.users: anonymized account tombstones (and any profile drift) hold
the email in auth.users while profiles.email is NULL. The guard missed,
createUser failed with email_exists (422), and the route surfaced a
dead-end 500 'Kunde inte skapa kontot. Forsok igen.' where retrying can
never succeed.

Drop the profiles pre-check and let createUser's own uniqueness check be
the guard: map email_exists to the existing 409 account_exists response
(Swedish message), which the register page already handles with a toast
and a redirect to login. Also removes the TOCTOU window between the old
pre-check and createUser.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(account): actually scrub auth.users metadata on account deletion

The delete route passed user_metadata: {} / app_metadata: {} to
auth.admin.updateUserById assuming replace semantics, but GoTrue MERGES
metadata maps, so the wipe was a silent no-op: the ~100-year tombstone
kept the user's full name in raw_user_meta_data (verified on production
2026-07-24).

Move the scrub into anonymize_user_account (migration 20260724150000):
raw_user_meta_data is cleared entirely, raw_app_meta_data drops the
app-specific keys (bankid_linked, has_password) while GoTrue's
provider/providers stay, and auth.users.email is still retained as the
documented legitimate-interest tombstone. The migration also repairs
existing tombstones (guarded by profiles.anonymized_at). The route keeps
only the ban, which the DB function cannot set.

Migration content already applied to staging; pg-real test extended.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs: log BankID signup guard decision

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(account): address PR review findings on anonymize scrub

- anonymize_user_account now rejects repeat invocations against an
  already-anonymized tombstone (SQLSTATE P0002) instead of re-churning
  the scrubbed row
- note that the tombstone repair UPDATE runs atomically inside the
  migration transaction
- tic signup failure log hashes the email (sha256 prefix, matching the
  pnrHashPrefix pattern) instead of logging the raw address
- pg-real test for the double-invocation guard

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(anonymization): ensure raw_app_meta_data is not null before scrubbing keys

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Mattsson
2026-07-24 16:36:35 +02:00
committed by GitHub
co-authored by Claude Fable 5
parent 213d611e54
commit 63fd5311ed
8 changed files with 334 additions and 78 deletions
@@ -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
])
@@ -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<ReturnType<typeof requireAuth>>)
}
function mockUnauthenticated() {
vi.mocked(requireAuth).mockResolvedValue({
user: null,
supabase: {},
error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }),
} as unknown as Awaited<ReturnType<typeof requireAuth>>)
}
type QueuedResult = { data?: unknown; error?: unknown }
@@ -73,8 +98,6 @@ function mockServiceClient(fromResults: QueuedResult[], appMetadata: Record<stri
return { admin, client }
}
const ctx = { userId: 'user-1' } as unknown as ExtensionContext
beforeEach(() => {
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<ReturnType<typeof collectBankIdResult>>
}
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)
+60 -41
View File
@@ -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 },
})