fix(bank): never pre-check or mirror another company's accounts in the EB callback (#2116)
* fix(bank): never pre-check or mirror another company's accounts in the EB callback At one-session banks (SEB) the PSU's single consent can cover accounts a sibling company books. The OAuth callback stored whatever the session returned into the active company: all pre-enabled, mirrored into its cash_accounts, ledgers allocated from its chart: one 'Spara val' away from booking another aktiebolag's transactions (user report F1, 2026-09-01). The deliberate reuse path (findReusableSessions) already guards claimed IBANs; the callback now runs the same check via fetchCrossCompanyAccountContext: - accounts claimed by another of the user's companies are stored disabled + flagged (claimed_by_company_*), skipped by the cash_accounts mirror, and the picker names the claiming company - a 'Synkas ej' deselection made on any other connection row is carried onto fresh rows (the recurring came-back-pre-checked complaint, C2) - lookup failure fails closed: new accounts stored deselected - accounts the row itself already carried keep their own state, so a renewal can never switch a working feed off Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197wmwP6zNaYvsGfbZuQHGA * fix(bank): close the skeptic-found holes in the cross-company claim guard Consolidated fixes from the three-skeptic review of PR #2116 (all three refuted the first cut): - Active-company standing state (enabled cash_accounts + enabled accounts on its live-ish connection rows) now outranks sibling claims company-wide, not row-wide: a bank-list renewal arrives on a FRESH row with no priors, and the old row-local check would have let a sibling claim switch a working feed off while supersede demoted its cash row. - pending_selection rows no longer claim accounts or feed deselection memory: their flags are unconfirmed callback output (including this guard's own fail-closed writes), so an abandoned picker or a transient lookup error can no longer poison later connects. - Guard-disabled accounts are never mirrored from the callback: upsertFromPsd2 with enabled:false for a new-to-row account could promote the seeded primary 1930 manual row and flip it to disabled under a foreign identity. - The selection save skips ledger allocation and the cash_accounts mirror for disabled never-mirrored accounts, so 'no cash row, no 19xx slot burned' holds past the mandatory Spara val, and strips the claimed_by_*/deselected flags when the user deliberately enables an account. - Deselection carry is no longer silent: deselected_elsewhere flag + picker note 'Tidigare bortvald'. - Claim lookups paginate via fetchAllRows: the bare select's silent 1000-row PostgREST cap failed open for exactly the multi-company consultants the guard exists for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197wmwP6zNaYvsGfbZuQHGA * fix(bank): claim-guard round 2: pending_selection claims asymmetrically, paged reads ordered Skeptic re-verification of 25a339810 found two holes: - Excluding pending_selection rows from claims reopened the attach-to-picker window: an attach-created row holds deliberately offered enabled accounts with no cash_accounts rows until its picker is saved, and a full-OAuth connect in another company inside that window could take the same physical account. Enabled accounts on pending_selection rows claim again; their disabled flags still stay out of the deselection memory (unconfirmed callback output, including the guard's own fail-closed writes). - Both fetchAllRows claim queries now order('id'): unordered .range() pagination can silently skip rows at page boundaries, and a skipped row is a missed claim, failing open at exactly the 1000+-row scale the pagination was added for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197wmwP6zNaYvsGfbZuQHGA --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5
Claude Fable 5
parent
b1f7116231
commit
fa69174aa0
@@ -9,13 +9,15 @@ vi.mock('@/extensions/general/enable-banking/lib/api-client', () => ({
|
||||
}))
|
||||
|
||||
// Use hoisted to safely create mock objects referenced in vi.mock factories
|
||||
const { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede } = vi.hoisted(() => {
|
||||
const mockFrom = vi.fn()
|
||||
const mockUpsertFromPsd2 = vi.fn()
|
||||
const mockAllocate = vi.fn()
|
||||
const mockSupersede = vi.fn()
|
||||
return { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede }
|
||||
})
|
||||
const { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede, mockCrossCompanyContext } =
|
||||
vi.hoisted(() => {
|
||||
const mockFrom = vi.fn()
|
||||
const mockUpsertFromPsd2 = vi.fn()
|
||||
const mockAllocate = vi.fn()
|
||||
const mockSupersede = vi.fn()
|
||||
const mockCrossCompanyContext = vi.fn()
|
||||
return { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede, mockCrossCompanyContext }
|
||||
})
|
||||
|
||||
// The supersede pass has its own unit tests (extensions/general/enable-banking/
|
||||
// __tests__/supersede.test.ts); here it is mocked so these tests assert the
|
||||
@@ -24,6 +26,19 @@ vi.mock('@/extensions/general/enable-banking/lib/supersede', () => ({
|
||||
supersedeSiblingConnections: (...args: unknown[]) => mockSupersede(...args),
|
||||
}))
|
||||
|
||||
// The cross-company claim lookup has its own unit tests (extensions/general/
|
||||
// enable-banking/lib/__tests__/session-sharing.test.ts); mocked here so the
|
||||
// per-test mockFrom scripts don't have to answer its queries too. Everything
|
||||
// else in session-sharing stays real.
|
||||
vi.mock('@/extensions/general/enable-banking/lib/session-sharing', async (importOriginal) => {
|
||||
const actual =
|
||||
await importOriginal<typeof import('@/extensions/general/enable-banking/lib/session-sharing')>()
|
||||
return {
|
||||
...actual,
|
||||
fetchCrossCompanyAccountContext: (...args: unknown[]) => mockCrossCompanyContext(...args),
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn().mockResolvedValue({
|
||||
from: mockFrom,
|
||||
@@ -91,6 +106,12 @@ describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
vi.clearAllMocks()
|
||||
mockUpsertFromPsd2.mockResolvedValue(undefined)
|
||||
mockSupersede.mockResolvedValue({ supersededIds: [], dedupScopeByIban: new Map() })
|
||||
// No sibling company claims anything by default; individual tests override.
|
||||
mockCrossCompanyContext.mockResolvedValue({
|
||||
claims: new Map(),
|
||||
deselectedIbans: new Set(),
|
||||
activeCompanyIbans: new Set(),
|
||||
})
|
||||
// Allocator stand-in mirroring the real behavior: currency default first,
|
||||
// then the next free 1931–1959 slot (skipping other currency defaults).
|
||||
mockAllocate.mockImplementation(
|
||||
@@ -271,6 +292,195 @@ describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
expect(mirrorLedgers).toEqual(['1930', '1931'])
|
||||
})
|
||||
|
||||
// The F1 scenario: at a one-session bank the PSU's single consent covers
|
||||
// accounts another of the user's companies books (and, before this guard,
|
||||
// stored them pre-enabled in the wrong company and mirrored them into its
|
||||
// cash_accounts, one save away from cross-company bookkeeping).
|
||||
function mockConnectionFlow(pendingRow: Record<string, unknown>) {
|
||||
const capturedUpdates: Record<string, unknown>[] = []
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation(() => {
|
||||
callIndex++
|
||||
if (callIndex === 1) {
|
||||
return mockChain({ data: pendingRow, error: null })
|
||||
}
|
||||
const chain: Record<string, unknown> = {}
|
||||
chain.update = vi.fn((payload: Record<string, unknown>) => {
|
||||
capturedUpdates.push(payload)
|
||||
return chain
|
||||
})
|
||||
chain.eq = vi.fn().mockReturnValue(chain)
|
||||
chain.select = vi.fn().mockReturnValue(chain)
|
||||
chain.single = vi.fn().mockResolvedValue({
|
||||
data: { id: 'conn-1', bank_name: 'SEB', company_id: 'company-1', user_id: 'user-1' },
|
||||
error: null,
|
||||
})
|
||||
chain.then = (resolve: (v: unknown) => void) => resolve({ data: null, error: null })
|
||||
return chain
|
||||
})
|
||||
return capturedUpdates
|
||||
}
|
||||
|
||||
it('stores accounts claimed by a sibling company disabled + flagged and never mirrors them', async () => {
|
||||
const capturedUpdates = mockConnectionFlow({
|
||||
id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'SEB', status: 'pending',
|
||||
})
|
||||
mockCrossCompanyContext.mockResolvedValue({
|
||||
claims: new Map([
|
||||
['SE9999', { companyId: 'company-2', companyName: 'Other Energy AB' }],
|
||||
]),
|
||||
deselectedIbans: new Set(),
|
||||
activeCompanyIbans: new Set(),
|
||||
})
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [
|
||||
{ uid: 'acc-own', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
{ uid: 'acc-foreign', account_id: { iban: 'SE9999' }, name: 'Annat bolags konto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2027-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'SEB', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
|
||||
expect(response.status).toBe(200)
|
||||
// Drain the stream: the finalize work completes behind the interim page.
|
||||
await response.text()
|
||||
const accountsData = capturedUpdates[0].accounts_data as Array<{
|
||||
uid: string
|
||||
enabled: boolean
|
||||
claimed_by_company_id?: string
|
||||
claimed_by_company_name?: string
|
||||
}>
|
||||
const own = accountsData.find(a => a.uid === 'acc-own')
|
||||
const foreign = accountsData.find(a => a.uid === 'acc-foreign')
|
||||
expect(own?.enabled).toBe(true)
|
||||
expect(own?.claimed_by_company_id).toBeUndefined()
|
||||
expect(foreign?.enabled).toBe(false)
|
||||
expect(foreign?.claimed_by_company_id).toBe('company-2')
|
||||
expect(foreign?.claimed_by_company_name).toBe('Other Energy AB')
|
||||
|
||||
// The claimed account gets NO cash_accounts row and NO 19xx slot in this
|
||||
// company's chart: only the own account is mirrored.
|
||||
expect(mockUpsertFromPsd2).toHaveBeenCalledTimes(1)
|
||||
expect((mockUpsertFromPsd2.mock.calls[0][2] as { external_uid: string }).external_uid).toBe('acc-own')
|
||||
expect(mockAllocate).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('keeps an account this row already carried enabled even when a sibling claims its IBAN', async () => {
|
||||
// A standing feed in the active company outranks a sibling's claim: a
|
||||
// renewal must never switch a working account off. (Legacy double-claims
|
||||
// from the pre-session-sharing era make this overlap real.)
|
||||
const capturedUpdates = mockConnectionFlow({
|
||||
id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'SEB', status: 'expired',
|
||||
accounts_data: [
|
||||
{ uid: 'acc-old', iban: 'SE1234', name: 'Företagskonto', currency: 'SEK', enabled: true },
|
||||
],
|
||||
})
|
||||
mockCrossCompanyContext.mockResolvedValue({
|
||||
claims: new Map([['SE1234', { companyId: 'company-2', companyName: 'Other AB' }]]),
|
||||
deselectedIbans: new Set(),
|
||||
activeCompanyIbans: new Set(),
|
||||
})
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-2',
|
||||
accounts: [
|
||||
{ uid: 'acc-new', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2027-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'SEB', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
// Drain the stream: the finalize work completes behind the interim page.
|
||||
await response.text()
|
||||
|
||||
const accountsData = capturedUpdates[0].accounts_data as Array<{
|
||||
uid: string
|
||||
enabled: boolean
|
||||
claimed_by_company_id?: string
|
||||
}>
|
||||
expect(accountsData[0].enabled).toBe(true)
|
||||
expect(accountsData[0].claimed_by_company_id).toBeUndefined()
|
||||
expect(mockUpsertFromPsd2).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('carries a deselection made on another connection row onto a fresh connect', async () => {
|
||||
// C2: "Synkas ej" chosen under one company must not come back pre-checked
|
||||
// when a new connection row (any company) sees the same IBAN.
|
||||
const capturedUpdates = mockConnectionFlow({
|
||||
id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'SEB', status: 'pending',
|
||||
})
|
||||
mockCrossCompanyContext.mockResolvedValue({
|
||||
claims: new Map(),
|
||||
deselectedIbans: new Set(['SE5555']),
|
||||
activeCompanyIbans: new Set(),
|
||||
})
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [
|
||||
{ uid: 'acc-card', account_id: { iban: 'SE5555' }, name: 'Privat kreditkort', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2027-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'SEB', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
// Drain the stream: the finalize work completes behind the interim page.
|
||||
await response.text()
|
||||
|
||||
const accountsData = capturedUpdates[0].accounts_data as Array<{
|
||||
uid: string
|
||||
enabled: boolean
|
||||
claimed_by_company_id?: string
|
||||
deselected_elsewhere?: boolean
|
||||
}>
|
||||
expect(accountsData[0].enabled).toBe(false)
|
||||
// Not a claim (no company books it), but flagged so the picker can say
|
||||
// WHY the box is unchecked instead of leaving a silent gap.
|
||||
expect(accountsData[0].claimed_by_company_id).toBeUndefined()
|
||||
expect(accountsData[0].deselected_elsewhere).toBe(true)
|
||||
// Guard-disabled accounts are never mirrored from the callback: writing
|
||||
// enabled:false for a new-to-row account can promote an existing manual
|
||||
// holder (the seeded primary 1930) and flip it to disabled.
|
||||
expect(mockUpsertFromPsd2).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('fails closed when the claim lookup errors: new accounts stored deselected, unflagged', async () => {
|
||||
const capturedUpdates = mockConnectionFlow({
|
||||
id: 'conn-1', user_id: 'user-1', company_id: 'company-1', bank_name: 'SEB', status: 'pending',
|
||||
})
|
||||
mockCrossCompanyContext.mockResolvedValue(null)
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [
|
||||
{ uid: 'acc-1', account_id: { iban: 'SE1234' }, name: 'Företagskonto', currency: 'SEK' },
|
||||
],
|
||||
access: { valid_until: '2027-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'SEB', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
|
||||
// The connect itself still succeeds: fail-closed costs a checkbox, not
|
||||
// the connection.
|
||||
expect(response.status).toBe(200)
|
||||
// Drain the stream: the finalize work completes behind the interim page.
|
||||
await response.text()
|
||||
const accountsData = capturedUpdates[0].accounts_data as Array<{
|
||||
uid: string
|
||||
enabled: boolean
|
||||
claimed_by_company_id?: string
|
||||
}>
|
||||
expect(accountsData[0].enabled).toBe(false)
|
||||
expect(accountsData[0].claimed_by_company_id).toBeUndefined()
|
||||
// Fail-closed accounts are not mirrored either: enabled:false for a
|
||||
// new-to-row account can promote and disable an existing manual holder.
|
||||
// The selection save mirrors whatever the user enables.
|
||||
expect(mockUpsertFromPsd2).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('preserves existing mirrored ledgers on reconnect instead of re-deriving them', async () => {
|
||||
let callIndex = 0
|
||||
mockFrom.mockImplementation((table: string) => {
|
||||
|
||||
@@ -12,7 +12,10 @@ import {
|
||||
defaultLedgerForCurrency,
|
||||
normalizeIban,
|
||||
} from '@/lib/cash-accounts/service'
|
||||
import { fanOutSessionRenewal } from '@/extensions/general/enable-banking/lib/session-sharing'
|
||||
import {
|
||||
fanOutSessionRenewal,
|
||||
fetchCrossCompanyAccountContext,
|
||||
} from '@/extensions/general/enable-banking/lib/session-sharing'
|
||||
import { supersedeSiblingConnections } from '@/extensions/general/enable-banking/lib/supersede'
|
||||
import { getBankConnectionErrorMessage } from '@/lib/errors/get-error-message'
|
||||
import { renderFinalizeShell, renderFinalizeRedirect } from './finalize-page'
|
||||
@@ -520,6 +523,84 @@ async function finalizeConnection(
|
||||
}
|
||||
}
|
||||
|
||||
// Cross-company guard: at one-session banks (SEB) the PSU's single consent
|
||||
// can cover accounts another of the user's companies already books. The
|
||||
// deliberate reuse path (findReusableSessions) never offers a claimed IBAN,
|
||||
// but this callback used to trust the session wholesale: a connect performed
|
||||
// under company B stored company A's accounts pre-enabled and mirrored them
|
||||
// into B's cash_accounts, one "Spara val" away from booking A's transactions
|
||||
// in B's ledger. Accounts claimed elsewhere are stored disabled + flagged
|
||||
// (the picker names the claiming company) and skipped by the mirror below.
|
||||
// Accounts this row itself carried before keep their own state: the active
|
||||
// company's standing choice outranks a sibling's claim, so a renewal can
|
||||
// never switch a working feed off.
|
||||
const crossCompany = await fetchCrossCompanyAccountContext(
|
||||
supabase,
|
||||
userId,
|
||||
pendingConnection.company_id,
|
||||
pendingConnection.id,
|
||||
)
|
||||
// Every account the guard itself disabled, whatever the branch. These are
|
||||
// excluded from the cash_accounts mirror below: mirroring enabled:false for
|
||||
// a new-to-row account can PROMOTE an existing manual holder (the seeded
|
||||
// primary 1930 included) and flip it to disabled with a foreign identity,
|
||||
// and a claimed account's row would double-claim the IBAN besides. The
|
||||
// selection save allocates + mirrors any of them the user turns on.
|
||||
const guardDisabledUids = new Set<string>()
|
||||
let claimedCount = 0
|
||||
for (const account of accountsMetadata) {
|
||||
const normalizedIban = normalizeIban(account.iban)
|
||||
// Row-local memory only. The active company's standing state on OTHER
|
||||
// rows (a bank-list renewal arrives on a fresh row while the old row is
|
||||
// waiting to be superseded) is already folded into crossCompany:
|
||||
// activeCompanyIbans outrank claims and deselections there, so such
|
||||
// accounts fall through to the enabled default below.
|
||||
const seenOnThisRow =
|
||||
priorEnabledByUid.has(account.uid) ||
|
||||
(normalizedIban ? priorEnabledByIban.has(normalizedIban) : false) ||
|
||||
pairedPriorUidByNewUid.has(account.uid)
|
||||
if (seenOnThisRow) continue
|
||||
|
||||
if (crossCompany === null) {
|
||||
// Fail closed: without the claim set a free account cannot be told from
|
||||
// one another company books, and pre-checking a claimed account is the
|
||||
// one outcome this guard must never produce. The user just re-ticks.
|
||||
account.enabled = false
|
||||
guardDisabledUids.add(account.uid)
|
||||
continue
|
||||
}
|
||||
const claim = normalizedIban ? crossCompany.claims.get(normalizedIban) : undefined
|
||||
if (claim) {
|
||||
account.enabled = false
|
||||
account.claimed_by_company_id = claim.companyId
|
||||
if (claim.companyName) account.claimed_by_company_name = claim.companyName
|
||||
guardDisabledUids.add(account.uid)
|
||||
claimedCount += 1
|
||||
continue
|
||||
}
|
||||
if (normalizedIban && crossCompany.deselectedIbans.has(normalizedIban)) {
|
||||
// The user already said "Synkas ej" to this account on another
|
||||
// connection row: a fresh row must not resurrect it pre-checked. The
|
||||
// flag makes the picker say so; an unexplained unchecked box reads as
|
||||
// a glitch and a silent one hides a sync gap.
|
||||
account.enabled = false
|
||||
account.deselected_elsewhere = true
|
||||
guardDisabledUids.add(account.uid)
|
||||
}
|
||||
}
|
||||
if (crossCompany === null) {
|
||||
log.error('cross-company claim lookup failed: storing new accounts deselected', {
|
||||
connectionId: pendingConnection.id,
|
||||
})
|
||||
} else if (claimedCount > 0) {
|
||||
log.warn('session covers accounts claimed by sibling companies', {
|
||||
connectionId: pendingConnection.id,
|
||||
companyId: pendingConnection.company_id,
|
||||
claimedCount,
|
||||
accountCount: accountsMetadata.length,
|
||||
})
|
||||
}
|
||||
|
||||
// Stay in 'pending_selection' until the user confirms which accounts to sync.
|
||||
// The cron and manual sync routes both skip this status, so no transactions
|
||||
// can be pulled before the user has had a chance to deselect accounts.
|
||||
@@ -657,6 +738,14 @@ async function finalizeConnection(
|
||||
let accountsDataDirty = carriedScopeDirty
|
||||
|
||||
for (const account of accountsMetadata) {
|
||||
// Nothing the guard disabled is mirrored here: a claimed account's row
|
||||
// would be another company's data in this routing table (and an enabled
|
||||
// one would double-claim the IBAN), and mirroring enabled:false for any
|
||||
// new-to-row account can promote an existing manual holder — the seeded
|
||||
// primary 1930 included — flipping it to disabled under a foreign
|
||||
// identity. No 19xx slot is burned either. The selection save allocates
|
||||
// and mirrors whichever of them the user deliberately turns on.
|
||||
if (guardDisabledUids.has(account.uid)) continue
|
||||
let targetLedger = mirroredByUid.get(account.uid)?.ledger_account
|
||||
let reuseCashAccountId: string | null = null
|
||||
if (!targetLedger) {
|
||||
|
||||
Reference in New Issue
Block a user