fix(organizations): close self-join via shadowed Organization policy subject

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}
@@ -78,9 +88,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')();
     }

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
@@ -16,16 +25,29 @@ 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');
+    return isOrganizationRoute(p) && !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');
 }
 

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();

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();
@@ -61,9 +79,60 @@ 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);
     });
   });
+
+  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');
+    });
+  });
 });