From 64c08ce961759eed9d479ea72b3d9a6a83cdca7e Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 6 Aug 2026 15:56:50 +0000 Subject: [PATCH] fix(security): fail closed on direct board-key mutations Co-Authored-By: Paperclip --- doc/BOARD-API-KEY-AUDIT-BOUNDARY.md | 67 +++++++ .../board-key-audit-atomicity.test.ts | 77 +++++++- .../src/security/board-key-audit-coupling.ts | 182 +++++++++++++++--- 3 files changed, 292 insertions(+), 34 deletions(-) create mode 100644 doc/BOARD-API-KEY-AUDIT-BOUNDARY.md diff --git a/doc/BOARD-API-KEY-AUDIT-BOUNDARY.md b/doc/BOARD-API-KEY-AUDIT-BOUNDARY.md new file mode 100644 index 0000000000..d7a4b19034 --- /dev/null +++ b/doc/BOARD-API-KEY-AUDIT-BOUNDARY.md @@ -0,0 +1,67 @@ +# Board API Key Audit Boundary + +Board API key allow decisions authorize a request only when the matching +security disposition is durable. PostgreSQL mutations and non-database effects +have different atomicity constraints, so this document states the boundary +explicitly. + +## PostgreSQL guarantee + +`server/src/security/board-key-audit-coupling.ts` stages an allow disposition in +the request context. + +- A mutation inside `db.transaction` writes the allow disposition on that + transaction before the first domain mutation. Both commit or both roll back. +- Direct `db.insert`, `db.update`, `db.delete`, and mutating `db.execute` calls + are replayed lazily inside a transaction with the staged disposition. An + audit insert failure therefore rolls back the direct domain mutation. +- Direct mutation-builder operations that cannot be replayed safely fail before + issuing SQL and must be moved into an explicit `db.transaction`. +- There is no successful untransacted fallback. Response settlement persists + only successful no-mutation dispositions; it cannot convert an uncoupled + mutation into success. + +This boundary deliberately classifies unknown raw SQL as mutating. Read-only +raw SQL is limited to statements beginning with `SELECT`, `SHOW`, `EXPLAIN`, +`TABLE`, or `VALUES`. + +## Non-database side-effect review + +The runtime route inventory denies undeclared board-key routes. Explicit denials +also cover the MCP surface, tool calls and sessions, plugin action/bridge/data +and webhook surfaces, skill-test execution, agent instruction-file mutation, +and auth/claim/invite surfaces. Board keys can still reach authorized management +routes whose handlers may combine PostgreSQL state with effects that PostgreSQL +cannot roll back. + +| Reachable route family | Effect outside PostgreSQL | Residual failure mode | +| --- | --- | --- | +| Artifacts, attachments, company import/export, skills | Object storage or local filesystem writes/removals | An audit or domain rollback can leave an orphaned object/materialized tree; compensation can also fail. | +| Agents, heartbeat/runtime, workspaces, environments | Process start/cancel, git/worktree changes, or provider calls | A crash can occur between durable intent and execution/acknowledgement, causing a missed action or a retry. | +| Plugin and tool management | Plugin lifecycle, OAuth/provider, or network activity | Remote state cannot join the database transaction and may be applied despite a later local failure. | +| Activity publication, plugin events, assignee wakeups | In-process publication or adapter dispatch | A post-commit crash can miss delivery; retry after an ambiguous acknowledgement can duplicate delivery. | + +The database audit coupling does not claim atomic rollback for these effects. +Side-effecting handlers must use the following pattern when the effect matters +to correctness or security: + +1. Persist the domain change, board-key allow disposition, and an intent/outbox + row in one transaction. +2. Execute the external effect only from the committed intent. +3. Give the effect a stable idempotency key derived from the intent, not from a + delivery attempt. +4. Persist completion or failure so a worker can retry ambiguous outcomes. +5. Use compensating cleanup for object/file/provider state and treat cleanup as + retryable, not guaranteed rollback. + +Existing durable wake requests and idempotency keys reduce duplicate/lost-work +risk where they are used. Live UI/plugin publications remain best effort. A +successful no-mutation request is audited during response settlement; if that +audit write fails there is no PostgreSQL domain commit to roll back. A handler +that performs only an external effect must therefore not rely on no-mutation +settlement: it needs a durable intent/outbox boundary before the effect. + +When a route becomes board-key reachable, security review must identify every +filesystem, object-store, process, adapter, plugin, and outbound-network effect +and either prove an existing outbox/idempotency boundary or record the residual +risk and remediation owner. diff --git a/server/src/security/board-key-audit-atomicity.test.ts b/server/src/security/board-key-audit-atomicity.test.ts index 393678c69a..f278e02e9d 100644 --- a/server/src/security/board-key-audit-atomicity.test.ts +++ b/server/src/security/board-key-audit-atomicity.test.ts @@ -215,7 +215,6 @@ describeEmbeddedPostgres("board-key allow audit / mutation atomicity", () => { }); await settleBoardKeyAuditContext(db, context, 200); - expect(context.untransacted).toBe(false); expect(await committedAudit(values.boardApiKeyId)).toEqual([ { decision: "allow", @@ -248,9 +247,9 @@ describeEmbeddedPostgres("board-key allow audit / mutation atomicity", () => { ]); }, 60_000); - it("marks a mutation that ran outside any transaction as uncoupled", async () => { + it("moves a direct mutation into the transaction that persists its allow disposition", async () => { const values = stagedAllow(); - const marker = `untransacted-${randomUUID()}`; + const marker = `direct-${randomUUID()}`; const context = createBoardKeyAuditContext(); await runWithBoardKeyAuditContext(context, async () => { @@ -259,17 +258,77 @@ describeEmbeddedPostgres("board-key allow audit / mutation atomicity", () => { }); await settleBoardKeyAuditContext(db, context, 201); - expect(context.untransacted).toBe(true); + expect(context.flushed).toBe(true); expect(await committedProbe(marker)).toHaveLength(1); expect(await committedAudit(values.boardApiKeyId)).toEqual([ { decision: "allow", reason: "authorized", - details: { coupling: BOARD_KEY_AUDIT_COUPLINGS.untransacted }, + details: { coupling: BOARD_KEY_AUDIT_COUPLINGS.transaction }, }, ]); }, 60_000); + it("rolls back a direct mutation when its coupled audit persistence fails", async () => { + const values = stagedAllow({ action: null as unknown as string }); + const marker = `direct-audit-failure-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await expect(runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.insert(probe).values({ value: marker }); + })).rejects.toThrow(); + + expect(context.flushed).toBe(false); + expect(await committedProbe(marker)).toHaveLength(0); + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + }, 60_000); + + it("couples direct update, delete, and mutating raw SQL entry points", async () => { + const updateFrom = `direct-update-from-${randomUUID()}`; + const updateTo = `direct-update-to-${randomUUID()}`; + const deleteMarker = `direct-delete-${randomUUID()}`; + await outside.insert(probe).values([ + { value: updateFrom }, + { value: deleteMarker }, + ]); + + const updateAudit = stagedAllow(); + const updateContext = createBoardKeyAuditContext(); + const [updated] = await runWithBoardKeyAuditContext(updateContext, async () => { + stageBoardKeyAllowAudit(updateAudit); + return await db + .update(probe) + .set({ value: updateTo }) + .where(eq(probe.value, updateFrom)) + .returning({ value: probe.value }); + }); + expect(updated).toEqual({ value: updateTo }); + expect(await committedAudit(updateAudit.boardApiKeyId)).toHaveLength(1); + + const deleteAudit = stagedAllow(); + const deleteContext = createBoardKeyAuditContext(); + const [deleted] = await runWithBoardKeyAuditContext(deleteContext, async () => { + stageBoardKeyAllowAudit(deleteAudit); + return await db + .delete(probe) + .where(eq(probe.value, deleteMarker)) + .returning({ value: probe.value }); + }); + expect(deleted).toEqual({ value: deleteMarker }); + expect(await committedAudit(deleteAudit.boardApiKeyId)).toHaveLength(1); + + const rawAudit = stagedAllow(); + const rawContext = createBoardKeyAuditContext(); + const rawMarker = `direct-raw-${randomUUID()}`; + await runWithBoardKeyAuditContext(rawContext, async () => { + stageBoardKeyAllowAudit(rawAudit); + await db.execute(sql`INSERT INTO board_key_atomicity_probe (value) VALUES (${rawMarker})`); + }); + expect(await committedProbe(rawMarker)).toHaveLength(1); + expect(await committedAudit(rawAudit.boardApiKeyId)).toHaveLength(1); + }, 60_000); + describe("through the live request pipeline", () => { const ownerId = randomUUID(); @@ -338,14 +397,12 @@ describeEmbeddedPostgres("board-key allow audit / mutation atomicity", () => { expect(await committedAudit(keyId)).toHaveLength(0); }, 60_000); - it("refuses to commit the mutation when the coupled audit write fails", async () => { + it("refuses to commit a direct mutation when the coupled audit write fails", async () => { const marker = `pipeline-audit-failure-${randomUUID()}`; // `board_api_key_id` is a uuid column, so this key identity makes the - // coupled audit insert fail inside the controller's own transaction. + // coupled audit insert fail inside the direct mutation's boundary. const app = createApp("not-a-uuid", async (_req, res) => { - await db.transaction(async (tx) => { - await tx.insert(probe).values({ value: marker }); - }); + await db.insert(probe).values({ value: marker }); res.status(201).json({ ok: true }); }); diff --git a/server/src/security/board-key-audit-coupling.ts b/server/src/security/board-key-audit-coupling.ts index f4fb835d84..2354adb052 100644 --- a/server/src/security/board-key-audit-coupling.ts +++ b/server/src/security/board-key-audit-coupling.ts @@ -32,8 +32,6 @@ export const BOARD_KEY_AUDIT_COUPLINGS = { transaction: "transaction", /** The authorized request completed without mutating anything. */ noMutation: "no_mutation", - /** A mutation ran outside any transaction, so it could not be coupled. */ - untransacted: "untransacted", /** No request context was active (direct gate invocation, not a live route). */ detached: "detached", } as const; @@ -48,8 +46,6 @@ export type BoardKeyAuditContext = { flushed: boolean; /** A domain mutation was issued at least once during this request. */ mutationAttempted: boolean; - /** A domain mutation was issued outside any transaction. */ - untransacted: boolean; /** Settlement already ran; further mutation marks are gate-owned writes. */ settled: boolean; }; @@ -59,8 +55,30 @@ const store = new AsyncLocalStorage(); // Mutating entry points on both the database handle and a transaction client. // `execute` covers raw SQL, which is also how several services mutate. const MUTATION_METHODS = ["insert", "update", "delete", "execute"] as const; +type MutationMethod = (typeof MUTATION_METHODS)[number]; const INSTRUMENTED = Symbol.for("paperclip.boardKeyAuditInstrumented"); +// Direct Drizzle mutation builders are lazy. Record only the fluent methods we +// know how to replay on a transaction client, then execute the completed query +// inside the same transaction as its allow disposition. Unknown terminals such +// as `prepare` fail before any SQL is issued instead of escaping the boundary. +const DIRECT_MUTATION_FLUENT_METHODS = new Set([ + "values", + "select", + "overridingSystemValue", + "onConflictDoNothing", + "onConflictDoUpdate", + "set", + "from", + "leftJoin", + "rightJoin", + "innerJoin", + "fullJoin", + "where", + "returning", + "$dynamic", +]); + // Raw SQL is the one entry point that can be read-only, so classify it instead // of reporting every `execute` as a mutation. Anything unrecognised counts as a // mutation: over-reporting keeps a disposition attached, under-reporting loses @@ -87,7 +105,6 @@ export function createBoardKeyAuditContext(): BoardKeyAuditContext { pending: null, flushed: false, mutationAttempted: false, - untransacted: false, settled: false, }; } @@ -112,25 +129,29 @@ export function stageBoardKeyAllowAudit(values: BoardKeyAuditValues): boolean { return true; } -function markMutation(context: BoardKeyAuditContext | undefined, transactional: boolean) { +function markMutation(context: BoardKeyAuditContext | undefined) { // Only requests carrying an uncoupled allow disposition need tracking, and // gate-owned settlement writes must never count as domain mutations. if (!context || !context.pending || context.settled) return; context.mutationAttempted = true; - if (!transactional) context.untransacted = true; } type MutatingClient = Record & { transaction?: (fn: (tx: unknown) => unknown, config?: unknown) => unknown; }; +type MutationInterception = { value: unknown }; + /** * Replace a client's mutating methods with hooked versions. The replacements are * non-enumerable so the client keeps the shape callers and drizzle expect. */ function shadowMutationMethods( client: object, - onMutation: (method: string, args: readonly unknown[]) => void, + onMutation: ( + method: MutationMethod, + args: readonly unknown[], + ) => MutationInterception | undefined, ) { const target = client as MutatingClient; for (const method of MUTATION_METHODS) { @@ -142,7 +163,9 @@ function shadowMutationMethods( enumerable: false, writable: true, value: (...args: unknown[]) => { - if (method !== "execute" || rawStatementMutates(args)) onMutation(method, args); + if (method === "execute" && !rawStatementMutates(args)) return bound(...args); + const intercepted = onMutation(method, args); + if (intercepted) return intercepted.value; return bound(...args); }, }); @@ -166,7 +189,10 @@ function instrumentTransactionClient( client: T, onMutation: () => void, ): T { - shadowMutationMethods(client, onMutation); + shadowMutationMethods(client, () => { + onMutation(); + return undefined; + }); const target = client as unknown as MutatingClient; const nested = target.transaction; if (typeof nested === "function") { @@ -193,11 +219,30 @@ export function instrumentDbForBoardKeyAudit(db: T): T { if (target[INSTRUMENTED] === true) return db; Object.defineProperty(target, INSTRUMENTED, { value: true, enumerable: false }); - shadowMutationMethods(db, () => markMutation(store.getStore(), false)); - const originalTransaction = target.transaction; if (typeof originalTransaction === "function") { const boundTransaction = originalTransaction.bind(db); + + // A direct mutation is otherwise committed before response settlement can + // persist its allow disposition. Lazily replay it inside a transaction so + // audit failure aborts the domain write instead of producing an unaudited + // success. Every direct mutation gets this boundary, including later ones + // in the same request after an earlier audit row has committed. + shadowMutationMethods(db, (method, args) => { + const context = store.getStore(); + const pending = context?.pending; + if (!context || !pending || context.settled) return undefined; + return { + value: createCoupledDirectMutation( + boundTransaction, + method, + args, + context, + pending, + ), + }; + }); + Object.defineProperty(target, "transaction", { configurable: true, enumerable: false, @@ -214,6 +259,104 @@ export function instrumentDbForBoardKeyAudit(db: T): T { return db; } +type RecordedMutationCall = { + property: PropertyKey; + args: readonly unknown[]; +}; + +/** + * Preserve Drizzle's lazy fluent mutation API while moving execution onto a + * transaction client. Merely constructing a query neither mutates nor audits; + * `await`/`then`/`execute` starts the coupled transaction exactly once. + */ +function createCoupledDirectMutation( + boundTransaction: (fn: (tx: unknown) => unknown, config?: unknown) => unknown, + method: MutationMethod, + initialArgs: readonly unknown[], + context: BoardKeyAuditContext, + pending: BoardKeyAuditValues, +): unknown { + if (method === "execute") { + return runCoupledTransaction( + boundTransaction, + (tx) => { + const execute = (tx as MutatingClient).execute; + if (typeof execute !== "function") { + throw new Error("Transaction client does not implement db.execute"); + } + return execute.apply(tx, initialArgs); + }, + undefined, + context, + pending, + ); + } + + const calls: RecordedMutationCall[] = []; + let execution: Promise | null = null; + let proxy: object; + + const run = (executeArgs?: readonly unknown[]) => { + if (!execution) { + execution = runCoupledTransaction( + boundTransaction, + async (tx) => { + const mutation = (tx as MutatingClient)[method]; + if (typeof mutation !== "function") { + throw new Error(`Transaction client does not implement db.${method}`); + } + let query = mutation.apply(tx, initialArgs); + for (const call of calls) { + const next = Reflect.get(query as object, call.property); + if (typeof next !== "function") { + throw new Error( + `Board-key direct db.${method} mutation cannot replay ${String(call.property)}`, + ); + } + query = next.apply(query, call.args); + } + if (executeArgs) { + const execute = Reflect.get(query as object, "execute"); + if (typeof execute !== "function") { + throw new Error(`Board-key direct db.${method} mutation is not executable`); + } + return await execute.apply(query, executeArgs); + } + return await query; + }, + undefined, + context, + pending, + ); + } + return execution; + }; + + proxy = new Proxy(Object.create(null) as object, { + get(_target, property) { + if (property === "then" || property === "catch" || property === "finally") { + const promise = run(); + return promise[property].bind(promise); + } + if (property === "execute") return (...args: unknown[]) => run(args); + if (property === Symbol.toStringTag) return "Promise"; + if (!DIRECT_MUTATION_FLUENT_METHODS.has(property)) { + throw new Error( + `Board-key direct db.${method} mutation cannot use ${String(property)} outside db.transaction`, + ); + } + return (...args: unknown[]) => { + if (execution) { + throw new Error(`Board-key direct db.${method} mutation was already executed`); + } + calls.push({ property, args }); + return proxy; + }; + }, + }); + return proxy; +} + async function runCoupledTransaction( boundTransaction: (fn: (tx: unknown) => unknown, config?: unknown) => unknown, fn: (tx: unknown) => unknown, @@ -231,7 +374,7 @@ async function runCoupledTransaction( }; }).insert.bind(tx); const instrumented = instrumentTransactionClient(tx as object, () => { - markMutation(context, true); + markMutation(context); if (flush) return; const query = rawInsert(boardApiKeyAuthorizationEvents).values({ ...pending, @@ -275,21 +418,12 @@ export async function settleBoardKeyAuditContext( const succeeded = statusCode >= 200 && statusCode < 400; if (!succeeded) return; - if (context.mutationAttempted && !context.untransacted) return; + if (context.mutationAttempted) return; - const coupling = context.untransacted - ? BOARD_KEY_AUDIT_COUPLINGS.untransacted - : BOARD_KEY_AUDIT_COUPLINGS.noMutation; - if (context.untransacted) { - logger.error( - { boardApiKeyId: pending.boardApiKeyId, action: pending.action }, - "Board-key mutation ran outside a transaction; allow disposition could not be coupled atomically", - ); - } try { await db.insert(boardApiKeyAuthorizationEvents).values({ ...pending, - details: { ...(pending.details ?? {}), coupling }, + details: { ...(pending.details ?? {}), coupling: BOARD_KEY_AUDIT_COUPLINGS.noMutation }, }); } catch (err) { logger.error(