From ea4da0eb07690b55b1626bf61240b4d9b373f7ad Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Sun, 6 Sep 2026 18:39:08 +0200 Subject: [PATCH] fix(invoices): scope-check article ids in buildInvoiceWriteData so every invoice write refuses a foreign company's article (part of #2059) (#2339) Part of #2059 (Part 2, the hardening bug). The FK on invoice_items.article_id proves the article exists, not that it belongs to the writing company: FK validation ignores RLS, and the v1 routes run on the service-role client with no RLS at all. Only the two MCP commit executors checked tenancy; the cookie POST/PATCH, v1 POST/PATCH, webshop and sales-order writers passed items[].article_id straight through the builder. Move the check to the one point every writer converges on: buildInvoiceWriteData collects the distinct article ids from product lines, runs one select scoped on company_id, and refuses with the new INVOICE_CREATE_ARTICLE_INVALID (400, Swedish message via the structured-error registry) on any miss. The MCP executor checks stay as the tamper gate for staged rows. Tests: builder unit cases (miss refused with details, dedupe + happy path, no article ids means no query, DB error surfaces as dbError), cookie PATCH and v1 POST refusal cases, and the existing v1 persist + MCP update tests now answer the builder's scoped select. Claude-Session: https://claude.ai/code/session_019SaJfqNi4VmsG8FMKq99G6 Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 --- app/api/invoices/[id]/__tests__/route.test.ts | 58 ++++++++++++- .../invoices/__tests__/route.test.ts | 41 +++++++++- lib/errors/structured-errors.ts | 5 ++ .../__tests__/build-invoice-write.test.ts | 82 +++++++++++++++++++ lib/invoices/build-invoice-write.ts | 34 ++++++++ .../__tests__/update-invoice-executor.test.ts | 1 + 6 files changed, 217 insertions(+), 4 deletions(-) diff --git a/app/api/invoices/[id]/__tests__/route.test.ts b/app/api/invoices/[id]/__tests__/route.test.ts index af31c0a6..e660426b 100644 --- a/app/api/invoices/[id]/__tests__/route.test.ts +++ b/app/api/invoices/[id]/__tests__/route.test.ts @@ -4,10 +4,11 @@ import { createMockRouteParams, parseJsonResponse, createQueuedMockSupabase, + makeCustomer, } from '@/tests/helpers' import { eventBus } from '@/lib/events' -const { supabase: mockSupabase, enqueue, reset } = createQueuedMockSupabase() +const { supabase: mockSupabase, enqueue, reset, findCall } = createQueuedMockSupabase() vi.mock('@/lib/supabase/server', () => ({ createClient: () => Promise.resolve(mockSupabase), })) @@ -225,4 +226,59 @@ describe('PATCH /api/invoices/[id]', () => { expect(status).toBe(409) expect(body.error.code).toBe('INVOICE_UPDATE_NOT_DRAFT') }) + + it('refuses an article id that belongs to another company before writing anything (issue #2059)', async () => { + // The FK on invoice_items.article_id proves the article exists, not that + // it is this company's; the cookie route relies on the builder's scoped + // select for that (RLS never sees FK validation). + const FOREIGN_ARTICLE = 'ffffffff-ffff-4fff-8fff-ffffffffffff' + enqueue({ + data: { + id: 'inv-1', + status: 'draft', + invoice_number: null, + journal_entry_id: null, + is_self_billed: false, + credited_invoice_id: null, + document_type: 'invoice', + quote_status: null, + deduction_personnummer_encrypted: null, + deduction_personnummer_last4: null, + }, + error: null, + }) // invoices: the draft being edited + enqueue({ data: makeCustomer({ id: '22222222-2222-4222-8222-222222222222', customer_type: 'swedish_business' }), error: null }) // customers + enqueue({ data: { vat_registered: true }, error: null }) // company_settings (builder VAT gate) + enqueue({ data: [], error: null }) // articles: no company-scoped hit + + const response = await PATCH( + createMockRequest('/api/invoices/inv-1', { + method: 'PATCH', + body: { + customer_id: '22222222-2222-4222-8222-222222222222', + invoice_date: '2026-07-14', + due_date: '2026-08-13', + currency: 'SEK', + items: [ + { + description: 'Konsult', + quantity: 1, + unit: 'tim', + unit_price: 1000, + vat_rate: 25, + article_id: FOREIGN_ARTICLE, + }, + ], + }, + }), + createMockRouteParams({ id: 'inv-1' }), + ) + const { status, body } = await parseJsonResponse<{ error: { code: string; details?: { invalidArticleIds?: string[] } } }>(response) + + expect(status).toBe(400) + expect(body.error.code).toBe('INVOICE_CREATE_ARTICLE_INVALID') + expect(findCall('articles', 'eq')).toEqual(['company_id', 'company-1']) + expect(findCall('invoices', 'update')).toBeUndefined() + expect(findCall('invoice_items', 'insert')).toBeUndefined() + }) }) diff --git a/app/api/v1/companies/[companyId]/invoices/__tests__/route.test.ts b/app/api/v1/companies/[companyId]/invoices/__tests__/route.test.ts index 17252d06..ca67308d 100644 --- a/app/api/v1/companies/[companyId]/invoices/__tests__/route.test.ts +++ b/app/api/v1/companies/[companyId]/invoices/__tests__/route.test.ts @@ -957,9 +957,11 @@ describe('POST /api/v1/companies/:companyId/invoices', () => { ? SWEDISH_BUSINESS_CUSTOMER : table === 'chart_of_accounts' ? [{ account_number: '3041' }] - : table === 'invoices' - ? createdInvoice - : null + : table === 'articles' + ? [{ id: ARTICLE_ID }] + : table === 'invoices' + ? createdInvoice + : null return (r: (v: unknown) => void) => r({ data, error: null }) } return () => new Proxy({}, handler) @@ -1100,6 +1102,39 @@ describe('POST /api/v1/companies/:companyId/invoices', () => { const body = await res.json() expect(body.error.code).toBe('INVOICE_CREATE_REVENUE_ACCOUNT_INVALID') }) + + it('rejects an article_id that belongs to another company (issue #2059)', async () => { + // v1 runs on the service-role client with no RLS: the FK on + // invoice_items.article_id proves existence only, so the scoped select in + // the builder is the only tenancy guard for article linkage. + withInvoiceWriteScope() + const FOREIGN_ARTICLE = 'ffffffff-ffff-4fff-8fff-ffffffffffff' + mockServiceClient.mockReturnValue( + makeFlexibleSupabase({ + company_members: { data: { company_id: COMPANY_ID, role: 'owner' }, error: null }, + customers: { data: SWEDISH_BUSINESS_CUSTOMER, error: null }, + articles: { data: [], error: null }, + }), + ) + + const res = await createInvoice( + makePostInvoice(`https://x.test/api/v1/companies/${COMPANY_ID}/invoices`, { + customer_id: CUSTOMER_ID, + invoice_date: '2026-05-12', + due_date: '2026-06-11', + currency: 'SEK', + items: [ + { description: 'x', quantity: 1, unit: 'st', unit_price: 100, article_id: FOREIGN_ARTICLE }, + ], + }), + companyParams(COMPANY_ID), + ) + + expect(res.status).toBe(400) + const body = await res.json() + expect(body.error.code).toBe('INVOICE_CREATE_ARTICLE_INVALID') + expect(body.error.details).toEqual({ invalidArticleIds: [FOREIGN_ARTICLE] }) + }) }) // ────────────────────────────────────────────────────────────────── diff --git a/lib/errors/structured-errors.ts b/lib/errors/structured-errors.ts index 9176df09..189c7daf 100644 --- a/lib/errors/structured-errors.ts +++ b/lib/errors/structured-errors.ts @@ -888,6 +888,11 @@ const INVOICE: Record = { message_sv: 'Ett angivet bokföringskonto finns inte eller är inte ett aktivt balans- eller intäktskonto (klass 1-3).', message_en: 'A supplied posting account does not exist or is not an active balance-sheet or revenue account (class 1-3).', }, + INVOICE_CREATE_ARTICLE_INVALID: { + httpStatus: 400, + message_sv: 'En angiven artikel finns inte i företaget.', + message_en: 'A supplied article does not exist in this company.', + }, INVOICE_CREATE_POSTING_ACCOUNT_VAT_CONFLICT: { httpStatus: 400, message_sv: 'Ett balanskonto (klass 1-2) kan bara användas på rader utan moms. Använd ett intäktskonto (3xxx) för momspliktiga rader.', diff --git a/lib/invoices/__tests__/build-invoice-write.test.ts b/lib/invoices/__tests__/build-invoice-write.test.ts index 0f388694..e96d86dc 100644 --- a/lib/invoices/__tests__/build-invoice-write.test.ts +++ b/lib/invoices/__tests__/build-invoice-write.test.ts @@ -597,3 +597,85 @@ describe('buildInvoiceWriteData kundkort fallback customer-type gate', () => { expect(result.items[0]).toMatchObject({ sales_order_item_id: null }) }) }) + +describe('buildInvoiceWriteData article company scope (issue #2059)', () => { + const ARTICLE_A = 'a1000000-0000-4000-8000-000000000001' + const ARTICLE_B = 'a1000000-0000-4000-8000-000000000002' + const FOREIGN_ARTICLE = 'a1000000-0000-4000-8000-0000000000ff' + const customer = makeCustomer({ customer_type: 'swedish_business' }) + + it('refuses an article id the company-scoped select cannot see', async () => { + const { supabase, enqueue, findCall } = createQueuedMockSupabase() + enqueue({ data: { vat_registered: true }, error: null }) // company_settings + enqueue({ data: [], error: null }) // articles: no company-scoped hit + + const result = await call(enqueue, supabase as unknown as SupabaseClient, customer, { + ...baseHeader, + items: [ + { description: 'Konsult', quantity: 1, unit: 'tim', unit_price: 1000, vat_rate: 25, article_id: FOREIGN_ARTICLE }, + ], + }) + + expect(result.ok).toBe(false) + if (result.ok || !('code' in result)) return + expect(result.code).toBe('INVOICE_CREATE_ARTICLE_INVALID') + expect(result.details).toEqual({ invalidArticleIds: [FOREIGN_ARTICLE] }) + // The select is scoped on company_id: the FK alone only proves existence. + expect(findCall('articles', 'eq')).toEqual(['company_id', 'company-1']) + expect(findCall('articles', 'in')).toEqual(['id', [FOREIGN_ARTICLE]]) + }) + + it('accepts company-owned articles, deduping repeated ids into one select', async () => { + const { supabase, enqueue, findCall } = createQueuedMockSupabase() + enqueue({ data: { vat_registered: true }, error: null }) // company_settings + enqueue({ data: [{ id: ARTICLE_A }, { id: ARTICLE_B }], error: null }) // articles + + const result = await call(enqueue, supabase as unknown as SupabaseClient, customer, { + ...baseHeader, + items: [ + { description: 'Rad 1', quantity: 1, unit: 'st', unit_price: 100, vat_rate: 25, article_id: ARTICLE_A }, + { description: 'Rad 2', quantity: 2, unit: 'st', unit_price: 100, vat_rate: 25, article_id: ARTICLE_A }, + { description: 'Rad 3', quantity: 1, unit: 'st', unit_price: 100, vat_rate: 25, article_id: ARTICLE_B }, + ], + }) + + expect(result.ok).toBe(true) + if (!result.ok) return + expect(result.items.map((i) => i.article_id)).toEqual([ARTICLE_A, ARTICLE_A, ARTICLE_B]) + expect(findCall('articles', 'in')).toEqual(['id', [ARTICLE_A, ARTICLE_B]]) + }) + + it('never queries articles when no product line carries an article id', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: { vat_registered: true }, error: null }) // company_settings + + const result = await call(enqueue, supabase as unknown as SupabaseClient, customer, { + ...baseHeader, + items: [ + { description: 'Konsult', quantity: 1, unit: 'tim', unit_price: 1000, vat_rate: 25 }, + // A text row never persists an article, so an id on it must not be checked. + { line_type: 'text', description: 'Fri text', quantity: 0, unit: '', unit_price: 0, article_id: FOREIGN_ARTICLE }, + ], + }) + + expect(result.ok).toBe(true) + expect(supabase.from).not.toHaveBeenCalledWith('articles') + }) + + it('surfaces a DB error on the article lookup as dbError, not as a refusal', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: { vat_registered: true }, error: null }) // company_settings + enqueue({ data: null, error: { message: 'connection reset' } }) // articles + + const result = await call(enqueue, supabase as unknown as SupabaseClient, customer, { + ...baseHeader, + items: [ + { description: 'Konsult', quantity: 1, unit: 'tim', unit_price: 1000, vat_rate: 25, article_id: ARTICLE_A }, + ], + }) + + expect(result.ok).toBe(false) + if (result.ok) return + expect('dbError' in result).toBe(true) + }) +}) diff --git a/lib/invoices/build-invoice-write.ts b/lib/invoices/build-invoice-write.ts index f70cfd00..6a6699b3 100644 --- a/lib/invoices/build-invoice-write.ts +++ b/lib/invoices/build-invoice-write.ts @@ -318,6 +318,40 @@ export async function buildInvoiceWriteData(params: { } } + // Article linkage is a tenancy invariant: the FK on invoice_items.article_id + // proves the article EXISTS, not that it belongs to THIS company. FK + // validation is internal to Postgres and ignores RLS, and the v1 routes run + // on the service-role client with no RLS at all, so a body carrying another + // company's article UUID would otherwise persist a cross-tenant reference. + // Every invoice write path converges here (cookie POST/PATCH, v1 POST/PATCH, + // webshop, sales-order conversion, MCP update), so one scoped select covers + // them all. The MCP executors keep their own pre-check as the tamper gate + // for staged rows. Text rows never persist an article (mapped to null below). + const articleIds = Array.from( + new Set( + items + .filter((item) => item.line_type !== 'text') + .map((item) => item.article_id) + .filter((a): a is string => !!a), + ), + ) + if (articleIds.length > 0) { + const { data: articleRows, error: articlesError } = await supabase + .from('articles') + .select('id') + .eq('company_id', companyId) + .in('id', articleIds) + + if (articlesError) { + return { ok: false, dbError: articlesError } + } + const foundArticleIds = new Set((articleRows ?? []).map((a) => a.id)) + const invalidArticleIds = articleIds.filter((a) => !foundArticleIds.has(a)) + if (invalidArticleIds.length > 0) { + return { ok: false, code: 'INVOICE_CREATE_ARTICLE_INVALID', details: { invalidArticleIds } } + } + } + // ROT/RUT-avdrag: validate prerequisites and compute the per-item + // invoice-level deduction. Computed server-side (never trusted from the // client) so a tampered request can't expand the 1513 receivable. Skipped diff --git a/lib/pending-operations/__tests__/update-invoice-executor.test.ts b/lib/pending-operations/__tests__/update-invoice-executor.test.ts index 1e951745..7bb01c1b 100644 --- a/lib/pending-operations/__tests__/update-invoice-executor.test.ts +++ b/lib/pending-operations/__tests__/update-invoice-executor.test.ts @@ -260,6 +260,7 @@ describe('commitPendingOperation: update_invoice', () => { enqueue({ data: [{ id: ARTICLE_ID }] }) // articles: company-scope gate enqueue({ data: { vat_registered: true } }) // company_settings (builder VAT gate) enqueue({ data: [{ account_number: '3041' }] }) // chart_of_accounts: override account + enqueue({ data: [{ id: ARTICLE_ID }] }) // articles: builder company-scope check (#2059) enqueue({ data: [{ id: INVOICE_ID }] }) // invoices update (draft-guarded) enqueue({ data: [] }) // invoice_items snapshot (replaceInvoiceItems) enqueue({ data: null }) // invoice_items delete