diff --git a/backend/__tests__/unit/controllers/pgPodController.test.js b/backend/__tests__/unit/controllers/pgPodController.test.js deleted file mode 100644 index 8559a0531..000000000 --- a/backend/__tests__/unit/controllers/pgPodController.test.js +++ /dev/null @@ -1,38 +0,0 @@ -const controller = require('../../../controllers/pgPodController'); -const PGPod = require('../../../models/pg/Pod'); -const PGMessage = require('../../../models/pg/Message'); - -jest.mock('../../../models/pg/Pod'); -jest.mock('../../../models/pg/Message'); - -describe('pgPodController', () => { - afterEach(() => jest.clearAllMocks()); - - it('joinPod returns 404 when pod does not exist', async () => { - PGPod.findById.mockResolvedValue(null); - const req = { params: { id: 'p1' }, user: { id: 'u1' } }; - const res = { - status: jest.fn().mockReturnThis(), - json: jest.fn(), - send: jest.fn(), - }; - await controller.joinPod(req, res); - expect(res.status).toHaveBeenCalledWith(404); - }); - - it('updatePod rejects if not creator', async () => { - PGPod.findById.mockResolvedValue({ created_by: 'other' }); - const req = { - params: { id: 'p1' }, - body: { name: 'n' }, - user: { id: 'u1' }, - }; - const res = { - status: jest.fn().mockReturnThis(), - json: jest.fn(), - send: jest.fn(), - }; - await controller.updatePod(req, res); - expect(res.status).toHaveBeenCalledWith(401); - }); -}); diff --git a/backend/__tests__/unit/routes/pg-pods.test.js b/backend/__tests__/unit/routes/pg-pods.test.js deleted file mode 100644 index 9638ed79a..000000000 --- a/backend/__tests__/unit/routes/pg-pods.test.js +++ /dev/null @@ -1,28 +0,0 @@ -const request = require('supertest'); -const express = require('express'); - -jest.mock('../../../controllers/pgPodController', () => ({ - getAllPods: jest.fn((req, res) => res.status(200).end()), - getPodById: jest.fn((req, res) => res.status(200).end()), - createPod: jest.fn((req, res) => res.status(201).end()), - updatePod: jest.fn((req, res) => res.status(200).end()), - deletePod: jest.fn((req, res) => res.status(200).end()), - joinPod: jest.fn((req, res) => res.status(200).end()), - leavePod: jest.fn((req, res) => res.status(200).end()), -})); - -jest.mock('../../../middleware/auth', () => (req, res, next) => next()); - -const routes = require('../../../routes/pg-pods'); -const controllers = require('../../../controllers/pgPodController'); - -describe('pg pods routes', () => { - const app = express(); - app.use(express.json()); - app.use('/api/pg/pods', routes); - - it('GET /api/pg/pods calls getAllPods', async () => { - await request(app).get('/api/pg/pods').expect(200); - expect(controllers.getAllPods).toHaveBeenCalled(); - }); -}); diff --git a/backend/__tests__/unit/server.test.js b/backend/__tests__/unit/server.test.js index 87c3c50ed..4acb0c8ab 100644 --- a/backend/__tests__/unit/server.test.js +++ b/backend/__tests__/unit/server.test.js @@ -43,10 +43,6 @@ jest.mock('../../routes/pg-status', () => { r.get('/', (req, res) => res.json({ available: true })); return r; }); -jest.mock('../../routes/pg-pods', () => { - const ex = require('express'); - return ex.Router(); -}); jest.mock('../../routes/pg-messages', () => { const ex = require('express'); return ex.Router(); diff --git a/backend/__tests__/unit/services/pgPodSyncService.attribution.test.js b/backend/__tests__/unit/services/pgPodSyncService.attribution.test.js new file mode 100644 index 000000000..c029e43cd --- /dev/null +++ b/backend/__tests__/unit/services/pgPodSyncService.attribution.test.js @@ -0,0 +1,88 @@ +/** + * Regression: the PG backfill let a non-member grant themselves access. + * + * `PGPod.create` inserts its `created_by` argument into `pod_members` as a side + * effect. `syncPodFromMongo` was passing the REQUESTING user as `created_by`, + * and `pgMessageController.getMessages` ran that backfill BEFORE its membership + * check — so a non-member asking for a pod not yet mirrored into PG caused the + * server to insert them as a member, and the check on the next line read that + * row back and let them through. A complete read bypass. + * + * The manufactured row is worse than a one-request bypass: PG `pod_members` is + * also trusted as a positive access signal by `reactionController` and by + * `isMemberWithFallback` on every later request, so it persists. + * + * Two properties are pinned here: + * 1. `created_by` mirrors the Mongo pod's real owner, never the requester + * 2. the requester is not silently added to the member list + */ + +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); + +const mockPGPod = { create: jest.fn(), addMember: jest.fn().mockResolvedValue(undefined) }; +const mockMongoPod = { findById: jest.fn() }; + +jest.mock('../../../models/pg/Pod', () => mockPGPod); +jest.mock('../../../models/Pod', () => mockMongoPod); + +const { syncPodFromMongo } = require('../../../services/pgPodSyncService'); + +const OWNER = 'real-owner-id'; +const INTRUDER = 'intruder-id'; +const MEMBER = 'legit-member-id'; +const POD = 'pod-123'; + +describe('syncPodFromMongo attribution', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockPGPod.create.mockResolvedValue({ id: POD }); + mockMongoPod.findById.mockReturnValue({ + lean: jest.fn().mockResolvedValue({ + name: 'Private Pod', + description: '', + type: 'chat', + createdBy: OWNER, + members: [MEMBER], + }), + }); + }); + + test('created_by is the Mongo pod owner, NOT whoever triggered the backfill', async () => { + await syncPodFromMongo(POD, INTRUDER); + + const createdBy = mockPGPod.create.mock.calls[0][3]; + expect(createdBy).toBe(OWNER); + expect(createdBy).not.toBe(INTRUDER); + }); + + test('the requester is not inserted into the member list', async () => { + await syncPodFromMongo(POD, INTRUDER); + + // addMember is called for the real Mongo members only. PGPod.create adds + // created_by separately, which is now the owner — so the intruder appears + // nowhere. + const added = mockPGPod.addMember.mock.calls.map((c) => c[1]); + expect(added).toContain(MEMBER); + expect(added).not.toContain(INTRUDER); + expect(mockPGPod.create.mock.calls[0][3]).not.toBe(INTRUDER); + }); + + test('falls back to the requester only when Mongo has no owner recorded', async () => { + mockMongoPod.findById.mockReturnValue({ + lean: jest.fn().mockResolvedValue({ + name: 'Ownerless', type: 'chat', members: [], + }), + }); + + await syncPodFromMongo(POD, INTRUDER); + + expect(mockPGPod.create.mock.calls[0][3]).toBe(INTRUDER); + }); + + test('returns null for a pod that does not exist in Mongo', async () => { + mockMongoPod.findById.mockReturnValue({ lean: jest.fn().mockResolvedValue(null) }); + + await expect(syncPodFromMongo(POD, INTRUDER)).resolves.toBeNull(); + expect(mockPGPod.create).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/controllers/pgMessageController.ts b/backend/controllers/pgMessageController.ts index 464eb7bea..eeda1aa59 100644 --- a/backend/controllers/pgMessageController.ts +++ b/backend/controllers/pgMessageController.ts @@ -51,10 +51,17 @@ exports.getMessages = async (req: AuthRequest, res: Response): Promise => return; } + // Order matters. syncPodFromMongo inserts the pod's owner into pod_members + // as a side effect of PGPod.create, so backfilling BEFORE the membership + // check let a non-member manufacture the very row the check then read back + // — a complete read bypass on any pod not yet mirrored into PG. Existence + // is resolved first (read-only, so it cannot grant anything) to preserve + // the 404-for-missing / 401-for-non-member contract, then authorization, + // and only then the backfill. let pod = await PGPod.findById(podId); if (!pod) { - pod = await syncPodFromMongo(podId, userId); - if (!pod) { + const mongoPod = await MongoPod.findById(podId).select('_id').lean(); + if (!mongoPod) { res.status(404).json({ msg: 'Pod not found' }); return; } @@ -66,6 +73,14 @@ exports.getMessages = async (req: AuthRequest, res: Response): Promise => return; } + if (!pod) { + pod = await syncPodFromMongo(podId, userId); + if (!pod) { + res.status(404).json({ msg: 'Pod not found' }); + return; + } + } + const messages = await PGMessage.findByPodId(podId, limit, before); res.json(messages); } catch (err) { diff --git a/backend/controllers/pgPodController.ts b/backend/controllers/pgPodController.ts deleted file mode 100644 index 331a03c59..000000000 --- a/backend/controllers/pgPodController.ts +++ /dev/null @@ -1,174 +0,0 @@ -import type { Request, Response } from 'express'; - -// eslint-disable-next-line global-require -const PGPod = require('../models/pg/Pod'); -// eslint-disable-next-line global-require -const PGMessage = require('../models/pg/Message'); - -interface AuthRequest extends Request { - user?: { id: string }; -} - -exports.getAllPods = async (req: Request, res: Response): Promise => { - try { - const { type } = req.query as { type?: string }; - const pods = await PGPod.findAll(type); - res.json(pods); - } catch (err) { - const e = err as { message?: string }; - console.error(e.message); - res.status(500).send('Server Error'); - } -}; - -exports.getPodById = async (req: Request, res: Response): Promise => { - try { - const pod = await PGPod.findById(req.params.id); - if (!pod) { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.json(pod); - } catch (err) { - const e = err as { message?: string; kind?: string }; - console.error(e.message); - if (e.kind === 'ObjectId') { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.status(500).send('Server Error'); - } -}; - -exports.createPod = async (req: AuthRequest, res: Response): Promise => { - try { - const { name, description, type } = req.body as { name?: string; description?: string; type?: string }; - const newPod = await PGPod.create(name, description, type, req.user?.id); - res.json(newPod); - } catch (err) { - const e = err as { message?: string }; - console.error(e.message); - res.status(500).send('Server Error'); - } -}; - -exports.updatePod = async (req: AuthRequest, res: Response): Promise => { - try { - const { name, description } = req.body as { name?: string; description?: string }; - const pod = await PGPod.findById(req.params.id); - if (!pod) { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - if ((pod as { created_by: string }).created_by !== req.user?.id) { - res.status(401).json({ msg: 'Not authorized to update this pod' }); - return; - } - const updatedPod = await PGPod.update(req.params.id, name, description); - res.json(updatedPod); - } catch (err) { - const e = err as { message?: string; kind?: string }; - console.error(e.message); - if (e.kind === 'ObjectId') { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.status(500).send('Server Error'); - } -}; - -exports.deletePod = async (req: AuthRequest, res: Response): Promise => { - try { - const pod = await PGPod.findById(req.params.id); - if (!pod) { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - if ((pod as { created_by: string }).created_by !== req.user?.id) { - res.status(401).json({ msg: 'Not authorized to delete this pod' }); - return; - } - await PGMessage.deleteByPodId(req.params.id); - await PGPod.delete(req.params.id); - res.json({ msg: 'Pod deleted' }); - } catch (err) { - const e = err as { message?: string; kind?: string }; - console.error(e.message); - if (e.kind === 'ObjectId') { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.status(500).send('Server Error'); - } -}; - -exports.joinPod = async (req: AuthRequest, res: Response): Promise => { - try { - console.log('Join pod request received:', { - podId: req.params.id, - userId: req.user?.id, - userIdType: typeof req.user?.id, - }); - - const pod = await PGPod.findById(req.params.id); - if (!pod) { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - - const isMember = await PGPod.isMember(req.params.id, req.user?.id); - if (isMember) { - console.log(`User ${req.user?.id} is already a member of pod ${req.params.id}`); - const updatedPod = await PGPod.findById(req.params.id); - res.json(updatedPod); - return; - } - - await PGPod.addMember(req.params.id, req.user?.id); - console.log(`User ${req.user?.id} successfully added to pod ${req.params.id}`); - - const membershipVerified = await PGPod.isMember(req.params.id, req.user?.id); - if (!membershipVerified) { - console.error(`Failed to verify membership after adding user ${req.user?.id} to pod ${req.params.id}`); - } - - const updatedPod = await PGPod.findById(req.params.id); - res.json(updatedPod); - } catch (err) { - const e = err as { message?: string; kind?: string }; - console.error('Error in pgPodController.joinPod:', e.message); - console.error('Request details:', { - podId: req.params.id, - userId: (req as AuthRequest).user?.id || 'undefined', - }); - if (e.kind === 'ObjectId') { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.status(500).send('Server Error'); - } -}; - -exports.leavePod = async (req: AuthRequest, res: Response): Promise => { - try { - const pod = await PGPod.findById(req.params.id); - if (!pod) { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - if ((pod as { created_by: string }).created_by === req.user?.id) { - res.status(400).json({ msg: 'Pod creator cannot leave the pod' }); - return; - } - await PGPod.removeMember(req.params.id, req.user?.id); - res.json({ msg: 'Left pod successfully' }); - } catch (err) { - const e = err as { message?: string; kind?: string }; - console.error(e.message); - if (e.kind === 'ObjectId') { - res.status(404).json({ msg: 'Pod not found' }); - return; - } - res.status(500).send('Server Error'); - } -}; diff --git a/backend/routes/pg-messages.ts b/backend/routes/pg-messages.ts index 7944bb215..4ce9dcedf 100644 --- a/backend/routes/pg-messages.ts +++ b/backend/routes/pg-messages.ts @@ -1,3 +1,9 @@ +// ESM import (not require) so CodeQL's js/missing-rate-limiting query can +// trace the middleware — it does not follow a rate-limit factory through a +// require() return. Same shape as routes/messages.ts. +import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import { createHash } from 'crypto'; + // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -10,12 +16,50 @@ const { deleteMessage, } = require('../controllers/pgMessageController'); +interface RateLimitReq { get?: (name: string) => string | undefined; ip?: string } +interface RateLimitRes { status: (code: number) => { json: (body: unknown) => void } } + +// Keyed on the bearer token (hashed) rather than IP, so users behind one NAT +// don't share a bucket; falls back to the IPv6-safe key generator for the +// unauth path. Limits mirror routes/messages.ts, which fronts the same tables. +const keyByToken = (req: RateLimitReq) => { + const authHeader = req.get?.('authorization'); + if (authHeader) { + return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; + } + return req.ip ? ipKeyGenerator(req.ip) : 'anon'; +}; + +const readLimit = rateLimit({ + windowMs: 60_000, + max: 240, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: keyByToken, + handler: (_req: unknown, res: RateLimitRes) => { + res.status(429).json({ msg: 'rate limit exceeded: 240 reads per 60s' }); + }, +}); + +const writeLimit = rateLimit({ + windowMs: 60_000, + max: 60, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: keyByToken, + handler: (_req: unknown, res: RateLimitRes) => { + res.status(429).json({ msg: 'rate limit exceeded: 60 writes per 60s' }); + }, +}); + const router: ReturnType = express.Router(); -router.get('/:podId', auth, getMessages); -router.post('/:podId', auth, createMessage); -router.put('/:id', auth, updateMessage); -router.delete('/:id', auth, deleteMessage); +// Limiter first on every route, so the query sees it ahead of the +// database-touching handler. +router.get('/:podId', readLimit, auth, getMessages); +router.post('/:podId', writeLimit, auth, createMessage); +router.put('/:id', writeLimit, auth, updateMessage); +router.delete('/:id', writeLimit, auth, deleteMessage); module.exports = router; diff --git a/backend/routes/pg-pods.ts b/backend/routes/pg-pods.ts deleted file mode 100644 index dcb877b7d..000000000 --- a/backend/routes/pg-pods.ts +++ /dev/null @@ -1,28 +0,0 @@ -// eslint-disable-next-line global-require -const express = require('express'); -// eslint-disable-next-line global-require -const auth = require('../middleware/auth'); -// eslint-disable-next-line global-require -const { - getAllPods, - getPodById, - createPod, - updatePod, - deletePod, - joinPod, - leavePod, -} = require('../controllers/pgPodController'); - -const router: ReturnType = express.Router(); - -router.get('/', auth, getAllPods); -router.get('/:id', auth, getPodById); -router.post('/', auth, createPod); -router.put('/:id', auth, updatePod); -router.delete('/:id', auth, deletePod); -router.post('/:id/join', auth, joinPod); -router.post('/:id/leave', auth, leavePod); - -module.exports = router; - -export {}; diff --git a/backend/server.ts b/backend/server.ts index dddec9ab5..7b3480bac 100644 --- a/backend/server.ts +++ b/backend/server.ts @@ -52,7 +52,6 @@ const agentEventsAdminRoutes = require('./routes/admin/agentEvents'); const adminUsersRoutes = require('./routes/admin/users'); const adminAnalyticsRoutes = require('./routes/admin/analytics'); // Conditionally load PostgreSQL routes and models -let pgPodRoutes: any; let pgMessageRoutes: any; let pgStatusRoutes: any; let PGMessage: any; @@ -66,7 +65,6 @@ const AgentMentionService = require('./services/agentMentionService'); let pgAvailable = false; if (process.env.PG_HOST) { - pgPodRoutes = require('./routes/pg-pods'); pgMessageRoutes = require('./routes/pg-messages'); pgStatusRoutes = require('./routes/pg-status'); PGMessage = require('./models/pg/Message'); @@ -309,7 +307,15 @@ if (process.env.PG_HOST) { // Set global flag that PostgreSQL is available pgAvailable = true; // Register PostgreSQL routes for chat functionality - app.use('/api/pg/pods', pgPodRoutes); + // '/api/pg/pods' is deliberately NOT mounted. It exposed an + // unauthorized shadow copy of the pod API: getAllPods returned + // every pod on the instance with no membership filter, joinPod + // had no join-policy check at all (a non-member could join a + // private pod and get a 200), and deletePod gated on a + // created_by value that the sync path let a requester claim. + // It had zero callers anywhere in the repo — the frontend uses + // /api/pods, and only /api/pg/messages + /api/pg/status are live + // (ChatRoom, SocketContext). Removed rather than patched. app.use('/api/pg/messages', pgMessageRoutes); app.use('/api/pg/status', pgStatusRoutes); console.log( diff --git a/backend/services/pgPodSyncService.ts b/backend/services/pgPodSyncService.ts index cda67d1a0..881b77c41 100644 --- a/backend/services/pgPodSyncService.ts +++ b/backend/services/pgPodSyncService.ts @@ -19,6 +19,7 @@ interface MongoPodLean { name?: string; description?: string; type?: string; + createdBy?: { toString(): string }; members?: Array<{ toString(): string }>; } @@ -37,11 +38,18 @@ export async function syncPodFromMongo( ): Promise { const mongoPod = await MongoPod.findById(podId).lean() as MongoPodLean | null; if (!mongoPod) return null; + // `created_by` MUST mirror Mongo's real owner, never whoever happened to + // trigger the backfill. PGPod.create also inserts created_by into + // pod_members, so attributing it to the requester manufactured a membership + // row for a non-member — which `isMemberWithFallback` and + // `reactionController.callerHasPodAccess` then trusted as proof of access, + // and which the now-removed pg deletePod trusted as proof of ownership. + const ownerId = mongoPod.createdBy ? String(mongoPod.createdBy) : requestingUserId; const pod = await PGPod.create( mongoPod.name, mongoPod.description || '', mongoPod.type || 'chat', - requestingUserId, + ownerId, podId, ); if (Array.isArray(mongoPod.members)) {