From 8cb500359e41fe9fa93109285fa108dbcb84e3bc Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 6 Aug 2026 15:04:58 +0000 Subject: [PATCH] fix(security): commit board-key allow audit with its mutation Stage the allow disposition in a request-scoped context and flush it into the first transaction that performs a domain mutation, so an authorized mutation and its audit record commit or roll back together. Denials and read-only allows stay immediately durable, and a mutation that runs outside any transaction is recorded and logged as uncoupled. Co-Authored-By: Paperclip --- server/src/app.ts | 6 + .../board-key-audit-atomicity.test.ts | 358 ++++++++++++++++++ .../src/security/board-key-audit-coupling.ts | 300 +++++++++++++++ .../security/board-key-authorization.test.ts | 49 ++- .../src/security/board-key-route-registry.ts | 65 +++- 5 files changed, 753 insertions(+), 25 deletions(-) create mode 100644 server/src/security/board-key-audit-atomicity.test.ts create mode 100644 server/src/security/board-key-audit-coupling.ts diff --git a/server/src/app.ts b/server/src/app.ts index 23f3bf3a64..3c2db2ec51 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -174,6 +174,7 @@ import { apiCompression } from "./middleware/api-compression.js"; import { chatWebhookBodyParser } from "./middleware/chat-webhook-body.js"; import { createChatWebhookDiagnostics } from "./services/chat-webhook-diagnostics.js"; import { boardKeyAuthorizationMiddleware } from "./security/board-key-route-registry.js"; +import { instrumentDbForBoardKeyAudit } from "./security/board-key-audit-coupling.js"; type UiMode = "none" | "static" | "vite-dev"; const FEEDBACK_EXPORT_FLUSH_INTERVAL_MS = 5_000; @@ -564,6 +565,11 @@ export async function createApp( // REPLACES whatever actor the request otherwise resolved to, and only on // the one endpoint it authorizes (see the middleware for the contract). app.use(cloudControlMiddleware()); + // In-place, idempotent instrumentation: the board-key gate stages its allow + // disposition and the handle commits it inside the transaction of the + // mutation it authorizes. Instrumenting the handle itself keeps object + // identity, so `dbOrTx === db` checks in services are unaffected. + instrumentDbForBoardKeyAudit(db); app.use(boardKeyAuthorizationMiddleware(db)); app.use("/api/auth", authRoutes(db)); if (opts.betterAuthHandler) { diff --git a/server/src/security/board-key-audit-atomicity.test.ts b/server/src/security/board-key-audit-atomicity.test.ts new file mode 100644 index 0000000000..393678c69a --- /dev/null +++ b/server/src/security/board-key-audit-atomicity.test.ts @@ -0,0 +1,358 @@ +import { randomUUID } from "node:crypto"; +import express from "express"; +import request from "supertest"; +import { eq, sql } from "drizzle-orm"; +import { pgTable, text, uuid } from "drizzle-orm/pg-core"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + boardApiKeyAuthorizationEvents, + createDb, + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, + type Db, + type EmbeddedPostgresTestDatabase, +} from "@paperclipai/db"; +import { errorHandler } from "../middleware/error-handler.js"; +import { + BOARD_KEY_AUDIT_COUPLINGS, + createBoardKeyAuditContext, + instrumentDbForBoardKeyAudit, + runWithBoardKeyAuditContext, + settleBoardKeyAuditContext, + stageBoardKeyAllowAudit, + type BoardKeyAuditValues, +} from "./board-key-audit-coupling.js"; +import { boardKeyAuthorizationMiddleware } from "./board-key-route-registry.js"; + +/** + * Stand-in for any board-key-protected domain table. Using a dedicated table + * keeps the regression focused on the audit/mutation boundary itself instead of + * the constraints of whichever real table a route happens to write. + */ +const probe = pgTable("board_key_atomicity_probe", { + id: uuid("id").primaryKey().defaultRandom(), + value: text("value").notNull(), +}); + +const support = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = support.supported ? describe : describe.skip; + +describeEmbeddedPostgres("board-key allow audit / mutation atomicity", () => { + let database: EmbeddedPostgresTestDatabase; + let db: Db; + // Independent connection pool: reads through it can only observe committed + // rows, which is how the test proves the audit row lives inside the domain + // transaction rather than in a separate one. + let outside: Db; + + beforeAll(async () => { + database = await startEmbeddedPostgresTestDatabase("paperclip-board-key-atomicity-"); + db = instrumentDbForBoardKeyAudit(createDb(database.connectionString)); + outside = createDb(database.connectionString); + await outside.execute(sql` + CREATE TABLE IF NOT EXISTS board_key_atomicity_probe ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + value text NOT NULL + ) + `); + }, 240_000); + + afterAll(async () => { + await database?.cleanup(); + }); + + function stagedAllow(overrides: Partial = {}): BoardKeyAuditValues { + return { + boardApiKeyId: randomUUID(), + ownerUserId: randomUUID(), + tokenPrefix: "pcp_board_atomic", + action: "issues:write", + classification: "company", + authoritativeCompanyId: null, + authoritativeResourceType: "company", + authoritativeResourceId: null, + decision: "allow", + reason: "authorized", + requestId: randomUUID(), + runId: null, + details: {}, + ...overrides, + }; + } + + async function committedAudit(boardApiKeyId: string) { + return await outside + .select({ + decision: boardApiKeyAuthorizationEvents.decision, + reason: boardApiKeyAuthorizationEvents.reason, + details: boardApiKeyAuthorizationEvents.details, + }) + .from(boardApiKeyAuthorizationEvents) + .where(eq(boardApiKeyAuthorizationEvents.boardApiKeyId, boardApiKeyId)); + } + + async function committedProbe(value: string) { + return await outside.select({ value: probe.value }).from(probe).where(eq(probe.value, value)); + } + + it("commits the allow disposition inside the transaction of the mutation it authorizes", async () => { + const values = stagedAllow(); + const marker = `commit-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + // Staging alone must never be durable: that is the non-atomic behaviour + // this coupling replaces. + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + await db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + // Still inside the transaction: neither the mutation nor its + // disposition is visible to another connection yet. + expect(await committedProbe(marker)).toHaveLength(0); + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + }); + }); + + 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.transaction }, + }, + ]); + }, 60_000); + + it("rolls the authorized mutation back when the audit write fails at the boundary", async () => { + // Forced failure exactly at the audit/mutation boundary: `action` is NOT + // NULL, so the coupled audit insert aborts the domain transaction. + const values = stagedAllow({ action: null as unknown as string }); + const marker = `audit-failure-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await expect(runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.transaction(async (tx) => { + await tx.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); + + // Settlement must not paper over the failure with a durable allow either. + await settleBoardKeyAuditContext(db, context, 500); + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + }, 60_000); + + it("keeps no durable allow disposition when the protected mutation rolls back", async () => { + const values = stagedAllow(); + const marker = `rollback-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await expect(runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + throw new Error("controller failed after mutating"); + }); + })).rejects.toThrow("controller failed after mutating"); + + expect(context.flushed).toBe(false); + expect(await committedProbe(marker)).toHaveLength(0); + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + }, 60_000); + + it("does not resurrect an allow disposition when a rolled-back mutation is swallowed", async () => { + const values = stagedAllow(); + const marker = `swallowed-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await expect(db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + throw new Error("swallowed"); + })).rejects.toThrow("swallowed"); + }); + + // The handler reports success, but nothing committed, so there is no + // mutation for an `authorized` record to describe. + await settleBoardKeyAuditContext(db, context, 200); + expect(await committedProbe(marker)).toHaveLength(0); + expect(await committedAudit(values.boardApiKeyId)).toHaveLength(0); + }, 60_000); + + it("records the disposition of an authorized request that mutates nothing", async () => { + const values = stagedAllow(); + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.select({ value: probe.value }).from(probe).limit(1); + }); + await settleBoardKeyAuditContext(db, context, 204); + + expect(await committedAudit(values.boardApiKeyId)).toEqual([ + { + decision: "allow", + reason: "authorized", + details: { coupling: BOARD_KEY_AUDIT_COUPLINGS.noMutation }, + }, + ]); + }, 60_000); + + it("does not treat a read-only raw statement as an uncoupled mutation", async () => { + const values = stagedAllow(); + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.execute(sql`SELECT 1`); + }); + await settleBoardKeyAuditContext(db, context, 200); + + expect(context.untransacted).toBe(false); + expect(await committedAudit(values.boardApiKeyId)).toEqual([ + { + decision: "allow", + reason: "authorized", + details: { coupling: BOARD_KEY_AUDIT_COUPLINGS.noMutation }, + }, + ]); + }, 60_000); + + it("couples a raw mutating statement issued inside a transaction", async () => { + const values = stagedAllow(); + const marker = `raw-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.transaction(async (tx) => { + await tx.execute(sql`INSERT INTO board_key_atomicity_probe (value) VALUES (${marker})`); + }); + }); + + 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.transaction }, + }, + ]); + }, 60_000); + + it("marks a mutation that ran outside any transaction as uncoupled", async () => { + const values = stagedAllow(); + const marker = `untransacted-${randomUUID()}`; + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, async () => { + stageBoardKeyAllowAudit(values); + await db.insert(probe).values({ value: marker }); + }); + await settleBoardKeyAuditContext(db, context, 201); + + expect(context.untransacted).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 }, + }, + ]); + }, 60_000); + + describe("through the live request pipeline", () => { + const ownerId = randomUUID(); + + function createApp(keyId: string, handler: express.RequestHandler) { + const app = express(); + app.use((req, _res, next) => { + (req as unknown as { id: string }).id = randomUUID(); + (req as unknown as { actor: Record }).actor = { + type: "board", + source: "board_key", + userId: ownerId, + keyId, + boardKeyOwnerId: ownerId, + boardKeyPrefix: "pcp_board_atomic", + boardKeyScope: null, + boardKeyLegacyUnrestricted: true, + isInstanceAdmin: true, + companyIds: [], + memberships: [], + }; + next(); + }); + app.use(boardKeyAuthorizationMiddleware(db)); + app.post("/api/companies", handler); + app.use(errorHandler); + return app; + } + + it("couples the gate decision to a controller transaction across the middleware chain", async () => { + const marker = `pipeline-commit-${randomUUID()}`; + const keyId = randomUUID(); + const app = createApp(keyId, async (_req, res) => { + await db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + }); + res.status(201).json({ ok: true }); + }); + + const response = await request(app).post("/api/companies").send({}); + expect(response.status).toBe(201); + expect(await committedProbe(marker)).toHaveLength(1); + expect(await committedAudit(keyId)).toEqual([ + { + decision: "allow", + reason: "authorized", + details: { coupling: BOARD_KEY_AUDIT_COUPLINGS.transaction }, + }, + ]); + }, 60_000); + + it("leaves no allow disposition when the controller transaction fails", async () => { + const marker = `pipeline-rollback-${randomUUID()}`; + const keyId = randomUUID(); + const app = createApp(keyId, async (_req, _res) => { + await db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + throw new Error("controller failed"); + }); + }); + + const response = await request(app).post("/api/companies").send({}); + expect(response.status).toBe(500); + // Give the response-completion settlement a chance to run. + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(await committedProbe(marker)).toHaveLength(0); + expect(await committedAudit(keyId)).toHaveLength(0); + }, 60_000); + + it("refuses to commit the 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. + const app = createApp("not-a-uuid", async (_req, res) => { + await db.transaction(async (tx) => { + await tx.insert(probe).values({ value: marker }); + }); + res.status(201).json({ ok: true }); + }); + + const response = await request(app).post("/api/companies").send({}); + expect(response.status).toBe(500); + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(await committedProbe(marker)).toHaveLength(0); + }, 60_000); + }); +}); diff --git a/server/src/security/board-key-audit-coupling.ts b/server/src/security/board-key-audit-coupling.ts new file mode 100644 index 0000000000..f4fb835d84 --- /dev/null +++ b/server/src/security/board-key-audit-coupling.ts @@ -0,0 +1,300 @@ +import { AsyncLocalStorage } from "node:async_hooks"; +import { boardApiKeyAuthorizationEvents, type Db } from "@paperclipai/db"; +import { logger } from "../middleware/logger.js"; + +/** + * Board-key allow dispositions must be atomic with the mutation they authorize. + * + * The central gate runs as request middleware, so it cannot see the transaction + * a controller will open later. Writing the allow row in the middleware leaves + * an `authorized` record behind whenever the controller rolls back or the + * process dies mid-request, and gives a committed mutation no durable + * disposition when the audit insert itself fails after the mutation. + * + * Instead the gate *stages* the allow disposition in a request-scoped context + * and the instrumented database handle flushes it into the first transaction + * that performs a domain mutation. From that point the disposition and the + * mutation share one transaction: they commit together or roll back together. + * + * Denials are unaffected — they abort the request, so there is no mutation to + * couple them to and they stay immediately durable. + */ + +export type BoardKeyAuditValues = typeof boardApiKeyAuthorizationEvents.$inferInsert; + +/** + * Fixed allowlist for the `details.coupling` audit field. The value records how + * the durable allow disposition was tied to the request's effects so incident + * reconstruction can tell an atomic record from a degraded one. + */ +export const BOARD_KEY_AUDIT_COUPLINGS = { + /** Written inside the same transaction as the first domain mutation. */ + 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; + +export type BoardKeyAuditCoupling = + (typeof BOARD_KEY_AUDIT_COUPLINGS)[keyof typeof BOARD_KEY_AUDIT_COUPLINGS]; + +export type BoardKeyAuditContext = { + /** Allow disposition awaiting a domain transaction to commit with. */ + pending: BoardKeyAuditValues | null; + /** True once the staged disposition committed inside a domain transaction. */ + 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; +}; + +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; +const INSTRUMENTED = Symbol.for("paperclip.boardKeyAuditInstrumented"); + +// 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 +// one. +const READ_ONLY_STATEMENT = /^\s*(?:select|show|explain|table|values)\b/i; + +function rawStatementMutates(args: readonly unknown[]): boolean { + const chunks = (args[0] as { queryChunks?: unknown[] } | null | undefined)?.queryChunks; + if (!Array.isArray(chunks)) return true; + let text = ""; + for (const chunk of chunks) { + if (typeof chunk === "string") text += chunk; + else { + const parts = (chunk as { value?: unknown } | null)?.value; + if (Array.isArray(parts)) text += parts.join(""); + } + if (text.trim().length > 0) break; + } + return text.trim().length === 0 ? true : !READ_ONLY_STATEMENT.test(text); +} + +export function createBoardKeyAuditContext(): BoardKeyAuditContext { + return { + pending: null, + flushed: false, + mutationAttempted: false, + untransacted: false, + settled: false, + }; +} + +export function runWithBoardKeyAuditContext( + context: BoardKeyAuditContext, + run: () => T, +): T { + return store.run(context, run); +} + +/** + * Stage an allow disposition for transactional coupling. Returns false when no + * request context is active, in which case the caller must fall back to an + * immediate insert so the disposition is never silently dropped. + */ +export function stageBoardKeyAllowAudit(values: BoardKeyAuditValues): boolean { + const context = store.getStore(); + if (!context) return false; + context.pending = values; + context.flushed = false; + return true; +} + +function markMutation(context: BoardKeyAuditContext | undefined, transactional: boolean) { + // 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; +}; + +/** + * 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, +) { + const target = client as MutatingClient; + for (const method of MUTATION_METHODS) { + const original = target[method]; + if (typeof original !== "function") continue; + const bound = (original as (...args: unknown[]) => unknown).bind(client); + Object.defineProperty(target, method, { + configurable: true, + enumerable: false, + writable: true, + value: (...args: unknown[]) => { + if (method !== "execute" || rawStatementMutates(args)) onMutation(method, args); + return bound(...args); + }, + }); + } +} + +/** + * Shadow the mutating methods of a transaction client so the staged allow + * disposition is dispatched on the same connection immediately before the + * first domain mutation. The client is created per transaction and discarded + * afterwards, so in-place instrumentation stays scoped to this request. + * + * Nested transactions (savepoints) share the outermost client's flush, so a + * savepoint never needs a disposition of its own. Residual: a mutation issued + * only inside a savepoint dispatches the disposition within that savepoint, so + * rolling the savepoint back while the outer transaction commits would drop it. + * Nothing in the server opens nested transactions today; a caller that starts + * doing so must flush at the outer level instead. + */ +function instrumentTransactionClient( + client: T, + onMutation: () => void, +): T { + shadowMutationMethods(client, onMutation); + const target = client as unknown as MutatingClient; + const nested = target.transaction; + if (typeof nested === "function") { + const boundNested = nested.bind(client); + Object.defineProperty(target, "transaction", { + configurable: true, + enumerable: false, + writable: true, + value: (fn: (tx: unknown) => unknown, config?: unknown) => + boundNested((inner: unknown) => fn(instrumentTransactionClient(inner as object, onMutation)), config), + }); + } + return client; +} + +/** + * Shadow `transaction` and the direct mutating methods on the process-wide + * database handle. The handle object is returned unchanged (same identity, so + * existing `dbOrTx === db` checks keep working) and every hook is inert unless + * a board-key request has staged an allow disposition. + */ +export function instrumentDbForBoardKeyAudit(db: T): T { + const target = db as unknown as MutatingClient; + 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); + Object.defineProperty(target, "transaction", { + configurable: true, + enumerable: false, + writable: true, + value: (fn: (tx: unknown) => unknown, config?: unknown) => { + const context = store.getStore(); + const pending = context && context.pending && !context.flushed ? context.pending : null; + if (!context || !pending) return boundTransaction(fn, config); + return runCoupledTransaction(boundTransaction, fn, config, context, pending); + }, + }); + } + + return db; +} + +async function runCoupledTransaction( + boundTransaction: (fn: (tx: unknown) => unknown, config?: unknown) => unknown, + fn: (tx: unknown) => unknown, + config: unknown, + context: BoardKeyAuditContext, + pending: BoardKeyAuditValues, +) { + let flush: Promise | null = null; + const result = await boundTransaction(async (tx: unknown) => { + // Captured before instrumentation so the audit insert does not re-enter the + // mutation hook it is triggered from. + const rawInsert = (tx as { + insert: (table: unknown) => { + values: (values: unknown) => { execute?: () => Promise } & PromiseLike; + }; + }).insert.bind(tx); + const instrumented = instrumentTransactionClient(tx as object, () => { + markMutation(context, true); + if (flush) return; + const query = rawInsert(boardApiKeyAuthorizationEvents).values({ + ...pending, + details: { ...(pending.details ?? {}), coupling: BOARD_KEY_AUDIT_COUPLINGS.transaction }, + }); + // Dispatched, not awaited, so the audit statement is queued on this + // transaction's connection ahead of the mutation that triggered it. + flush = typeof query.execute === "function" ? query.execute() : Promise.resolve(query); + // The transaction wrapper awaits `flush` before committing; this guard + // only keeps a rejection from surfacing as an unhandled rejection first. + flush.catch(() => {}); + }); + const value = await fn(instrumented); + // A failed audit insert aborts the transaction, so the mutation cannot + // commit without its disposition. + if (flush) await flush; + return value; + }, config); + if (flush) context.flushed = true; + return result; +} + +/** + * Resolve the request's allow disposition once the response is complete. + * + * Nothing is written when a transactional mutation rolled back or the request + * failed: there is no committed effect to attest, and a durable `allow` in that + * situation is exactly the false association this coupling exists to prevent. A + * durable allow therefore means "this mutation was authorized and committed"; + * denials remain logged in full regardless of outcome. + */ +export async function settleBoardKeyAuditContext( + db: Db, + context: BoardKeyAuditContext, + statusCode: number, +): Promise { + if (context.settled) return; + context.settled = true; + const pending = context.pending; + if (!pending || context.flushed) return; + + const succeeded = statusCode >= 200 && statusCode < 400; + if (!succeeded) return; + if (context.mutationAttempted && !context.untransacted) 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 }, + }); + } catch (err) { + logger.error( + { err, boardApiKeyId: pending.boardApiKeyId, action: pending.action }, + "Failed to persist settled board-key allow disposition", + ); + } +} diff --git a/server/src/security/board-key-authorization.test.ts b/server/src/security/board-key-authorization.test.ts index 60d04176da..8fad84fb5a 100644 --- a/server/src/security/board-key-authorization.test.ts +++ b/server/src/security/board-key-authorization.test.ts @@ -12,6 +12,10 @@ import { import { BOARD_API_KEY_SCOPE_PRESETS } from "@paperclipai/shared"; import { HttpError } from "../errors.js"; import { boardAuthService, hashBearerToken } from "../services/board-auth.js"; +import { + createBoardKeyAuditContext, + runWithBoardKeyAuditContext, +} from "./board-key-audit-coupling.js"; import { authorizeBoardKey, lookupBoardKeyRoute } from "./board-key-route-registry.js"; const TOKEN = "pcp_board_authorization_matrix"; @@ -92,7 +96,10 @@ function createLiveAuthorityDb() { return { db, state, key, companyId, ownerId, audit }; } -function requestFor(authentication: Awaited["authenticateBoardApiKey"]>>) { +function requestFor( + authentication: Awaited["authenticateBoardApiKey"]>>, + method = "POST", +) { if (!authentication.ok) throw new Error("Expected successful board-key authentication"); const { key, scopeConfig, access } = authentication; const companyIds = scopeConfig @@ -100,6 +107,7 @@ function requestFor(authentication: Awaited, status: number) { } describe("board-key effective authority", () => { - it("audits an allow before the protected side effect runs", async () => { + it("stages a mutating allow for its transaction instead of auditing ahead of the mutation", async () => { const { db, companyId, audit } = createLiveAuthorityDb(); const authentication = await boardAuthService(db).authenticateBoardApiKey(TOKEN); const req = requestFor(authentication); const metadata = lookupBoardKeyRoute("POST", `/api/companies/${companyId}/issues`); - const sideEffect = vi.fn(); + const context = createBoardKeyAuditContext(); - await authorizeBoardKey( + await runWithBoardKeyAuditContext(context, () => authorizeBoardKey( db, req, metadata.action, async () => ({ companyId, resourceType: "company", resourceId: companyId }), metadata, - ); - expect(audit.at(-1)).toMatchObject({ decision: "allow", action: "issues:write" }); - sideEffect(); - expect(sideEffect).toHaveBeenCalledOnce(); + )); + + // Nothing durable yet: the disposition commits with the mutation it + // authorizes, so a rolled-back controller leaves no `authorized` record. + expect(audit).toHaveLength(0); + expect(context.pending).toMatchObject({ decision: "allow", action: "issues:write" }); }); - it("fails closed before side effects when the allow audit cannot be persisted", async () => { + it("audits a read-only allow immediately because it has no mutation to couple to", async () => { + const { db, companyId, audit } = createLiveAuthorityDb(); + const authentication = await boardAuthService(db).authenticateBoardApiKey(TOKEN); + const req = requestFor(authentication, "GET"); + const metadata = lookupBoardKeyRoute("GET", `/api/companies/${companyId}/issues`); + const context = createBoardKeyAuditContext(); + + await runWithBoardKeyAuditContext(context, () => authorizeBoardKey( + db, + req, + metadata.action, + async () => ({ companyId, resourceType: "company", resourceId: companyId }), + metadata, + )); + + expect(context.pending).toBeNull(); + expect(audit.at(-1)).toMatchObject({ decision: "allow", action: "issues:read" }); + }); + + it("fails closed before side effects when a detached allow audit cannot be persisted", async () => { const { db, companyId } = createLiveAuthorityDb(); const authentication = await boardAuthService(db).authenticateBoardApiKey(TOKEN); const req = requestFor(authentication); @@ -148,6 +177,8 @@ describe("board-key effective authority", () => { db.insert = vi.fn(() => ({ values: vi.fn().mockRejectedValue(new Error("audit unavailable")) })); const sideEffect = vi.fn(); + // No request context, so the gate cannot couple the disposition to a + // transaction and must persist it before the caller proceeds. try { await authorizeBoardKey( db, diff --git a/server/src/security/board-key-route-registry.ts b/server/src/security/board-key-route-registry.ts index d8759ba64d..a2fd259a21 100644 --- a/server/src/security/board-key-route-registry.ts +++ b/server/src/security/board-key-route-registry.ts @@ -38,6 +38,13 @@ import { import { isUuidLike, type BoardPermissionKey, type PermissionKey } from "@paperclipai/shared"; import { HttpError, forbidden, notFound } from "../errors.js"; import { logger } from "../middleware/logger.js"; +import { + BOARD_KEY_AUDIT_COUPLINGS, + createBoardKeyAuditContext, + runWithBoardKeyAuditContext, + settleBoardKeyAuditContext, + stageBoardKeyAllowAudit, +} from "./board-key-audit-coupling.js"; export type BoardKeyRouteClassification = | "company" @@ -623,7 +630,7 @@ async function auditDecision( reason: string, ) { if (req.actor.source !== "board_key" || !req.actor.keyId || !req.actor.boardKeyOwnerId) return; - await db.insert(boardApiKeyAuthorizationEvents).values({ + const values = { boardApiKeyId: req.actor.keyId, ownerUserId: req.actor.boardKeyOwnerId, tokenPrefix: req.actor.boardKeyPrefix ?? null, @@ -636,8 +643,21 @@ async function auditDecision( reason, requestId: typeof req.id === "string" ? req.id : null, runId: isUuidLike(req.actor.runId) ? req.actor.runId : null, - details: {}, - }); + details: {} as Record, + }; + // An allow on an unsafe method authorizes a mutation, so its disposition is + // staged and committed inside that mutation's transaction. Denials abort the + // request and safe methods have no mutation to couple to, so both stay + // immediately durable. + if (decision === "allow" && !SAFE_METHODS.has(req.method) && stageBoardKeyAllowAudit(values)) return; + if (decision === "allow" && !SAFE_METHODS.has(req.method)) { + logger.warn( + { boardApiKeyId: req.actor.keyId, action: metadata.action }, + "Board-key allow audited without a request context; disposition is not transactionally coupled", + ); + values.details = { coupling: BOARD_KEY_AUDIT_COUPLINGS.detached }; + } + await db.insert(boardApiKeyAuthorizationEvents).values(values); } async function denyBoardKey( @@ -746,23 +766,36 @@ export async function authorizeBoardKey( } export function boardKeyAuthorizationMiddleware(db: Db): RequestHandler { - return async (req, _res, next) => { + return async (req, res, next) => { if (req.actor.source !== "board_key") { next(); return; } const metadata = lookupBoardKeyRoute(req.method, req.originalUrl); - try { - await authorizeBoardKey( - db, - req, - metadata.action, - () => resolveAuthoritativeResource(db, metadata), - metadata, - ); - next(); - } catch (err) { - next(err); - } + const context = createBoardKeyAuditContext(); + // A staged allow disposition that never reached a domain transaction is + // resolved once the response is complete: recorded when the request + // succeeded without a rollback, dropped otherwise. + const settle = (statusCode: number) => { + void settleBoardKeyAuditContext(db, context, statusCode); + }; + res.on("finish", () => settle(res.statusCode)); + res.on("close", () => settle(res.writableEnded ? res.statusCode : 0)); + // The context stays active across the downstream handler chain, so the + // instrumented database handle can couple the disposition to its mutation. + await runWithBoardKeyAuditContext(context, async () => { + try { + await authorizeBoardKey( + db, + req, + metadata.action, + () => resolveAuthoritativeResource(db, metadata), + metadata, + ); + next(); + } catch (err) { + next(err); + } + }); }; }