fix(import): company-scope the bank_file_imports dedup key (#925)
* fix(import): company-scope the bank_file_imports dedup key The bank_file_imports unique constraint was (user_id, file_hash), predating multi-tenancy: a user importing the same statement file into a second company hit an upsert that resolved onto the first company's row, which RLS rejected (42501). Widen it to (company_id, file_hash) - the swap 20260330130000 made for sie_imports but missed here - and drop the now-obsolete BANK_IMPORT_DUPLICATE_OTHER_COMPANY cross-company pre-check from the v1 route (the structured-error code stays for API compat). The migration was already applied to prod; committing it reconciles the orphan (prod schema_migrations had 20260707130000 with no matching repo file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(import): fix stale unique-constraint comment (CodeRabbit) The completion-update comment still described the old (user_id, file_hash) constraint; it is (company_id, file_hash) since 20260707130000. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5
Claude Fable 5
parent
19cbb0094b
commit
63e05c4eec
@@ -79,7 +79,7 @@ export const POST = withRouteContext(
|
||||
status: 'processing',
|
||||
date_from: transactions.map((t) => t.date).sort()[0] || null,
|
||||
date_to: transactions.map((t) => t.date).sort().reverse()[0] || null,
|
||||
}, { onConflict: 'user_id,file_hash' })
|
||||
}, { onConflict: 'company_id,file_hash' })
|
||||
.select()
|
||||
.single()
|
||||
|
||||
|
||||
@@ -199,57 +199,13 @@ export const POST = withApiV1<{ params: Promise<{ companyId: string }> }>(
|
||||
)
|
||||
|
||||
try {
|
||||
// Cross-company collision pre-check. The `bank_file_imports` unique
|
||||
// constraint is `(user_id, file_hash)`: set when the table was
|
||||
// designed for the single-tenant single-company-per-user world. If
|
||||
// the same user is a member of two companies and uploads the same
|
||||
// file to both, a naive upsert with onConflict='user_id,file_hash'
|
||||
// would silently overwrite the first company's row with the second
|
||||
// company_id. Pre-check for that case and surface a structured
|
||||
// error so an agent sees the explicit conflict instead of a
|
||||
// silently-stolen row.
|
||||
//
|
||||
// A migration to widen the unique constraint to (user_id, file_hash,
|
||||
// company_id) is the proper fix; that's an engine-PR concern.
|
||||
const { data: existingImport } = await ctx.supabase
|
||||
.from('bank_file_imports')
|
||||
.select('id, company_id, filename, imported_at, status')
|
||||
.eq('user_id', ctx.userId)
|
||||
.eq('file_hash', fileHash)
|
||||
.maybeSingle()
|
||||
if (existingImport && (existingImport as { company_id: string }).company_id !== ctx.companyId) {
|
||||
// Log the cross-tenant collision details server-side for operator
|
||||
// investigation (CC7.2: audit trail), but do NOT echo the other
|
||||
// company's id or the other import's id back to the caller. Doing
|
||||
// so would be a cross-tenant enumeration vector (V8.2.1 / CC6.1).
|
||||
// The caller sees a fixed error code + a generic message; the
|
||||
// server log carries enough context to debug.
|
||||
ctx.log.warn('bank import: cross-company file-hash collision', {
|
||||
fileHash,
|
||||
attemptedCompanyId: ctx.companyId,
|
||||
existingCompanyId: (existingImport as { company_id: string }).company_id,
|
||||
existingImportId: (existingImport as { id: string }).id,
|
||||
})
|
||||
await failOperation(
|
||||
ctx.supabase,
|
||||
{
|
||||
id: op.id,
|
||||
error: {
|
||||
code: 'BANK_IMPORT_DUPLICATE_OTHER_COMPANY',
|
||||
message: 'This file has already been imported into another company by this user.',
|
||||
},
|
||||
},
|
||||
ctx.log,
|
||||
)
|
||||
return v1ErrorResponseFromCode('BANK_IMPORT_DUPLICATE_OTHER_COMPANY', ctx.log, {
|
||||
requestId: ctx.requestId,
|
||||
// Deliberately empty details: see comment above.
|
||||
})
|
||||
}
|
||||
|
||||
// Record the import row so the dashboard's "bank file imports" tab
|
||||
// shows v1 imports too. `upsert` on (user_id, file_hash) gives
|
||||
// duplicate-rerun protection for the same-company case.
|
||||
// shows v1 imports too. The unique constraint is (company_id,
|
||||
// file_hash) since 20260707130000, so the same user importing the
|
||||
// same statement into two companies is two independent rows, and the
|
||||
// upsert gives duplicate-rerun protection within one company. The
|
||||
// old (user_id, file_hash) key and its cross-company pre-check
|
||||
// (BANK_IMPORT_DUPLICATE_OTHER_COMPANY) are gone.
|
||||
await ctx.supabase
|
||||
.from('bank_file_imports')
|
||||
.upsert(
|
||||
@@ -264,7 +220,7 @@ export const POST = withApiV1<{ params: Promise<{ companyId: string }> }>(
|
||||
date_from: parseResult.date_from,
|
||||
date_to: parseResult.date_to,
|
||||
},
|
||||
{ onConflict: 'user_id,file_hash' },
|
||||
{ onConflict: 'company_id,file_hash' },
|
||||
)
|
||||
|
||||
// Convert parsed transactions to the RawTransaction shape that
|
||||
@@ -288,12 +244,10 @@ export const POST = withApiV1<{ params: Promise<{ companyId: string }> }>(
|
||||
raw,
|
||||
)
|
||||
|
||||
// Mark the bank_file_imports row complete. Scope by all three
|
||||
// identifying fields: `(user_id, file_hash)` is the unique
|
||||
// constraint today but adding `company_id` is defense in depth:
|
||||
// even if a concurrent same-user same-hash import in a different
|
||||
// company slipped past the pre-check, this update can never
|
||||
// overwrite the wrong company's status row.
|
||||
// Mark the bank_file_imports row complete. The unique constraint is
|
||||
// `(company_id, file_hash)` since 20260707130000; scoping the update by
|
||||
// user_id as well is defense in depth so a concurrent same-hash import
|
||||
// can never overwrite the wrong company's status row.
|
||||
await ctx.supabase
|
||||
.from('bank_file_imports')
|
||||
.update({
|
||||
|
||||
Reference in New Issue
Block a user