fix(organizations): close self-join via shadowed Organization policy subject#3868
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3868 +/- ##
=======================================
Coverage 92.48% 92.49%
=======================================
Files 165 165
Lines 5400 5407 +7
Branches 1735 1741 +6
=======================================
+ Hits 4994 5001 +7
Misses 326 326
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the organizations authorization model by preventing a CASL subject-resolution shadowing bug that allowed authenticated non-members to hit POST /api/organizations/:id/members and bypass the intended Membership (owner/admin) gate.
Changes:
- Tighten Organization document-subject resolution to exclude
/membersroutes so membership management routes resolve via the Membership path-subject. - Add a defense-in-depth role gate in
addMemberto require owner/admin (or platform admin) before proceeding. - Add new unit/e2e coverage to prevent regressions and verify the legitimate join-request flow remains functional.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/organizations/policies/organizations.policy.js | Adjusts CASL subject registration/guards to prevent Organization doc-subject from shadowing /members. |
| modules/organizations/controllers/organizations.membership.controller.js | Adds an explicit actor-role guard to addMember as a mandatory authorization backstop. |
| modules/organizations/tests/organizations.policy.unit.tests.js | Expands unit tests to verify the updated Organization guard and path-subject ordering behavior. |
| modules/organizations/tests/organizations.membership.controller.unit.tests.js | Adds unit tests asserting non-member and plain-member cannot call addMember. |
| modules/organizations/tests/organizations.selfJoin.e2e.tests.js | Adds an end-to-end regression test for the self-join exploit and confirms join-request flow still works. |
| // 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. |
| // 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) => { |
Summary
Organization) that matched the/membersroute, shadowing the Membership path-subject and grantingcreate Organizationunconditionally — any authenticated non-member could inject a membership into any org. Fixed by carving/membersand/requestsout of the doc-subject and adding a symmetric carve-out on the admin path-subject so both routes land on the correct Membership owner/admin gate. The join-request flow (/requests) is preserved. An actor-role assertion was added toaddMemberas a mandatory guard.Scope
modules/organizations(policy + membership controller + tests)nonemedium(policy logic change; membership creation path unchanged for legitimate flows)Validation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
/membersalways resolves through the Membership gate.