diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd51b8e0c..8476a1d2c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. request for an administrator to switch on a work app, grant it to a Bot, or connect a Shared app's team account is kept, with who answered it and when. It starts empty and nothing is backfilled. +### The audit trail says which Slack or Teams account a link bound + +Linking a Bot to Slack or Teams recorded a `delivery.linked` row filed under the Bot's owner, with +the transport and workspace but not the chat account that sent the code. Whoever sends a link code +is the account that then speaks as the owner, so a code redeemed by somebody else read on the trail +as the owner connecting their own account, and once the link was removed nothing said which account +it had been. The row now names it, for Slack and Teams only: for SMS and push the same field is a +phone number or a device token, which stays off a trail every administrator can read. + ### A Bot can send, draft and reply in Gmail OpenBot left out every Composio action that takes a file, because a Bot has no way to supply one. diff --git a/server/src/admin/controls.ts b/server/src/admin/controls.ts index fe456083c6..fb88c24881 100644 --- a/server/src/admin/controls.ts +++ b/server/src/admin/controls.ts @@ -43,6 +43,7 @@ import type { Database } from "../db/client"; import { agentProfiles, agents, auditEvents } from "../db/schema"; import { createOtelEventExporter, type EventExporter } from "../telemetry/otel"; import { scrubCommand } from "../telemetry/scrub"; +import type { WebBotAuth } from "../web-bot-auth"; import { type Capability, type CapabilityRow, @@ -58,7 +59,6 @@ import { type EnterpriseStore, type NetworkPolicyRow, } from "./settings-store"; -import type { WebBotAuth } from "../web-bot-auth"; /** Action Recording keeps 90 days, as Grok Bot's does. */ export const ACTION_RECORD_RETENTION_DAYS = 90; @@ -669,11 +669,9 @@ export function guardDeliveryStore( const bind = store.bind.bind(store); const removeBinding = store.removeBinding.bind(store); store.bind = (async (input: Parameters[0]) => { + const chat = input.transport === "slack" || input.transport === "teams"; const controls = enterpriseControls(); - if ( - controls && - (input.transport === "slack" || input.transport === "teams") - ) { + if (controls && chat) { let allowed = false; try { allowed = await controls.capabilityFor(input.ownerUserId, "slackTeams"); @@ -693,6 +691,20 @@ export function guardDeliveryStore( realm: input.realm, channelId: input.channelId ?? null, agentId: input.agentId ?? null, + /* + * Which chat account now speaks as the owner, for Slack and Teams only. + * + * A link code there is redeemed by whoever sends it, and that account then runs the Bot as + * its owner and decides approvals as them. The row is filed under the owner, so without + * this a binding somebody else redeemed reads as the owner connecting their own account, + * and once the binding is removed nothing anywhere says which account it was. Recorded on + * the link only: `delivery.unlinked` carries the same `targetId`, so the trail joins them. + * + * Not for SMS or push. There `identity` is a phone number or a device token, the trail is + * readable by every administrator, and the code-in-a-shared-chat risk does not exist — + * an SMS code proves possession of the phone it was sent to. + */ + ...(chat ? { identity: input.identity } : {}), }, }).catch(() => undefined); return bound; diff --git a/server/tests/delivery-link-audit.test.ts b/server/tests/delivery-link-audit.test.ts new file mode 100644 index 0000000000..f22f7c8338 --- /dev/null +++ b/server/tests/delivery-link-audit.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, test } from "bun:test"; +import { guardDeliveryStore } from "../src/admin/controls"; +import type { AuditEventInput } from "../src/audit"; + +/** + * Which chat account a link bound, on the `delivery.linked` row. + * + * A Slack or Teams link code is redeemed by whoever sends it, and that account then runs the Bot as + * its owner. The row is filed under the owner, so it has to say which account, or a binding someone + * else redeemed reads as the owner connecting their own. For SMS and push the identity is a phone + * number or a device token, which does not belong on a trail every administrator can read. + */ +function linkRow(transport: string, identity: string) { + const written: AuditEventInput[] = []; + const store = guardDeliveryStore( + { + bind: async (_input: { + ownerUserId: string; + transport: string; + realm: string; + identity: string; + }) => ({ id: "binding-1" }), + removeBinding: async () => undefined, + }, + { + insert: async (event: AuditEventInput) => { + written.push(event); + }, + } as never, + (message) => new Error(message), + ); + return { + written, + store, + input: { ownerUserId: "owner-1", transport, realm: "T1", identity }, + }; +} + +describe("the delivery.linked row", () => { + test.each(["slack", "teams"])( + "names the %s account that was bound", + async (transport) => { + const { written, store, input } = linkRow( + transport, + `${transport}:T1:U9`, + ); + await store.bind(input); + + expect(written[0]?.eventType).toBe("delivery.linked"); + expect(written[0]?.payload).toMatchObject({ + identity: `${transport}:T1:U9`, + }); + }, + ); + + test.each([ + ["sms", "+15555550123"], + ["push", "device-token-abc"], + ])("leaves the %s identity off the trail", async (transport, identity) => { + const { written, store, input } = linkRow(transport, identity); + await store.bind(input); + + expect(written[0]?.eventType).toBe("delivery.linked"); + expect(written[0]?.payload).not.toHaveProperty("identity"); + expect(JSON.stringify(written[0]?.payload)).not.toContain(identity); + }); +});