From baa2188d56d83685688e2db3c8b4d71fd51ef470 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Fri, 30 Oct 2026 02:42:48 -0400 Subject: [PATCH] fix: audit unreconciled authorization attempts atomically --- CHANGELOG.md | 4 + server/src/audit.ts | 2 + server/src/plugins/plugin-store/context.ts | 77 ++++++++++++++----- ...completion-transaction.integration.test.ts | 76 ++++++++++++++++++ 4 files changed, 139 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27745c6f84..ffe20846ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +- Authorization attempts whose vendor answer cannot be reconciled now leave a safe, correlated + audit event when they settle as unreconciled. If that event cannot be saved, the attempt remains + unsettled rather than losing the audit trail. + **Before upgrading.** Sixteen things change for an existing deployment: - Bots now share one computer per person instead of each having its own. Migration `0069_isolated_bots` adds `agent_profiles.isolated`, off for every existing Bot. With the diff --git a/server/src/audit.ts b/server/src/audit.ts index 329ddbf4aa..1f131ba8e3 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -217,6 +217,8 @@ export const auditEventTypes = [ * thing they are entitled to see a record of. Carries the scope the vendor actually granted. */ "mcp.account_connected", + /** A vendor authorization answer could not be reconciled with its pending attempt. */ + "mcp.authorization_unreconciled", /* * One person's connector access retired, by them or on their behalf. * diff --git a/server/src/plugins/plugin-store/context.ts b/server/src/plugins/plugin-store/context.ts index cec7bf727c..3d540a6179 100644 --- a/server/src/plugins/plugin-store/context.ts +++ b/server/src/plugins/plugin-store/context.ts @@ -1,6 +1,7 @@ import { connectAppRefusal } from "@openbot/shared/connect-prompt"; import { and, asc, eq, inArray, isNull, sql } from "drizzle-orm"; -import { recordAuditEvent } from "../../audit"; +import { createAuditStore, recordAuditEvent } from "../../audit"; +import type { Database } from "../../db/client"; import { type CredentialExecutor, CredentialUnusableError, @@ -370,10 +371,11 @@ export function createStoreContext(options: PluginStoreOptions) { ); /** - * The one statement behind `failAuthorizationAttempt`, `markAuthorizationUnreconciled` and - * `markAuthorizationWithdrawn`, so the three verdicts cannot drift into three row shapes. - * `completed_at` is truncated to milliseconds for the reason `claimAuthorizationAttempt` - * truncates `created_at`. + * The shared transition behind `failAuthorizationAttempt`, `markAuthorizationUnreconciled` and + * `markAuthorizationWithdrawn`, so the verdicts cannot drift into three row shapes. + * An unreconciled transition and its safe audit record commit together; a failed audit leaves + * the attempt unsettled. `completed_at` is truncated to milliseconds for the reason + * `claimAuthorizationAttempt` truncates `created_at`. * * CONDITIONAL ON `completed_at IS NULL`, EXACTLY AS THE COMPLETION ABOVE IS, BECAUSE A SETTLED * ATTEMPT IS A RECORD AND NOT A DRAFT. Without that condition this UPDATE matched on @@ -404,21 +406,56 @@ export function createStoreContext(options: PluginStoreOptions) { input: { actorId: string; authId: string }, outcome: "failed" | "unreconciled" | "withdrawn", ) => { - const written = await database - .update(pendingAuthorizations) - .set({ - completedAt: sql`date_trunc('milliseconds', now())`, - outcome, - }) - .where( - and( - eq(pendingAuthorizations.actorId, input.actorId), - eq(pendingAuthorizations.authId, input.authId), - isNull(pendingAuthorizations.completedAt), - ), - ) - .returning({ actorId: pendingAuthorizations.actorId }); - return written.length > 0; + return await database.transaction(async (transaction) => { + const [attempt] = await transaction + .update(pendingAuthorizations) + .set({ + completedAt: sql`date_trunc('milliseconds', now())`, + outcome, + }) + .where( + and( + eq(pendingAuthorizations.actorId, input.actorId), + eq(pendingAuthorizations.authId, input.authId), + isNull(pendingAuthorizations.completedAt), + ), + ) + .returning({ + serverId: pendingAuthorizations.serverId, + action: pendingAuthorizations.action, + holder: pendingAuthorizations.holder, + createdAt: pendingAuthorizations.createdAt, + }); + if (!attempt) return false; + if (outcome === "unreconciled") { + const [server] = await transaction + .select({ url: mcpServers.url }) + .from(mcpServers) + .where(eq(mcpServers.id, attempt.serverId)) + .limit(1); + const toolkit = server + ? toolkitOfFor(brokerProvider, server.url) + : null; + await recordAuditEvent( + createAuditStore(transaction as unknown as Database), + { + eventType: "mcp.authorization_unreconciled", + targetType: "mcp_server", + targetId: toolkit ?? attempt.serverId, + payload: { + actor: input.actorId, + server: toolkit, + serverId: attempt.serverId, + action: attempt.action, + holder: attempt.holder, + attemptStartedAt: attempt.createdAt.toISOString(), + outcome, + }, + }, + ); + } + return true; + }); }; /** diff --git a/server/tests/arcade-completion-transaction.integration.test.ts b/server/tests/arcade-completion-transaction.integration.test.ts index 553b236d14..a2598f6b98 100644 --- a/server/tests/arcade-completion-transaction.integration.test.ts +++ b/server/tests/arcade-completion-transaction.integration.test.ts @@ -573,3 +573,79 @@ describe("failAuthorizationAttempt / markAuthorizationUnreconciled", () => { expect([withdrew, unreconciled]).toEqual([false, false]); }); }); + +describe("unreconciled authorization audit", () => { + const eventsFor = async (actorId: string) => { + const rows = await database + .select() + .from(auditEvents) + .where( + and( + eq(auditEvents.eventType, "mcp.authorization_unreconciled"), + eq(auditEvents.targetId, serverId), + ), + ); + return rows.filter((row) => row.payload.actor === actorId); + }; + + test("unreconciled audit: persists a safe event correlated with the closed attempt", async () => { + const a = await actor("audit-unreconciled"); + const authId = "ar_must_not_reach_the_audit_payload"; + const attempt = await attemptWith(a, authId); + expect( + await store.markAuthorizationUnreconciled({ actorId: a, authId }), + ).toBe(true); + const events = await eventsFor(a); + expect(events).toHaveLength(1); + expect(events[0]?.payload).toEqual({ + actor: a, + server: null, + serverId, + action: "Google.SendEmail", + holder: "person", + attemptStartedAt: attempt.createdAt.toISOString(), + outcome: "unreconciled", + }); + expect(JSON.stringify(events)).not.toContain(authId); + expect((await attemptRow(a)).outcome).toBe("unreconciled"); + }); + + test("unreconciled audit: two concurrent terminal callbacks record one event", async () => { + const a = await actor("audit-concurrent"); + const authId = "ar_concurrent_unreconciled"; + await attemptWith(a, authId); + const outcomes = await Promise.all([ + store.markAuthorizationUnreconciled({ actorId: a, authId }), + store.markAuthorizationUnreconciled({ actorId: a, authId }), + ]); + expect(outcomes.sort()).toEqual([false, true]); + expect(await eventsFor(a)).toHaveLength(1); + }); + + test("unreconciled audit: an audit insert failure leaves the attempt unsettled", async () => { + const a = await actor("audit-rollback"); + const authId = "ar_rollback_unreconciled"; + await attemptWith(a, authId); + await expect( + brokenAuditStore.markAuthorizationUnreconciled({ actorId: a, authId }), + ).rejects.toThrow("injected audit failure"); + const after = await attemptRow(a); + expect(after.completedAt).toBeNull(); + expect(after.outcome).toBeNull(); + expect(await eventsFor(a)).toHaveLength(0); + }); + + test("unreconciled audit: a previously settled attempt emits no false event", async () => { + const a = await actor("audit-already-settled"); + const authId = "ar_already_failed"; + await attemptWith(a, authId); + expect(await store.failAuthorizationAttempt({ actorId: a, authId })).toBe( + true, + ); + expect( + await store.markAuthorizationUnreconciled({ actorId: a, authId }), + ).toBe(false); + expect(await eventsFor(a)).toHaveLength(0); + expect((await attemptRow(a)).outcome).toBe("failed"); + }); +});