From 0a588fa7952722063a1937c0ca1e41bbb73035c2 Mon Sep 17 00:00:00 2001 From: Yuval Dinodia <102706514+yetval@users.noreply.github.com> Date: Sat, 18 Jul 2026 20:38:48 -0400 Subject: [PATCH] fix(acp): /acp sessions exposes every gateway session to non-owner senders (#110745) * fix(acp): scope /acp sessions listing for non-owner senders /acp sessions listed every ACP session on the gateway for any sender allowlisted via commands.allowFrom, exposing other senders' session labels, agent ids, runtime state, and thread bindings. The handler now returns only the current bound or requester session for non-owner senders, while owner identity and operator.admin clients keep the full gateway-wide listing, matching the documented contract. Fixes #103055 * test(acp): cover empty and missing-session cases for /acp sessions scoping * fix(acp): avoid non-owner session scans * docs(acp): remove duplicated session scope text * test(acp): cover internal session visibility * fix(acp): require current ACP metadata * test(acp): reject sessions target tokens --------- Co-authored-by: Peter Steinberger --- docs/tools/acp-agents.md | 12 +- src/auto-reply/reply/commands-acp.test.ts | 130 ++++++++++++++++-- .../reply/commands-acp/diagnostics.ts | 13 +- 3 files changed, 135 insertions(+), 20 deletions(-) diff --git a/docs/tools/acp-agents.md b/docs/tools/acp-agents.md index e55a0e909c27..04837fd70a8b 100644 --- a/docs/tools/acp-agents.md +++ b/docs/tools/acp-agents.md @@ -790,14 +790,16 @@ Runtime controls (`spawn`, `cancel`, `steer`, `close`, `status`, `set-mode`, `set`, `cwd`, `permissions`, `timeout`, `model`, and `reset-options`) require owner identity from external channels and `operator.admin` from internal Gateway clients. Authorized non-owner senders can still use `sessions`, -`doctor`, `install`, and `help`. +`doctor`, `install`, and `help`. For non-owner senders, `/acp sessions` +lists only the current bound or requester session; owner identity and +`operator.admin` clients see all recent sessions. `/acp status` shows the effective runtime options plus runtime-level and backend-level session identifiers. Unsupported-control errors surface -clearly when a backend lacks a capability. `/acp sessions` reads the store -for the current bound or requester session; target tokens (`session-key`, -`session-id`, or `session-label`) resolve through gateway session discovery, -including custom per-agent `session.store` roots. +clearly when a backend lacks a capability. Commands that accept target tokens +(`session-key`, `session-id`, or `session-label`) resolve them through gateway +session discovery, including custom per-agent `session.store` roots. `/acp sessions` +does not accept a target token. ### Runtime options mapping diff --git a/src/auto-reply/reply/commands-acp.test.ts b/src/auto-reply/reply/commands-acp.test.ts index de10d62a5673..7fecd62ec57a 100644 --- a/src/auto-reply/reply/commands-acp.test.ts +++ b/src/auto-reply/reply/commands-acp.test.ts @@ -1910,6 +1910,105 @@ describe("/acp command", () => { expect(result?.reply?.text).toContain(`thread:${defaultThreadId}`); }); + it("lists all stored ACP sessions for the owner", async () => { + hoisted.sessionBindingResolveByConversationMock.mockReturnValue( + createBoundThreadSession("agent:codex:acp:own"), + ); + hoisted.listAcpSessionEntriesMock.mockResolvedValue([ + createAcpSessionEntry({ sessionKey: "agent:codex:acp:own" }), + createAcpSessionEntry({ sessionKey: "agent:claude:acp:foreign" }), + ]); + + const result = await runDiscordAcpCommand("/acp sessions", baseCfg); + + expect(result?.reply?.text).toContain("agent:codex:acp:own"); + expect(result?.reply?.text).toContain("agent:claude:acp:foreign"); + expect(hoisted.readAcpSessionEntryMock).not.toHaveBeenCalled(); + }); + + it("lists only the current raw ACP session for an authorized non-owner sender", async () => { + const currentSessionKey = "agent:codex:acp:current"; + hoisted.readAcpSessionEntryMock.mockReturnValue( + createAcpSessionEntry({ sessionKey: currentSessionKey }), + ); + const params = createDiscordParams("/acp sessions"); + params.command.senderIsOwner = false; + params.sessionKey = currentSessionKey; + + const result = await handleAcpCommand(params, true); + + expect(result?.reply?.text).toContain(currentSessionKey); + expect(hoisted.readAcpSessionEntryMock).toHaveBeenCalledWith({ + cfg: baseCfg, + sessionKey: currentSessionKey, + }); + expect(hoisted.listAcpSessionEntriesMock).not.toHaveBeenCalled(); + }); + + it("prefers the bound-thread ACP session over a non-owner sender's raw session key", async () => { + const boundSessionKey = "agent:codex:acp:bound"; + hoisted.sessionBindingResolveByConversationMock.mockReturnValue( + createBoundThreadSession(boundSessionKey), + ); + hoisted.readAcpSessionEntryMock.mockReturnValue( + createAcpSessionEntry({ sessionKey: boundSessionKey }), + ); + const params = createDiscordParams("/acp sessions"); + params.command.senderIsOwner = false; + params.sessionKey = "agent:main:raw-requester"; + + const result = await handleAcpCommand(params, true); + + expect(result?.reply?.text).toContain(boundSessionKey); + expect(result?.reply?.text).not.toContain("agent:main:raw-requester"); + expect(hoisted.readAcpSessionEntryMock).toHaveBeenCalledWith({ + cfg: baseCfg, + sessionKey: boundSessionKey, + }); + expect(hoisted.listAcpSessionEntriesMock).not.toHaveBeenCalled(); + }); + + it("returns an empty listing when a non-owner's raw session key is not an ACP session", async () => { + hoisted.readAcpSessionEntryMock.mockReturnValue({ + ...createAcpSessionEntry({ sessionKey: "agent:main:raw-requester" }), + acp: undefined, + }); + const params = createDiscordParams("/acp sessions"); + params.command.senderIsOwner = false; + params.sessionKey = "agent:main:raw-requester"; + + const result = await handleAcpCommand(params, true); + + expect(result?.reply?.text).toContain("(none)"); + expect(hoisted.readAcpSessionEntryMock).toHaveBeenCalledWith({ + cfg: baseCfg, + sessionKey: "agent:main:raw-requester", + }); + expect(hoisted.listAcpSessionEntriesMock).not.toHaveBeenCalled(); + }); + + it("warns when no session key resolves for /acp sessions", async () => { + hoisted.listAcpSessionEntriesMock.mockResolvedValue([createAcpSessionEntry()]); + const params = createDiscordParams("/acp sessions"); + params.command.senderIsOwner = false; + params.sessionKey = ""; + + const result = await handleAcpCommand(params, true); + + expect(result?.reply?.text).toContain("Missing session key"); + }); + + it("rejects explicit target tokens for /acp sessions", async () => { + const params = createDiscordParams("/acp sessions agent:claude:acp:foreign"); + params.command.senderIsOwner = false; + + const result = await handleAcpCommand(params, true); + + expect(result?.reply?.text).toBe("Usage: /acp sessions"); + expect(hoisted.readAcpSessionEntryMock).not.toHaveBeenCalled(); + expect(hoisted.listAcpSessionEntriesMock).not.toHaveBeenCalled(); + }); + it("shows ACP status for the thread-bound ACP session", async () => { mockBoundThreadSession({ identity: { @@ -2088,17 +2187,7 @@ describe("/acp command", () => { }); it("keeps read-only /acp actions available to internal operator.write clients", async () => { - hoisted.listAcpSessionEntriesMock.mockResolvedValue([ - createAcpSessionEntry({ - identity: { - state: "resolved", - source: "status", - acpxSessionId: "runtime-1", - agentSessionId: "session-1", - lastUpdatedAt: Date.now(), - }, - }), - ]); + hoisted.readAcpSessionEntryMock.mockReturnValue(createAcpSessionEntry()); const result = await runInternalAcpCommand({ commandBody: "/acp sessions", @@ -2106,7 +2195,24 @@ describe("/acp command", () => { }); expect(result?.shouldContinue).toBe(false); - expect(result?.reply?.text).toContain("ACP sessions"); + expect(result?.reply?.text).toContain(defaultAcpSessionKey); + expect(hoisted.listAcpSessionEntriesMock).not.toHaveBeenCalled(); + }); + + it("lists all ACP sessions for internal operator.admin clients", async () => { + hoisted.listAcpSessionEntriesMock.mockResolvedValue([ + createAcpSessionEntry({ sessionKey: "agent:codex:acp:own" }), + createAcpSessionEntry({ sessionKey: "agent:claude:acp:foreign" }), + ]); + + const result = await runInternalAcpCommand({ + commandBody: "/acp sessions", + scopes: ["operator.admin"], + }); + + expect(result?.reply?.text).toContain("agent:codex:acp:own"); + expect(result?.reply?.text).toContain("agent:claude:acp:foreign"); + expect(hoisted.readAcpSessionEntryMock).not.toHaveBeenCalled(); }); it("allows mutating /acp actions for internal operator.admin clients", async () => { diff --git a/src/auto-reply/reply/commands-acp/diagnostics.ts b/src/auto-reply/reply/commands-acp/diagnostics.ts index 60e15cc6f735..6587619c6141 100644 --- a/src/auto-reply/reply/commands-acp/diagnostics.ts +++ b/src/auto-reply/reply/commands-acp/diagnostics.ts @@ -7,7 +7,7 @@ import { import { getAcpSessionManager } from "../../../acp/control-plane/manager.js"; import { toAcpRuntimeError } from "../../../acp/runtime/errors.js"; import { getAcpRuntimeBackend, requireAcpRuntimeBackend } from "../../../acp/runtime/registry.js"; -import { listAcpSessionEntries } from "../../../acp/runtime/session-meta.js"; +import { listAcpSessionEntries, readAcpSessionEntry } from "../../../acp/runtime/session-meta.js"; import type { SessionEntry } from "../../../config/sessions/types.js"; import type { SessionAcpMeta } from "../../../config/sessions/types.js"; import { getSessionBindingService } from "../../../infra/outbound/session-binding-service.js"; @@ -190,9 +190,16 @@ export async function handleAcpSessionsAction( const normalizedChannel = bindingContext.channel; const normalizedAccountId = bindingContext.accountId || undefined; const bindingService = getSessionBindingService(); - const entries = await listAcpSessionEntries({ cfg: params.cfg }); + const currentEntry = params.command.senderIsOwner + ? null + : readAcpSessionEntry({ cfg: params.cfg, sessionKey: currentSessionKey }); + const visibleEntries = params.command.senderIsOwner + ? await listAcpSessionEntries({ cfg: params.cfg }) + : currentEntry?.entry && currentEntry.acp + ? [currentEntry] + : []; - const rows = entries + const rows = visibleEntries .toSorted((a, b) => (b.entry?.updatedAt ?? 0) - (a.entry?.updatedAt ?? 0)) .slice(0, 20) .map(({ storeSessionKey, entry, acp }) => {