From d5d4d0e89c6e456bda2ea4eb3359873cabf448f6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:41:45 -0700 Subject: [PATCH 1/2] fix(activity): gate both pod-scoped write routes on membership (#1300) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/activity/create was `auth`-only: no membership check, no pod-existence check, with `type` and `podId` taken straight off the body. It mattered beyond an ordinary missing gate because the caller does not need to know the approval schema exists — `Activity.approval.status` declares `default: 'pending'`, so Mongoose materialises exactly the two fields `Activity.getPendingApprovals` filters on. A row of `type: 'approval_needed'` therefore lands in an arbitrary pod's ADMINS' decision queue with attacker-controlled content. POST /api/activity/seed/:podId is the same hole by another door, and is the DESIGNED producer of approval_needed rows: it checked that the pod and the user exist and wrote four rows into any pod. Both now resolve the pod and refuse a non-member. `/create` additionally refuses `approval_needed` outright — it is the generic client-facing create, and the approval kind is what fills a decision queue. The membership predicate moves to backend/utils/isPodMember.ts rather than being copied: podInvites.ts already had this exact function and now imports it, so there is one definition of who may write into a pod. It deliberately omits the admin bypass DMService.canViewPod carries — that bypass exists for read observability. Every test asserts the write did not happen, not just the status code. Mutation table, exclusion arm on each row (86 suites / 523 tests): /create membership deleted 1 red / 0 without approval_needed guard deleted 1 red / 0 without /seed membership deleted 1 red / 0 without isPodMember always true 4 red (2 of them podInvites' own) Nothing else in the repo catches any of them. Co-Authored-By: Claude Opus 5 --- .../routes/activity.write-membership.test.js | 127 ++++++++++++++++++ backend/routes/activity.ts | 17 +++ backend/routes/podInvites.ts | 9 +- backend/utils/isPodMember.ts | 18 +++ 4 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 backend/__tests__/unit/routes/activity.write-membership.test.js create mode 100644 backend/utils/isPodMember.ts diff --git a/backend/__tests__/unit/routes/activity.write-membership.test.js b/backend/__tests__/unit/routes/activity.write-membership.test.js new file mode 100644 index 000000000..85179598e --- /dev/null +++ b/backend/__tests__/unit/routes/activity.write-membership.test.js @@ -0,0 +1,127 @@ +const request = require('supertest'); +const express = require('express'); + +// Both write routes on /api/activity landed a row into an arbitrary pod for any +// authenticated caller. `/create` mattered most because `type` comes straight +// off the body and `Activity.approval.status` defaults to 'pending', so the row +// materialises exactly the two fields `getPendingApprovals` filters on — i.e. it +// arrives in that pod's ADMINS' decision queue with attacker-chosen content. +// Every case below asserts the write did not happen, not just the status code. + +const CALLER = 'caller-1'; +const OTHER = 'someone-else'; + +const mockAuth = () => jest.doMock('../../../middleware/auth', () => (req, res, next) => { + req.userId = CALLER; + next(); +}); + +const mockPod = (pod) => jest.doMock('../../../models/Pod', () => ({ + findById: jest.fn(() => ({ select: () => ({ lean: async () => pod }) })), +})); + +const buildApp = () => { + const app = express(); + app.use(express.json()); + app.use('/api/activity', require('../../../routes/activity')); + return app; +}; + +const setup = (pod) => { + mockAuth(); + mockPod(pod); + const create = jest.fn(async () => ({ + _id: { toString: () => 'act-1' }, type: 'message', action: 'message', content: 'x', createdAt: new Date(), + })); + jest.doMock('../../../models/Activity', () => ({ create })); + jest.doMock('../../../models/User', () => ({ + findById: jest.fn(() => ({ select: () => ({ lean: async () => ({ username: 'someone' }) }) })), + })); + const seedPodActivities = jest.fn(async () => ({ success: true, count: 4 })); + jest.doMock('../../../services/activityService', () => ({ + seedPodActivities, isAgentUsername: jest.fn(() => false), + })); + return { app: buildApp(), create, seedPodActivities }; +}; + +const body = (over = {}) => ({ + type: 'message', action: 'message', content: 'hi', podId: 'pod-1', ...over, +}); + +describe('POST /api/activity/create — pod membership', () => { + afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); + + it('refuses a non-member and writes nothing', async () => { + const { app, create } = setup({ createdBy: OTHER, members: [OTHER] }); + await request(app).post('/api/activity/create').send(body()).expect(403); + expect(create).not.toHaveBeenCalled(); + }); + + it('allows a member', async () => { + const { app, create } = setup({ createdBy: OTHER, members: [OTHER, CALLER] }); + await request(app).post('/api/activity/create').send(body()).expect(200); + expect(create).toHaveBeenCalledTimes(1); + }); + + it('allows the creator, who is not always listed in members', async () => { + const { app, create } = setup({ createdBy: CALLER, members: [] }); + await request(app).post('/api/activity/create').send(body()).expect(200); + expect(create).toHaveBeenCalledTimes(1); + }); + + it('accepts a member listed as a populated subdocument', async () => { + const { app, create } = setup({ createdBy: OTHER, members: [{ _id: CALLER }] }); + await request(app).post('/api/activity/create').send(body()).expect(200); + expect(create).toHaveBeenCalledTimes(1); + }); + + it('404s an unknown pod rather than writing an orphan row', async () => { + const { app, create } = setup(null); + await request(app).post('/api/activity/create').send(body()).expect(404); + expect(create).not.toHaveBeenCalled(); + }); +}); + +describe('POST /api/activity/create — the approval_needed kind', () => { + afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); + + // Independent of membership: a member of the pod still cannot mint one here, + // because this is the generic client-facing create and the approval kind is + // what fills an admin decision queue. + it('refuses approval_needed even from a member', async () => { + const { app, create } = setup({ createdBy: CALLER, members: [CALLER] }); + await request(app).post('/api/activity/create') + .send(body({ type: 'approval_needed' })).expect(400); + expect(create).not.toHaveBeenCalled(); + }); + + it('positive control — the same member may create an ordinary kind', async () => { + const { app, create } = setup({ createdBy: CALLER, members: [CALLER] }); + await request(app).post('/api/activity/create').send(body()).expect(200); + expect(create).toHaveBeenCalledTimes(1); + }); +}); + +describe('POST /api/activity/seed/:podId — pod membership', () => { + afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); + + // The seeder is the DESIGNED producer of approval_needed rows, so an ungated + // seed route is the same injection by another door. + it('refuses a non-member and never reaches the seeder', async () => { + const { app, seedPodActivities } = setup({ createdBy: OTHER, members: [OTHER] }); + await request(app).post('/api/activity/seed/pod-1').send({}).expect(403); + expect(seedPodActivities).not.toHaveBeenCalled(); + }); + + it('allows a member', async () => { + const { app, seedPodActivities } = setup({ createdBy: OTHER, members: [CALLER] }); + await request(app).post('/api/activity/seed/pod-1').send({}).expect(200); + expect(seedPodActivities).toHaveBeenCalledTimes(1); + }); + + it('404s an unknown pod', async () => { + const { app, seedPodActivities } = setup(null); + await request(app).post('/api/activity/seed/pod-1').send({}).expect(404); + expect(seedPodActivities).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/routes/activity.ts b/backend/routes/activity.ts index 0c669a541..3c3358a88 100644 --- a/backend/routes/activity.ts +++ b/backend/routes/activity.ts @@ -16,6 +16,10 @@ const User = require('../models/User'); const ActivityService = require('../services/activityService'); // eslint-disable-next-line global-require const getAuthenticatedUserId = require('../utils/getAuthenticatedUserId'); +// eslint-disable-next-line global-require +const Pod = require('../models/Pod'); +// eslint-disable-next-line global-require +const isPodMember = require('../utils/isPodMember'); interface Req { query?: Record; @@ -209,6 +213,9 @@ router.post('/seed/:podId', auth, async (req: Req, res: Res) => { try { const { podId } = req.params || {}; const userId = getAuthenticatedUserId(req); + const pod = await Pod.findById(podId).select('members createdBy').lean(); + if (!pod) return res.status(404).json({ error: 'Pod not found' }); + if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can seed activities' }); const result = await ActivityService.seedPodActivities(podId, userId) as { success?: boolean; error?: string }; if (!result.success) return res.status(400).json({ error: result.error }); return res.json(result); @@ -223,6 +230,16 @@ router.post('/create', auth, async (req: Req, res: Res) => { const { type, action, content, podId, target, agentMetadata } = (req.body || {}) as Record; const userId = getAuthenticatedUserId(req); if (!type || !action || !podId) return res.status(400).json({ error: 'type, action, and podId are required' }); + // `approval_needed` is what `getPendingApprovals` selects into a pod's + // admins' decision queue. This is the generic client-facing create, so it + // must not be able to mint one: the caller supplies no `approval` subdoc + // and does not need to, because `Activity.approval.status` defaults to + // 'pending' and Mongoose materialises exactly the two fields that filter + // selects on. + if (type === 'approval_needed') return res.status(400).json({ error: 'approval_needed activities cannot be created through this route' }); + const pod = await Pod.findById(podId).select('members createdBy').lean(); + if (!pod) return res.status(404).json({ error: 'Pod not found' }); + if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can create activities in a pod' }); const user = await User.findById(userId).select('username').lean() as { username?: string } | null; const activity = await Activity.create({ type, actor: { id: userId, name: user?.username || 'Unknown', type: ActivityService.isAgentUsername(user?.username) ? 'agent' : 'human', verified: false }, action, content, podId, target, agentMetadata }); return res.json({ success: true, activity: { id: activity._id.toString(), type: activity.type, action: activity.action, content: activity.content, createdAt: activity.createdAt } }); diff --git a/backend/routes/podInvites.ts b/backend/routes/podInvites.ts index 0319a59a8..a5e011cf0 100644 --- a/backend/routes/podInvites.ts +++ b/backend/routes/podInvites.ts @@ -44,13 +44,8 @@ const inviteWriteRateLimit = rateLimit({ handler: (_req: any, res: any) => res.status(429).json({ msg: 'rate limit exceeded: 20 invite writes per 60s' }), }); -const isPodMember = (pod: any, userId: string) => { - if (!pod || !userId) return false; - if (pod.createdBy?.toString?.() === userId.toString()) return true; - return (pod.members || []).some((m: any) => ( - (m?._id?.toString?.() || m?.toString?.() || '') === userId.toString() - )); -}; +// eslint-disable-next-line global-require +const isPodMember = require('../utils/isPodMember'); // POST /api/pods/:podId/invites — issue a fresh invite token. Caller must // be a member or creator. Body: { expiresInHours?, maxUses? } — both diff --git a/backend/utils/isPodMember.ts b/backend/utils/isPodMember.ts new file mode 100644 index 000000000..5e5b66c96 --- /dev/null +++ b/backend/utils/isPodMember.ts @@ -0,0 +1,18 @@ +// Membership predicate for pod-scoped WRITES. Deliberately strict: it does +// not carry the admin bypass `DMService.canViewPod` has, because that bypass +// exists for read observability and would make "only members can write here" +// untrue for the one account most able to do damage by accident. +// +// The creator counts as a member — `Pod.members` does not always list them. +const isPodMember = (pod: any, userId: unknown): boolean => { + if (!pod || !userId) return false; + const id = String(userId); + if (pod.createdBy?.toString?.() === id) return true; + return (pod.members || []).some((m: any) => ( + (m?._id?.toString?.() || m?.toString?.() || '') === id + )); +}; + +module.exports = isPodMember; + +export {}; From 51f7a13c6fe84a9a7157b7a68744c214dc263d9d Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:53:42 -0700 Subject: [PATCH 2/2] fix(activity): coerce the body podId and store the resolved pod id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged the pod lookup I added: js/sql-injection, high, at the new Pod.findById in /create. It is right. `podId` arrives as `unknown` off the body, so a raw object reaches the query as Mongo operators rather than as an id. Coerced with String() on both routes — /seed takes its id from params, where it is always a string, but the two lookups should not differ on a security-relevant detail. The created row now stores `pod._id` — the pod actually resolved and authorised — rather than the body's copy of it. Two cases added. The coercion one asserts on the ARGUMENT handed to findById, not on the response status: what a mocked findById returns for a malformed id is a property of the mock, while what the route passes it is the thing under test. Pod fixtures gained the `_id` they should always have had. Mutations, both 1 red / 12 green: String() reverted on /create podId: pod._id reverted to the body's podId Co-Authored-By: Claude Opus 5 --- .../routes/activity.write-membership.test.js | 53 +++++++++++++++---- backend/routes/activity.ts | 11 ++-- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/backend/__tests__/unit/routes/activity.write-membership.test.js b/backend/__tests__/unit/routes/activity.write-membership.test.js index 85179598e..5e6572431 100644 --- a/backend/__tests__/unit/routes/activity.write-membership.test.js +++ b/backend/__tests__/unit/routes/activity.write-membership.test.js @@ -16,9 +16,11 @@ const mockAuth = () => jest.doMock('../../../middleware/auth', () => (req, res, next(); }); -const mockPod = (pod) => jest.doMock('../../../models/Pod', () => ({ - findById: jest.fn(() => ({ select: () => ({ lean: async () => pod }) })), -})); +const podFindById = jest.fn(); +const mockPod = (pod) => { + podFindById.mockImplementation(() => ({ select: () => ({ lean: async () => pod }) })); + jest.doMock('../../../models/Pod', () => ({ findById: podFindById })); +}; const buildApp = () => { const app = express(); @@ -29,6 +31,7 @@ const buildApp = () => { const setup = (pod) => { mockAuth(); + podFindById.mockReset(); mockPod(pod); const create = jest.fn(async () => ({ _id: { toString: () => 'act-1' }, type: 'message', action: 'message', content: 'x', createdAt: new Date(), @@ -52,25 +55,25 @@ describe('POST /api/activity/create — pod membership', () => { afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); it('refuses a non-member and writes nothing', async () => { - const { app, create } = setup({ createdBy: OTHER, members: [OTHER] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: OTHER, members: [OTHER] }); await request(app).post('/api/activity/create').send(body()).expect(403); expect(create).not.toHaveBeenCalled(); }); it('allows a member', async () => { - const { app, create } = setup({ createdBy: OTHER, members: [OTHER, CALLER] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: OTHER, members: [OTHER, CALLER] }); await request(app).post('/api/activity/create').send(body()).expect(200); expect(create).toHaveBeenCalledTimes(1); }); it('allows the creator, who is not always listed in members', async () => { - const { app, create } = setup({ createdBy: CALLER, members: [] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [] }); await request(app).post('/api/activity/create').send(body()).expect(200); expect(create).toHaveBeenCalledTimes(1); }); it('accepts a member listed as a populated subdocument', async () => { - const { app, create } = setup({ createdBy: OTHER, members: [{ _id: CALLER }] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: OTHER, members: [{ _id: CALLER }] }); await request(app).post('/api/activity/create').send(body()).expect(200); expect(create).toHaveBeenCalledTimes(1); }); @@ -82,6 +85,34 @@ describe('POST /api/activity/create — pod membership', () => { }); }); +describe('POST /api/activity/create — the body\'s podId is untrusted input', () => { + afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); + + // `podId` arrives as `unknown`. A raw object reaching findById would be read + // as Mongo operators rather than as an id. + // Asserted on the ARGUMENT rather than the status: what the mocked findById + // returns for a malformed id is a property of the mock, but what the route + // hands it is the thing under test. + it('coerces the body podId to a string before it reaches a query', async () => { + const { app } = setup({ _id: 'pod-1', createdBy: CALLER, members: [CALLER] }); + await request(app).post('/api/activity/create').send(body({ podId: { $ne: null } })); + expect(podFindById).toHaveBeenCalledTimes(1); + expect(typeof podFindById.mock.calls[0][0]).toBe('string'); + }); + + it('passes the params podId to the seeder lookup as a string too', async () => { + const { app } = setup({ _id: 'pod-1', createdBy: CALLER, members: [CALLER] }); + await request(app).post('/api/activity/seed/pod-1').send({}).expect(200); + expect(typeof podFindById.mock.calls[0][0]).toBe('string'); + }); + + it('stores the resolved pod id, not the body\'s copy of it', async () => { + const { app, create } = setup({ _id: 'resolved-pod', createdBy: CALLER, members: [CALLER] }); + await request(app).post('/api/activity/create').send(body()).expect(200); + expect(create.mock.calls[0][0].podId).toBe('resolved-pod'); + }); +}); + describe('POST /api/activity/create — the approval_needed kind', () => { afterEach(() => { jest.resetModules(); jest.clearAllMocks(); }); @@ -89,14 +120,14 @@ describe('POST /api/activity/create — the approval_needed kind', () => { // because this is the generic client-facing create and the approval kind is // what fills an admin decision queue. it('refuses approval_needed even from a member', async () => { - const { app, create } = setup({ createdBy: CALLER, members: [CALLER] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [CALLER] }); await request(app).post('/api/activity/create') .send(body({ type: 'approval_needed' })).expect(400); expect(create).not.toHaveBeenCalled(); }); it('positive control — the same member may create an ordinary kind', async () => { - const { app, create } = setup({ createdBy: CALLER, members: [CALLER] }); + const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [CALLER] }); await request(app).post('/api/activity/create').send(body()).expect(200); expect(create).toHaveBeenCalledTimes(1); }); @@ -108,13 +139,13 @@ describe('POST /api/activity/seed/:podId — pod membership', () => { // The seeder is the DESIGNED producer of approval_needed rows, so an ungated // seed route is the same injection by another door. it('refuses a non-member and never reaches the seeder', async () => { - const { app, seedPodActivities } = setup({ createdBy: OTHER, members: [OTHER] }); + const { app, seedPodActivities } = setup({ _id: 'pod-1', createdBy: OTHER, members: [OTHER] }); await request(app).post('/api/activity/seed/pod-1').send({}).expect(403); expect(seedPodActivities).not.toHaveBeenCalled(); }); it('allows a member', async () => { - const { app, seedPodActivities } = setup({ createdBy: OTHER, members: [CALLER] }); + const { app, seedPodActivities } = setup({ _id: 'pod-1', createdBy: OTHER, members: [CALLER] }); await request(app).post('/api/activity/seed/pod-1').send({}).expect(200); expect(seedPodActivities).toHaveBeenCalledTimes(1); }); diff --git a/backend/routes/activity.ts b/backend/routes/activity.ts index 3c3358a88..deec6464d 100644 --- a/backend/routes/activity.ts +++ b/backend/routes/activity.ts @@ -213,7 +213,7 @@ router.post('/seed/:podId', auth, async (req: Req, res: Res) => { try { const { podId } = req.params || {}; const userId = getAuthenticatedUserId(req); - const pod = await Pod.findById(podId).select('members createdBy').lean(); + const pod = await Pod.findById(String(podId)).select('members createdBy').lean(); if (!pod) return res.status(404).json({ error: 'Pod not found' }); if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can seed activities' }); const result = await ActivityService.seedPodActivities(podId, userId) as { success?: boolean; error?: string }; @@ -237,11 +237,16 @@ router.post('/create', auth, async (req: Req, res: Res) => { // 'pending' and Mongoose materialises exactly the two fields that filter // selects on. if (type === 'approval_needed') return res.status(400).json({ error: 'approval_needed activities cannot be created through this route' }); - const pod = await Pod.findById(podId).select('members createdBy').lean(); + // `podId` arrives as `unknown` off the body, so it is coerced before it + // reaches a query: a raw object would otherwise be interpreted as Mongo + // operators rather than an id. + const pod = await Pod.findById(String(podId)).select('members createdBy').lean(); if (!pod) return res.status(404).json({ error: 'Pod not found' }); if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can create activities in a pod' }); const user = await User.findById(userId).select('username').lean() as { username?: string } | null; - const activity = await Activity.create({ type, actor: { id: userId, name: user?.username || 'Unknown', type: ActivityService.isAgentUsername(user?.username) ? 'agent' : 'human', verified: false }, action, content, podId, target, agentMetadata }); + // Store the id of the pod that was actually resolved and authorised, not + // the body's copy of it. + const activity = await Activity.create({ type, actor: { id: userId, name: user?.username || 'Unknown', type: ActivityService.isAgentUsername(user?.username) ? 'agent' : 'human', verified: false }, action, content, podId: pod._id, target, agentMetadata }); return res.json({ success: true, activity: { id: activity._id.toString(), type: activity.type, action: activity.action, content: activity.content, createdAt: activity.createdAt } }); } catch (error) { console.error('Error creating activity:', error);