Fix/mcp fixes and bugs (#518)
* feat(voucher): add create voucher and correct entry previews; update commit methods * feat: add support for pending operations in API key scopes and OAuth client management - Introduced new API key scopes for reading and approving pending operations. - Updated the scope groups to include pending operations. - Added new tools for listing and managing pending operations. - Implemented OAuth client registration and revocation endpoints. - Created a UI panel for managing OAuth clients, including registration and revocation. - Added tests for pending operations tools and OAuth allowlist functionality. - Implemented a database migration for OAuth client registrations with appropriate policies and constraints. * feat: Implement OAuth client registration rate limiting and enhance security measures - Added IP-based rate limiting to the OAuth client registration endpoint to prevent enumeration attacks. - Introduced a service-role client for allowlist lookups, ensuring trust boundaries are maintained. - Updated error responses to be uniform across different types of redirect URI validation failures. - Enhanced tests to reflect changes in OAuth scope handling, ensuring fallback to read-only scopes when no scopes are provided. - Improved handling of high-risk pending operations, requiring explicit confirmation for approvals. - Added audit logging for OAuth client revocations and pending operation approvals/rejections to maintain a security audit trail. - Refactored API key scope management to include default read-only scopes for OAuth-issued keys and added segregation-of-duties checks.
This commit is contained in:
Mattsson
@@ -0,0 +1,62 @@
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
import { isBuiltInRedirectUri, isAllowedRedirectUri } from '../oauth-allowlist'
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
|
||||
describe('isBuiltInRedirectUri', () => {
|
||||
it.each([
|
||||
['https://claude.ai/api/oauth/callback', true],
|
||||
['https://claude.com/api/oauth/callback', true],
|
||||
['http://localhost:3000/cb', true],
|
||||
['http://localhost/cb', true],
|
||||
['http://127.0.0.1:8080/cb', true],
|
||||
['https://evil.com/cb', false],
|
||||
['https://example.com/api/foo', false],
|
||||
['ftp://localhost/cb', false],
|
||||
['', false],
|
||||
])('classifies %s as %s', (uri, expected) => {
|
||||
expect(isBuiltInRedirectUri(uri)).toBe(expected)
|
||||
})
|
||||
})
|
||||
|
||||
function makeFakeSupabase(rows: Array<{ id: string }>): SupabaseClient {
|
||||
// Chainable thenable that resolves to { data, error } when awaited via
|
||||
// .maybeSingle(). Matches the shape isAllowedRedirectUri actually invokes.
|
||||
const chain = {
|
||||
from() { return chain },
|
||||
select() { return chain },
|
||||
eq() { return chain },
|
||||
is() { return chain },
|
||||
limit() { return chain },
|
||||
async maybeSingle() {
|
||||
return { data: rows[0] ?? null, error: null }
|
||||
},
|
||||
}
|
||||
return chain as unknown as SupabaseClient
|
||||
}
|
||||
|
||||
describe('isAllowedRedirectUri', () => {
|
||||
it('short-circuits to true for built-in patterns without touching the DB', async () => {
|
||||
const sb = {
|
||||
from: vi.fn(() => {
|
||||
throw new Error('should not be called')
|
||||
}),
|
||||
} as unknown as SupabaseClient
|
||||
expect(await isAllowedRedirectUri('https://claude.ai/api/cb', sb)).toBe(true)
|
||||
expect(await isAllowedRedirectUri('http://localhost:3000/cb', sb)).toBe(true)
|
||||
})
|
||||
|
||||
it('returns true when the DB has a registration for the URI', async () => {
|
||||
const sb = makeFakeSupabase([{ id: 'reg-1' }])
|
||||
expect(await isAllowedRedirectUri('https://myapp.example.com/cb', sb)).toBe(true)
|
||||
})
|
||||
|
||||
it('returns false when no registration exists', async () => {
|
||||
const sb = makeFakeSupabase([])
|
||||
expect(await isAllowedRedirectUri('https://evil.com/cb', sb)).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for empty / non-string inputs', async () => {
|
||||
expect(await isAllowedRedirectUri('')).toBe(false)
|
||||
expect(await isAllowedRedirectUri(undefined as unknown as string)).toBe(false)
|
||||
})
|
||||
})
|
||||
+78
-7
@@ -27,6 +27,8 @@ export const API_KEY_SCOPES = {
|
||||
'documents:read': { label: 'Dokument — läs', description: 'Lista och hämta dokumentbilagor' },
|
||||
'documents:write': { label: 'Dokument — skriv', description: 'Ladda upp och koppla dokument till verifikationer' },
|
||||
'compliance:read': { label: 'Compliance — läs', description: 'Pre-flight-kontroller: momsstängning, bokslutsberedskap, voucher-gap, IB/UB-kontinuitet' },
|
||||
'pending_operations:read': { label: 'Stagade operationer — läs', description: 'Lista pending_operations (staged writes awaiting approval)' },
|
||||
'pending_operations:approve': { label: 'Stagade operationer — godkänn', description: 'Godkänn eller avvisa stagade operationer via API/MCP — agenten ersätter web-UI:s granskning' },
|
||||
} as const
|
||||
|
||||
export type ApiKeyScope = keyof typeof API_KEY_SCOPES
|
||||
@@ -42,15 +44,79 @@ export const DEFAULT_SCOPES: ApiKeyScope[] = [
|
||||
'reports:read',
|
||||
]
|
||||
|
||||
/**
|
||||
* Read-only fallback granted to OAuth-issued keys when the client did not
|
||||
* pass an explicit `scope` parameter at /authorize. Per GDPR Art. 25(2)
|
||||
* (data protection by default), the silent fallback must never include
|
||||
* destructive scopes (*:write, pending_operations:approve, bookkeeping:write).
|
||||
* Destructive scopes must be requested explicitly by the client and consented
|
||||
* to by the user.
|
||||
*/
|
||||
export const DEFAULT_OAUTH_SCOPES: ApiKeyScope[] = [
|
||||
'transactions:read',
|
||||
'customers:read',
|
||||
'invoices:read',
|
||||
'suppliers:read',
|
||||
'reports:read',
|
||||
'companies:read',
|
||||
'events:read',
|
||||
'operations:read',
|
||||
'documents:read',
|
||||
'compliance:read',
|
||||
'payroll:read',
|
||||
'pending_operations:read',
|
||||
]
|
||||
|
||||
/**
|
||||
* Scopes advertised in the RFC 8414 authorization-server metadata document
|
||||
* (/.well-known/oauth-authorization-server). Restricted to the same set that
|
||||
* /authorize will grant by default — destructive scopes still work when
|
||||
* requested explicitly, they just aren't enumerated for unauthenticated
|
||||
* callers (defense-in-depth against scope-escalation reconnaissance).
|
||||
*/
|
||||
export const PUBLIC_OAUTH_METADATA_SCOPES: ApiKeyScope[] = [...DEFAULT_OAUTH_SCOPES]
|
||||
|
||||
/**
|
||||
* Scopes that allow staging a pending_operation. Used to detect a
|
||||
* segregation-of-duties conflict when paired with `pending_operations:approve`
|
||||
* on the same API key (ISO 27001:2022 A.5.3, SOC 2 CC6.1).
|
||||
*/
|
||||
export const STAGING_SCOPES: ApiKeyScope[] = [
|
||||
'transactions:write',
|
||||
'customers:write',
|
||||
'invoices:write',
|
||||
'suppliers:write',
|
||||
'bookkeeping:write',
|
||||
'payroll:write',
|
||||
'documents:write',
|
||||
]
|
||||
|
||||
/**
|
||||
* Detect a segregation-of-duties conflict between staging and approval scopes
|
||||
* on the same key. Returns the offending staging scope, or null when the
|
||||
* combination is clean. Callers may choose to block, warn, or record an
|
||||
* acknowledged risk acceptance.
|
||||
*
|
||||
* Granting both stage+approve to the same actor lets an automated agent both
|
||||
* stage AND commit financial postings without a human-in-the-loop review,
|
||||
* which is the explicit control surface for BFNAR 2013:2 (behandlingshistorik)
|
||||
* and BFL 5 kap 5§ traceability requirements.
|
||||
*/
|
||||
export function findStageApproveConflict(scopes: ApiKeyScope[]): ApiKeyScope | null {
|
||||
if (!scopes.includes('pending_operations:approve')) return null
|
||||
return scopes.find((s) => STAGING_SCOPES.includes(s)) ?? null
|
||||
}
|
||||
|
||||
/** Scope domain groups for UI rendering */
|
||||
export const SCOPE_GROUPS = [
|
||||
{ domain: 'transactions', label: 'Transaktioner', read: 'transactions:read' as const, write: 'transactions:write' as const },
|
||||
{ domain: 'customers', label: 'Kunder', read: 'customers:read' as const, write: 'customers:write' as const },
|
||||
{ domain: 'invoices', label: 'Fakturor', read: 'invoices:read' as const, write: 'invoices:write' as const },
|
||||
{ domain: 'suppliers', label: 'Leverantörer', read: 'suppliers:read' as const, write: 'suppliers:write' as const },
|
||||
{ domain: 'reports', label: 'Rapporter', read: 'reports:read' as const, write: null },
|
||||
{ domain: 'bookkeeping', label: 'Bokföring', read: null, write: 'bookkeeping:write' as const },
|
||||
{ domain: 'payroll', label: 'Löner', read: 'payroll:read' as const, write: 'payroll:write' as const },
|
||||
{ domain: 'transactions', label: 'Transaktioner', read: 'transactions:read' as const, write: 'transactions:write' as const },
|
||||
{ domain: 'customers', label: 'Kunder', read: 'customers:read' as const, write: 'customers:write' as const },
|
||||
{ domain: 'invoices', label: 'Fakturor', read: 'invoices:read' as const, write: 'invoices:write' as const },
|
||||
{ domain: 'suppliers', label: 'Leverantörer', read: 'suppliers:read' as const, write: 'suppliers:write' as const },
|
||||
{ domain: 'reports', label: 'Rapporter', read: 'reports:read' as const, write: null },
|
||||
{ domain: 'bookkeeping', label: 'Bokföring', read: null, write: 'bookkeeping:write' as const },
|
||||
{ domain: 'payroll', label: 'Löner', read: 'payroll:read' as const, write: 'payroll:write' as const },
|
||||
{ domain: 'pending_operations', label: 'Stagade operationer', read: 'pending_operations:read' as const, write: 'pending_operations:approve' as const },
|
||||
] as const
|
||||
|
||||
/** Map MCP tool name → required scope. Tools omitted from this map are available to any authenticated key (e.g. discovery/search/skill loading). */
|
||||
@@ -126,6 +192,7 @@ export const TOOL_SCOPE_MAP: Record<string, ApiKeyScope> = {
|
||||
gnubok_approve_supplier_invoice: 'suppliers:write',
|
||||
gnubok_credit_supplier_invoice: 'suppliers:write',
|
||||
gnubok_create_supplier_invoice_from_inbox: 'suppliers:write',
|
||||
gnubok_set_inbox_extracted_data: 'suppliers:write',
|
||||
// Invoice conversion + crediting
|
||||
gnubok_convert_invoice: 'invoices:write',
|
||||
gnubok_credit_invoice: 'invoices:write',
|
||||
@@ -133,6 +200,10 @@ export const TOOL_SCOPE_MAP: Record<string, ApiKeyScope> = {
|
||||
gnubok_create_voucher: 'bookkeeping:write',
|
||||
gnubok_correct_entry: 'bookkeeping:write',
|
||||
gnubok_reverse_journal_entry: 'bookkeeping:write',
|
||||
// Pending operations approval (mirrors the /pending web UI)
|
||||
gnubok_list_pending_operations: 'pending_operations:read',
|
||||
gnubok_approve_pending_operation: 'pending_operations:approve',
|
||||
gnubok_reject_pending_operation: 'pending_operations:approve',
|
||||
}
|
||||
|
||||
export function validateScopes(scopes: unknown): ApiKeyScope[] | null {
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
import { createServiceClientNoCookies } from './api-keys'
|
||||
|
||||
/**
|
||||
* Built-in redirect URI patterns. These bypass the DB lookup entirely so
|
||||
* Claude's connector keeps working without seeded rows, and so local
|
||||
* development never depends on having a registration.
|
||||
*/
|
||||
export const BUILT_IN_REDIRECT_PATTERNS: readonly RegExp[] = [
|
||||
/^https:\/\/claude\.ai\/api\//,
|
||||
/^https:\/\/claude\.com\/api\//,
|
||||
/^http:\/\/localhost(:\d+)?(\/|$)/,
|
||||
/^http:\/\/127\.0\.0\.1(:\d+)?(\/|$)/,
|
||||
]
|
||||
|
||||
export function isBuiltInRedirectUri(uri: string): boolean {
|
||||
return BUILT_IN_REDIRECT_PATTERNS.some((pattern) => pattern.test(uri))
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve whether a redirect URI is allowed. Built-in patterns short-circuit;
|
||||
* otherwise we look for a non-revoked registration in oauth_client_registrations.
|
||||
*
|
||||
* The supabase client should be supplied explicitly by the caller so the
|
||||
* trust boundary is visible at the callsite (SOC 2 CC6.1). When omitted, the
|
||||
* function falls back to a service-role client — required for the /register
|
||||
* endpoint which has no user session yet. The lookup is by exact URI; the
|
||||
* unique partial index on the table ensures at most one active row.
|
||||
*
|
||||
* Fails closed on any error (client construction, DB query) — for an
|
||||
* allowlist, "unknown → deny" is the safe default.
|
||||
*/
|
||||
export async function isAllowedRedirectUri(
|
||||
uri: string,
|
||||
supabase?: SupabaseClient
|
||||
): Promise<boolean> {
|
||||
if (typeof uri !== 'string' || uri.length === 0) return false
|
||||
if (isBuiltInRedirectUri(uri)) return true
|
||||
|
||||
// Service-role client construction can throw when Supabase env vars are
|
||||
// absent (unit tests, misconfigured deploys). Treat that as "not allowed"
|
||||
// — failing closed is the safe default for an allowlist.
|
||||
let client: SupabaseClient
|
||||
try {
|
||||
client = supabase ?? createServiceClientNoCookies()
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
|
||||
const { data, error } = await client
|
||||
.from('oauth_client_registrations')
|
||||
.select('id')
|
||||
.eq('redirect_uri', uri)
|
||||
.is('revoked_at', null)
|
||||
.limit(1)
|
||||
.maybeSingle()
|
||||
|
||||
if (error) return false
|
||||
return data !== null
|
||||
}
|
||||
@@ -22,6 +22,12 @@ export interface AuthCodePayload {
|
||||
userId: string
|
||||
codeChallenge: string
|
||||
redirectUri: string
|
||||
/**
|
||||
* Scopes the user consented to grant the resulting API key. Undefined on
|
||||
* codes minted before this field was added — the token endpoint falls back
|
||||
* to ALL_SCOPES so existing Claude flows are unaffected.
|
||||
*/
|
||||
scopes?: string[]
|
||||
exp: number
|
||||
}
|
||||
|
||||
|
||||
@@ -104,7 +104,10 @@ describe('commitPendingOperation: create_voucher', () => {
|
||||
description: 'Capitalize Cursor subscription to 1010',
|
||||
source_type: 'manual',
|
||||
}),
|
||||
'mcp_create_voucher'
|
||||
// Default commit_method when opts.commitMethod is not passed.
|
||||
// Must be one of the values allowed by the DB CHECK constraint
|
||||
// (migration 20260420120001_journal_entry_commit_metadata.sql).
|
||||
'user_accept'
|
||||
)
|
||||
// findFiscalPeriod must NOT be called when fiscal_period_id is supplied —
|
||||
// it's the caller's explicit choice.
|
||||
@@ -218,7 +221,7 @@ describe('commitPendingOperation: create_voucher', () => {
|
||||
'company-1',
|
||||
'user-1',
|
||||
expect.objectContaining({ source_type: 'manual' }),
|
||||
'mcp_create_voucher'
|
||||
'user_accept'
|
||||
)
|
||||
expect(createJournalEntry).not.toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
@@ -229,6 +232,46 @@ describe('commitPendingOperation: create_voucher', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('passes bulk_accept commit_method when invoked from the bulk-commit path', async () => {
|
||||
// The bulk-commit route passes opts.commitMethod = 'bulk_accept' so the
|
||||
// resulting journal_entry rows are tagged correctly per BFNAR 2013:2
|
||||
// behandlingshistorik. Without this assertion, a regression that drops
|
||||
// opts on the way to the engine would silently book everything as
|
||||
// 'user_accept'.
|
||||
vi.mocked(createJournalEntry).mockResolvedValueOnce(
|
||||
makeJournalEntry({ id: 'je-bulk', voucher_number: 9 })
|
||||
)
|
||||
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim
|
||||
enqueue({ data: null, error: null }) // dispatcher's commit update
|
||||
|
||||
const op = makePendingOp({
|
||||
params: {
|
||||
entry_date: '2026-05-12',
|
||||
description: 'bulk-approved voucher',
|
||||
fiscal_period_id: 'fp-1',
|
||||
lines: [
|
||||
{ account_number: '1010', debit_amount: 100, credit_amount: 0 },
|
||||
{ account_number: '1930', debit_amount: 0, credit_amount: 100 },
|
||||
],
|
||||
},
|
||||
})
|
||||
|
||||
const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op, {
|
||||
commitMethod: 'bulk_accept',
|
||||
})
|
||||
|
||||
expect(result.status).toBe('committed')
|
||||
expect(createJournalEntry).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'company-1',
|
||||
'user-1',
|
||||
expect.anything(),
|
||||
'bulk_accept'
|
||||
)
|
||||
})
|
||||
|
||||
it('returns 400 with Swedish error when params are unbalanced (tamper defense)', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim
|
||||
|
||||
@@ -82,6 +82,16 @@ export interface CommitResult {
|
||||
export interface CommitOptions {
|
||||
/** Email address used as cc on send_invoice (typically the human user's email). */
|
||||
userEmail?: string
|
||||
/**
|
||||
* commit_method recorded on any journal_entries created by this operation.
|
||||
* Must match the CHECK constraint on journal_entries.commit_method:
|
||||
* 'user_accept' | 'bulk_accept' | 'timing_ceiling' | 'migration' | 'legacy'.
|
||||
* Single-approval route passes 'user_accept' (default); bulk-approval passes
|
||||
* 'bulk_accept'. Defaults to 'user_accept' since the dispatcher is only
|
||||
* invoked from human-approval paths after agent auto-commit was removed
|
||||
* (migration 20260505190027_drop_agent_auto_commit).
|
||||
*/
|
||||
commitMethod?: 'user_accept' | 'bulk_accept'
|
||||
}
|
||||
|
||||
// ── Helper: ensure fiscal period covers the date ──────────────────
|
||||
@@ -1628,7 +1638,8 @@ async function commitCreateVoucher(
|
||||
supabase: SupabaseClient,
|
||||
userId: string,
|
||||
companyId: string,
|
||||
params: Record<string, unknown>
|
||||
params: Record<string, unknown>,
|
||||
opts: CommitOptions = {}
|
||||
): Promise<ExecutorResult> {
|
||||
const entryDate = params.entry_date as string
|
||||
const description = params.description as string
|
||||
@@ -1681,7 +1692,11 @@ async function commitCreateVoucher(
|
||||
notes: (params.notes as string) || undefined,
|
||||
lines,
|
||||
},
|
||||
'mcp_create_voucher'
|
||||
// commit_method records HOW it was committed, not who staged it. MCP-
|
||||
// staged ops still go through human approval, so 'user_accept' (or
|
||||
// 'bulk_accept' from the bulk route) is the correct value. The DB CHECK
|
||||
// constraint rejects anything else (migration 20260420120001).
|
||||
opts.commitMethod ?? 'user_accept'
|
||||
)
|
||||
|
||||
return {
|
||||
@@ -1981,7 +1996,7 @@ export async function commitPendingOperation(
|
||||
result = await commitImportSie(supabase, userId, companyId, pendingOp.params)
|
||||
break
|
||||
case 'create_voucher':
|
||||
result = await commitCreateVoucher(supabase, userId, companyId, pendingOp.params)
|
||||
result = await commitCreateVoucher(supabase, userId, companyId, pendingOp.params, opts)
|
||||
break
|
||||
case 'correct_entry':
|
||||
result = await commitCorrectEntry(supabase, userId, companyId, pendingOp.params)
|
||||
|
||||
Reference in New Issue
Block a user