From 7d0ac2db7d66660c9e903a2e5fd1e68a804ad8a1 Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Fri, 15 May 2026 02:27:10 -0700 Subject: [PATCH] fix(pods): stop leaking other users' private DMs into admin sidebars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getAllPods` and `getPodsByType` were bypassing the membership filter for global admins. The intent was a moderation view; the effect was that admins (e.g. the xcjsam dev account) saw every agent-room and agent-dm in the instance in their default sidebar — including 1:1 DMs other users had with their installed agents. Clicking one of those rows looked like "Talk to" was navigating to a stranger's DM, and posting failed with "Not authorized to post in this pod" because the pod-message route correctly checks membership. Three changes: 1. `getAllPods`: default `scope=mine` now filters to caller membership for everyone, admins included. `?scope=all` is a separate admin- only opt-in for moderation tooling; non-admins are silently downgraded to `scope=mine`. 2. `getPodsByType`: personal pod types (agent-room, agent-dm, agent-admin) are always filtered to caller membership — the instance-wide audit is a separate admin tool, not this generic listing. 3. `getPodById`: add a membership gate for personal pod types and 404 non-members. The route previously returned the full pod payload to anyone authenticated, which leaked existence and membership of every private DM by direct ID lookup. Unit tests rewritten to encode the corrected invariant + cover the new `scope=all` admin path and the `getPodById` 404 case. --- .../unit/controllers/podController.test.js | 97 +++++++++++++++++-- backend/controllers/podController.ts | 53 ++++++---- 2 files changed, 123 insertions(+), 27 deletions(-) diff --git a/backend/__tests__/unit/controllers/podController.test.js b/backend/__tests__/unit/controllers/podController.test.js index d1b058f0..00ac233e 100644 --- a/backend/__tests__/unit/controllers/podController.test.js +++ b/backend/__tests__/unit/controllers/podController.test.js @@ -259,9 +259,14 @@ describe('podController', () => { expect(res.json).toHaveBeenCalledWith([myPod]); }); - it('getPodsByType returns ALL agent-rooms when caller is a global admin', async () => { + it('getPodsByType filters agent-room to caller membership even for global admins', async () => { + // Regression: admins used to bypass membership and see every private DM + // in the instance in their sidebar. That leaked other users' rooms and + // produced the "Talk to → can't post" UX bug. Admins now see only + // their own personal pods on this listing; moderation goes via a + // dedicated admin tool, not this generic endpoint. const otherPod = { _id: 'p1', type: 'agent-room', members: [{ _id: 'agent-id' }, { _id: 'someone-else' }] }; - const myPod = { _id: 'p2', type: 'agent-room', members: [{ _id: 'agent-id' }, { _id: 'me' }] }; + const myPod = { _id: 'p2', type: 'agent-room', members: [{ _id: 'agent-id' }, { _id: 'admin-id' }] }; const sort = jest.fn().mockResolvedValue([otherPod, myPod]); const populateSecond = jest.fn(() => ({ sort })); const populateFirst = jest.fn(() => ({ populate: populateSecond, sort })); @@ -273,14 +278,32 @@ describe('podController', () => { const req = { params: { type: 'agent-room' }, userId: 'admin-id', user: {} }; const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; await podController.getPodsByType(req, res); - // Admin sees both pods, not just their own. - expect(res.json).toHaveBeenCalledWith([otherPod, myPod]); + expect(res.json).toHaveBeenCalledWith([myPod]); }); - it('getAllPods returns ALL agent-rooms when caller is a global admin (parity with getPodsByType)', async () => { - const otherPod = { _id: 'p1', type: 'agent-room', members: [{ _id: 'agent-id' }, { _id: 'someone-else' }] }; - const myPod = { _id: 'p2', type: 'agent-room', members: [{ _id: 'agent-id' }, { _id: 'me' }] }; - // getAllPods has an extra .populate() chain (parentPod) — match it. + it('getAllPods default scope=mine filters to caller membership even for admins', async () => { + const otherPod = { _id: 'p1', type: 'chat', members: [{ _id: 'someone-else' }] }; + const myPod = { _id: 'p2', type: 'chat', members: [{ _id: 'admin-id' }] }; + const sort = jest.fn().mockResolvedValue([otherPod, myPod]); + const populateThird = jest.fn(() => ({ sort })); + const populateSecond = jest.fn(() => ({ populate: populateThird, sort })); + const populateFirst = jest.fn(() => ({ populate: populateSecond, sort })); + Pod.find.mockReturnValue({ populate: populateFirst }); + User.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue({ role: 'admin' }) }), + }); + + const req = { query: {}, userId: 'admin-id', user: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getAllPods(req, res); + // Default scope=mine: admin is filtered to their own pods, NOT every + // chat pod in the instance. + expect(res.json).toHaveBeenCalledWith([myPod]); + }); + + it('getAllPods scope=all returns everything for admins (explicit moderation view)', async () => { + const otherPod = { _id: 'p1', type: 'chat', members: [{ _id: 'someone-else' }] }; + const myPod = { _id: 'p2', type: 'chat', members: [{ _id: 'admin-id' }] }; const sort = jest.fn().mockResolvedValue([otherPod, myPod]); const populateThird = jest.fn(() => ({ sort })); const populateSecond = jest.fn(() => ({ populate: populateThird, sort })); @@ -290,9 +313,65 @@ describe('podController', () => { select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue({ role: 'admin' }) }), }); - const req = { query: { type: 'agent-room' }, userId: 'admin-id', user: {} }; + const req = { query: { scope: 'all' }, userId: 'admin-id', user: {} }; const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; await podController.getAllPods(req, res); expect(res.json).toHaveBeenCalledWith([otherPod, myPod]); }); + + it('getAllPods scope=all is silently downgraded to scope=mine for non-admins', async () => { + const otherPod = { _id: 'p1', type: 'chat', members: [{ _id: 'someone-else' }] }; + const myPod = { _id: 'p2', type: 'chat', members: [{ _id: 'me' }] }; + const sort = jest.fn().mockResolvedValue([otherPod, myPod]); + const populateThird = jest.fn(() => ({ sort })); + const populateSecond = jest.fn(() => ({ populate: populateThird, sort })); + const populateFirst = jest.fn(() => ({ populate: populateSecond, sort })); + Pod.find.mockReturnValue({ populate: populateFirst }); + User.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue({ role: 'user' }) }), + }); + + const req = { query: { scope: 'all' }, userId: 'me', user: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getAllPods(req, res); + expect(res.json).toHaveBeenCalledWith([myPod]); + }); + + it('getPodById returns 404 for personal pod types when caller is not a member', async () => { + const pod = { + _id: 'agent-room-1', + 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-1' }, userId: 'me', user: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getPodById(req, res); + expect(res.status).toHaveBeenCalledWith(404); + }); + + it('getPodById returns the pod for personal pod types when caller IS a member', async () => { + const pod = { + _id: 'agent-room-1', + type: 'agent-room', + members: [{ _id: 'agent-id' }, { _id: 'me' }], + }; + Pod.findById.mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockReturnValue({ + populate: jest.fn().mockResolvedValue(pod), + }), + }), + }); + const req = { params: { id: 'agent-room-1' }, userId: 'me', user: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await podController.getPodById(req, res); + expect(res.json).toHaveBeenCalledWith(pod); + }); }); diff --git a/backend/controllers/podController.ts b/backend/controllers/podController.ts index fad673c5..f7c873bf 100644 --- a/backend/controllers/podController.ts +++ b/backend/controllers/podController.ts @@ -172,19 +172,21 @@ exports.getAllPods = async (req: any, res: any) => { // a multi-tenant or shared dev instance and broke isolation for // demos / test accounts. // - // To opt back into the legacy "list everything I have access to read" - // behavior — useful for admin tooling, marketplace browsing, and - // the pod-discovery surface — pass `?scope=all`. That path still - // requires admin or explicit join-policy. + // To opt into the cross-instance "everything I can audit" view — + // useful for admin tooling — pass `?scope=all`. That path is admin- + // only; non-admins are silently downgraded to `scope=mine`. // - // Global admins bypass membership filtering so they can audit every - // pod in the instance — same moderation surface as before for the - // 1:1 invariant on agent-rooms (ADR-001 §3.10). + // Admins do NOT bypass membership on the default sidebar listing: + // their own pod list must be their own pods, otherwise every + // private DM in the instance leaks into their sidebar (which made + // xcjsam see — and try to post into — sam-demo's agent-rooms). const scope = String(req.query?.scope || 'mine').toLowerCase(); const isPersonal = type === 'agent-admin' || type === 'agent-room' || type === 'agent-dm'; - if (req.userId && (isPersonal || scope === 'mine')) { - const isAdmin = await isGlobalAdminRequest(req); - if (!isAdmin) { + if (req.userId) { + const wantsAll = scope === 'all'; + const isAdmin = wantsAll ? await isGlobalAdminRequest(req) : false; + const filterToMine = isPersonal || !wantsAll || !isAdmin; + if (filterToMine) { const uid = String(req.userId); pods = pods.filter((p: any) => p.members.some((m: any) => String(m._id || m) === uid)); } @@ -245,14 +247,13 @@ exports.getPodsByType = async (req: any, res: any) => { .populate('members', 'username profilePicture isBot') .sort({ updatedAt: -1 }); - // Same admin-bypass rule as getAllPods — see comment there. - if ((type === 'agent-admin' || type === 'agent-room') && req.userId) { - const isAdmin = await isGlobalAdminRequest(req); - if (!isAdmin) { - const uid = String(req.userId); - const memberPods = pods.filter((p: any) => p.members.some((m: any) => String(m._id || m) === uid)); - return res.json(memberPods); - } + // Personal pod types (agent-admin, agent-room, agent-dm) are always + // filtered to caller membership — admins included. The instance-wide + // moderation view is a separate admin tool, not this typed listing. + if ((type === 'agent-admin' || type === 'agent-room' || type === 'agent-dm') && req.userId) { + const uid = String(req.userId); + const memberPods = pods.filter((p: any) => p.members.some((m: any) => String(m._id || m) === uid)); + return res.json(memberPods); } return res.json(pods); @@ -288,6 +289,22 @@ 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. + const personalTypes = new Set(['agent-room', 'agent-dm', 'agent-admin']); + if (personalTypes.has(pod.type) && req.userId) { + const uid = String(req.userId); + const isMember = (pod.members || []).some( + (m: any) => String(m?._id || m) === uid, + ); + if (!isMember) { + return res.status(404).json({ error: 'Pod not found' }); + } + } + return res.json(pod); } catch (err: any) { console.error('Error in getPodById:', err.message);