From 1a27b5bd4a50ba8126b4ea849970cdf34dbcbc59 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Wed, 26 Aug 2026 13:34:31 +0200 Subject: [PATCH] fix(auth): route document integrity, transaction delete/list and agent categorize through withRouteContext (#1926) Two handlers hand-rolled supabase.auth.getUser() and therefore skipped the MFA (AAL2) gate on hosted: DELETE /api/transactions/[id] and GET /api/transactions. Both sat next to a sibling handler that was already wrapped, and the raw-route-auth ratchet exempted a file as soon as any withRouteContext call appeared in it, so they were never flagged. GET /api/documents/[id]/integrity and POST /api/agent/categorize called requireAuth() directly (MFA enforced, but no request id, no completion log, no canonical error envelope). All four are now withRouteContext handlers with identical company scoping and responses; the transaction delete keeps its viewer rejection via requireWrite. The guard now judges each top-level export segment of a route file on its own, so a wrapped handler no longer exempts a hand-rolled sibling. Baseline is unchanged (mcp-oauth/authorize remains the one grandfathered file). Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- DECISIONS.md | 2 + .../agent/categorize/__tests__/route.test.ts | 43 +++- app/api/agent/categorize/route.ts | 164 +++++++------- .../[id]/integrity/__tests__/route.test.ts | 15 ++ app/api/documents/[id]/integrity/route.ts | 156 ++++++------- .../transactions/[id]/__tests__/route.test.ts | 40 +++- app/api/transactions/[id]/route.ts | 210 +++++++++--------- app/api/transactions/__tests__/route.test.ts | 34 ++- app/api/transactions/route.ts | 17 +- scripts/checks/no-new-antipatterns.mjs | 23 +- 10 files changed, 393 insertions(+), 311 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index d74fb745..d1c284a6 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1247,6 +1247,8 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-25] Woo bulk revenue template = per-rate account choice, no hardcoded varor/tjanster preset: BAS 2026 has no standard 30xx goods/services subdivision (3040-series is company-specific), so presets would invent accounts; chosen accounts are validated against the company chart instead, and only diffs from the 3001-series default are sent. [2026-08-26] Support-dialog attachments use the existing email delivery path without storage or schema changes: this keeps the feature scoped to the contact form. The budget is 5 files / 4 MB total under the 4.5 MB hosted request-body ceiling, with client-side image shrinking when needed. [2026-08-26] RFC 9728 protected-resource metadata is served at THREE locations (root, path-based /.well-known/oauth-protected-resource/, and /.well-known/oauth-protected-resource): Claude.ai's connector setup derives the metadata URL from the server URL and fetches it before any 401, so the root document our WWW-Authenticate header points at was not enough ('Authorization with Accounted failed' with only 404s in the logs). One builder, three routes; the path-based route answers 404 for any path other than the MCP endpoint so no phantom resource is advertised. +[2026-08-26] raw-route-auth guard judges each top-level export segment of a route file, not the whole file: transactions/[id] (wrapped PATCH + hand-rolled DELETE) and transactions (wrapped POST + hand-rolled GET) passed the file-level check for months because one withRouteContext call exempted every sibling handler. Baseline unchanged (mcp-oauth/authorize is the one grandfathered file). +[2026-08-26] No ratchet on direct requireAuth() calls in app/api: requireAuth() is the MFA (AAL2) guard withRouteContext itself calls, and .claude/rules/api-routes.md sanctions it for routes without a company context (onboarding, account, user prefs). The 20 remaining direct callers skip request ids and the canonical envelope, not MFA; migrating them is a consistency campaign, not a security fix, so it was not folded into the bypass PR. [2026-08-26] defer_invoice_booking (#967) now gates booking on every door, not just the dashboard: MCP send_invoice / mark_invoice_sent / create_supplier_invoice_from_inbox, v1 invoices send / mark-sent and supplier-invoices create, and the inbox convert route all checked accounting_method === 'accrual' and posted a verifikat at issue for deferred companies. All six now call booksInvoicesOnIssue() (lib/bookkeeping/booking-mode.ts), the same helper the dashboard routes use, so the setting has one meaning. No data repair attempted: vouchers already posted for deferred companies through these doors are legitimate entries and stay. [2026-08-20] The swedish-e-invoicing skill now names Upphandlingsmyndigheten as Sweden Peppol Authority across all eight files, not just the one that was flagged: the handover completed 1 July 2026 (regeringsbeslut Fi2025/01826) and the skill was written in future tense, so a partial fix would have left the atom internally contradictory and still pointed agents at peppol@digg.se. Four digg.se URLs were repointed to their verified 301 targets on upphandlingsmyndigheten.se; the fifth, DIGG Peppol testbadd, is a hard 404 with no redirect and no successor page at the new authority, so it was replaced with the SFTI Validex verification service (https://sfti.validex.net/) rather than left dead or guessed at. Historical attributions (Q4 2025 traffic statistics, the 0007:2021006883 Peppol-ID example) deliberately still say DIGG because they were accurate when published. [2026-08-26] Webhook event catalogue lives in lib/webhooks/public-events.ts (grouped, with docs prose) and the fan-out handler set, the v1 create enum (so the OpenAPI spec and skills/accounted-api), and the docs page all derive from it: the enum and the docs had drifted to 24 of the 28 events the handler delivered, so the four reconciliation.* events were rejected at subscribe time. No API_V1_VERSION bump: the changelog already lists them as additive. diff --git a/app/api/agent/categorize/__tests__/route.test.ts b/app/api/agent/categorize/__tests__/route.test.ts index 4a637998..9ed364d8 100644 --- a/app/api/agent/categorize/__tests__/route.test.ts +++ b/app/api/agent/categorize/__tests__/route.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { NextResponse } from 'next/server' -import { createMockRequest, parseJsonResponse } from '@/tests/helpers' +import { createMockRequest, createMockRouteParams, parseJsonResponse } from '@/tests/helpers' const requireAuthMock = vi.fn() vi.mock('@/lib/auth/require-auth', () => ({ requireAuth: () => requireAuthMock() })) @@ -26,11 +26,12 @@ vi.mock('@/lib/agent/categorize/select-account', () => ({ selectAccount: (...a: import { POST } from '../route' // supabase router: membership + transactions + companies + company_settings. -function makeSupabase(opts: { tx?: unknown } = {}) { +function makeSupabase(opts: { tx?: unknown; member?: boolean } = {}) { return { + auth: { getUser: vi.fn() }, from(table: string) { const rows: Record = { - company_members: { user_id: 'user-1' }, + company_members: opts.member === false ? null : { user_id: 'user-1' }, transactions: opts.tx === undefined ? { id: 'tx-1' } : opts.tx, companies: { entity_type: 'aktiebolag' }, company_settings: { vat_registered: true }, @@ -67,23 +68,41 @@ beforeEach(() => { describe('POST /api/agent/categorize', () => { it('401 when unauthenticated', async () => { requireAuthMock.mockResolvedValue({ user: null, supabase, error: NextResponse.json({ error: 'x' }, { status: 401 }) }) - expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }))).status).toBe(401) + expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({}))).status).toBe(401) + expect(selectAccount).not.toHaveBeenCalled() + }) + it('resolves the session through requireAuth (withRouteContext), never a hand-rolled getUser()', async () => { + await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({})) + expect(requireAuthMock).toHaveBeenCalledTimes(1) + expect(supabase.auth.getUser).not.toHaveBeenCalled() + }) + it('403 when the body names a company the caller is not a member of', async () => { + const other = '22222222-2222-4222-8222-222222222222' + requireAuthMock.mockResolvedValue({ user: { id: 'user-1' }, supabase: makeSupabase({ member: false }), error: null }) + const res = await POST(createMockRequest('/x', { method: 'POST', body: body({ company_id: other }) }), createMockRouteParams({})) + expect(res.status).toBe(403) + expect(selectAccount).not.toHaveBeenCalled() + }) + it('uses the active company without a membership round trip when no override is given', async () => { + const res = await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({})) + expect(res.status).toBe(200) + expect(gatherCandidates).toHaveBeenCalledWith(expect.anything(), 'company-1', expect.anything()) }) it('429 when rate limited', async () => { checkRate.mockResolvedValue({ ok: false }) - expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }))).status).toBe(429) + expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({}))).status).toBe(429) }) it('400 on a missing/invalid transaction_id', async () => { - expect((await POST(createMockRequest('/x', { method: 'POST', body: {} }))).status).toBe(400) - expect((await POST(createMockRequest('/x', { method: 'POST', body: { transaction_id: 'nope' } }))).status).toBe(400) + expect((await POST(createMockRequest('/x', { method: 'POST', body: {} }), createMockRouteParams({}))).status).toBe(400) + expect((await POST(createMockRequest('/x', { method: 'POST', body: { transaction_id: 'nope' } }), createMockRouteParams({}))).status).toBe(400) }) it('403 without the ai capability', async () => { requireCapability.mockResolvedValue(NextResponse.json({ error: 'pay' }, { status: 403 })) - expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }))).status).toBe(403) + expect((await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({}))).status).toBe(403) }) it('503 when no backend is configured', async () => { aiStatus.mockReturnValue({ configured: false }) - const res = await POST(createMockRequest('/x', { method: 'POST', body: body() })) + const res = await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({})) const { status, body: b } = await parseJsonResponse<{ code: string }>(res) expect(status).toBe(503) expect(b.code).toBe('ai_unconfigured') @@ -91,12 +110,12 @@ describe('POST /api/agent/categorize', () => { }) it('404 when the transaction is not found / not this company', async () => { requireAuthMock.mockResolvedValue({ user: { id: 'user-1' }, supabase: makeSupabase({ tx: null }), error: null }) - const res = await POST(createMockRequest('/x', { method: 'POST', body: body() })) + const res = await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({})) expect(res.status).toBe(404) expect(selectAccount).not.toHaveBeenCalled() }) it('returns the selection + candidate slate on the happy path', async () => { - const res = await POST(createMockRequest('/x', { method: 'POST', body: body({ samples: 3, underlag: 'Biltema AB 499 kr' }) })) + const res = await POST(createMockRequest('/x', { method: 'POST', body: body({ samples: 3, underlag: 'Biltema AB 499 kr' }) }), createMockRouteParams({})) const { status, body: b } = await parseJsonResponse<{ data: { account: string; confidence: number; candidates: { account: string }[] } }>(res) @@ -112,7 +131,7 @@ describe('POST /api/agent/categorize', () => { }) it('gathers underlag server-side when the caller did not supply it', async () => { - await POST(createMockRequest('/x', { method: 'POST', body: body() })) + await POST(createMockRequest('/x', { method: 'POST', body: body() }), createMockRouteParams({})) expect(gatherUnderlag).toHaveBeenCalled() expect(selectAccount).toHaveBeenCalledWith( expect.objectContaining({ underlag: 'Kvitto: Biltema, totalt 499 SEK.' }), diff --git a/app/api/agent/categorize/route.ts b/app/api/agent/categorize/route.ts index f0fa8fea..e4f1b199 100644 --- a/app/api/agent/categorize/route.ts +++ b/app/api/agent/categorize/route.ts @@ -1,7 +1,6 @@ import { NextResponse } from 'next/server' import { z } from 'zod' -import { requireAuth } from '@/lib/auth/require-auth' -import { getActiveCompanyId } from '@/lib/company/context' +import { withRouteContext } from '@/lib/api/with-route-context' import { checkAgentRateLimit, agentRateLimitResponseBody } from '@/lib/rate-limits/agent' import { guardSandbox } from '@/lib/sandbox/guard' import { requireCapability } from '@/lib/entitlements/has-capability' @@ -37,91 +36,96 @@ const Schema = z.object({ samples: z.number().int().min(1).max(5).optional(), }) -export async function POST(request: Request): Promise { - const { user, supabase, error } = await requireAuth() - if (error) return error +// withRouteContext enforces auth (MFA on hosted) and resolves the active +// company; the body may still name another company the caller belongs to. +export const POST = withRouteContext( + 'agent.categorize', + async (request, { user, supabase, companyId: activeCompanyId }) => { + const rate = await checkAgentRateLimit(supabase, user.id) + if (!rate.ok) return NextResponse.json(agentRateLimitResponseBody(rate), { status: 429 }) - const rate = await checkAgentRateLimit(supabase, user.id) - if (!rate.ok) return NextResponse.json(agentRateLimitResponseBody(rate), { status: 429 }) + let body: unknown + try { + body = await request.json() + } catch { + return NextResponse.json({ error: 'Invalid JSON' }, { status: 400 }) + } + const parsed = Schema.safeParse(body) + if (!parsed.success) { + return NextResponse.json({ error: 'Ogiltig förfrågan.', type: 'validation_error' }, { status: 400 }) + } - let body: unknown - try { - body = await request.json() - } catch { - return NextResponse.json({ error: 'Invalid JSON' }, { status: 400 }) - } - const parsed = Schema.safeParse(body) - if (!parsed.success) { - return NextResponse.json({ error: 'Ogiltig förfrågan.', type: 'validation_error' }, { status: 400 }) - } + const companyId = parsed.data.company_id ?? activeCompanyId - const companyId = parsed.data.company_id ?? (await getActiveCompanyId(supabase, user.id)) - if (!companyId) return NextResponse.json({ error: 'No active company' }, { status: 400 }) + // The wrapper already guarantees membership of the active company; an + // explicit company_id override must be verified the same way. + if (companyId !== activeCompanyId) { + const { data: membership } = await supabase + .from('company_members') + .select('user_id') + .eq('company_id', companyId) + .eq('user_id', user.id) + .maybeSingle() + if (!membership) return NextResponse.json({ error: 'Forbidden' }, { status: 403 }) + } - const { data: membership } = await supabase - .from('company_members') - .select('user_id') - .eq('company_id', companyId) - .eq('user_id', user.id) - .maybeSingle() - if (!membership) return NextResponse.json({ error: 'Forbidden' }, { status: 403 }) + const blocked = await guardSandbox(supabase, companyId) + if (blocked) return blocked - const blocked = await guardSandbox(supabase, companyId) - if (blocked) return blocked + const capBlocked = await requireCapability(supabase, companyId, CAPABILITY.ai) + if (capBlocked) return capBlocked - const capBlocked = await requireCapability(supabase, companyId, CAPABILITY.ai) - if (capBlocked) return capBlocked + if (!getAiStatus().configured) { + return NextResponse.json( + { error: 'Assistenten är inte konfigurerad på den här installationen.', code: 'ai_unconfigured' }, + { status: 503 }, + ) + } - if (!getAiStatus().configured) { - return NextResponse.json( - { error: 'Assistenten är inte konfigurerad på den här installationen.', code: 'ai_unconfigured' }, - { status: 503 }, - ) - } + const { data: tx } = await supabase + .from('transactions') + .select('id, merchant_name, description, original_description, amount, date, currency, category, is_business, document_id') + .eq('id', parsed.data.transaction_id) + .eq('company_id', companyId) + .maybeSingle() + if (!tx) return NextResponse.json({ error: 'Transaktionen hittades inte.' }, { status: 404 }) - const { data: tx } = await supabase - .from('transactions') - .select('id, merchant_name, description, original_description, amount, date, currency, category, is_business, document_id') - .eq('id', parsed.data.transaction_id) - .eq('company_id', companyId) - .maybeSingle() - if (!tx) return NextResponse.json({ error: 'Transaktionen hittades inte.' }, { status: 404 }) + const [{ data: company }, { data: settings }] = await Promise.all([ + supabase.from('companies').select('entity_type').eq('id', companyId).maybeSingle(), + supabase.from('company_settings').select('vat_registered').eq('company_id', companyId).maybeSingle(), + ]) - const [{ data: company }, { data: settings }] = await Promise.all([ - supabase.from('companies').select('entity_type').eq('id', companyId).maybeSingle(), - supabase.from('company_settings').select('vat_registered').eq('company_id', companyId).maybeSingle(), - ]) + try { + // Gather the matched receipt/invoice text when the caller didn't supply it: + // this is what lifts the cold-start case — the model reads the actual + // supplier + line items, not just the bank line. Best-effort; '' if none. + const underlag = + parsed.data.underlag ?? + (await gatherUnderlag( + supabase, + companyId, + (tx as Transaction).id, + (tx as { document_id?: string | null }).document_id, + )) - try { - // Gather the matched receipt/invoice text when the caller didn't supply it: - // this is what lifts the cold-start case — the model reads the actual - // supplier + line items, not just the bank line. Best-effort; '' if none. - const underlag = - parsed.data.underlag ?? - (await gatherUnderlag( - supabase, - companyId, - (tx as Transaction).id, - (tx as { document_id?: string | null }).document_id, - )) - - const candidates = await gatherCandidates(supabase, companyId, tx as Transaction) - const selection = await selectAccount({ - transaction: { - merchantName: (tx as Transaction).merchant_name, - description: (tx as Transaction).description, - amount: (tx as Transaction).amount, - date: (tx as Transaction).date, - currency: (tx as Transaction).currency, - }, - underlag, - candidates, - entityType: ((company?.entity_type as EntityType | undefined) ?? 'enskild_firma'), - vatRegistered: settings?.vat_registered ?? false, - samples: parsed.data.samples, - }) - return NextResponse.json({ data: { ...selection, candidates } }) - } catch (err) { - return NextResponse.json({ error: getUserErrorMessage(err) }, { status: 500 }) - } -} + const candidates = await gatherCandidates(supabase, companyId, tx as Transaction) + const selection = await selectAccount({ + transaction: { + merchantName: (tx as Transaction).merchant_name, + description: (tx as Transaction).description, + amount: (tx as Transaction).amount, + date: (tx as Transaction).date, + currency: (tx as Transaction).currency, + }, + underlag, + candidates, + entityType: ((company?.entity_type as EntityType | undefined) ?? 'enskild_firma'), + vatRegistered: settings?.vat_registered ?? false, + samples: parsed.data.samples, + }) + return NextResponse.json({ data: { ...selection, candidates } }) + } catch (err) { + return NextResponse.json({ error: getUserErrorMessage(err) }, { status: 500 }) + } + }, +) diff --git a/app/api/documents/[id]/integrity/__tests__/route.test.ts b/app/api/documents/[id]/integrity/__tests__/route.test.ts index 99c06172..ec461fc7 100644 --- a/app/api/documents/[id]/integrity/__tests__/route.test.ts +++ b/app/api/documents/[id]/integrity/__tests__/route.test.ts @@ -7,10 +7,17 @@ import { const { supabase: mockSupabase, enqueue, reset } = createQueuedMockSupabase() +// The route goes through withRouteContext: requireAuth (MFA on hosted) plus +// active-company resolution. The document is still authorized against its +// own company's membership inside the handler. vi.mock('@/lib/auth/require-auth', () => ({ requireAuth: vi.fn(), })) +vi.mock('@/lib/company/context', () => ({ + getActiveCompanyId: vi.fn().mockResolvedValue('33333333-3333-4333-a333-333333333333'), +})) + // The storage download goes through the service-role client: the storage // SELECT policy only covers the uploader's own folder, so the user-scoped // client cannot read colleague-uploaded files within the same company. @@ -61,6 +68,14 @@ describe('GET /api/documents/[id]/integrity', () => { const res = await GET(req(), createMockRouteParams({ id: validDocId })) const { status } = await parseJsonResponse(res) expect(status).toBe(401) + expect(serviceStorageFromMock).not.toHaveBeenCalled() + }) + + it('resolves the session through requireAuth, never a hand-rolled getUser()', async () => { + enqueue({ data: null, error: null }) + await GET(req(), createMockRouteParams({ id: validDocId })) + expect(requireAuth).toHaveBeenCalledTimes(1) + expect(mockSupabase.auth.getUser).not.toHaveBeenCalled() }) it('returns 400 when id is not a UUID', async () => { diff --git a/app/api/documents/[id]/integrity/route.ts b/app/api/documents/[id]/integrity/route.ts index 0d0418e3..86f4735e 100644 --- a/app/api/documents/[id]/integrity/route.ts +++ b/app/api/documents/[id]/integrity/route.ts @@ -1,11 +1,8 @@ import { NextResponse } from 'next/server' import { z } from 'zod' -import { requireAuth } from '@/lib/auth/require-auth' +import { withRouteContext } from '@/lib/api/with-route-context' import { createServiceClient } from '@/lib/supabase/server' import { validateDocumentMagicBytes } from '@/lib/core/documents/document-service' -import { createLogger } from '@/lib/logger' - -const log = createLogger('documents.integrity') const ParamsSchema = z.object({ id: z.string().uuid() }) @@ -26,88 +23,91 @@ const ParamsSchema = z.object({ id: z.string().uuid() }) * reason for an invalid result is logged server-side rather than returned * to the client to avoid information disclosure (V1.2.5 / GDPR Art 25(2)) * and to keep this from being a probe surface for storage internals. + * + * Wrapped in withRouteContext so auth (MFA on hosted), request ids and the + * completion log follow the house pattern. Authorization is still keyed on + * the document's own company (a member may probe a document in any company + * they belong to, same as document_attachments RLS), not on the wrapper's + * active company. */ -export async function GET( - _request: Request, - { params }: { params: Promise<{ id: string }> } -) { - const { user, supabase, error } = await requireAuth() - if (error) return error +export const GET = withRouteContext<{ params: Promise<{ id: string }> }>( + 'document.integrity', + async (_request, { supabase, user, log }, { params }) => { + const rawParams = await params + const parsed = ParamsSchema.safeParse(rawParams) + if (!parsed.success) { + return NextResponse.json({ error: 'Invalid document id' }, { status: 400 }) + } + const { id } = parsed.data - const rawParams = await params - const parsed = ParamsSchema.safeParse(rawParams) - if (!parsed.success) { - return NextResponse.json({ error: 'Invalid document id' }, { status: 400 }) - } - const { id } = parsed.data + // Filter to the current version. The integrity check is meaningful only + // on the live file; superseded versions are archived bytes and should + // not be re-probed (they're already preserved in the version chain + // exactly as uploaded). + const { data: doc, error: docError } = await supabase + .from('document_attachments') + .select('id, company_id, mime_type, storage_path') + .eq('id', id) + .eq('is_current_version', true) + .single() - // Filter to the current version. The integrity check is meaningful only - // on the live file; superseded versions are archived bytes and should - // not be re-probed (they're already preserved in the version chain - // exactly as uploaded). - const { data: doc, error: docError } = await supabase - .from('document_attachments') - .select('id, company_id, mime_type, storage_path') - .eq('id', id) - .eq('is_current_version', true) - .single() + if (docError || !doc) { + return NextResponse.json({ error: 'Document not found' }, { status: 404 }) + } - if (docError || !doc) { - return NextResponse.json({ error: 'Document not found' }, { status: 404 }) - } + // Tenant membership: even with the user-scoped supabase client below, + // we want a clear 404 rather than relying on a storage-layer RLS deny + // (which can present as a generic error). RLS on document_attachments + // is the primary control; this is defense in depth. + const { data: membership } = await supabase + .from('company_members') + .select('company_id') + .eq('company_id', doc.company_id) + .eq('user_id', user.id) + .maybeSingle() - // Tenant membership: even with the user-scoped supabase client below, - // we want a clear 404 rather than relying on a storage-layer RLS deny - // (which can present as a generic error). RLS on document_attachments - // is the primary control; this is defense in depth. - const { data: membership } = await supabase - .from('company_members') - .select('company_id') - .eq('company_id', doc.company_id) - .eq('user_id', user.id) - .maybeSingle() + if (!membership) { + return NextResponse.json({ error: 'Document not found' }, { status: 404 }) + } - if (!membership) { - return NextResponse.json({ error: 'Document not found' }, { status: 404 }) - } + if (!doc.mime_type) { + return NextResponse.json({ data: { valid: true } }) + } - if (!doc.mime_type) { - return NextResponse.json({ data: { valid: true } }) - } + // Download via the service-role client: the storage SELECT policy only + // covers the uploader's own folder (documents/{uid}/...), so the + // user-scoped client cannot read colleague-uploaded files even within + // the same company. The document_attachments RLS fetch plus the explicit + // membership check above are the authorization (same model as the + // inline proxy route). + const serviceClient = createServiceClient() + const { data: blob, error: downloadError } = await serviceClient.storage + .from('documents') + .download(doc.storage_path) - // Download via the service-role client: the storage SELECT policy only - // covers the uploader's own folder (documents/{uid}/...), so the - // user-scoped client cannot read colleague-uploaded files even within - // the same company. The document_attachments RLS fetch plus the explicit - // membership check above are the authorization (same model as the - // inline proxy route). - const serviceClient = createServiceClient() - const { data: blob, error: downloadError } = await serviceClient.storage - .from('documents') - .download(doc.storage_path) + if (downloadError || !blob) { + log.error('storage download failed for integrity check', downloadError as Error, { + documentId: id, + companyId: doc.company_id, + }) + return NextResponse.json({ error: 'Integrity check unavailable' }, { status: 500 }) + } - if (downloadError || !blob) { - log.error('storage download failed for integrity check', downloadError as Error, { - documentId: id, - companyId: doc.company_id, - }) - return NextResponse.json({ error: 'Integrity check unavailable' }, { status: 500 }) - } + // Only the first 16 bytes are needed for magic-byte detection (PDF/PNG + // use at most 8, WebP needs 12). Trimming here doesn't change bandwidth + // (the full blob is already downloaded) but it makes the intent explicit + // and keeps memory churn off the hot path for large PDFs. + const headerBuffer = await blob.slice(0, 16).arrayBuffer() + const magicError = validateDocumentMagicBytes(headerBuffer, doc.mime_type) - // Only the first 16 bytes are needed for magic-byte detection (PDF/PNG - // use ≤8, WebP needs 12). Trimming here doesn't change bandwidth (the - // full blob is already downloaded) but it makes the intent explicit and - // keeps memory churn off the hot path for large PDFs. - const headerBuffer = await blob.slice(0, 16).arrayBuffer() - const magicError = validateDocumentMagicBytes(headerBuffer, doc.mime_type) + if (magicError) { + log.warn('document failed magic-byte integrity check', { + documentId: id, + companyId: doc.company_id, + reason: magicError, + }) + } - if (magicError) { - log.warn('document failed magic-byte integrity check', { - documentId: id, - companyId: doc.company_id, - reason: magicError, - }) - } - - return NextResponse.json({ data: { valid: magicError === null } }) -} + return NextResponse.json({ data: { valid: magicError === null } }) + }, +) diff --git a/app/api/transactions/[id]/__tests__/route.test.ts b/app/api/transactions/[id]/__tests__/route.test.ts index 47dd45df..de2e9471 100644 --- a/app/api/transactions/[id]/__tests__/route.test.ts +++ b/app/api/transactions/[id]/__tests__/route.test.ts @@ -20,7 +20,8 @@ vi.mock('@/lib/auth/require-write', () => ({ requireWritePermission: vi.fn().mockResolvedValue({ ok: true }), })) -// PATCH (edit title) goes through withRouteContext → requireAuth. +// Both handlers go through withRouteContext, which resolves the session via +// requireAuth (the MFA-enforcing guard on hosted). vi.mock('@/lib/auth/require-auth', () => ({ requireAuth: vi.fn(), })) @@ -31,6 +32,7 @@ vi.mock('@/lib/sandbox/guard', () => ({ import { DELETE, PATCH } from '../route' import { requireAuth } from '@/lib/auth/require-auth' +import { requireWritePermission } from '@/lib/auth/require-write' import { guardSandbox } from '@/lib/sandbox/guard' import { NextResponse } from 'next/server' @@ -40,11 +42,20 @@ describe('DELETE /api/transactions/[id]', () => { beforeEach(() => { vi.clearAllMocks() reset() - mockSupabase.auth.getUser.mockResolvedValue({ data: { user: mockUser } }) + vi.mocked(requireAuth).mockResolvedValue({ + user: mockUser as never, + supabase: mockSupabase as never, + error: null, + }) + vi.mocked(requireWritePermission).mockResolvedValue({ ok: true } as never) }) it('returns 401 when not authenticated', async () => { - mockSupabase.auth.getUser.mockResolvedValue({ data: { user: null } }) + vi.mocked(requireAuth).mockResolvedValue({ + user: null as never, + supabase: mockSupabase as never, + error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }), + }) const request = new Request('http://localhost/api/transactions/tx-1', { method: 'DELETE' }) const response = await DELETE(request, createMockRouteParams({ id: 'tx-1' })) @@ -54,6 +65,29 @@ describe('DELETE /api/transactions/[id]', () => { expect(body).toEqual({ error: 'Unauthorized' }) }) + it('never calls supabase.auth.getUser() directly (MFA is enforced by the wrapper)', async () => { + const tx = makeTransaction({ journal_entry_id: null, bank_connection_id: null, import_source: null }) + enqueue({ data: tx, error: null }) // fetch + enqueue({ data: null, error: null }) // delete + + const request = new Request('http://localhost/api/transactions/tx-1', { method: 'DELETE' }) + await DELETE(request, createMockRouteParams({ id: 'tx-1' })) + + expect(requireAuth).toHaveBeenCalledTimes(1) + expect(mockSupabase.auth.getUser).not.toHaveBeenCalled() + }) + + it('returns 403 for a viewer (requireWrite)', async () => { + vi.mocked(requireWritePermission).mockResolvedValue({ + ok: false, + response: NextResponse.json({ error: 'Forbidden' }, { status: 403 }), + } as never) + + const request = new Request('http://localhost/api/transactions/tx-1', { method: 'DELETE' }) + const response = await DELETE(request, createMockRouteParams({ id: 'tx-1' })) + expect(response.status).toBe(403) + }) + it('returns 404 when transaction not found', async () => { enqueue({ data: null, error: { message: 'Not found' } }) diff --git a/app/api/transactions/[id]/route.ts b/app/api/transactions/[id]/route.ts index 980d3061..148432a9 100644 --- a/app/api/transactions/[id]/route.ts +++ b/app/api/transactions/[id]/route.ts @@ -1,7 +1,4 @@ -import { createClient } from '@/lib/supabase/server' import { NextResponse } from 'next/server' -import { requireCompanyId } from '@/lib/company/context' -import { requireWritePermission } from '@/lib/auth/require-write' import { withRouteContext } from '@/lib/api/with-route-context' import { errorResponse, errorResponseFromCode } from '@/lib/errors/get-structured-error' import { validateBody } from '@/lib/api/validate' @@ -10,126 +7,119 @@ import { guardSandbox } from '@/lib/sandbox/guard' import { isImportedTransaction } from '@/lib/transactions/origin' import type { Transaction } from '@/types' -export async function DELETE( - _request: Request, - { params }: { params: Promise<{ id: string }> } -) { - const supabase = await createClient() - const { id } = await params +// withRouteContext enforces auth (MFA on hosted), resolves the active +// companyId and rejects viewers via requireWrite. The previous hand-rolled +// session lookup in this handler skipped the MFA gate entirely. +export const DELETE = withRouteContext<{ params: Promise<{ id: string }> }>( + 'transaction.delete', + async (_request, { supabase, companyId }, { params }) => { + const { id } = await params - const { data: { user } } = await supabase.auth.getUser() - if (!user) { - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) - } + // Fetch the transaction with ownership check. `bank_connection_id` + + // `import_source` tell us where the row came from (see the imported guard below). + const { data: transaction, error: fetchError } = await supabase + .from('transactions') + .select('id, journal_entry_id, bank_connection_id, import_source') + .eq('id', id) + .eq('company_id', companyId) + .single() - const writeCheck = await requireWritePermission(supabase, user.id) - if (!writeCheck.ok) return writeCheck.response - - const companyId = await requireCompanyId(supabase, user.id) - - // Fetch the transaction with ownership check. `bank_connection_id` + - // `import_source` tell us where the row came from (see the imported guard below). - const { data: transaction, error: fetchError } = await supabase - .from('transactions') - .select('id, journal_entry_id, bank_connection_id, import_source') - .eq('id', id) - .eq('company_id', companyId) - .single() - - if (fetchError || !transaction) { - return NextResponse.json( - { - error: { - code: 'TRANSACTION_NOT_FOUND', - message: 'Transaktionen hittades inte.', - message_en: 'Transaction not found.', - }, - }, - { status: 404 } - ) - } - - // Guard: only unbooked transactions can be deleted. A booked/matched row is - // räkenskapsinformation: the fix is to unlink (reconciliation) or storno, not - // delete. Return a structured bilingual envelope so the UI shows this clear, - // actionable message instead of the generic "Ladda om sidan" 409 fallback. - if (transaction.journal_entry_id) { - return NextResponse.json( - { - error: { - code: 'TRANSACTION_DELETE_BOOKED', - message: - 'Transaktionen är redan bokförd eller kopplad till en verifikation och kan inte raderas. Koppla bort den under Rapporter → Bankavstämning om kopplingen är fel, eller storna verifikationen.', - message_en: - 'The transaction is already booked or linked to a journal entry and cannot be deleted. Unlink it under Reports → Bank reconciliation if the link is wrong, or reverse (storno) the voucher.', - }, - }, - { status: 409 } - ) - } - - // Guard: only transactions the user created in the app can be deleted. Rows - // fetched via bank sync or uploaded via a bank-file import are an external - // record of money that moved: deleting one would silently drop a real bank - // line (and a re-sync would just bring it back). The user can *ignore* such a - // row (POST /api/transactions/[id]/ignore) to take it off the to-book list, - // but never delete it. See lib/transactions/origin.ts for the origin rule. - if (isImportedTransaction(transaction)) { - return NextResponse.json( - { - error: { - code: 'TRANSACTION_DELETE_IMPORTED', - message: - 'Transaktionen har hämtats från banken eller importerats via fil och kan inte raderas. Du kan ignorera den så att den döljs från listan över transaktioner att bokföra.', - message_en: - 'This transaction was fetched from your bank or imported from a file and cannot be deleted. You can ignore it to hide it from the list of transactions to book.', - }, - }, - { status: 409 } - ) - } - - const { error: deleteError } = await supabase - .from('transactions') - .delete() - .eq('id', id) - .eq('company_id', companyId) - - if (deleteError) { - // An unbooked row can still carry payment_match_log rows (written at ingest - // for every auto-suggested match). Their FK cascades on delete, but the - // audit-immutability trigger raises P0001: surface that as an actionable - // message (match or ignore instead) rather than a bare 500. - const code = (deleteError as { code?: string }).code - const message = (deleteError as { message?: string }).message ?? '' - if (code === 'P0001' || /Audit log entries cannot be modified or deleted/i.test(message)) { + if (fetchError || !transaction) { return NextResponse.json( { error: { - code: 'TRANSACTION_DELETE_HAS_AUDIT_TRAIL', + code: 'TRANSACTION_NOT_FOUND', + message: 'Transaktionen hittades inte.', + message_en: 'Transaction not found.', + }, + }, + { status: 404 } + ) + } + + // Guard: only unbooked transactions can be deleted. A booked/matched row is + // räkenskapsinformation: the fix is to unlink (reconciliation) or storno, not + // delete. Return a structured bilingual envelope so the UI shows this clear, + // actionable message instead of the generic "Ladda om sidan" 409 fallback. + if (transaction.journal_entry_id) { + return NextResponse.json( + { + error: { + code: 'TRANSACTION_DELETE_BOOKED', message: - 'Transaktionen kan inte raderas eftersom den har en kopplad matchningshistorik (räkenskapsinformation, BFL 7 kap.). Matcha den mot en befintlig verifikation, eller ignorera den under Rapporter → Bankavstämning om du inte vill bokföra den.', + 'Transaktionen är redan bokförd eller kopplad till en verifikation och kan inte raderas. Koppla bort den under Rapporter → Bankavstämning om kopplingen är fel, eller storna verifikationen.', message_en: - 'The transaction cannot be deleted because it has linked match-history records (accounting information, BFL ch. 7). Match it to an existing voucher, or ignore it under Reports → Bank reconciliation if you do not want to book it.', + 'The transaction is already booked or linked to a journal entry and cannot be deleted. Unlink it under Reports → Bank reconciliation if the link is wrong, or reverse (storno) the voucher.', }, }, { status: 409 } ) } - return NextResponse.json( - { - error: { - code: 'TRANSACTION_DELETE_FAILED', - message: 'Kunde inte ta bort transaktionen. Försök igen.', - message_en: 'Could not delete the transaction. Please try again.', - }, - }, - { status: 500 } - ) - } - return NextResponse.json({ success: true }) -} + // Guard: only transactions the user created in the app can be deleted. Rows + // fetched via bank sync or uploaded via a bank-file import are an external + // record of money that moved: deleting one would silently drop a real bank + // line (and a re-sync would just bring it back). The user can *ignore* such a + // row (POST /api/transactions/[id]/ignore) to take it off the to-book list, + // but never delete it. See lib/transactions/origin.ts for the origin rule. + if (isImportedTransaction(transaction)) { + return NextResponse.json( + { + error: { + code: 'TRANSACTION_DELETE_IMPORTED', + message: + 'Transaktionen har hämtats från banken eller importerats via fil och kan inte raderas. Du kan ignorera den så att den döljs från listan över transaktioner att bokföra.', + message_en: + 'This transaction was fetched from your bank or imported from a file and cannot be deleted. You can ignore it to hide it from the list of transactions to book.', + }, + }, + { status: 409 } + ) + } + + const { error: deleteError } = await supabase + .from('transactions') + .delete() + .eq('id', id) + .eq('company_id', companyId) + + if (deleteError) { + // An unbooked row can still carry payment_match_log rows (written at ingest + // for every auto-suggested match). Their FK cascades on delete, but the + // audit-immutability trigger raises P0001: surface that as an actionable + // message (match or ignore instead) rather than a bare 500. + const code = (deleteError as { code?: string }).code + const message = (deleteError as { message?: string }).message ?? '' + if (code === 'P0001' || /Audit log entries cannot be modified or deleted/i.test(message)) { + return NextResponse.json( + { + error: { + code: 'TRANSACTION_DELETE_HAS_AUDIT_TRAIL', + message: + 'Transaktionen kan inte raderas eftersom den har en kopplad matchningshistorik (räkenskapsinformation, BFL 7 kap.). Matcha den mot en befintlig verifikation, eller ignorera den under Rapporter → Bankavstämning om du inte vill bokföra den.', + message_en: + 'The transaction cannot be deleted because it has linked match-history records (accounting information, BFL ch. 7). Match it to an existing voucher, or ignore it under Reports → Bank reconciliation if you do not want to book it.', + }, + }, + { status: 409 } + ) + } + return NextResponse.json( + { + error: { + code: 'TRANSACTION_DELETE_FAILED', + message: 'Kunde inte ta bort transaktionen. Försök igen.', + message_en: 'Could not delete the transaction. Please try again.', + }, + }, + { status: 500 } + ) + } + + return NextResponse.json({ success: true }) + }, + { requireWrite: true }, +) /** * Edit a bank transaction's title (description). diff --git a/app/api/transactions/__tests__/route.test.ts b/app/api/transactions/__tests__/route.test.ts index f4178cb0..1273bd09 100644 --- a/app/api/transactions/__tests__/route.test.ts +++ b/app/api/transactions/__tests__/route.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { createMockRequest, + createMockRouteParams, parseJsonResponse, createQueuedMockSupabase, makeTransaction, @@ -16,7 +17,14 @@ vi.mock('@/lib/company/context', () => ({ getActiveCompanyId: vi.fn().mockResolvedValue('company-1'), })) +// GET goes through withRouteContext, which resolves the session via requireAuth. +vi.mock('@/lib/auth/require-auth', () => ({ + requireAuth: vi.fn(), +})) + import { GET } from '../route' +import { requireAuth } from '@/lib/auth/require-auth' +import { NextResponse } from 'next/server' describe('GET /api/transactions', () => { const mockUser = { id: 'user-1', email: 'test@test.se' } @@ -26,14 +34,22 @@ describe('GET /api/transactions', () => { vi.clearAllMocks() reset() mockSupabase.from = originalFrom - mockSupabase.auth.getUser.mockResolvedValue({ data: { user: mockUser } }) + vi.mocked(requireAuth).mockResolvedValue({ + user: mockUser as never, + supabase: mockSupabase as never, + error: null, + }) }) it('returns 401 when not authenticated', async () => { - mockSupabase.auth.getUser.mockResolvedValue({ data: { user: null } }) + vi.mocked(requireAuth).mockResolvedValue({ + user: null as never, + supabase: mockSupabase as never, + error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }), + }) const request = createMockRequest('/api/transactions') - const response = await GET(request) + const response = await GET(request, createMockRouteParams({})) const { status, body } = await parseJsonResponse(response) expect(status).toBe(401) @@ -48,7 +64,7 @@ describe('GET /api/transactions', () => { enqueue({ data: txs, error: null }) const request = createMockRequest('/api/transactions') - const response = await GET(request) + const response = await GET(request, createMockRouteParams({})) const { status, body } = await parseJsonResponse<{ data: typeof txs has_more: boolean @@ -70,7 +86,7 @@ describe('GET /api/transactions', () => { enqueue({ data: txs, error: null }) const request = createMockRequest('/api/transactions') - const response = await GET(request) + const response = await GET(request, createMockRouteParams({})) const { status, body } = await parseJsonResponse<{ data: typeof txs has_more: boolean @@ -102,7 +118,7 @@ describe('GET /api/transactions', () => { mockSupabase.from = fromSpy as unknown as typeof mockSupabase.from const request = createMockRequest('/api/transactions?unmatched=true') - await GET(request) + await GET(request, createMockRouteParams({})) expect(fromSpy).toHaveBeenCalledWith('transactions') const chain = fromSpy.mock.results[0].value as { __calls: { method: string; args: unknown[] }[] } @@ -129,7 +145,7 @@ describe('GET /api/transactions', () => { mockSupabase.from = fromSpy as unknown as typeof mockSupabase.from const request = createMockRequest('/api/transactions?reconciled=true') - await GET(request) + await GET(request, createMockRouteParams({})) const chain = fromSpy.mock.results[0].value as { __calls: { method: string; args: unknown[] }[] } const notCall = chain.__calls.find((c) => c.method === 'not') @@ -160,7 +176,7 @@ describe('GET /api/transactions', () => { const request = createMockRequest( '/api/transactions?currency=SEK&date_from=2024-01-01&date_to=2024-12-31' ) - await GET(request) + await GET(request, createMockRouteParams({})) const chain = fromSpy.mock.results[0].value as { __calls: { method: string; args: unknown[] }[] } const eqCalls = chain.__calls.filter((c) => c.method === 'eq') @@ -183,7 +199,7 @@ describe('GET /api/transactions', () => { enqueue({ data: null, error: { message: 'boom' } }) const request = createMockRequest('/api/transactions') - const response = await GET(request) + const response = await GET(request, createMockRouteParams({})) const { status, body } = await parseJsonResponse<{ error: string }>(response) expect(status).toBe(500) diff --git a/app/api/transactions/route.ts b/app/api/transactions/route.ts index e56781a1..87b3dbee 100644 --- a/app/api/transactions/route.ts +++ b/app/api/transactions/route.ts @@ -1,6 +1,4 @@ -import { createClient } from '@/lib/supabase/server' import { NextResponse } from 'next/server' -import { requireCompanyId } from '@/lib/company/context' import { scopeTransactionsToAccount } from '@/lib/reconciliation/bank-reconciliation' import { withRouteContext } from '@/lib/api/with-route-context' import { validateBody } from '@/lib/api/validate' @@ -9,16 +7,9 @@ import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-m const MAX_ROWS = 500 -export async function GET(request: Request) { - const supabase = await createClient() - const { data: { user } } = await supabase.auth.getUser() - - if (!user) { - return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) - } - - const companyId = await requireCompanyId(supabase, user.id) - +// withRouteContext enforces auth (MFA on hosted) and resolves companyId; the +// previous hand-rolled session lookup in this handler skipped the MFA gate. +export const GET = withRouteContext('transaction.list', async (request, { supabase, companyId }) => { const { searchParams } = new URL(request.url) const unmatched = searchParams.get('unmatched') === 'true' const reconciled = searchParams.get('reconciled') === 'true' @@ -107,7 +98,7 @@ export async function GET(request: Request) { const truncated = hasMore ? rows.slice(0, MAX_ROWS) : rows return NextResponse.json({ data: truncated, has_more: hasMore, limit: MAX_ROWS }) -} +}) // Manual bank-transaction creation. This is the server-side boundary the form // now goes through (it used to insert straight into Supabase from the browser). diff --git a/scripts/checks/no-new-antipatterns.mjs b/scripts/checks/no-new-antipatterns.mjs index 9ac29d4a..c158a9d5 100644 --- a/scripts/checks/no-new-antipatterns.mjs +++ b/scripts/checks/no-new-antipatterns.mjs @@ -11,8 +11,11 @@ * 1. raw-route-auth : an `app/api/**\/route.ts` that calls * `supabase.auth.getUser()` directly instead of going through * `requireAuth()` / `withRouteContext()` (the only guards that enforce - * MFA AAL2 on hosted). Tracked as a file-set so a NEW offending route - * fails CI even if an old one was fixed in the same PR. + * MFA AAL2 on hosted). Judged per exported handler, not per file: a + * wrapped PATCH next to a hand-rolled DELETE in the same file is still + * a violation (that exact shape hid two MFA bypasses until 2026-08-26). + * Tracked as a file-set so a NEW offending route fails CI even if an + * old one was fixed in the same PR. * 2. naive-ore-round: `Math.round(x * 100) / 100`, which is subtly wrong on * exact-half values (see lib/money.ts `roundOre`). Tracked as a count. * The canonical rounding modules are excluded. @@ -136,6 +139,10 @@ const RAW_AUTH_RE = /\.auth\.getUser\(/ // flagged. withRouteContext is usually called with a generic (`withRouteContext<…>(`), // so accept either `<` or `(` after the name. const GUARD_RE = /requireAuth\(|withRouteContext[<(]/ +// Each top-level `export` starts a new segment, so every handler (and the +// preamble of shared helpers above the first export) is judged on its own. +// Without this split, one wrapped handler exempted the whole file. +const TOP_LEVEL_EXPORT_RE = /^(?=export\s)/m const NAIVE_ROUND_RE = /Math\.round\([^\n]*\*\s*100\s*\)\s*\/\s*100/ // 8. hand-rolled-invariant. Shared format contracts live in lib/invariants/ @@ -178,14 +185,18 @@ function walk(dir, exts, out = []) { const rel = (p) => path.relative(ROOT, p).split(path.sep).join('/') +/** True when any handler segment calls getUser() without an MFA-enforcing guard. */ +function handRollsRouteAuth(src) { + return src + .split(TOP_LEVEL_EXPORT_RE) + .some((segment) => RAW_AUTH_RE.test(segment) && !GUARD_RE.test(segment)) +} + /** Route files that hand-roll auth instead of the MFA-enforcing guard. */ function findRawRouteAuth() { const apiDir = path.join(ROOT, 'app', 'api') return walk(apiDir, ['route.ts']) - .filter((f) => { - const src = fs.readFileSync(f, 'utf8') - return RAW_AUTH_RE.test(src) && !GUARD_RE.test(src) - }) + .filter((f) => handRollsRouteAuth(fs.readFileSync(f, 'utf8'))) .map(rel) .sort() }