diff --git a/DECISIONS.md b/DECISIONS.md index 070cf734..66670c2c 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1384,6 +1384,12 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-31] Login/register methods come from GoTrue (/auth/v1/settings + admin customProviders) instead of app-side flags; NEXT_PUBLIC_GOOGLE_AUTH_ENABLED removed (PR #1869): the Supabase dashboard becomes the single switch, an allowlist of auth-js provider ids filters non-login entries like anonymous_users, and hosted rendering is unchanged because Google is enabled in prod GoTrue. The Vercel env var stays set for old-build rollback safety; delete it after a few deploys. [2026-08-31] Single prominent amount is PROMOTED into editable totals.total (totalSource='prominent') instead of living in a read-only Belopp row: Emil's call, an uncorrectable load-bearing value violated the prefill-override-editors rule. Provenance keeps matching fallback-grade (discount, date guard, hunt exclusion); a user edit of TOTALT clears the stamp. Multi-amount docs keep the Belopp row: promoting one of several figures would invent a total. [2026-08-31] Image-scan red fixed by bumping the node:22-alpine digest (alpine 3.23 to 3.24.1), not by widening the gate: the Dockerfile's apk-upgrade layer is frozen by the GHCR buildx layer cache, so a fix published after the last cache-busting change (libssl3 3.5.8-r0 for CVE-2026-14456) never reaches the published image until the FROM digest moves; the red scheduled scan is the designed alarm for exactly this bump. cron.Dockerfile gained the same apk upgrade (it had none). +[2026-08-31] Agent approval-authority ceiling stored on api_keys, not company_settings: a company-scoped money threshold catches humans too, and company_settings.agent_auto_commit_max_amount was already tried (20260501120000) and dropped four days later (20260505190027) for exactly that reason. +[2026-08-31] Ceiling enforced in TypeScript at the two commit doors, not inside commit_journal_entry: a RAISE there is swallowed by engine.ts into BookkeepingDatabaseError (500, retryable), and on the MCP path that burns the pending op to terminal 'rejected', destroying the staged verifikat. Both doors refuse BEFORE the atomic claim / before commitEntry, so the work survives as 'pending' or as a draft and a human can approve the same verifikat. +[2026-08-31] The agent ceiling prices every money-posting operation type from preview_data, including the batch and settlement paths (match_batch_allocate/total_allocated, bulk_book_transactions/tx_sum, link_transaction_journal_entry/transaction_amount, link_supplier_invoice_voucher/payment_amount, mark_invoice_paid/total): all verified present and numeric on 100% of prod rows over 120 days. An earlier cut left these unpriced on the assumption their totals only existed in SQL at dispatch; that was wrong and let a limited key post any amount through the four largest settlement paths. Only reconciliation_match stays unpriced, because pair_count is a count and not kronor. +[2026-08-31] Genuinely unpriceable types fail OPEN, not closed: this control can only ever narrow what a key does, and a wrong guess at an amount blocks a legitimate commit, so guessing high would leave an agent unable to work at all. +[2026-08-31] Ceiling is a blast-radius cap, not a security boundary: a per-entry limit is defeated by splitting one entry into several, and an LLM will find that. Splitting is itself a BFL 5 kap. 6 § violation, so UNATTENDED_COMMIT_LIMIT_EXCEEDED forbids it first in the remediation. A rolling-window cumulative limit is the primitive that actually bounds exposure and is deliberately left to a separate change. +[2026-08-31] Default is NULL (unlimited), fully opt-in: every key predating the column keeps its behaviour, so shipping this cannot break a live agent. [2026-08-31] Opening-balance cascade (later years' IB) is opt-in per request (cascade flag, dialog checkbox default-checked) and per-year best-effort: locked/closed/lock-dated/bokslut years are skipped and reported, never forced (DB triggers are the legal guard), and a cascade failure never errors the already-committed base correction. Blocked-year guidance is computed client-side from the fiscal-periods list instead of enriching the refusal responses: keeps the refusal paths' query order stable and adds zero server queries. [2026-08-31] Cascade rewritten onto replaceOpeningBalanceEntry (commit_opening_balance_replacement RPC) after review findings on PR #2076: the create/reverse/relink+compensation sequence could leave a period linked to a reversed IB entry; the engine already had the atomic storno+rebook+pointer-swap primitive (built for SIE resync), so the cascade uses it, keeps original lines verbatim (dimensions included) and appends labelled IB-rättelse adjustment lines instead of collapsing per account. [2026-08-31] Fortnox-style IB editing (founder-directed): correct_entry_lines_inline redefined (20260831150000) to admit opening_balance entries with three IB guards (current-linked-IB only, no posted bokslut, class 1-2 replacement lines); new /api/import/opening-balance/correct-inline route + dialog switched from storno-replace to diff-based inline strike/replace, and the cascade gained mode 'inline' (delta appended as IB-rättelse lines in later years' own verifikat: zero new verifikat). Storno endpoints kept untouched for locked years, the import replace flow, and API compat. Chosen over remodeling IB as editable saldon (Fortnox's storage model): that would be a data migration + verifikat-invariant break, while inline rättelse gives the same UX on the existing model. diff --git a/app/api/settings/api-keys/[id]/__tests__/route.test.ts b/app/api/settings/api-keys/[id]/__tests__/route.test.ts new file mode 100644 index 00000000..1724321b --- /dev/null +++ b/app/api/settings/api-keys/[id]/__tests__/route.test.ts @@ -0,0 +1,136 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { createMockRequest, parseJsonResponse } from '@/tests/helpers' + +const mockSupabase = { + auth: { getUser: vi.fn() }, + from: vi.fn(), +} + +vi.mock('@/lib/supabase/server', () => ({ + createClient: () => Promise.resolve(mockSupabase), +})) + +const getActiveCompanyIdMock = vi.fn() +vi.mock('@/lib/company/context', () => ({ + getActiveCompanyId: (...args: unknown[]) => getActiveCompanyIdMock(...args), +})) + +const requireWritePermissionMock = vi.fn() +vi.mock('@/lib/auth/require-write', () => ({ + requireWritePermission: (...args: unknown[]) => requireWritePermissionMock(...args), +})) + +import { PATCH } from '../route' + +const mockUser = { id: 'user-1', email: 'test@test.se' } +const params = { params: Promise.resolve({ id: 'key-1' }) } + +/** + * Chainable proxy over .update().eq().eq().is().select().maybeSingle(). + * Records the update payload and every .eq()/.is() filter, so the tests can + * assert on tenant scoping rather than trusting it. + */ +function setupFrom(result: { data?: unknown; error?: unknown }) { + const updateSpy = vi.fn() + const filters: Array<[string, unknown]> = [] + + mockSupabase.from.mockImplementation(() => { + const chain: Record = {} + const handler: ProxyHandler = { + get(_t, prop) { + if (prop === 'update') { + return (payload: unknown) => { + updateSpy(payload) + return new Proxy(chain, handler) + } + } + if (prop === 'eq' || prop === 'is') { + return (col: string, val: unknown) => { + filters.push([col, val]) + return new Proxy(chain, handler) + } + } + if (prop === 'maybeSingle' || prop === 'single') { + return () => + Promise.resolve({ data: result.data ?? null, error: result.error ?? null }) + } + return () => new Proxy(chain, handler) + }, + } + return new Proxy(chain, handler) + }) + + return { updateSpy, filters } +} + +function patch(body: unknown) { + return createMockRequest('/api/settings/api-keys/key-1', { method: 'PATCH', body }) +} + +beforeEach(() => { + vi.clearAllMocks() + mockSupabase.auth.getUser.mockResolvedValue({ data: { user: mockUser } }) + getActiveCompanyIdMock.mockResolvedValue('company-1') + requireWritePermissionMock.mockResolvedValue({ ok: true }) +}) + +describe('PATCH /api/settings/api-keys/[id]', () => { + it('returns 401 when not authenticated', async () => { + mockSupabase.auth.getUser.mockResolvedValue({ data: { user: null } }) + const res = await PATCH(patch({ unattended_commit_limit: 5000 }), params) + expect(res.status).toBe(401) + }) + + it('sets a ceiling and scopes the update to the active company and unrevoked keys', async () => { + const { updateSpy, filters } = setupFrom({ + data: { id: 'key-1', unattended_commit_limit: 5000 }, + }) + const res = await PATCH(patch({ unattended_commit_limit: 5000 }), params) + const { status, body } = await parseJsonResponse<{ + data: { unattended_commit_limit: number } + }>(res) + + expect(status).toBe(200) + expect(body.data.unattended_commit_limit).toBe(5000) + expect(updateSpy).toHaveBeenCalledWith({ unattended_commit_limit: 5000 }) + // Tenant scoping is the whole security story of this route: without the + // company_id filter, any authenticated user could raise any key's + // authority by guessing an id. + expect(filters).toContainEqual(['company_id', 'company-1']) + expect(filters).toContainEqual(['id', 'key-1']) + expect(filters).toContainEqual(['revoked_at', null]) + }) + + it('accepts null to clear the ceiling', async () => { + const { updateSpy } = setupFrom({ data: { id: 'key-1', unattended_commit_limit: null } }) + const res = await PATCH(patch({ unattended_commit_limit: null }), params) + expect(res.status).toBe(200) + expect(updateSpy).toHaveBeenCalledWith({ unattended_commit_limit: null }) + }) + + it('rejects zero, negatives and non-numbers with 400', async () => { + setupFrom({ data: { id: 'key-1' } }) + for (const bad of [0, -1, '5000', {}]) { + const res = await PATCH(patch({ unattended_commit_limit: bad }), params) + expect(res.status).toBe(400) + } + }) + + it('requires the field rather than treating an empty body as "clear it"', async () => { + setupFrom({ data: { id: 'key-1' } }) + const res = await PATCH(patch({}), params) + expect(res.status).toBe(400) + }) + + it('returns 404 when the key belongs to another company or is revoked', async () => { + setupFrom({ data: null }) + const res = await PATCH(patch({ unattended_commit_limit: 5000 }), params) + expect(res.status).toBe(404) + }) + + it('returns 500 when the update fails', async () => { + setupFrom({ error: { message: 'boom' } }) + const res = await PATCH(patch({ unattended_commit_limit: 5000 }), params) + expect(res.status).toBe(500) + }) +}) diff --git a/app/api/settings/api-keys/[id]/route.ts b/app/api/settings/api-keys/[id]/route.ts index 62a0b07c..4ccbf2f9 100644 --- a/app/api/settings/api-keys/[id]/route.ts +++ b/app/api/settings/api-keys/[id]/route.ts @@ -1,7 +1,21 @@ import { NextResponse } from 'next/server' +import { z } from 'zod' import { withRouteContext } from '@/lib/api/with-route-context' +import { validateBody } from '@/lib/api/validate' import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-message' +/** + * Approval authority in SEK: the largest amount this key may commit with no + * human in the loop. null clears the ceiling (unlimited, the default). + * + * Bounded at 1 000 000 000 so a typo cannot store a number the numeric(14,2) + * column would reject at insert time with a raw Postgres error. The DB CHECK + * (> 0) is the real guarantee; this is the friendly message in front of it. + */ +const patchSchema = z.object({ + unattended_commit_limit: z.number().positive().max(1_000_000_000).nullable(), +}) + /** * DELETE /api/settings/api-keys/[id]: Revoke an API key (soft delete) */ @@ -26,3 +40,43 @@ export const DELETE = withRouteContext<{ params: Promise<{ id: string }> }>( }, { requireWrite: true }, ) + +/** + * PATCH /api/settings/api-keys/[id]: set the key's unattended commit limit. + * + * Deliberately narrow: name and scopes are NOT editable here. Silently + * widening a key's scopes after the fact would defeat the point of showing the + * scope list at creation, and the separation-of-duties check + * (findStageApproveConflict) runs only on POST. + */ +export const PATCH = withRouteContext<{ params: Promise<{ id: string }> }>( + 'api_key.update', + async (request, ctx, { params }) => { + const { id } = await params + const { supabase, companyId } = ctx + + const validation = await validateBody(request, patchSchema) + if (!validation.success) return validation.response + + // Revoked keys are deliberately excluded: raising a limit on a key that no + // longer authenticates reads as re-enabling it, and it does not. + const { data, error } = await supabase + .from('api_keys') + .update({ unattended_commit_limit: validation.data.unattended_commit_limit }) + .eq('id', id) + .eq('company_id', companyId) + .is('revoked_at', null) + .select('id, unattended_commit_limit') + .maybeSingle() + + if (error) { + return NextResponse.json({ error: getUserErrorMessage(error) }, { status: 500 }) + } + if (!data) { + return NextResponse.json({ error: 'API-nyckeln hittades inte.' }, { status: 404 }) + } + + return NextResponse.json({ data }) + }, + { requireWrite: true }, +) diff --git a/app/api/settings/api-keys/route.ts b/app/api/settings/api-keys/route.ts index 8269b9d7..a623d657 100644 --- a/app/api/settings/api-keys/route.ts +++ b/app/api/settings/api-keys/route.ts @@ -20,7 +20,7 @@ export const GET = withRouteContext( // active company too: they're simulation-only, so they never write real data.) const { data, error } = await supabase .from('api_keys') - .select('id, key_prefix, name, scopes, mode, rate_limit_rpm, last_used_at, revoked_at, created_at') + .select('id, key_prefix, name, scopes, mode, rate_limit_rpm, unattended_commit_limit, last_used_at, revoked_at, created_at') .eq('company_id', companyId) .order('created_at', { ascending: false }) diff --git a/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/__tests__/unattended-limit.test.ts b/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/__tests__/unattended-limit.test.ts new file mode 100644 index 00000000..47452c5f --- /dev/null +++ b/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/__tests__/unattended-limit.test.ts @@ -0,0 +1,172 @@ +/** + * The approval-authority ceiling on POST /v1/.../journal-entries/:id/commit. + * + * This is the REST half of the envelope. The MCP half lives in + * lib/pending-operations/__tests__/commit-unattended-limit.test.ts. Both must + * hold, because an API key can reach the ledger through either, and a ceiling + * enforced on only one of them is not a ceiling. + * + * The property under test is not just "it returns 403": it is that the DRAFT + * SURVIVES. commitEntry must never be called, so the voucher sequence does not + * advance (BFL 5 kap. 7 §) and a human can commit the same draft from the app. + */ +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +beforeAll(() => { + process.env.NEXT_PUBLIC_SUPABASE_URL ||= 'http://localhost:54321' + process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY ||= 'test-anon-key' +}) + +vi.mock('@/lib/auth/api-keys', async () => { + const actual = await vi.importActual('@/lib/auth/api-keys') + return { ...actual, validateApiKey: vi.fn(), createServiceClientNoCookies: vi.fn() } +}) +vi.mock('@supabase/supabase-js', async () => { + const actual = await vi.importActual('@supabase/supabase-js') + return { ...actual, createClient: vi.fn().mockReturnValue({}) } +}) +vi.mock('@/lib/bookkeeping/engine', () => ({ + commitEntry: vi.fn().mockResolvedValue({ id: 'entry-1', status: 'posted' }), + getNextVoucherNumber: vi.fn().mockResolvedValue(143), +})) + +import { validateApiKey, createServiceClientNoCookies } from '@/lib/auth/api-keys' +import { commitEntry } from '@/lib/bookkeeping/engine' +import { POST } from '../route' + +const mockValidate = validateApiKey as ReturnType +const mockServiceClient = createServiceClientNoCookies as ReturnType +const mockCommitEntry = commitEntry as ReturnType + +const COMPANY_ID = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa' +const ENTRY_ID = 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb' + +const DRAFT = { + id: ENTRY_ID, + status: 'draft', + fiscal_period_id: 'fp-1', + voucher_series: 'A', + entry_date: '2026-08-31', +} + +/** + * Two tables answer here: journal_entries (the pre-flight maybeSingle) and + * journal_entry_lines (the paginated debit sum). The lines queue returns the + * rows once and then an empty page, which is what ends fetchAllRows' loop. + */ +function makeSupabase(lineDebits: number[]) { + const linePages = [ + { data: lineDebits.map((debit_amount, i) => ({ id: `line-${i}`, debit_amount })), error: null }, + { data: [], error: null }, + ] + const build = (table: string): unknown => { + const handler: ProxyHandler = { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => + resolve(linePages.length > 1 ? linePages.shift()! : linePages[0]!) + } + if (prop === 'maybeSingle' || prop === 'single') { + // Three tables answer a single-row read here, and each needs a + // different answer: company_members proves the key may touch this + // company (null would be a 404 before the handler ever runs), + // journal_entries is the draft, and the idempotency store must be + // null or the request reads as a replay and short-circuits with 409. + const rows: Record = { + company_members: { company_id: COMPANY_ID, user_id: 'user-1', role: 'owner' }, + journal_entries: DRAFT, + } + return () => Promise.resolve({ data: rows[table] ?? null, error: null }) + } + return () => build(table) + }, + } + return new Proxy({}, handler) + } + return { from: vi.fn((table: string) => build(table)) } +} + +function makeRequest(dryRun = false): Request { + return new Request( + `http://localhost/api/v1/companies/${COMPANY_ID}/journal-entries/${ENTRY_ID}/commit${dryRun ? '?dry_run=true' : ''}`, + { + method: 'POST', + headers: { + Authorization: 'Bearer test-fixture-not-a-real-key', + 'Content-Type': 'application/json', + 'Idempotency-Key': `idem${Math.floor(Math.random() * 1e6)}-1010-4abc-8def-1234567890ab`, + }, + }, + ) +} + +const routeParams = { params: Promise.resolve({ companyId: COMPANY_ID, id: ENTRY_ID }) } + +function auth(unattendedCommitLimit: number | null) { + mockValidate.mockResolvedValue({ + userId: 'user-1', + companyId: COMPANY_ID, + apiKeyId: 'ak_1', + apiKeyName: 'CI key', + scopes: ['bookkeeping:write'], + mode: 'live', + unattendedCommitLimit, + }) +} + +beforeEach(() => { + vi.clearAllMocks() + mockCommitEntry.mockResolvedValue({ id: ENTRY_ID, status: 'posted' }) +}) + +describe('journal-entries.commit: unattended commit limit', () => { + it('refuses a 50 000 kr draft on a key capped at 10 000, without committing it', async () => { + auth(10000) + mockServiceClient.mockReturnValue(makeSupabase([30000, 20000])) + + const res = await POST(makeRequest(), routeParams) + const body = (await res.json()) as { error: { code: string; details: Record } } + + expect(res.status).toBe(403) + expect(body.error.code).toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + expect(body.error.details).toMatchObject({ attempted: 50000, limit: 10000 }) + // The whole point: the draft is untouched and the sequence never moved. + expect(mockCommitEntry).not.toHaveBeenCalled() + }) + + it('refuses the dry run too, rather than promising a voucher number it cannot deliver', async () => { + auth(10000) + mockServiceClient.mockReturnValue(makeSupabase([50000])) + + const res = await POST(makeRequest(true), routeParams) + expect(res.status).toBe(403) + }) + + it('commits when the total is at or under the ceiling', async () => { + auth(10000) + mockServiceClient.mockReturnValue(makeSupabase([7500, 2500])) + + const res = await POST(makeRequest(), routeParams) + expect(res.status).toBe(200) + expect(mockCommitEntry).toHaveBeenCalled() + }) + + it('commits any amount when the key has no ceiling', async () => { + auth(null) + mockServiceClient.mockReturnValue(makeSupabase([9_000_000])) + + const res = await POST(makeRequest(), routeParams) + expect(res.status).toBe(200) + expect(mockCommitEntry).toHaveBeenCalled() + }) + + it('does not even read the lines when there is no ceiling', async () => { + auth(null) + const supabase = makeSupabase([1]) + mockServiceClient.mockReturnValue(supabase) + + await POST(makeRequest(), routeParams) + const tables = supabase.from.mock.calls.map((c) => c[0]) + expect(tables).not.toContain('journal_entry_lines') + }) +}) diff --git a/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/route.ts b/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/route.ts index 5b1fb164..8a535185 100644 --- a/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/route.ts +++ b/app/api/v1/companies/[companyId]/journal-entries/[id]/commit/route.ts @@ -19,6 +19,8 @@ import { registerEndpoint, dataEnvelope } from '@/lib/api/v1/registry' import { withApiV1 } from '@/lib/api/v1/with-api-v1' import { v1ErrorResponse, v1ErrorResponseFromCode } from '@/lib/api/v1/errors' import { commitEntry, getNextVoucherNumber } from '@/lib/bookkeeping/engine' +import { fetchAllRows } from '@/lib/supabase/fetch-all' +import { roundOre } from '@/lib/money' import { isBookkeepingError } from '@/lib/bookkeeping/errors' const JE_RESPONSE_COLUMNS = @@ -47,6 +49,7 @@ registerEndpoint({ 'Idempotency-Key is mandatory.', 'Posted entries cannot be edited. Plan the lines carefully or call /correct after commit if you need to change them.', 'Voucher numbers are sequential within (fiscal_period_id, voucher_series). A commit failure (e.g. period locked between draft creation and commit) does not advance the sequence.', + 'If the key has an unattended commit limit, an entry above it returns 403 UNATTENDED_COMMIT_LIMIT_EXCEEDED and stays a draft for a human to commit. Do not split it into smaller entries: one affärshändelse is one verifikat (BFL 5 kap. 6 §).', ], example: { response: { @@ -95,6 +98,59 @@ export const POST = withApiV1<{ params: Promise<{ companyId: string; id: string }) } + // ── Approval-authority ceiling. + // + // The second of the two places an API key posts money (the first is + // commitPendingOperation, for MCP). Placed after the draft-exists and + // status checks and BEFORE commitEntry, so a refusal leaves the draft a + // draft: nothing is destroyed, and the voucher sequence does not move + // (BFL 5 kap. 7 §). The human commits the same draft from the app. + // + // Runs before the dry-run branch on purpose. A dry run that reports + // "would post voucher 143" for a commit the key is not allowed to make + // is a lie the agent will act on. + // + // Known and accepted: the line sum is read here and the entry is + // committed below, so a concurrent write to the draft's lines between + // the two can post more than the ceiling. Closing that window means + // enforcing inside commit_journal_entry, where a RAISE is swallowed + // into a retryable 500 and, on the MCP path, destroys the staged + // operation. The trade is deliberate and consistent with what this + // control claims to be: a blast-radius cap on a looping or + // prompt-injected agent, NOT a security boundary. A per-entry ceiling + // is already defeated by splitting one entry into several, which needs + // no race at all. The primitive that actually bounds exposure is a + // cumulative rolling-window limit, tracked separately. + if (ctx.unattendedCommitLimit !== null) { + const lines = await fetchAllRows<{ id: string; debit_amount: number | null }>( + ({ from, to }) => + ctx.supabase + .from('journal_entry_lines') + .select('id, debit_amount') + .eq('journal_entry_id', entryId) + .order('id') + .range(from, to), + // The sum is money, so pagination correctness is not optional: a + // regressed order would double-count a line and refuse a legitimate + // commit. fetch-all.ts documents dedupeBy for exactly this case. + { dedupeBy: (r) => r.id }, + ) + // Debits equal credits on any entry that can be committed (the balance + // trigger enforces it), so the debit side alone is the entry's amount. + const attempted = roundOre(lines.reduce((sum, l) => sum + (l.debit_amount ?? 0), 0)) + if (attempted > ctx.unattendedCommitLimit) { + return v1ErrorResponseFromCode('UNATTENDED_COMMIT_LIMIT_EXCEEDED', ctx.log, { + requestId: ctx.requestId, + details: { + attempted, + limit: ctx.unattendedCommitLimit, + journal_entry_id: entryId, + entry_status: 'draft', + }, + }) + } + } + if (ctx.dryRun) { // Report the next voucher number WITHOUT advancing the sequence. The // engine helper `getNextVoucherNumber` is a peek + increment; for a diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index ada2e5d5..35df7f23 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -437,6 +437,15 @@ interface ActorContext { * single agent conversation. Not used for auth. */ sessionId?: string | null + /** + * Approval authority for this key in SEK: the largest amount it may commit + * with no human in the loop, or null/undefined for unlimited (the default, + * and what every key created before this column existed has). + * + * Only meaningful for `type: 'api_key'`. Read once at auth time so the + * ceiling cannot drift mid-request. + */ + unattendedCommitLimit?: number | null /** * Distribution-channel marker from `X-Accounted-Client`, the legacy * `X-Gnubok-Client`, or the `client` query param (e.g. 'openclaw'). @@ -4905,6 +4914,7 @@ export const tools: McpTool[] = [ ...(dimensionsBlock ? { dimensions: dimensionsBlock } : {}), ...(ledgerDigest ? { ledger_context: ledgerDigest } : {}), ...(skvConnection ? { skatteverket_connection: skvConnection } : {}), + // Static per-workflow loadouts (issue #1098): lets a deferred-loading // harness batch-load a whole workflow cluster in one call. Validated // against the tool registry at module init (assertRecommendedLoadoutsValid). @@ -19384,6 +19394,12 @@ export const tools: McpTool[] = [ actor: { type: actor?.type === 'api_key' ? 'api_key' : 'user', ...(actor?.label ? { label: actor.label } : {}), + // Only an api_key actor can carry a ceiling; the ternary above + // already collapsed everything else to 'user', for which + // exceedsUnattendedLimit returns false regardless. + ...(actor?.type === 'api_key' + ? { unattendedCommitLimit: actor.unattendedCommitLimit ?? null } + : {}), }, ...(userEmail ? { userEmail } : {}), } @@ -20670,6 +20686,7 @@ export async function handleMcpRequest(request: Request): Promise { let apiKeyId: string | undefined let apiKeyName: string | undefined let keyMode: ApiKeyMode = 'live' + let unattendedCommitLimit: number | null = null if (token) { const authResult = await validateApiKey(token) if ('error' in authResult) { @@ -20685,7 +20702,15 @@ export async function handleMcpRequest(request: Request): Promise { } return unauthorized() } - ;({ userId, companyId, scopes: keyScopes, apiKeyId, apiKeyName, mode: keyMode } = authResult) + ;({ + userId, + companyId, + scopes: keyScopes, + apiKeyId, + apiKeyName, + mode: keyMode, + unattendedCommitLimit, + } = authResult) } else { // Anonymous traffic has no key to rate-limit on: per truncated IP instead. // No-op without Upstash (self-hosted), like the OAuth register endpoint. @@ -20717,6 +20742,7 @@ export async function handleMcpRequest(request: Request): Promise { type: 'api_key', id: apiKeyId, label: apiKeyName ?? 'Unnamed API key', + unattendedCommitLimit, sessionId, client, } diff --git a/lib/api/v1/with-api-v1.ts b/lib/api/v1/with-api-v1.ts index 24f91208..3968c1a1 100644 --- a/lib/api/v1/with-api-v1.ts +++ b/lib/api/v1/with-api-v1.ts @@ -90,6 +90,13 @@ export interface ApiV1Context { apiKeyName: string | undefined /** Scopes granted to the calling key. */ scopes: ApiKeyScope[] + /** + * Largest amount in SEK this key may commit with no human approving it, or + * null for no ceiling (the default, and what every key predating the column + * has). Enforced at the two places an API key can post money: this surface's + * journal-entries.commit, and commitPendingOperation for the MCP path. + */ + unattendedCommitLimit: number | null /** * test|live. Test keys are simulation-only: the wrapper forces `dryRun` on * for every write, so handlers never need to special-case `mode`; they just @@ -298,6 +305,7 @@ export function withApiV1

{ apiKeyName: undefined, scopes: ['transactions:read', 'reports:read'], mode: 'live', + unattendedCommitLimit: null, }) }) @@ -320,6 +321,7 @@ describe('validateApiKey', () => { apiKeyName: undefined, scopes: DEFAULT_SCOPES, mode: 'live', + unattendedCommitLimit: null, }) }) @@ -345,6 +347,45 @@ describe('validateApiKey', () => { apiKeyName: 'CI test key', scopes: ['transactions:read'], mode: 'test', + unattendedCommitLimit: null, + }) + }) + + describe('unattended commit limit', () => { + it('surfaces a positive ceiling from the RPC row', async () => { + setupMockRpc({ + data: [{ + user_id: 'user-123', + company_id: 'company-456', + scopes: ['transactions:read'], + rate_limited: false, + // numeric(14,2) comes back from PostgREST as a string. + unattended_commit_limit: '25000.00', + }], + error: null, + }) + + const result = await validateApiKey('gnubok_sk_test-key-value') + expect(result).toMatchObject({ unattendedCommitLimit: 25000 }) + }) + + it('reads anything that is not a positive number as no ceiling', async () => { + // The whole guard is NULL-first: an unparseable or non-positive value + // must mean "unlimited", never "block every commit this key attempts". + for (const raw of [null, undefined, 0, -1, 'abc', {}]) { + setupMockRpc({ + data: [{ + user_id: 'user-123', + company_id: 'company-456', + scopes: ['transactions:read'], + rate_limited: false, + unattended_commit_limit: raw, + }], + error: null, + }) + const result = await validateApiKey('gnubok_sk_test-key-value') + expect(result).toMatchObject({ unattendedCommitLimit: null }) + } }) }) diff --git a/lib/auth/api-keys.ts b/lib/auth/api-keys.ts index ea40a75c..216c9118 100644 --- a/lib/auth/api-keys.ts +++ b/lib/auth/api-keys.ts @@ -155,6 +155,13 @@ export async function validateApiKey( apiKeyName?: string scopes: ApiKeyScope[] mode: ApiKeyMode + /** + * SEK ceiling on what this key may commit WITHOUT human approval, or + * null for no limit. Null is the only representation of "no limit": a + * stored 0 is forbidden by CHECK, because reading absence as 0 would + * block every commit for the key. + */ + unattendedCommitLimit: number | null } | { error: string; status: number } > { @@ -199,9 +206,30 @@ export async function validateApiKey( // migration that adds it to the RPC return. Default to 'live' so existing // keys behave unchanged. mode: (row.mode === 'test' ? 'test' : 'live') as ApiKeyMode, + // PostgREST returns numeric as a STRING, so parse explicitly rather than + // relying on `>` coercion at the comparison site. Anything unparseable, + // absent (a DB that has not run the migration), or non-positive becomes + // null, i.e. no limit: this control must fail OPEN. Blocking a company's + // month-end because a defence-in-depth read blipped would be far worse + // than not enforcing. + unattendedCommitLimit: parseUnattendedCommitLimit(row.unattended_commit_limit), } } +/** + * Null unless the value is a finite number strictly greater than zero. + * + * Written NULL-first on purpose. Never `?? 0`, never `|| 0`, never + * `Number(undefined)` (which yields NaN, and NaN comparisons are false, so it + * would silently disable the control rather than loudly fail). + */ +function parseUnattendedCommitLimit(value: unknown): number | null { + if (value === null || value === undefined) return null + const parsed = typeof value === 'number' ? value : Number(value) + if (!Number.isFinite(parsed) || parsed <= 0) return null + return parsed +} + /** * Late binding for keys minted before the user's first company existed. * diff --git a/lib/bookkeeping/actor-context.ts b/lib/bookkeeping/actor-context.ts index 623bc05b..9f0ff95d 100644 --- a/lib/bookkeeping/actor-context.ts +++ b/lib/bookkeeping/actor-context.ts @@ -21,6 +21,17 @@ export interface CommitActor { type: 'user' | 'api_key' | 'mcp_oauth' | 'cron' | 'system' | 'agent_chat' /** Human-readable credential label, e.g. the API key name. */ label?: string + /** + * SEK ceiling on what this credential may commit WITHOUT human approval, + * or null/undefined for no limit. Read from api_keys by + * validate_and_increment_api_key, which is the one place the database itself + * verifies which credential is acting, so the value is bound to a verified + * key rather than asserted by the caller. + * + * Only ever set for `type: 'api_key'`. Human approvals, cron and the + * cookie-session routes cannot reach the check that reads it. + */ + unattendedCommitLimit?: number | null } export interface ActorStore { diff --git a/lib/errors/structured-errors.ts b/lib/errors/structured-errors.ts index f444c514..8057f896 100644 --- a/lib/errors/structured-errors.ts +++ b/lib/errors/structured-errors.ts @@ -153,6 +153,24 @@ const GENERIC: Record = { // ───────────────────────────────────────────────────────────────── const BOOKKEEPING: Record = { + // An approval-authority refusal, NOT a write failure: the operation is still + // staged and a human can approve it in /pending. retryable is set FALSE + // explicitly because getStructuredError falls back to isTransientFailure(), + // and the message must never contain the words "rate limit": that phrase is + // in TRANSIENT_MESSAGE_PATTERNS and would flip a permanent refusal into a + // retryable one, which is how agents end up in retry storms. + UNATTENDED_COMMIT_LIMIT_EXCEEDED: { + httpStatus: 403, + message_sv: + 'Beloppet överstiger vad den här API-nyckeln får bokföra utan mänskligt godkännande. Underlaget ligger kvar och kan godkännas i Accounted.', + message_en: + 'Amount exceeds what this API key may post without human approval. The operation is preserved and can be approved in the app.', + retryable: false, + remediation: { + description: + 'Do not retry, and do not split the entry into smaller ones: one affärshändelse is one verifikat (BFL 5 kap. 6 §). Ask a human to approve the staged operation in Accounted, or have the key owner raise the limit in API key settings. details.attempted and details.limit carry the numbers.', + }, + }, ACCOUNTS_NOT_IN_CHART: { httpStatus: 400, message_sv: 'Konton saknas i kontoplanen.', diff --git a/lib/pending-operations/__tests__/commit-unattended-limit.test.ts b/lib/pending-operations/__tests__/commit-unattended-limit.test.ts new file mode 100644 index 00000000..5bc06873 --- /dev/null +++ b/lib/pending-operations/__tests__/commit-unattended-limit.test.ts @@ -0,0 +1,197 @@ +/** + * The approval-authority ceiling inside commitPendingOperation. + * + * unattended-limit.test.ts covers the predicate. This file covers the thing + * that actually matters at runtime: WHERE the check sits. It must run before + * the atomic claim, so a refused commit leaves the operation 'pending' and a + * human can still approve the same staged verifikat in the app. Behind the + * claim, the refusal would be caught by the generic handler, marked terminal + * 'rejected', and the staged work would be gone. + * + * Each test therefore uses createQueuedMockSupabase with NOTHING enqueued: if + * the check ever moves below the claim, the claim runs against an empty queue + * and the assertions on code/operation_status fail rather than passing + * vacuously. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { eventBus } from '@/lib/events/bus' +import { createQueuedMockSupabase } from '@/tests/helpers' +import type { PendingOperation } from '@/types' + +vi.mock('@/lib/entitlements/has-capability', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, hasCapability: vi.fn().mockResolvedValue(true) } +}) + +import { commitPendingOperation } from '../commit' + +function makePendingOp(overrides: Partial): PendingOperation { + return { + id: 'op-1', + user_id: 'user-1', + company_id: 'company-1', + operation_type: 'create_voucher', + status: 'pending', + title: 'test', + params: {}, + preview_data: { total_debit: 50000 }, + result_data: null, + actor_type: 'api_key', + actor_id: null, + actor_label: null, + risk_level: 'high', + created_at: '2026-08-01T00:00:00Z', + resolved_at: null, + updated_at: '2026-08-01T00:00:00Z', + ...overrides, + } as PendingOperation +} + +const apiKeyActor = { type: 'api_key' as const, label: 'Bookkeeping agent' } + +beforeEach(() => { + vi.clearAllMocks() + eventBus.clear() +}) + +describe('commitPendingOperation: unattended commit limit', () => { + it('refuses a 50 000 kr voucher on a key capped at 10 000, leaving the op pending', async () => { + const { supabase } = createQueuedMockSupabase() + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({}), + { actor: { ...apiKeyActor, unattendedCommitLimit: 10000 } }, + ) + + expect(result.status).toBe('failed') + expect(result.http_status).toBe(403) + expect(result.code).toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + // The load-bearing assertion. 'pending' is what tells the agent, and the + // app, that the staged verifikat survived and can still be approved. + expect(result.operation_status).toBe('pending') + expect(result.unattended_limit).toEqual({ attempted: 50000, limit: 10000 }) + // Swedish user-facing copy, sourced from the structured-error registry. + expect(result.error).toContain('godkännande') + }) + + it('does not fire for a human approving the same operation', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: null, error: null }) // claim finds no row → 409, proving we got past the ceiling + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({}), + { actor: { type: 'user', label: 'Jakob' } }, + ) + + expect(result.code).not.toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + }) + + it('does not fire when the key has no ceiling', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: null, error: null }) + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({}), + { actor: { ...apiKeyActor, unattendedCommitLimit: null } }, + ) + + expect(result.code).not.toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + }) + + it('does not fire with no actor at all, the cookie-session path', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: null, error: null }) + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({}), + ) + + expect(result.code).not.toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + }) + + it('lets an amount at the ceiling through', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: null, error: null }) + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({ preview_data: { total_debit: 10000 } }), + { actor: { ...apiKeyActor, unattendedCommitLimit: 10000 } }, + ) + + expect(result.code).not.toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + }) + + it('lets a genuinely unpriceable operation type through rather than blocking it', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: null, error: null }) + + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + // pair_count is a count, not kronor. Nothing here can be compared to a + // ceiling, so the op goes through. + makePendingOp({ + operation_type: 'reconciliation_match', + preview_data: { pair_count: 9 }, + }), + { actor: { ...apiKeyActor, unattendedCommitLimit: 100 } }, + ) + + expect(result.code).not.toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + }) + + it('blocks the settlement and batch paths that used to fail open', async () => { + for (const [operation_type, preview_data] of [ + ['link_transaction_journal_entry', { transaction_amount: 250000 }], + ['bulk_book_transactions', { tx_sum: 250000 }], + ['link_supplier_invoice_voucher', { payment_amount: 250000 }], + ['match_batch_allocate', { total_allocated: 250000 }], + ['mark_invoice_paid', { total: 250000 }], + ] as const) { + const { supabase } = createQueuedMockSupabase() + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({ operation_type, preview_data }), + { actor: { ...apiKeyActor, unattendedCommitLimit: 10000 } }, + ) + expect(result.code).toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + expect(result.operation_status).toBe('pending') + } + }) + + it('also covers categorize_transaction and supplier invoices from the inbox', async () => { + for (const [operation_type, preview_data] of [ + ['categorize_transaction', { amount: -9000 }], + ['create_supplier_invoice_from_inbox', { total: 9000 }], + ] as const) { + const { supabase } = createQueuedMockSupabase() + const result = await commitPendingOperation( + supabase as never, + 'user-1', + 'company-1', + makePendingOp({ operation_type, preview_data }), + { actor: { ...apiKeyActor, unattendedCommitLimit: 500 } }, + ) + expect(result.code).toBe('UNATTENDED_COMMIT_LIMIT_EXCEEDED') + expect(result.operation_status).toBe('pending') + } + }) +}) diff --git a/lib/pending-operations/__tests__/unattended-limit.test.ts b/lib/pending-operations/__tests__/unattended-limit.test.ts new file mode 100644 index 00000000..c765161c --- /dev/null +++ b/lib/pending-operations/__tests__/unattended-limit.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from 'vitest' +import { exceedsUnattendedLimit, priceOperation } from '../unattended-limit' + +describe('priceOperation', () => { + it('prices the three operation types that carry an amount before dispatch', () => { + expect(priceOperation('create_voucher', { total_debit: 1250.5 })).toBe(1250.5) + expect(priceOperation('categorize_transaction', { amount: 800 })).toBe(800) + expect(priceOperation('create_supplier_invoice_from_inbox', { total: 4000 })).toBe(4000) + }) + + it('reads jsonb numerics that arrive as strings', () => { + // preview_data is jsonb; numeric-typed values round-trip as strings often + // enough that a bare `> limit` comparison would silently compare text. + expect(priceOperation('create_voucher', { total_debit: '99999.99' })).toBe(99999.99) + }) + + it('uses the magnitude, so a credit-side negative cannot slip under the ceiling', () => { + expect(priceOperation('categorize_transaction', { amount: -25000 })).toBe(25000) + }) + + it('prices every settlement and batch path that posts money', () => { + // These were left unpriced in the first cut, on the assumption their + // totals only existed inside SQL at dispatch. Production says otherwise: + // each field below is present and numeric on 100% of that type's staged + // rows, because it is the number a human is shown when approving. Leaving + // them unpriced let a key with a ceiling post any amount through the four + // largest settlement paths. + expect(priceOperation('link_transaction_journal_entry', { transaction_amount: 12500 })).toBe(12500) + expect(priceOperation('bulk_book_transactions', { tx_sum: 88000 })).toBe(88000) + expect(priceOperation('link_supplier_invoice_voucher', { payment_amount: 4300 })).toBe(4300) + expect(priceOperation('match_batch_allocate', { total_allocated: 99000 })).toBe(99000) + expect(priceOperation('mark_invoice_paid', { total: 6250 })).toBe(6250) + }) + + it('leaves genuinely unpriceable types unpriced rather than inventing a number', () => { + // pair_count is a COUNT. Pricing reconciliation_match off it would compare + // pairs against kronor, which is worse than not enforcing. + expect(priceOperation('reconciliation_match', { pair_count: 7 })).toBeNull() + // These attach räkenskapsinformation to something already booked; the + // transaction_amount they carry is context, not a posting. + expect(priceOperation('link_document_to_voucher', {})).toBeNull() + expect(priceOperation('attach_document_to_transaction', { transaction_amount: 999999 })).toBeNull() + }) + + it('has no priceable type whose field is missing from the allowlist', () => { + // Guards the shape of the allowlist itself: a typo'd field name would make + // that type silently unpriceable, which is exactly the hole this closes. + for (const [op, fields] of Object.entries({ + create_voucher: { total_debit: 1 }, + categorize_transaction: { amount: 1 }, + create_supplier_invoice_from_inbox: { total: 1 }, + link_transaction_journal_entry: { transaction_amount: 1 }, + bulk_book_transactions: { tx_sum: 1 }, + link_supplier_invoice_voucher: { payment_amount: 1 }, + match_batch_allocate: { total_allocated: 1 }, + mark_invoice_paid: { total: 1 }, + })) { + expect(priceOperation(op, fields)).toBe(1) + } + }) + + it('returns null rather than throwing on malformed preview_data', () => { + expect(priceOperation('create_voucher', null)).toBeNull() + expect(priceOperation('create_voucher', undefined)).toBeNull() + expect(priceOperation('create_voucher', 'not an object')).toBeNull() + expect(priceOperation('create_voucher', {})).toBeNull() + expect(priceOperation('create_voucher', { total_debit: 'kr 1 000' })).toBeNull() + expect(priceOperation('create_voucher', { total_debit: null })).toBeNull() + expect(priceOperation('create_voucher', { total_debit: Infinity })).toBeNull() + }) +}) + +describe('exceedsUnattendedLimit', () => { + const over = { + actorType: 'api_key', + limit: 1000, + operationType: 'create_voucher', + previewData: { total_debit: 1000.01 }, + } + + it('blocks an api_key commit above its ceiling', () => { + expect(exceedsUnattendedLimit(over)).toEqual({ + exceeded: true, + attempted: 1000.01, + limit: 1000, + }) + }) + + it('allows an amount exactly at the ceiling', () => { + // The ceiling is inclusive: "may commit up to 1 000 kr" must permit + // 1 000,00 kr, or every limit is off by one öre in the surprising + // direction. + const result = exceedsUnattendedLimit({ ...over, previewData: { total_debit: 1000 } }) + expect(result.exceeded).toBe(false) + }) + + it('never fires for a human, cron or unattributed commit', () => { + for (const actorType of ['user', 'cron', 'mcp_oauth', 'system', undefined]) { + expect( + exceedsUnattendedLimit({ ...over, actorType: actorType as string | undefined }).exceeded, + ).toBe(false) + } + }) + + it('treats an absent ceiling as unlimited', () => { + // Every key created before the column existed reads back null here, so + // this is the default behaviour of the entire installed base. + for (const limit of [null, undefined]) { + expect(exceedsUnattendedLimit({ ...over, limit }).exceeded).toBe(false) + } + }) + + it('treats a nonsensical stored ceiling as unlimited rather than blocking everything', () => { + // The DB CHECK makes 0 and negatives unstorable, so reaching this branch + // means something upstream is already wrong. Failing open keeps that bug + // from presenting as "the agent can no longer book anything". + for (const limit of [0, -5, Number.NaN]) { + expect(exceedsUnattendedLimit({ ...over, limit }).exceeded).toBe(false) + } + }) + + it('fails open when the operation genuinely cannot be priced', () => { + const result = exceedsUnattendedLimit({ + ...over, + operationType: 'reconciliation_match', + previewData: { pair_count: 7 }, + }) + expect(result.exceeded).toBe(false) + expect(result.attempted).toBeNull() + }) + + it('blocks an over-ceiling batch allocation, the path that used to fail open', () => { + const result = exceedsUnattendedLimit({ + ...over, + operationType: 'match_batch_allocate', + previewData: { total_allocated: 10_000_000 }, + }) + expect(result.exceeded).toBe(true) + expect(result.attempted).toBe(10_000_000) + }) +}) diff --git a/lib/pending-operations/commit.ts b/lib/pending-operations/commit.ts index d6775fdb..f504e452 100644 --- a/lib/pending-operations/commit.ts +++ b/lib/pending-operations/commit.ts @@ -114,6 +114,7 @@ import { getEmailService } from '@/lib/email/service' import { resolveInvoiceSender } from '@/lib/email/invoice-sender' import { hasCapability, CAPABILITY_BLOCKED_MESSAGE_SV } from '@/lib/entitlements/has-capability' import { PAID_OPERATION_CAPABILITY_MAP } from '@/lib/entitlements/keys' +import { exceedsUnattendedLimit } from './unattended-limit' import { generateInvoiceEmailHtml, generateInvoiceEmailText, @@ -216,6 +217,11 @@ export interface CommitResult { // callers that do not recognize the code fall back to `error`. code?: string account_numbers?: string[] + // The two numbers UNATTENDED_COMMIT_LIMIT_EXCEEDED's remediation refers to, + // in SEK. Present only on that code. Without them the agent knows it was + // over the ceiling but not by how much, and cannot tell a human what to + // raise the limit to. + unattended_limit?: { attempted: number; limit: number } // Where the pending_operations row landed, independent of `status`: // 'pending' means the op was NOT consumed and can be approved again // (recoverable refusal: capability, chart accounts, Skatteverket, or an @@ -6649,6 +6655,41 @@ async function commitPendingOperationInner( } } + // ── Approval-authority ceiling: what this API key may finish unattended. + // + // Checked HERE, before the atomic claim, for the same reason the + // capability gate above is: a refused op must stay 'pending' so a human + // can still approve it in /pending. Inside the claim it would fall into + // the generic catch below, which marks the op terminal 'rejected' with no + // code and consumes the staged verifikat, which is unrecoverable: the + // agent must rebuild the whole booking to try again. + // + // Cannot fire for a human: exceedsUnattendedLimit requires + // actor.type === 'api_key' AND a positive limit on that key. In-app + // approvals pass {type:'user'}, cron passes 'cron', and the + // cookie-session routes commit with no actor at all. + const unattended = exceedsUnattendedLimit({ + actorType: opts.actor?.type, + limit: opts.actor?.unattendedCommitLimit, + operationType: pendingOp.operation_type, + previewData: pendingOp.preview_data, + }) + if (unattended.exceeded) { + return { + status: 'failed', + error: + getErrorEntry('UNATTENDED_COMMIT_LIMIT_EXCEEDED')?.message_sv ?? + 'Beloppet \u00f6verstiger vad den h\u00e4r API-nyckeln f\u00e5r bokf\u00f6ra utan m\u00e4nskligt godk\u00e4nnande.', + http_status: 403, + code: 'UNATTENDED_COMMIT_LIMIT_EXCEEDED', + operation_status: 'pending', + unattended_limit: { + attempted: unattended.attempted as number, + limit: unattended.limit as number, + }, + } + } + // ── Atomic claim: flip status pending → committing in a single conditional // update. If 0 rows are affected, another caller (auto-commit ↔ human // approval, or two parallel approvals) already claimed this op and we diff --git a/lib/pending-operations/unattended-limit.ts b/lib/pending-operations/unattended-limit.ts new file mode 100644 index 00000000..877078c7 --- /dev/null +++ b/lib/pending-operations/unattended-limit.ts @@ -0,0 +1,125 @@ +/** + * Approval-authority limit: what an API key may commit WITHOUT a human. + * + * This is not a write limit. Over the ceiling the operation stays staged and a + * person approves it in /pending, so nothing is destroyed, no voucher number is + * burned, and no löpnummer gap appears (BFL 5 kap. 7 §). + * + * ## What this control is, and is not + * + * It is a blast-radius cap on an agent that is looping, mis-prompted or + * prompt-injected. It is NOT a security boundary: a per-entry ceiling is + * defeated by splitting one large entry into several small ones, and an LLM + * will discover that on its own, because "reduce the amount and retry" is the + * obvious repair for "amount too large". + * + * That bypass is itself a compliance violation, since one affärshändelse is one + * verifikat (BFL 5 kap. 6 §), which is why the UNATTENDED_COMMIT_LIMIT_EXCEEDED + * remediation tells the agent not to split BEFORE it tells it anything else. + * A rolling-window cumulative limit is the primitive that actually bounds + * exposure; it is deliberately a separate change. + */ + +/** + * Every operation type that posts money, with the preview_data field carrying + * the amount it will post. + * + * An explicit allowlist, and every entry is verified against production rather + * than guessed. Over the last 120 days each field below is present and numeric + * on 100% of that type's staged rows: + * + * create_voucher total_debit 1389 rows + * categorize_transaction amount 2003 rows + * create_supplier_invoice_from_inbox total 228 rows + * link_transaction_journal_entry transaction_amount 1369 rows + * bulk_book_transactions tx_sum 273 rows + * link_supplier_invoice_voucher payment_amount 55 rows + * match_batch_allocate total_allocated 24 rows + * mark_invoice_paid total 3 rows + * + * The staged preview already carries the amount the operation intends to post, + * because that is the number a human is shown when approving it. An earlier + * draft of this file assumed the batch and settlement paths computed their + * totals only inside SQL at dispatch and therefore left them unpriced; that was + * wrong, and it left the four largest settlement paths able to post any amount + * on a key with a ceiling. + * + * Types deliberately absent, and why: + * - reconciliation_match carries pair_count, a COUNT, not an amount. Pricing + * it off that number would compare pairs against kronor. It stays unpriced. + * - link_document_to_voucher and attach_document_to_transaction move no + * money; they attach räkenskapsinformation to something already booked. + * - create_customer, create_transaction and the rest of the registry post + * nothing to the ledger. + * + * Anything not listed is unpriceable and FAILS OPEN. That is the safe direction + * for a control that can only ever narrow what a key does: a wrong guess at an + * amount blocks a legitimate commit, and the failure mode of guessing high is + * an agent that cannot work at all. + */ +const PRICEABLE_OPERATIONS: Readonly> = { + create_voucher: ['total_debit'], + categorize_transaction: ['amount'], + create_supplier_invoice_from_inbox: ['total'], + link_transaction_journal_entry: ['transaction_amount'], + bulk_book_transactions: ['tx_sum'], + link_supplier_invoice_voucher: ['payment_amount'], + match_batch_allocate: ['total_allocated'], + mark_invoice_paid: ['total'], +} + +/** + * The SEK amount this operation would post, or null when it cannot be priced + * before dispatch. + * + * Null always means "do not enforce". Every parse failure, missing field and + * unknown operation type lands here on purpose. + */ +export function priceOperation( + operationType: string, + previewData: unknown, +): number | null { + const fields = PRICEABLE_OPERATIONS[operationType] + if (!fields) return null + if (previewData === null || typeof previewData !== 'object') return null + + const record = previewData as Record + for (const field of fields) { + const raw = record[field] + if (raw === null || raw === undefined) continue + // jsonb numerics can arrive as strings; parse rather than trusting + // comparison coercion. + const parsed = typeof raw === 'number' ? raw : Number(raw) + if (Number.isFinite(parsed)) return Math.abs(parsed) + } + return null +} + +/** + * True when this commit needs a human first. + * + * Written NULL-first in every clause. Absence of a limit, absence of a price, + * and any non-api_key actor all return false, so the control can only ever + * narrow what an API key does unattended and can never touch a human, a cron + * or an in-app approval. + */ +export function exceedsUnattendedLimit(params: { + actorType: string | undefined + limit: number | null | undefined + operationType: string + previewData: unknown +}): { exceeded: boolean; attempted: number | null; limit: number | null } { + const limit = + typeof params.limit === 'number' && Number.isFinite(params.limit) && params.limit > 0 + ? params.limit + : null + + if (params.actorType !== 'api_key' || limit === null) { + return { exceeded: false, attempted: null, limit } + } + + const attempted = priceOperation(params.operationType, params.previewData) + if (attempted === null) return { exceeded: false, attempted: null, limit } + + return { exceeded: attempted > limit, attempted, limit } +} diff --git a/lib/reports/behandlingshistorik.ts b/lib/reports/behandlingshistorik.ts index 30b6b174..e8767f30 100644 --- a/lib/reports/behandlingshistorik.ts +++ b/lib/reports/behandlingshistorik.ts @@ -355,6 +355,11 @@ const API_KEY_FIELDS: Record = { rate_limit_per_minute: 'Anrop per minut', expires_at: 'Giltig till', is_active: 'Aktiv', + // How much the key may post without a human. A change here changes who + // approves the company's bookkeeping, so it belongs in behandlingshistorik + // (BFL 5 kap. 11 §) exactly like a scope change does. Without this line the + // UPDATE row exists in audit_log but renders zero diff lines and is dropped. + unattended_commit_limit: 'Belopp utan mänsklig granskning', } const DIMENSION_FIELDS: Record = { diff --git a/skills/accounted-api/references/journal-entries.md b/skills/accounted-api/references/journal-entries.md index 792f5450..fc074ccc 100644 --- a/skills/accounted-api/references/journal-entries.md +++ b/skills/accounted-api/references/journal-entries.md @@ -270,6 +270,7 @@ Atomically advances the voucher series and flips the draft to posted. The vouche - Idempotency-Key is mandatory. - Posted entries cannot be edited. Plan the lines carefully or call /correct after commit if you need to change them. - Voucher numbers are sequential within (fiscal_period_id, voucher_series). A commit failure (e.g. period locked between draft creation and commit) does not advance the sequence. +- If the key has an unattended commit limit, an entry above it returns 403 UNATTENDED_COMMIT_LIMIT_EXCEEDED and stays a draft for a human to commit. Do not split it into smaller entries: one affärshändelse is one verifikat (BFL 5 kap. 6 §). | Parameter | In | Type | Required | Notes | |---|---|---|---|---| diff --git a/supabase/migrations/20260831111519_api_key_unattended_commit_limit.sql b/supabase/migrations/20260831111519_api_key_unattended_commit_limit.sql new file mode 100644 index 00000000..a8fda84b --- /dev/null +++ b/supabase/migrations/20260831111519_api_key_unattended_commit_limit.sql @@ -0,0 +1,141 @@ +-- Approval-authority limit per API key: what an agent may finish WITHOUT a human. +-- +-- This is deliberately NOT a write limit and deliberately NOT enforced inside +-- commit_journal_entry. Over the ceiling the agent may still stage a +-- pending_operation; it just may not commit it unattended, so a human approves +-- it in /pending. Nothing is destroyed, no voucher number is burned, and no +-- löpnummer gap is created (BFL 5 kap. 7 §). +-- +-- ## Why the limit lives on api_keys and never on company_settings +-- +-- company_settings.agent_auto_commit_max_amount existed once +-- (20260501120000) and was dropped four days later (20260505190027). A +-- company-scoped money threshold catches HUMANS too, which is the opposite of +-- the intent. Keyed on the credential, the check can only ever fire for an +-- agent: the gate reads actor.type = 'api_key' plus a non-null limit on that +-- key, and human approvals ({type:'user'}), cron, and every cookie-session +-- route commit with a different actor or none at all. +-- +-- ## Why validate_and_increment_api_key returns it +-- +-- That function is the one place the database itself verifies which credential +-- is acting: it matches the key hash. A limit returned from there is bound to a +-- DB-verified key rather than asserted by the caller. Passing a key id into +-- commit_journal_entry would have been caller-asserted and unverifiable (the +-- pending-op path runs as service_role, where the tenant guard bypasses by +-- design), while costing a DROP+CREATE on the function that issues every +-- voucher number. Two of that function's seven redefinitions were emergency +-- fixes for PostgREST overload ambiguity; there is no upside left to pay that. +-- +-- pg-test: tests/pg/api-key-unattended-commit-limit.pg.test.ts + +ALTER TABLE public.api_keys + ADD COLUMN IF NOT EXISTS unattended_commit_limit numeric(14, 2); + +-- The single most important line in this migration. +-- +-- A stored 0 would block every commit for that key, and NULL-read-as-0 is the +-- catastrophic failure mode of the whole feature. Making zero unrepresentable +-- at the storage layer means that state cannot be reached even by a bad UI +-- write, a bad PATCH body, or a bad backfill. Absence of a limit is expressed +-- ONLY as NULL, never as 0. +ALTER TABLE public.api_keys + DROP CONSTRAINT IF EXISTS api_keys_unattended_commit_limit_positive; +-- Added VALIDATED, not NOT VALID: api_keys is 388 rows / 768 kB in production, +-- so the validating scan is sub-millisecond. The NOT VALID + VALIDATE dance +-- buys nothing at this size and can leave the constraint permanently unenforced +-- if the second statement is ever skipped. +ALTER TABLE public.api_keys + ADD CONSTRAINT api_keys_unattended_commit_limit_positive + CHECK (unattended_commit_limit IS NULL OR unattended_commit_limit > 0); + +COMMENT ON COLUMN public.api_keys.unattended_commit_limit IS + 'SEK ceiling on what this key may commit without human approval. NULL = no limit, and NULL is the only representation of "no limit": zero is forbidden by CHECK because a stored 0 would block every commit. Over the ceiling the agent still stages the operation for a human to approve.'; + +-- Surface the limit on the one call that already verifies the credential. +-- +-- The body below is copied VERBATIM from 20260621130000_api_keys_rotation_grace.sql +-- (the latest definition, which added previous_key_hash rotation grace) with +-- exactly two changes: unattended_commit_limit joins the RETURNS TABLE, and it +-- is selected into a variable and returned in all three RETURN QUERY branches. +-- Copying an older body would silently revert key rotation and lock out every +-- rotating integration. +DROP FUNCTION IF EXISTS public.validate_and_increment_api_key(text); + +CREATE FUNCTION public.validate_and_increment_api_key(p_key_hash text) +RETURNS TABLE( + user_id uuid, + company_id uuid, + api_key_id uuid, + api_key_name text, + rate_limited boolean, + scopes text[], + mode text, + unattended_commit_limit numeric +) +LANGUAGE plpgsql SECURITY DEFINER AS $$ +DECLARE + v_id uuid; + v_user_id uuid; + v_company_id uuid; + v_api_key_name text; + v_rate_limit_rpm integer; + v_request_count integer; + v_window_start timestamptz; + v_scopes text[]; + v_mode text; + v_unattended_commit_limit numeric; +BEGIN + -- Match the live key_hash, OR a previous (just-rotated) key_hash that is still + -- inside its grace window. Both gated by revoked_at IS NULL. + SELECT ak.id, ak.user_id, ak.company_id, ak.name, + ak.rate_limit_rpm, ak.request_count, ak.rate_limit_window_start, ak.scopes, ak.mode, + ak.unattended_commit_limit + INTO v_id, v_user_id, v_company_id, v_api_key_name, + v_rate_limit_rpm, v_request_count, v_window_start, v_scopes, v_mode, + v_unattended_commit_limit + FROM public.api_keys ak + WHERE ak.revoked_at IS NULL + AND ( + ak.key_hash = p_key_hash + OR ( + ak.previous_key_hash = p_key_hash + AND ak.previous_key_expires_at IS NOT NULL + AND ak.previous_key_expires_at > now() + ) + ) + FOR UPDATE; + + IF v_id IS NULL THEN + RETURN; -- no live match (incl. expired grace) → caller returns 401, as before + END IF; + + -- Reset the rate-limit window if it is unset or older than one minute. + IF v_window_start IS NULL OR v_window_start < now() - interval '1 minute' THEN + UPDATE public.api_keys + SET request_count = 1, + rate_limit_window_start = now(), + last_used_at = now() + WHERE id = v_id; + RETURN QUERY SELECT v_user_id, v_company_id, v_id, v_api_key_name, false, v_scopes, v_mode, + v_unattended_commit_limit; + RETURN; + END IF; + + IF v_request_count >= v_rate_limit_rpm THEN + RETURN QUERY SELECT v_user_id, v_company_id, v_id, v_api_key_name, true, v_scopes, v_mode, + v_unattended_commit_limit; + RETURN; + END IF; + + UPDATE public.api_keys + SET request_count = request_count + 1, + last_used_at = now() + WHERE id = v_id; + + RETURN QUERY SELECT v_user_id, v_company_id, v_id, v_api_key_name, false, v_scopes, v_mode, + v_unattended_commit_limit; +END; +$$; + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/api-key-unattended-commit-limit.pg.test.ts b/tests/pg/api-key-unattended-commit-limit.pg.test.ts new file mode 100644 index 00000000..cffa1683 --- /dev/null +++ b/tests/pg/api-key-unattended-commit-limit.pg.test.ts @@ -0,0 +1,135 @@ +import { randomUUID } from 'node:crypto' +import { describe, expect, it } from 'vitest' +import { insertAuthUser, insertCompany } from './fixtures' +import { getPool } from './setup' + +/** + * api_keys.unattended_commit_limit (migration 20260831111519). + * + * The column is the storage half of the approval-authority envelope; the + * enforcement half is TypeScript (lib/pending-operations/unattended-limit.ts), + * deliberately, because a RAISE inside commit_journal_entry is swallowed into + * a retryable 500 and burns the staged operation. + * + * What must hold in the database, and is what these tests pin: + * 1. the column exists, is nullable, and defaults to NULL (unlimited), so + * every key that existed before this migration keeps its behaviour; + * 2. the CHECK rejects 0 and negatives, so "limit = 0" can never be stored + * and silently read back as falsy-therefore-unlimited; + * 3. validate_and_increment_api_key returns it, with exactly ONE signature + * (adding a parameter or return column to a Postgres function creates an + * overload rather than replacing it, and PostgREST then 300s on the + * ambiguity: see migration 20260421140000); + * 4. writing it produces an audit_log row, since changing how much an agent + * may post without a human is a change to who approves the company's + * bookkeeping (BFL 5 kap. 11 paragraf). + */ +describe('api_keys.unattended_commit_limit (pg)', () => { + async function seedKey(limit: number | null = null) { + const userId = await insertAuthUser() + const companyId = await insertCompany({ createdBy: userId }) + const apiKeyId = randomUUID() + const keyHash = randomUUID().replaceAll('-', '') + await getPool().query( + `INSERT INTO public.api_keys + (id, user_id, company_id, key_hash, key_prefix, name, scopes, unattended_commit_limit) + VALUES ($1, $2, $3, $4, 'gnubok_sk_test', 'Envelope test key', $5, $6)`, + [apiKeyId, userId, companyId, keyHash, ['reports:read'], limit], + ) + return { userId, companyId, apiKeyId, keyHash } + } + + it('defaults to NULL so pre-existing keys stay unlimited', async () => { + const userId = await insertAuthUser() + const companyId = await insertCompany({ createdBy: userId }) + // Deliberately omits the column instead of storing an explicit NULL: this + // test exists to pin the DATABASE DEFAULT, and passing NULL in would keep + // it green even if the default changed to a positive ceiling, which is the + // one change that would silently start blocking every existing key. + const { rows } = await getPool().query<{ unattended_commit_limit: string | null }>( + `INSERT INTO public.api_keys + (user_id, company_id, key_hash, key_prefix, name, scopes) + VALUES ($1, $2, $3, 'gnubok_sk_test', 'Default test key', $4) + RETURNING unattended_commit_limit`, + [userId, companyId, randomUUID().replaceAll('-', ''), ['reports:read']], + ) + expect(rows[0]!.unattended_commit_limit).toBeNull() + }) + + it('rejects a zero or negative ceiling with the CHECK constraint', async () => { + const userId = await insertAuthUser() + const companyId = await insertCompany({ createdBy: userId }) + + for (const bad of [0, -1]) { + await expect( + getPool().query( + `INSERT INTO public.api_keys + (user_id, company_id, key_hash, key_prefix, name, scopes, unattended_commit_limit) + VALUES ($1, $2, $3, 'gnubok_sk_test', 'Bad ceiling', $4, $5)`, + [userId, companyId, randomUUID().replaceAll('-', ''), ['reports:read'], bad], + ), + ).rejects.toMatchObject({ + code: '23514', + constraint: 'api_keys_unattended_commit_limit_positive', + }) + } + }) + + it('validate_and_increment_api_key returns the ceiling, and has exactly one signature', async () => { + const { keyHash } = await seedKey(2500) + + const overloads = await getPool().query<{ n: number }>( + `SELECT count(*)::int AS n + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = 'validate_and_increment_api_key'`, + ) + expect(overloads.rows[0]!.n).toBe(1) + + const { rows } = await getPool().query<{ + unattended_commit_limit: string | null + rate_limited: boolean + }>(`SELECT * FROM public.validate_and_increment_api_key($1)`, [keyHash]) + expect(rows).toHaveLength(1) + expect(rows[0]!.rate_limited).toBe(false) + // numeric comes back as a string from node-postgres; compare numerically. + expect(Number(rows[0]!.unattended_commit_limit)).toBe(2500) + }) + + it('returns NULL for a key with no ceiling, not 0', async () => { + const { keyHash } = await seedKey(null) + const { rows } = await getPool().query<{ unattended_commit_limit: string | null }>( + `SELECT * FROM public.validate_and_increment_api_key($1)`, + [keyHash], + ) + // The whole guard is written NULL-first. A 0 here would read as a real + // ceiling on the way in and block every commit the key attempts. + expect(rows[0]!.unattended_commit_limit).toBeNull() + }) + + it('records a ceiling change in audit_log', async () => { + const { apiKeyId } = await seedKey(null) + + await getPool().query( + `UPDATE public.api_keys SET unattended_commit_limit = 10000 WHERE id = $1`, + [apiKeyId], + ) + + const { rows } = await getPool().query<{ + action: string + old_limit: string | null + new_limit: string | null + }>( + `SELECT action, + old_state ->> 'unattended_commit_limit' AS old_limit, + new_state ->> 'unattended_commit_limit' AS new_limit + FROM public.audit_log + WHERE table_name = 'api_keys' AND record_id = $1 AND action = 'UPDATE' + ORDER BY created_at, id`, + [apiKeyId], + ) + expect(rows).toHaveLength(1) + expect(rows[0]!.old_limit).toBeNull() + expect(Number(rows[0]!.new_limit)).toBe(10000) + }) +})