From f1adf39bb5f75f62cd55c44f8b1511c242b31908 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:07:03 -0700 Subject: [PATCH] refactor(authz): the socket write path runs the shared membership rule, not a copy (TASK-165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `server.ts` defined its own `isPodMember` for the socket post path and exported it, so the strict rule had two definitions in `backend/`. The copy had also drifted from the rule it exists to mirror: it compared `member.toString()`, which on a member document Mongo has populated renders `[object Object]` — so a populated member was admitted by `createMessage` and refused by the socket path beside it. The socket write path now calls `isListedPodMember` from `utils/isPodMember` — the export TASK-161 placed beside the permissive predicate, and the one #1942's two readers import — and this module no longer exports a membership predicate of its own, so the rule has one home and no second name. Witnesses sit at the call site, not the definition: a departed creator (`createdBy` present, `members` empty, which is the shape `leavePod` leaves) is refused on the socket write path, and a populated member document is admitted — the second arm is the one the removed copy failed, so it reddens if a local copy returns, and the first reddens if the write path is pointed at the permissive predicate instead. The helper arm that read the removed export now reads the rule where it lives. No behaviour changes for string or ObjectId members. --- backend/__tests__/unit/server.test.js | 64 +++++++++++++++++++++++++-- backend/server.ts | 17 ++++--- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/backend/__tests__/unit/server.test.js b/backend/__tests__/unit/server.test.js index 0bae80238..482048a9f 100644 --- a/backend/__tests__/unit/server.test.js +++ b/backend/__tests__/unit/server.test.js @@ -178,13 +178,16 @@ describe('server websocket authorization helpers', () => { delete process.env.PG_HOST; }); - it('treats string and ObjectId-like members as valid pod members', () => { + it('treats string and ObjectId-like members as valid pod members, and no creator', () => { jest.resetModules(); + // The rule moved out of this module in TASK-165, so this arm now reads it + // where it lives; the socket path's own use of it is covered below, at the + // call site rather than at the definition. // eslint-disable-next-line global-require, import/no-unresolved, import/extensions - const { isPodMember } = require('../../server'); + const { isListedPodMember } = require('../../utils/isPodMember'); expect( - isPodMember( + isListedPodMember( { members: [ { toString: () => 'user-1' }, @@ -194,6 +197,61 @@ describe('server websocket authorization helpers', () => { 'user-2', ), ).toBe(true); + expect( + isListedPodMember({ createdBy: { toString: () => 'user-3' }, members: [] }, 'user-3'), + ).toBe(false); + }); + + it('refuses a departed creator on the socket write path', async () => { + jest.resetModules(); + // eslint-disable-next-line global-require, import/no-unresolved, import/extensions + const Pod = require('../../models/Pod'); + // eslint-disable-next-line global-require, import/no-unresolved, import/extensions + const { authorizeSocketPodAccess } = require('../../server'); + // `leavePod` filters `members` and leaves `createdBy` in place, so this is + // the shape a departed creator has: still named by the pod, no longer listed. + Pod.findById.mockResolvedValue({ + _id: 'pod-1', + createdBy: { toString: () => 'user-1' }, + members: [], + }); + const socket = { + userId: 'user-1', + emit: jest.fn(), + }; + + const result = await authorizeSocketPodAccess(socket, 'pod-1', 'post'); + + expect(result).toBeNull(); + expect(socket.emit).toHaveBeenCalledWith('error', { + message: 'Not authorized to post for this pod', + }); + }); + + it('admits a populated member document, so the socket path runs the shared predicate', async () => { + jest.resetModules(); + // eslint-disable-next-line global-require, import/no-unresolved, import/extensions + const Pod = require('../../models/Pod'); + // eslint-disable-next-line global-require, import/no-unresolved, import/extensions + const { authorizeSocketPodAccess } = require('../../server'); + // The copy TASK-165 removed compared `member.toString()`, which on a + // populated document renders `[object Object]` — this member was refused by + // the socket path and admitted by `createMessage` at the same moment. The + // arm reddens if a local copy comes back. + const pod = { + _id: 'pod-1', + members: [{ _id: { toString: () => 'user-1' } }], + }; + Pod.findById.mockResolvedValue(pod); + const socket = { + userId: 'user-1', + emit: jest.fn(), + }; + + const result = await authorizeSocketPodAccess(socket, 'pod-1', 'post'); + + expect(result).toBe(pod); + expect(socket.emit).not.toHaveBeenCalled(); }); it('rejects socket pod joins for non-members', async () => { diff --git a/backend/server.ts b/backend/server.ts index eeaa5e234..6dbe60bd3 100644 --- a/backend/server.ts +++ b/backend/server.ts @@ -500,13 +500,13 @@ const emitPresence = async (podId: any) => { } }; -const isPodMember = (pod: any, userId: any) => { - if (!pod || !userId) { - return false; - } - - return (pod.members || []).some((member: any) => member?.toString() === userId.toString()); -}; +// The pod's own membership rule has ONE definition: `utils/isPodMember`. This +// module kept a copy, and the copy had already drifted from `createMessage` on +// populated member docs — `member.toString()` renders `[object Object]`, so a +// member Mongo had populated was admitted by the write path and refused by the +// socket that mirrors it. TASK-165. +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { isListedPodMember } = require('./utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const DMServiceForSocketAuth = require('./services/dmService'); @@ -533,7 +533,7 @@ const authorizeSocketPodAccess = async (socket: any, podId: any, action: any) => const isReadAction = action === 'join'; const allowed = isReadAction ? await DMServiceForSocketAuth.canViewPod(socket.userId, pod) - : isPodMember(pod, socket.userId); + : isListedPodMember(pod, socket.userId); if (!allowed) { console.error(`Socket error: Not authorized to ${action} for this pod`, { @@ -811,6 +811,5 @@ if (require.main === module) { module.exports = { app, server, - isPodMember, authorizeSocketPodAccess, };