From a9facf7c0915a93d87619e5ab363fbf65429abc2 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 21:08:24 +0200 Subject: [PATCH 1/4] fix(organizations): close self-join via shadowed Organization policy subject --- .../organizations.membership.controller.js | 17 +- .../policies/organizations.policy.js | 11 +- ...ations.membership.controller.unit.tests.js | 20 ++ .../tests/organizations.policy.unit.tests.js | 15 ++ .../tests/organizations.selfJoin.e2e.tests.js | 213 ++++++++++++++++++ 5 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 modules/organizations/tests/organizations.selfJoin.e2e.tests.js diff --git a/modules/organizations/controllers/organizations.membership.controller.js b/modules/organizations/controllers/organizations.membership.controller.js index d2f2a41a5..944780983 100644 --- a/modules/organizations/controllers/organizations.membership.controller.js +++ b/modules/organizations/controllers/organizations.membership.controller.js @@ -78,9 +78,22 @@ const addMember = async (req, res) => { const { userId, role } = req.body; const requestedRole = role || MEMBERSHIP_ROLES.MEMBER; - // Elevated-role guard: only owners (or global admins) may invite an owner/admin. + // Actor-role gate (mirrors updateRole/remove): only an org owner/admin (or a global + // admin) may add a member. The type-level CASL `create Membership` check does not + // carry the `{organizationId}` org-scope condition, so a non-member / plain member + // must be rejected here. Without this, the Organization-subject shadowing bug aside, + // a plain member could still inject a membership into their own org. const isPlatformAdmin = isGlobalAdmin(req.user); - const actorIsOwner = req.membership?.role === MEMBERSHIP_ROLES.OWNER; + const actorRole = req.membership?.role; + const canAdd = isPlatformAdmin + || actorRole === MEMBERSHIP_ROLES.OWNER + || actorRole === MEMBERSHIP_ROLES.ADMIN; + if (!canAdd) { + return responses.error(res, 403, 'Forbidden', 'Insufficient organization role')(); + } + + // Elevated-role guard: only owners (or global admins) may invite an owner/admin. + const actorIsOwner = actorRole === MEMBERSHIP_ROLES.OWNER; if (requestedRole !== MEMBERSHIP_ROLES.MEMBER && !isPlatformAdmin && !actorIsOwner) { return responses.error(res, 403, 'Forbidden', 'Only owners can add a member with an elevated role')(); } diff --git a/modules/organizations/policies/organizations.policy.js b/modules/organizations/policies/organizations.policy.js index 08c45f188..574317a4b 100644 --- a/modules/organizations/policies/organizations.policy.js +++ b/modules/organizations/policies/organizations.policy.js @@ -16,12 +16,21 @@ export function organizationSubjectRegistration({ registerDocumentSubject, regis registerDocumentSubject('membershipDoc', 'Membership'); // Guard: only resolve req.organization as an Organization subject on actual organization routes. // Other modules (billing, tasks, etc.) also set req.organization but authorize via their own subjects. + // + // The /members routes are deliberately EXCLUDED: req.organization is loaded there too, + // but membership management must authorize via the dedicated Membership path-subject + // (owner/admin gate), NOT the unconditional `create Organization` grant — otherwise the + // Organization document-subject is resolved first (resolveSubject is first-match-wins) and + // shadows the Membership subject, letting any authenticated user inject a membership. + // Do NOT exclude /requests — that is the any-user JOIN-REQUEST flow, which legitimately + // relies on `create Organization` (excluding it would 403 legitimate join requests). registerDocumentSubject('organization', 'Organization', (req) => { if (!req.route?.path) { return false; } const p = req.route.path; - return p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations'); + const onOrgRoute = p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations'); + return onOrgRoute && !p.includes('/members'); }); registerPathSubject((p) => p.startsWith('/api/admin/organizations'), 'Organization'); registerPathSubject((p) => p.startsWith('/api/organizations') && p.includes('/requests'), 'Membership'); diff --git a/modules/organizations/tests/organizations.membership.controller.unit.tests.js b/modules/organizations/tests/organizations.membership.controller.unit.tests.js index fc895d0a3..58d801558 100644 --- a/modules/organizations/tests/organizations.membership.controller.unit.tests.js +++ b/modules/organizations/tests/organizations.membership.controller.unit.tests.js @@ -253,6 +253,26 @@ describe('Membership controller unit tests:', () => { expect(res.status).not.toHaveBeenCalledWith(403); }); + test('a plain MEMBER cannot add anyone → 403, service not called', async () => { + const req = mockReq({ membership: { role: MEMBERSHIP_ROLES.MEMBER }, body: { userId: 'u9' } }); + const res = mockRes(); + + await membershipController.addMember(req, res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(mockAddMember).not.toHaveBeenCalled(); + }); + + test('a non-member (no req.membership) cannot add anyone → 403, fail closed', async () => { + const req = mockReq({ user: { _id: 'u1', roles: ['user'] }, membership: undefined, body: { userId: 'u9' } }); + const res = mockRes(); + + await membershipController.addMember(req, res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(mockAddMember).not.toHaveBeenCalled(); + }); + test('admin CANNOT add an elevated role (owner/admin) → 403, service not called', async () => { const req = mockReq({ membership: { role: MEMBERSHIP_ROLES.ADMIN }, body: { userId: 'u9', role: MEMBERSHIP_ROLES.ADMIN } }); const res = mockRes(); diff --git a/modules/organizations/tests/organizations.policy.unit.tests.js b/modules/organizations/tests/organizations.policy.unit.tests.js index 2b3b88c82..fc936d16c 100644 --- a/modules/organizations/tests/organizations.policy.unit.tests.js +++ b/modules/organizations/tests/organizations.policy.unit.tests.js @@ -61,6 +61,21 @@ describe('organizationSubjectRegistration policy unit tests:', () => { expect(guard({ route: { path: '/api/admin/organizations/:id' } })).toBe(true); }); + test('should still return true for the join-request flow (/:id/requests)', () => { + // The any-user join-request flow legitimately relies on the Organization + // subject's unconditional `create` grant — it must NOT be carved out. + expect(guard({ route: { path: '/api/organizations/:organizationId/requests' } })).toBe(true); + }); + + test('should return false for the /members management routes', () => { + // /members must authorize via the dedicated Membership path-subject (owner/admin + // gate), not the unconditional `create Organization` grant — excluding it here + // prevents the Organization subject from shadowing the Membership subject. + expect(guard({ route: { path: '/api/organizations/:organizationId/members' } })).toBe(false); + expect(guard({ route: { path: '/api/organizations/:organizationId/members/:memberId' } })).toBe(false); + expect(guard({ route: { path: '/api/admin/organizations/:id/members' } })).toBe(false); + }); + test('should return false for unrelated paths (e.g. billing routes)', () => { expect(guard({ route: { path: '/api/billing/plans' } })).toBe(false); expect(guard({ route: { path: '/api/tasks' } })).toBe(false); diff --git a/modules/organizations/tests/organizations.selfJoin.e2e.tests.js b/modules/organizations/tests/organizations.selfJoin.e2e.tests.js new file mode 100644 index 000000000..cd5095596 --- /dev/null +++ b/modules/organizations/tests/organizations.selfJoin.e2e.tests.js @@ -0,0 +1,213 @@ +/** + * @desc Security regression E2E: a non-member must NOT be able to self-join (or + * inject any membership into) an organization via POST /:id/members. + * + * Mechanism guarded: the Organization document-subject was resolved before the + * dedicated Membership path-subject on the /members route, so the unconditional + * `create Organization` grant authorized the request for ANY authenticated user. + * The members POST must instead authorize via the Membership subject (owner/admin + * gate) — a non-member has no `create Membership` ability → 403, no row created. + * + * Also asserts the legitimate any-user JOIN-REQUEST flow (POST /:id/requests) is + * unaffected — that flow LEGITIMATELY relies on the create-Organization grant. + */ +import request from 'supertest'; +import path from 'path'; + +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; +import config from '../../../config/index.js'; + +describe('Organizations self-join authorization E2E tests:', () => { + let UserService; + let OrganizationsRepository; + let MembershipRepository; + let agent; + + const password = 'W@os.jsI$Aw3$0m3'; + + // Store original config + const originalOrganizations = { ...config.organizations }; + + /** + * @description Reset organizations config to original state. + */ + const resetOrgConfig = () => { + config.organizations = { ...originalOrganizations }; + }; + + /** + * @description Clean up a user and their associated organizations/memberships. + * @param {Object} user - The user object to clean up. + * @returns {Promise} + */ + const cleanupUser = async (user) => { + if (!user) return; + try { + const memberships = await MembershipRepository.list({ userId: user.id || user._id }); + for (const m of memberships) { + const orgId = m.organizationId._id || m.organizationId; + await MembershipRepository.deleteMany({ organizationId: orgId }); + await OrganizationsRepository.deleteMany({ _id: orgId }); + } + await UserService.remove(user); + } catch (_) { /* cleanup — ignore errors */ } + }; + + beforeAll(async () => { + try { + const init = await bootstrap(); + UserService = (await import(path.resolve('./modules/users/services/users.service.js'))).default; + OrganizationsRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.repository.js'))).default; + MembershipRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.membership.repository.js'))).default; + agent = request.agent(init.app); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + describe('Non-member cannot self-join via POST /:id/members', () => { + let owner; + let outsider; + let org; + let agentOwner; + let agentOutsider; + + // Unique-per-run emails to avoid dirty-DB re-run flakiness + const suffix = Date.now(); + const emailOwner = `self-join-owner-${suffix}@test.com`; + const emailOutsider = `self-join-outsider-${suffix}@test.com`; + + beforeAll(() => { + config.organizations = { enabled: true, autoCreate: true, domainMatching: false }; + }); + + afterAll(async () => { + resetOrgConfig(); + try { + if (org) { + await MembershipRepository.deleteMany({ organizationId: org._id }); + await OrganizationsRepository.deleteMany({ _id: org._id }); + } + } catch (_) { /* cleanup */ } + await cleanupUser(outsider); + await cleanupUser(owner); + }); + + test('a non-member self-adding to an org is rejected and creates no membership', async () => { + agentOwner = request.agent(agent.app); + agentOutsider = request.agent(agent.app); + + // 1. Signup owner (auto-creates org), then an unrelated outsider + try { + const resOwner = await agentOwner + .post('/api/auth/signup') + .send({ + firstName: 'SelfJoinOwner', + lastName: 'User', + email: emailOwner, + password, + provider: 'local', + }) + .expect(200); + owner = resOwner.body.user; + org = resOwner.body.organization; + expect(org).toBeDefined(); + expect(org).not.toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + try { + const resOutsider = await agentOutsider + .post('/api/auth/signup') + .send({ + firstName: 'SelfJoinOutsider', + lastName: 'User', + email: emailOutsider, + password, + provider: 'local', + }) + .expect(200); + outsider = resOutsider.body.user; + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // 2. The outsider (NOT a member of org) tries to inject a membership for + // themselves. This must be forbidden (401/403) — the prior bug let it + // through via the unconditional create-Organization grant. + try { + const selfAddRes = await agentOutsider + .post(`/api/organizations/${org._id}/members`) + .send({ userId: outsider.id, role: 'member' }); + expect([401, 403]).toContain(selfAddRes.status); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // 3. No membership row may have been created for the outsider in this org. + try { + const injected = await MembershipRepository.findOne({ + userId: outsider.id, + organizationId: org._id, + }); + expect(injected).toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + test('the legitimate any-user JOIN-REQUEST flow (POST /:id/requests) still works for a non-member', async () => { + // Precondition: the prior test must have seeded the outsider + org + expect(org && outsider && agentOutsider).toBeTruthy(); + + let requestId; + try { + const joinRes = await agentOutsider + .post(`/api/organizations/${org._id}/requests`) + .expect(200); + expect(joinRes.body.message).toBe('membership request created'); + requestId = joinRes.body.data._id; + expect(requestId).toBeDefined(); + expect(joinRes.body.data.status).toBe('pending'); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // The owner approving it confirms the request is a real, owner-approvable + // join_request (not an owner_add) — i.e. the carve-out preserved the flow. + try { + await agentOwner + .put(`/api/organizations/${org._id}/requests/${requestId}/approve`) + .expect(200); + const active = await MembershipRepository.findOne({ + userId: outsider.id, + organizationId: org._id, + status: 'active', + }); + expect(active).not.toBeNull(); + expect(active.role).toBe('member'); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + }); + + // Mongoose disconnect + afterAll(async () => { + try { + await mongooseService.disconnect(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); +}); From de34bbfd3fbcdd1b4d98ff79952dc5e39cd9880b Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 21:21:47 +0200 Subject: [PATCH 2/4] fix(organizations): mirror /members carve-out on the admin path-subject --- .../policies/organizations.policy.js | 20 ++++++- .../tests/organizations.policy.unit.tests.js | 58 ++++++++++++++++++- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/modules/organizations/policies/organizations.policy.js b/modules/organizations/policies/organizations.policy.js index 574317a4b..266dc9982 100644 --- a/modules/organizations/policies/organizations.policy.js +++ b/modules/organizations/policies/organizations.policy.js @@ -3,6 +3,15 @@ */ import { MEMBERSHIP_ROLES } from '../lib/constants.js'; +/** + * Whether a route path targets an organization resource (regular or platform-admin form). + * Both surfaces share the same /members and /requests sub-resources, so their dedicated + * Membership path-subjects must match either prefix. + * @param {string} p - Express route path + * @returns {boolean} true for both /api/organizations and /api/admin/organizations + */ +const isOrganizationRoute = (p) => p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations'); + /** * Register organization-related subjects for document-level and path-level resolution. * The organization document subject uses a guard to exclude billing routes (which @@ -32,9 +41,14 @@ export function organizationSubjectRegistration({ registerDocumentSubject, regis const onOrgRoute = p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations'); return onOrgRoute && !p.includes('/members'); }); - registerPathSubject((p) => p.startsWith('/api/admin/organizations'), 'Organization'); - registerPathSubject((p) => p.startsWith('/api/organizations') && p.includes('/requests'), 'Membership'); - registerPathSubject((p) => p.startsWith('/api/organizations') && p.includes('/members'), 'Membership'); + // Admin organization routes resolve to the Organization subject, EXCEPT /members: + // membership management must authorize via the dedicated Membership path-subject below, + // mirroring the organization document-subject guard. Without this carve-out the broad + // admin Organization match would shadow the Membership entry (resolution is first-match-wins), + // so an admin /members path would resolve to Organization instead of Membership. + registerPathSubject((p) => p.startsWith('/api/admin/organizations') && !p.includes('/members'), 'Organization'); + registerPathSubject((p) => isOrganizationRoute(p) && p.includes('/requests'), 'Membership'); + registerPathSubject((p) => isOrganizationRoute(p) && p.includes('/members'), 'Membership'); registerPathSubject((p) => p.startsWith('/api/organizations'), 'Organization'); } diff --git a/modules/organizations/tests/organizations.policy.unit.tests.js b/modules/organizations/tests/organizations.policy.unit.tests.js index fc936d16c..020ebf0ee 100644 --- a/modules/organizations/tests/organizations.policy.unit.tests.js +++ b/modules/organizations/tests/organizations.policy.unit.tests.js @@ -7,19 +7,37 @@ import { organizationSubjectRegistration } from '../policies/organizations.polic /** * Build a mock subject-registration registry that captures registered resolvers. - * @returns {{ registerDocumentSubject: jest.Mock, registerPathSubject: jest.Mock, _documentResolvers: Map }} + * @returns {{ registerDocumentSubject: jest.Mock, registerPathSubject: jest.Mock, _documentResolvers: Map, _pathResolvers: Array }} */ function mockRegistry() { const documentResolvers = new Map(); + const pathResolvers = []; return { registerDocumentSubject: jest.fn((prop, type, guard) => { documentResolvers.set(prop, { type, guard }); }), - registerPathSubject: jest.fn(), + registerPathSubject: jest.fn((routeMatch, subjectType) => { + pathResolvers.push({ routeMatch, subjectType }); + }), _documentResolvers: documentResolvers, + _pathResolvers: pathResolvers, }; } +/** + * Replicate the production first-match-wins path-subject resolution + * (see lib/middlewares/policy.js → deriveSubjectType) over the captured resolvers. + * @param {Array} pathResolvers - Captured { routeMatch, subjectType } entries, in registration order + * @param {string} routePath - Express route path to resolve + * @returns {string|null} CASL subject type or null if not mappable + */ +function deriveSubjectType(pathResolvers, routePath) { + for (const { routeMatch, subjectType } of pathResolvers) { + if (routeMatch(routePath)) return subjectType; + } + return null; +} + describe('organizationSubjectRegistration policy unit tests:', () => { test('should register membershipDoc, organization, and path subjects', () => { const registry = mockRegistry(); @@ -81,4 +99,40 @@ describe('organizationSubjectRegistration policy unit tests:', () => { expect(guard({ route: { path: '/api/tasks' } })).toBe(false); }); }); + + describe('path subject resolution (first-match-wins):', () => { + let pathResolvers; + + beforeEach(() => { + const registry = mockRegistry(); + organizationSubjectRegistration(registry); + pathResolvers = registry._pathResolvers; + }); + + test('should resolve regular /:organizationId/members to the Membership path-subject', () => { + expect(deriveSubjectType(pathResolvers, '/api/organizations/:organizationId/members')).toBe('Membership'); + }); + + test('should resolve admin /:id/members to the Membership path-subject (mirrors the /members carve-out)', () => { + // The admin path-subject excludes /members so it falls through to the + // dedicated Membership entry — consistent with the regular /members route + // and the organization document-subject guard. Without the carve-out the + // broad admin Organization match would shadow it (first-match-wins). + expect(deriveSubjectType(pathResolvers, '/api/admin/organizations/:id/members')).toBe('Membership'); + }); + + test('should resolve admin organization routes (non-members) to the Organization path-subject', () => { + expect(deriveSubjectType(pathResolvers, '/api/admin/organizations')).toBe('Organization'); + expect(deriveSubjectType(pathResolvers, '/api/admin/organizations/:id')).toBe('Organization'); + }); + + test('should resolve /:organizationId/requests to the Membership path-subject', () => { + expect(deriveSubjectType(pathResolvers, '/api/organizations/:organizationId/requests')).toBe('Membership'); + }); + + test('should resolve plain organization routes to the Organization path-subject', () => { + expect(deriveSubjectType(pathResolvers, '/api/organizations')).toBe('Organization'); + expect(deriveSubjectType(pathResolvers, '/api/organizations/:id')).toBe('Organization'); + }); + }); }); From 33806d28eba17eb4c03cd5bbc19cd6a60bcaf83a Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 21:56:50 +0200 Subject: [PATCH 3/4] test(organizations): assert exact 403 on self-add + dedup org-route guard helper --- modules/organizations/policies/organizations.policy.js | 3 +-- .../tests/organizations.selfJoin.e2e.tests.js | 7 ++++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/organizations/policies/organizations.policy.js b/modules/organizations/policies/organizations.policy.js index 266dc9982..63465334b 100644 --- a/modules/organizations/policies/organizations.policy.js +++ b/modules/organizations/policies/organizations.policy.js @@ -38,8 +38,7 @@ export function organizationSubjectRegistration({ registerDocumentSubject, regis return false; } const p = req.route.path; - const onOrgRoute = p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations'); - return onOrgRoute && !p.includes('/members'); + return isOrganizationRoute(p) && !p.includes('/members'); }); // Admin organization routes resolve to the Organization subject, EXCEPT /members: // membership management must authorize via the dedicated Membership path-subject below, diff --git a/modules/organizations/tests/organizations.selfJoin.e2e.tests.js b/modules/organizations/tests/organizations.selfJoin.e2e.tests.js index cd5095596..467145b81 100644 --- a/modules/organizations/tests/organizations.selfJoin.e2e.tests.js +++ b/modules/organizations/tests/organizations.selfJoin.e2e.tests.js @@ -138,13 +138,14 @@ describe('Organizations self-join authorization E2E tests:', () => { } // 2. The outsider (NOT a member of org) tries to inject a membership for - // themselves. This must be forbidden (401/403) — the prior bug let it - // through via the unconditional create-Organization grant. + // themselves. The outsider IS authenticated, so this must be an + // AUTHORIZATION denial — exactly 403, never 401 (a 401 would mask the + // authz bypass the prior bug let through via the create-Organization grant). try { const selfAddRes = await agentOutsider .post(`/api/organizations/${org._id}/members`) .send({ userId: outsider.id, role: 'member' }); - expect([401, 403]).toContain(selfAddRes.status); + expect(selfAddRes.status).toBe(403); } catch (err) { console.log(err); expect(err).toBeFalsy(); From c299edbcb9e80a634cb8b6b7b6390331b3a01950 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 22:25:58 +0200 Subject: [PATCH 4/4] docs(organizations): clarify addMember authorization JSDoc post-fix --- .../organizations.membership.controller.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/modules/organizations/controllers/organizations.membership.controller.js b/modules/organizations/controllers/organizations.membership.controller.js index 944780983..3cbc9f3bf 100644 --- a/modules/organizations/controllers/organizations.membership.controller.js +++ b/modules/organizations/controllers/organizations.membership.controller.js @@ -67,8 +67,18 @@ const findUserByEmail = async (req, res) => { * @description Endpoint for an org owner/admin to add a user as a member. Creates a * PENDING owner_add membership the INVITED USER must accept (consent — invariant #1). * Role gate mirrors updateRole: only OWNERS may grant owner/admin; admins may only - * add plain members. CASL (`create Membership`) on the /members POST route already - * restricts this to org owners/admins (+ global admins). + * add plain members. + * + * Authorization is enforced at two layers: + * (1) The `/members` POST route resolves to the `Membership` path-subject (owner/admin + * gate via `create Membership`) because both the Organization document-subject and + * the admin Organization path-subject explicitly exclude `/members` paths — preventing + * the broader `create Organization` grant (available to any authenticated user) from + * shadowing the Membership subject and allowing unauthenticated membership injection. + * (2) The explicit actor-role check at the top of this handler (isGlobalAdmin || OWNER + * || ADMIN) fails closed for non-members and plain members, because the type-level + * CASL `create Membership` grant does not carry an org-scope condition on its own + * and would otherwise pass for any user who holds that grant in the same org. * @param {Object} req - Express request object * @param {Object} res - Express response object * @returns {void}