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..5e6572431 --- /dev/null +++ b/backend/__tests__/unit/routes/activity.write-membership.test.js @@ -0,0 +1,158 @@ +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 podFindById = jest.fn(); +const mockPod = (pod) => { + podFindById.mockImplementation(() => ({ select: () => ({ lean: async () => pod }) })); + jest.doMock('../../../models/Pod', () => ({ findById: podFindById })); +}; + +const buildApp = () => { + const app = express(); + app.use(express.json()); + app.use('/api/activity', require('../../../routes/activity')); + return app; +}; + +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(), + })); + 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({ _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({ _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({ _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({ _id: 'pod-1', 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 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(); }); + + // 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({ _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({ _id: 'pod-1', 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({ _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({ _id: 'pod-1', 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..deec6464d 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(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 }; if (!result.success) return res.status(400).json({ error: result.error }); return res.json(result); @@ -223,8 +230,23 @@ 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' }); + // `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); 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 {};