fix: BankID signup takeover + supplier invoice dedup + event_log RLS visibility (#358)
* fix(tic): reject BankID signup when email is already registered (CWE-287) Signup previously linked BankID to any pre-existing profile matching the submitted email. Because BankID proves identity but not email ownership, an attacker who knew a victim's email could bind their own BankID to the victim's account and then log in via BankID (which skips TOTP MFA). Now signup returns 409 account_exists; the register page shows a Swedish error toast and redirects to /login so the user can authenticate with their password first and link BankID from settings (via the authenticated /bankid/link route that already exists). Covered by new bankid-complete.test.ts with an explicit regression test asserting no side-effects occur on the account_exists path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(bookkeeping): prevent duplicate registration entries for supplier invoices The invoice-inbox convert flow creates the registration journal entry inline and then emits supplier_invoice.confirmed. The core handler was also creating one, producing a second voucher on 2440/2641/expense and overwriting registration_journal_entry_id on the row. Two guards in the core handler: - Payload guard: skip if supplierInvoice.registration_journal_entry_id is already set (fast path for callers that include it in the payload). - DB re-fetch guard: re-read the row and skip if it has been linked since the payload was built (handles stale-payload callers). The inbox extension now stamps registration_journal_entry_id onto the in-memory invoice before emitting so the fast path trips. Also fixes a latent bug where the handler filtered company_settings by userId instead of companyId, which would have selected the wrong row (or none) on multi-company accounts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(events): write company_id on event_log rows and refuse unscoped events event_log has a company-scoped SELECT RLS policy, so rows written with NULL company_id are invisible to every user — effectively silently dropped from the automation feed. The handler now reads companyId from the payload (all persisted event types mandate it in TS) and includes it in both single and batch inserts. If a caller ever bypasses the type system and emits without companyId, the handler logs an error and skips persistence rather than writing a poisoned row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address PR review nits (event-log meta strip, redirect timer) - event-log-handler: rename stripUserId → stripMetaFields and also drop companyId from the stored data JSONB so it isn't duplicated alongside its dedicated column. - register page: drop the 1500ms setTimeout before router.push('/login') on the account_exists branch — the timer had no cleanup and fired a stale-closure push if the component unmounted first. The toast survives the route change via the root layout's Toaster. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
cc41ae0f1d
commit
a0485ca1c1
@@ -213,6 +213,14 @@ describe('POST /items/:id/convert', () => {
|
||||
expect(status).toBe(200)
|
||||
expect(body.data.registration_journal_entry_id).toBe('je-1')
|
||||
expect(createSupplierInvoiceRegistrationEntry).toHaveBeenCalled()
|
||||
|
||||
// The emitted supplier_invoice.confirmed payload must reflect the just-written
|
||||
// registration_journal_entry_id so the core handler's payload-level guard
|
||||
// short-circuits instead of double-posting.
|
||||
const emitCalls = (ctx.emit as ReturnType<typeof vi.fn>).mock.calls
|
||||
const confirmed = emitCalls.find((c) => c[0].type === 'supplier_invoice.confirmed')
|
||||
expect(confirmed).toBeDefined()
|
||||
expect(confirmed![0].payload.supplierInvoice.registration_journal_entry_id).toBe('je-1')
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -1349,6 +1349,7 @@ export const invoiceInboxExtension: Extension = {
|
||||
)
|
||||
if (journalEntry) {
|
||||
registrationJournalEntryId = journalEntry.id
|
||||
;(invoice as SupplierInvoice).registration_journal_entry_id = journalEntry.id
|
||||
await ctx.supabase
|
||||
.from('supplier_invoices')
|
||||
.update({ registration_journal_entry_id: journalEntry.id })
|
||||
|
||||
@@ -0,0 +1,244 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
|
||||
import { createMockRequest, parseJsonResponse } from '@/tests/helpers'
|
||||
|
||||
vi.mock('../lib/bankid-client', () => ({
|
||||
startBankIdAuth: vi.fn(),
|
||||
pollBankIdSession: vi.fn(),
|
||||
collectBankIdResult: vi.fn(),
|
||||
cancelBankIdSession: vi.fn(),
|
||||
requestEnrichment: vi.fn().mockResolvedValue({ status: 'failed', completedTypes: [] }),
|
||||
fetchEnrichmentData: vi.fn(),
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn(),
|
||||
createClient: vi.fn(),
|
||||
}))
|
||||
|
||||
import { collectBankIdResult } from '../lib/bankid-client'
|
||||
import { createServiceClient } from '@/lib/supabase/server'
|
||||
import { ticExtension } from '../index'
|
||||
|
||||
const TEST_KEY = 'a'.repeat(64)
|
||||
|
||||
function findCompleteHandler() {
|
||||
const route = ticExtension.apiRoutes!.find(
|
||||
(r) => r.method === 'POST' && r.path === '/bankid/complete'
|
||||
)
|
||||
if (!route) throw new Error('POST /bankid/complete route not found in ticExtension.apiRoutes')
|
||||
return route.handler
|
||||
}
|
||||
|
||||
function makeSession(overrides: Partial<{ status: string; user: unknown }> = {}) {
|
||||
return {
|
||||
sessionId: 'test-session',
|
||||
status: 'complete',
|
||||
user: {
|
||||
personalNumber: '199001011234',
|
||||
givenName: 'Anna',
|
||||
surname: 'Andersson',
|
||||
name: 'Anna Andersson',
|
||||
},
|
||||
...overrides,
|
||||
} as unknown as Awaited<ReturnType<typeof collectBankIdResult>>
|
||||
}
|
||||
|
||||
type QueuedResult = { data?: unknown; error?: unknown }
|
||||
|
||||
function mockServiceClient(fromResults: QueuedResult[]) {
|
||||
const queue = [...fromResults]
|
||||
|
||||
const chain = (): unknown => {
|
||||
const result = queue.shift() ?? { data: null, error: null }
|
||||
const handler: ProxyHandler<object> = {
|
||||
get(_t, prop) {
|
||||
if (prop === 'then') return (resolve: (v: unknown) => void) => resolve(result)
|
||||
return () => chain2(result)
|
||||
},
|
||||
}
|
||||
return new Proxy({}, handler)
|
||||
}
|
||||
const chain2 = (result: QueuedResult): unknown => {
|
||||
const handler: ProxyHandler<object> = {
|
||||
get(_t, prop) {
|
||||
if (prop === 'then') return (resolve: (v: unknown) => void) => resolve(result)
|
||||
return () => chain2(result)
|
||||
},
|
||||
}
|
||||
return new Proxy({}, handler)
|
||||
}
|
||||
|
||||
const admin = {
|
||||
createUser: vi.fn().mockResolvedValue({ data: { user: { id: 'new-user-uuid' } }, error: null }),
|
||||
updateUserById: vi.fn().mockResolvedValue({ data: {}, error: null }),
|
||||
generateLink: vi.fn().mockResolvedValue({
|
||||
data: { properties: { hashed_token: 'magic-token-hash' } },
|
||||
error: null,
|
||||
}),
|
||||
getUserById: vi.fn().mockResolvedValue({
|
||||
data: { user: { id: 'existing-user', email: 'existing@example.com' } },
|
||||
}),
|
||||
}
|
||||
|
||||
const client = {
|
||||
from: vi.fn().mockImplementation(() => chain()),
|
||||
auth: { admin },
|
||||
}
|
||||
|
||||
vi.mocked(createServiceClient).mockReturnValue(client as unknown as ReturnType<typeof createServiceClient>)
|
||||
|
||||
return { admin, client }
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
vi.stubEnv('BANKID_ENCRYPTION_KEY', TEST_KEY)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllEnvs()
|
||||
})
|
||||
|
||||
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 () => {
|
||||
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
|
||||
])
|
||||
|
||||
const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', {
|
||||
method: 'POST',
|
||||
body: { sessionId: 'test-session', mode: 'signup', email: 'victim@example.com' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{ error?: string; data?: unknown }>(
|
||||
await findCompleteHandler()(req)
|
||||
)
|
||||
|
||||
expect(status).toBe(409)
|
||||
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()
|
||||
expect(admin.updateUserById).not.toHaveBeenCalled()
|
||||
expect(admin.generateLink).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.
|
||||
const fromCalls = vi.mocked(client.from).mock.calls
|
||||
expect(fromCalls.map((c) => c[0])).toEqual(['bankid_identities', 'profiles'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('signup mode — happy path', () => {
|
||||
it('creates a new user, marks bankid_linked, and returns the magic link tokenHash', async () => {
|
||||
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
|
||||
])
|
||||
|
||||
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<{
|
||||
data?: { tokenHash?: string; type?: string; isNewUser?: boolean }
|
||||
}>(await findCompleteHandler()(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.data?.tokenHash).toBe('magic-token-hash')
|
||||
expect(body.data?.type).toBe('magiclink')
|
||||
expect(body.data?.isNewUser).toBe(true)
|
||||
|
||||
expect(admin.createUser).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ email: 'fresh@example.com', email_confirm: true })
|
||||
)
|
||||
expect(admin.updateUserById).toHaveBeenCalledWith(
|
||||
'new-user-uuid',
|
||||
expect.objectContaining({ app_metadata: { bankid_linked: true } })
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('signup mode — pnr already linked', () => {
|
||||
it('returns 409 already_linked before email lookup', async () => {
|
||||
vi.mocked(collectBankIdResult).mockResolvedValue(makeSession())
|
||||
const { admin, client } = mockServiceClient([
|
||||
{ data: { user_id: 'some-other-user' } }, // pnr lookup → LINKED
|
||||
])
|
||||
|
||||
const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', {
|
||||
method: 'POST',
|
||||
body: { sessionId: 'test-session', mode: 'signup', email: 'x@example.com' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{ error?: string }>(
|
||||
await findCompleteHandler()(req)
|
||||
)
|
||||
|
||||
expect(status).toBe(409)
|
||||
expect(body.error).toBe('already_linked')
|
||||
expect(admin.createUser).not.toHaveBeenCalled()
|
||||
// Only the pnr lookup ran — no profiles query.
|
||||
expect(vi.mocked(client.from).mock.calls.map((c) => c[0])).toEqual(['bankid_identities'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('login mode', () => {
|
||||
it('returns 404 no_account when the BankID pnr is not linked to any user', async () => {
|
||||
vi.mocked(collectBankIdResult).mockResolvedValue(makeSession())
|
||||
const { admin } = mockServiceClient([
|
||||
{ data: null }, // pnr lookup → not linked
|
||||
])
|
||||
|
||||
const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', {
|
||||
method: 'POST',
|
||||
body: { sessionId: 'test-session', mode: 'login' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{ error?: string }>(
|
||||
await findCompleteHandler()(req)
|
||||
)
|
||||
|
||||
expect(status).toBe(404)
|
||||
expect(body.error).toBe('no_account')
|
||||
expect(admin.generateLink).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('input validation', () => {
|
||||
it('returns 400 session_invalid when BankID session is not complete', async () => {
|
||||
vi.mocked(collectBankIdResult).mockResolvedValue(
|
||||
makeSession({ status: 'pending', user: undefined })
|
||||
)
|
||||
mockServiceClient([])
|
||||
|
||||
const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', {
|
||||
method: 'POST',
|
||||
body: { sessionId: 'test-session', mode: 'signup', email: 'x@example.com' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{ error?: string }>(
|
||||
await findCompleteHandler()(req)
|
||||
)
|
||||
|
||||
expect(status).toBe(400)
|
||||
expect(body.error).toBe('session_invalid')
|
||||
})
|
||||
|
||||
it('returns 400 when email is missing in signup mode', async () => {
|
||||
mockServiceClient([])
|
||||
|
||||
const req = createMockRequest('/api/extensions/ext/tic/bankid/complete', {
|
||||
method: 'POST',
|
||||
body: { sessionId: 'test-session', mode: 'signup' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await findCompleteHandler()(req))
|
||||
|
||||
expect(status).toBe(400)
|
||||
// collectBankIdResult should never be called — validation happens first.
|
||||
expect(collectBankIdResult).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -690,51 +690,53 @@ export const ticExtension: Extension = {
|
||||
)
|
||||
}
|
||||
|
||||
// Check if email is already taken by a non-BankID user
|
||||
// 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()
|
||||
|
||||
let userId: string
|
||||
let isNewUser = true
|
||||
|
||||
if (existingByEmail) {
|
||||
// Email already exists — link BankID to existing account
|
||||
userId = existingByEmail.id
|
||||
isNewUser = false
|
||||
|
||||
await supabase.auth.admin.updateUserById(userId, {
|
||||
app_metadata: { bankid_linked: true },
|
||||
user_metadata: { full_name: name },
|
||||
})
|
||||
} else {
|
||||
// Create new Supabase user
|
||||
const randomPassword = crypto.randomBytes(32).toString('base64url')
|
||||
const { data: newUser, error: createError } = await supabase.auth.admin.createUser({
|
||||
email: trimmedEmail!,
|
||||
email_confirm: true,
|
||||
password: randomPassword,
|
||||
user_metadata: { full_name: name },
|
||||
})
|
||||
|
||||
if (createError || !newUser?.user) {
|
||||
log.error('createUser failed', { email: trimmedEmail, status: createError?.status, code: createError?.code, message: createError?.message })
|
||||
return NextResponse.json(
|
||||
{ error: 'Failed to create account', message: createError?.message },
|
||||
{ status: 500 }
|
||||
)
|
||||
}
|
||||
|
||||
userId = newUser.user.id
|
||||
|
||||
// Mark user as BankID-linked (skips TOTP MFA)
|
||||
await supabase.auth.admin.updateUserById(userId, {
|
||||
app_metadata: { bankid_linked: true },
|
||||
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
|
||||
const randomPassword = crypto.randomBytes(32).toString('base64url')
|
||||
const { data: newUser, error: createError } = await supabase.auth.admin.createUser({
|
||||
email: trimmedEmail!,
|
||||
email_confirm: true,
|
||||
password: randomPassword,
|
||||
user_metadata: { full_name: name },
|
||||
})
|
||||
|
||||
if (createError || !newUser?.user) {
|
||||
log.error('createUser failed', { email: trimmedEmail, status: createError?.status, code: createError?.code, message: createError?.message })
|
||||
return NextResponse.json(
|
||||
{ error: 'Failed to create account', message: createError?.message },
|
||||
{ status: 500 }
|
||||
)
|
||||
}
|
||||
|
||||
const userId = newUser.user.id
|
||||
|
||||
// Mark user as BankID-linked (skips TOTP MFA)
|
||||
await supabase.auth.admin.updateUserById(userId, {
|
||||
app_metadata: { bankid_linked: true },
|
||||
})
|
||||
|
||||
// Store BankID identity
|
||||
const { error: insertError } = await supabase
|
||||
.from('bankid_identities')
|
||||
@@ -775,7 +777,7 @@ export const ticExtension: Extension = {
|
||||
data: {
|
||||
tokenHash: link.properties.hashed_token,
|
||||
type: 'magiclink',
|
||||
isNewUser,
|
||||
isNewUser: true,
|
||||
},
|
||||
})
|
||||
} catch (error) {
|
||||
|
||||
@@ -153,7 +153,7 @@ export interface BankIdCompleteResponse {
|
||||
}
|
||||
|
||||
export interface BankIdCompleteErrorResponse {
|
||||
error: 'no_account' | 'already_linked' | 'session_invalid' | 'session_expired'
|
||||
error: 'no_account' | 'already_linked' | 'account_exists' | 'session_invalid' | 'session_expired'
|
||||
givenName?: string
|
||||
surname?: string
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user