From a0b26ebace4807a640397e01911d8848f6b778a3 Mon Sep 17 00:00:00 2001 From: hotragn Date: Thu, 22 Oct 2026 20:15:29 -0400 Subject: [PATCH] Say which Slack or Teams account a link bound on the audit trail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while reviewing #825. Linking a Bot to Slack or Teams writes a `delivery.linked` row from the wrapper in `admin/controls.ts`, filed under the Bot's owner, carrying `transport`, `realm`, `channelId` and `agentId`. It did not carry the chat account that redeemed the code, although `bind()` receives it as `identity`. That account is the one that matters. A link code is redeemed by whoever sends it, and that account then runs the Bot as its owner and decides approvals as them — which is why #763 and #825 stop codes being redeemed where others can read them. On the trail, a binding somebody else redeemed read as the owner connecting their own account. `delivery.unlinked` writes an empty payload, so once the binding row was removed nothing anywhere said which account it had been. Recording it on the link is enough: the unlink row carries the same `targetId`, so the trail joins the two itself. For Slack and Teams only. The SMS path binds with `identity: challenge.address`, a phone number, and push binds a device token. Both would land on a trail every administrator can read, and neither has the code-in-a-shared-chat risk: an SMS code proves possession of the phone it was sent to. The wrapper already distinguishes exactly those two transports for its capability check, so the condition is computed once and used for both. --- CHANGELOG.md | 9 ++++ server/src/admin/controls.ts | 22 ++++++-- server/tests/delivery-link-audit.test.ts | 67 ++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 server/tests/delivery-link-audit.test.ts 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); + }); +});