fix(security): fail closed on direct board-key mutations
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
cryppadotta
parent
0a0489876f
commit
64c08ce961
|
|
@ -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.
|
||||
|
|
@ -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 });
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<BoardKeyAuditContext>();
|
|||
// 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<PropertyKey>([
|
||||
"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<string | symbol, unknown> & {
|
||||
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<T extends object>(
|
|||
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<T extends Db>(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<T extends Db>(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<unknown> | 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(
|
||||
|
|
|
|||
Loading…
Reference in New Issue