From 1646c35b65647db28e56174cff30f69ab5fb8b17 Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Fri, 15 May 2026 17:14:53 -0700 Subject: [PATCH] =?UTF-8?q?fix(pods):=20carve=20out=20=C2=A73.7=20fan-out?= =?UTF-8?q?=20on=20getPodById=20for=20agent-dm=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #375 made getPodById 404 non-members of all personal pod types (agent-room / agent-dm / agent-admin) to prevent admins from accidentally landing in other users' DMs via the sidebar. That was correct for the sidebar bug — but it also broke the V2 inspector's "Direct messages" list: clicking an a2a DM row navigates to /v2/pods/, the layout fetches the pod, and the 404 prevented the read-only chat from rendering even though the user has §3.7 fan-out access (canViewPod) to the underlying messages. Carve-out: - agent-dm: defers to DMService.canViewPod (admins + §3.7 fan-out) - agent-room: unchanged — strict membership (user↔agent is private) - agent-admin: unchanged — strict membership (ops surface) The frontend already renders an "isReadOnly" banner in place of the composer for non-member viewers of agent-dm (V2PodChat.tsx:499/752), so this purely-backend change unblocks the navigation path. Composer write gates (POST /api/messages/:podId) keep their existing membership check. Tests: - agent-dm + canViewPod true → 200 - agent-dm + canViewPod false → 404 (no leak) - agent-room non-member still → 404 (strict) 20/20 podController tests pass. --- CLAUDE.md | 2 +- .../unit/controllers/podController.test.js | 86 +++++++++++++++++++ backend/controllers/podController.ts | 28 ++++-- 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f37d6159..fd041510 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -372,7 +372,7 @@ These are prescriptive rules not derivable from reading the code: - **DM pods are strictly 1:1 (ADR-001 §3.10).** `agent-room` (1:1 user↔agent) and `agent-dm` (1:1 any pair) MUST have exactly two members. Single source of truth: `agentIdentityService.DM_POD_TYPES_GUARD = {'agent-room', 'agent-dm'}`. `ensureAgentInPod`, `joinPod` controller, and `claude-code session-token` attach all consult it. **`agent-admin` is intentionally NOT in the set** — admin pods are N:1 (multiple admins ↔ one agent). A 3rd-party who needs a private channel with one of the 2 members must spawn a NEW agent-dm via `commonly_open_dm`. Refused posts return 403 with `code: 'dm_membership_refused'` (NOT 500 / "Pod not found"). Sweep scripts: `scripts/migrate-agent-{dm,room}-multimember.ts`. -- **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in (PRs #375 / #377 / #378, 2026-05-15).** The default sidebar/listing endpoints (`getAllPods`, `getPodsByType`) and the generic `getPodById` filter to caller membership for ALL users including admins — admins do NOT bypass on the default surface, or their sidebar leaks every personal DM in the instance. Cross-instance moderation is an explicit `?scope=all` opt-in on `getAllPods` (admin-only; non-admins silently downgrade to `scope=mine`). Personal pod types (`agent-room`, `agent-dm`, `agent-admin`) 404 non-members on direct GET. Pod-scoped read endpoints for content — `/api/posts?podId=`, `/api/posts/:id`, `/api/pods/:id/external-links`, `/api/pods/:id/announcements`, `/api/pods/:id/files`, `/api/pods/:id/children`, `/api/summaries/pod/:id` — all run through `DMService.canViewPod` (members + admins + agent-dm §3.7 fan-out; everyone else 403). Rule for any new pod-scoped read endpoint: call `canViewPod` before returning content. The §3.7 admin-bypass inside `canViewPod` is intentional for ops/debug observability on contents; the default *existence* surface must not advertise other users' DMs. +- **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in (PRs #375 / #377 / #378, 2026-05-15).** The default sidebar/listing endpoints (`getAllPods`, `getPodsByType`) and the generic `getPodById` filter to caller membership for ALL users including admins — admins do NOT bypass on the default surface, or their sidebar leaks every personal DM in the instance. Cross-instance moderation is an explicit `?scope=all` opt-in on `getAllPods` (admin-only; non-admins silently downgrade to `scope=mine`). Personal pod types (`agent-room`, `agent-admin`) 404 non-members on direct GET; `agent-dm` carves out the §3.7 fan-out (PR #381, 2026-05-15) so humans sharing a pod with either agent participant can navigate to the a2a DM read-only — the V2 inspector "Direct messages" list links there. Pod-scoped read endpoints for content — `/api/posts?podId=`, `/api/posts/:id`, `/api/pods/:id/external-links`, `/api/pods/:id/announcements`, `/api/pods/:id/files`, `/api/pods/:id/children`, `/api/summaries/pod/:id` — all run through `DMService.canViewPod` (members + admins + agent-dm §3.7 fan-out; everyone else 403). Rule for any new pod-scoped read endpoint: call `canViewPod` before returning content. The §3.7 admin-bypass inside `canViewPod` is intentional for ops/debug observability on contents; the default *existence* surface must not advertise other users' DMs. - **DM display labels — never use `botMetadata.agentName`.** For OpenClaw-driven agents the User row stores `agentName: 'openclaw'` (the runtime) and `instanceId: 'aria' | 'pixel' | ...` (the actual identity). Pod names + `AgentInstallation.displayName` + chat.mention DM cues all resolve via `agentIdentityService.resolveAgentDisplayLabel(user, fallback)` with the chain: `botMetadata.displayName` → `instanceId` (when not 'default') → `username` → fallback. **Never** falls back to `botMetadata.agentName` — that produces "openclaw ↔ openclaw" pod names. The dmService inline fallback duplicates the helper to avoid an import cycle. Sweep script for stale data: `scripts/rename-agent-dm-pods.ts` (also handles `agent-room`). diff --git a/backend/__tests__/unit/controllers/podController.test.js b/backend/__tests__/unit/controllers/podController.test.js index 00ac233e..c3b9cab8 100644 --- a/backend/__tests__/unit/controllers/podController.test.js +++ b/backend/__tests__/unit/controllers/podController.test.js @@ -34,6 +34,10 @@ jest.mock('../../../models/AgentProfile', () => ({ updateOne: jest.fn(), deleteMany: jest.fn(), })); +jest.mock('../../../services/dmService', () => ({ + __esModule: true, + default: { canViewPod: jest.fn() }, +})); jest.mock('../../../services/agentIdentityService', () => ({ getAgentTypeConfig: jest.fn(), getOrCreateAgentUser: jest.fn(), @@ -374,4 +378,86 @@ describe('podController', () => { await podController.getPodById(req, res); expect(res.json).toHaveBeenCalledWith(pod); }); + + // ── Gap 2 (PR #381): agent-dm §3.7 fan-out carve-out for direct ID lookup ─ + // PR #375 made getPodById 404 non-members of all personal pod types so + // admins couldn't accidentally land in someone else's DM via the sidebar. + // But agent-dm specifically has §3.7 fan-out: humans who share a pod with + // either DM member can observe (canViewPod returns true). Without this + // carve-out, V2's "Direct messages" list in the inspector navigates to + // /v2/pods/ and the layout 404s — user reports "clicking the + // DM row doesn't open the pod." Carve-out is agent-dm only — agent-room + // (user↔agent) and agent-admin (ops) keep strict membership. + it('getPodById allows agent-dm read when canViewPod (§3.7 fan-out) allows', async () => { + const DMService = require('../../../services/dmService').default; + DMService.canViewPod.mockResolvedValueOnce(true); + + const pod = { + _id: 'a2a-dm-1', + type: 'agent-dm', + members: [{ _id: 'agent-a' }, { _id: 'agent-b' }], + }; + Pod.findById.mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockResolvedValue(pod), + }), + }), + }); + const req = { + params: { id: 'a2a-dm-1' }, + userId: 'observer-human', + user: {}, + }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getPodById(req, res); + expect(res.json).toHaveBeenCalledWith(pod); + expect(res.status).not.toHaveBeenCalledWith(404); + }); + + it('getPodById still 404s agent-dm when canViewPod denies (no shared pod, not admin)', async () => { + const DMService = require('../../../services/dmService').default; + DMService.canViewPod.mockResolvedValueOnce(false); + + const pod = { + _id: 'a2a-dm-2', + type: 'agent-dm', + members: [{ _id: 'agent-a' }, { _id: 'agent-b' }], + }; + Pod.findById.mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockResolvedValue(pod), + }), + }), + }); + const req = { + params: { id: 'a2a-dm-2' }, + userId: 'stranger', + user: {}, + }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getPodById(req, res); + expect(res.status).toHaveBeenCalledWith(404); + }); + + it('getPodById keeps strict membership 404 for agent-room (no §3.7 fan-out)', async () => { + const pod = { + _id: 'agent-room-x', + type: 'agent-room', + members: [{ _id: 'agent-id' }, { _id: 'someone-else' }], + }; + Pod.findById.mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockResolvedValue(pod), + }), + }), + }); + const req = { params: { id: 'agent-room-x' }, userId: 'observer', user: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getPodById(req, res); + // agent-room never gets the §3.7 carve-out — strict membership only. + expect(res.status).toHaveBeenCalledWith(404); + }); }); diff --git a/backend/controllers/podController.ts b/backend/controllers/podController.ts index f7c873bf..79176ce5 100644 --- a/backend/controllers/podController.ts +++ b/backend/controllers/podController.ts @@ -289,11 +289,19 @@ exports.getPodById = async (req: any, res: any) => { .json({ error: 'Pod not found or is not of specified type' }); } - // Personal pod types are private 1:1/N:1 surfaces. Only members can - // read them — direct ID lookups by non-members 404 to avoid leaking - // existence and to keep admins from accidentally landing in another - // user's DM (the sidebar bug that surfaced this fix). Admins can - // still moderate via dedicated admin tooling, not this generic GET. + // Personal pod types are private 1:1/N:1 surfaces. Direct ID lookups + // by non-members 404 to avoid leaking existence (the sidebar bug that + // surfaced PR #375). Two carve-outs that match canViewPod semantics: + // - `agent-dm` (agent ↔ agent): humans who share a pod with either + // DM participant get read access via the §3.7 fan-out — they can + // observe their team's agent collaborations without being a + // formal DM member. Without this carve-out a user can't navigate + // to an a2a DM from the agent inspector even though messages + // under /api/messages/:podId already serve them (canViewPod). + // - Global admins reading agent-dm — moderation observability. + // `agent-room` and `agent-admin` keep strict membership: those are + // truly private surfaces (user↔agent rooms; ops admin pods) and we + // don't want admins resolving someone else's agent-room by ID either. const personalTypes = new Set(['agent-room', 'agent-dm', 'agent-admin']); if (personalTypes.has(pod.type) && req.userId) { const uid = String(req.userId); @@ -301,7 +309,15 @@ exports.getPodById = async (req: any, res: any) => { (m: any) => String(m?._id || m) === uid, ); if (!isMember) { - return res.status(404).json({ error: 'Pod not found' }); + if (pod.type === 'agent-dm') { + // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require + const DMService = require('../services/dmService').default + || require('../services/dmService'); + const allowed = await DMService.canViewPod(uid, pod); + if (!allowed) return res.status(404).json({ error: 'Pod not found' }); + } else { + return res.status(404).json({ error: 'Pod not found' }); + } } }