[WEB-8333] fix(security): guard undecorated viewset routes with project-membership check - #9461
[WEB-8333] fix(security): guard undecorated viewset routes with project-membership check#9461mguptahub wants to merge 5 commits into
Conversation
…ct-membership check (GHSA-27v6 / GHSA-w83f) Per-project authz lives in the @allow_permission(level="PROJECT") method decorator, not the queryset. IssueViewSet/ModuleViewSet had no `def update` (PUT), and IntakeViewSet had no `retrieve`/`partial_update` — those routes fell through to stock DRF ModelViewSet under the project-wide default IsAuthenticated (no has_object_permission anywhere), letting any authenticated non-member act cross-project (read/modify issues, modules, intakes in projects they don't belong to). Define + decorate the missing handlers, matching each viewset's sibling role set: - IssueViewSet.update → @allow_permission([ADMIN, MEMBER], creator=True, model=Issue), routes PUT through the authorized partial_update path - ModuleViewSet.update → @allow_permission([ADMIN, MEMBER]), routes PUT through partial_update - IntakeViewSet.retrieve + partial_update → @allow_permission([ADMIN, MEMBER]), delegate to stock DRF PUT is routed through PATCH because no full-replace semantics exist for these resources (the FE only ever PATCHes); the fix is purely additive authorization. Adds 8 contract tests; fail-before verified (4 attack routes returned 200 unpatched → all 8 pass patched). Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds explicit role-guarded handlers for issue, module, and intake routes, routing updates through existing partial-update logic. Contract tests verify project members succeed while non-members receive forbidden responses across current and legacy intake routes. ChangesProject-scope authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/plane/tests/contract/app/test_undecorated_route_project_scope_app.py`:
- Around line 55-56: The contract tests currently cover only the intakes route
and omit its legacy inboxes alias. Update _intake_detail_url and the related
GET/PATCH member and non-member test cases to parameterize or otherwise exercise
both /intakes/ and /inboxes/ while preserving the existing assertions for each
route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4074818a-876d-470a-9404-621bdf231418
📒 Files selected for processing (4)
apps/api/plane/app/views/intake/base.pyapps/api/plane/app/views/issue/base.pyapps/api/plane/app/views/module/base.pyapps/api/plane/tests/contract/app/test_undecorated_route_project_scope_app.py
There was a problem hiding this comment.
Pull request overview
This PR fixes a high-severity cross-project/tenant IDOR by ensuring routed DRF actions that previously fell through to ModelViewSet defaults (and thus only IsAuthenticated) are now explicitly implemented and guarded with the existing project-scoped @allow_permission(...) checks.
Changes:
- Add
update()handlers for Issue/Module viewsets and route PUT through the already-authorizedpartial_update()path. - Add explicit
retrieve()andpartial_update()handlers toIntakeViewSet, delegating to DRF’s stock implementations but now protected by@allow_permission. - Add a contract test suite covering the previously-undecorated routes (403 for cross-project non-members + positive controls).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/api/plane/app/views/issue/base.py | Adds a guarded update() implementation to prevent undecorated PUT falling through without project authorization. |
| apps/api/plane/app/views/module/base.py | Adds a guarded update() implementation to prevent undecorated PUT falling through without project authorization. |
| apps/api/plane/app/views/intake/base.py | Adds guarded retrieve() and partial_update() implementations so routed actions no longer fall through without project authorization. |
| apps/api/plane/tests/contract/app/test_undecorated_route_project_scope_app.py | Adds regression/contract coverage for the previously-fallthrough routes. |
…lot #9461) The positive-control tests authenticate as create_user, who was an ADMIN (role 20) member of project_b — so they exercised the ADMIN path, not MEMBER, despite the "member" docstrings. Make create_user a project MEMBER (role 15) so the positive controls validate the ROLE.MEMBER branch of @allow_permission on the newly-guarded handlers (all allow [ADMIN, MEMBER]; admin is covered elsewhere). Co-authored-by: Plane AI <noreply@plane.so>
…ssue PUT (CodeRabbit/Copilot #9461) Address two review comments: - CodeRabbit: parameterize the IntakeViewSet retrieve/partial_update contract tests over both the `intakes/` route and the legacy `inboxes/` alias, so the alias (which routes to the same viewset) cannot regress independently. - Copilot: issue_b is now authored by a distinct project-B member (via save(created_by_id=...)) instead of create_user, so test_member_can_put_issue is authorized through the ROLE.MEMBER branch of allow_permission rather than the creator=True short-circuit. (BaseModel.save silently nulls created_by under tests, so the previous created_by=create_user kwarg was a no-op.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ilot #9461) IssueViewSet.update and ModuleViewSet.update are decorated with allow_permission and delegated to the *decorated* partial_update, so allow_permission (and its ProjectMember/WorkspaceMember lookups) ran twice on every PUT. Delegate to partial_update.__wrapped__ (the undecorated implementation) instead: update's own decorator has already authorized the request, so the check runs exactly once while the routed action stays visibly guarded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mguptahub
changed the title
Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so>
|
React Doctor found 4 issues in 4 files · 1 error & 3 warnings · score 82 / 100 (Needs work) · vs Errors
3 warnings
|
Summary
HIGH — fixes a cross-project/tenant IDOR (WEB-8333). Confirmed-vulnerable on
origin/preview.Per-project authorization in these viewsets lives in the
@allow_permission(..., level="PROJECT")method decorator, not the queryset (get_querysetfilters onlyworkspace__slug+project_id, no membership predicate). The project-wide default permission isIsAuthenticatedonly, and there's nohas_object_permissionanywhere — so any routed action without the decorator falls through to stock DRFModelViewSetand any authenticated user (even a non-member of the project) can act cross-project.Undecorated fall-through routes:
IssueViewSet.update(PUT) — nodef updateModuleViewSet.update(PUT) — nodef updateIntakeViewSet.retrieve(GET) +IntakeViewSet.partial_update(PATCH) — only list/create/destroy were definedFix
Define + decorate the missing handlers, matching each viewset's sibling role set:
update@allow_permission([ADMIN, MEMBER], creator=True, model=Issue)update@allow_permission([ADMIN, MEMBER])retrieve,partial_update@allow_permission([ADMIN, MEMBER])PUT is routed through the authorized
partial_updatepath for Issue/Module — no full-replace semantics exist for these resources (the FE only PATCHes), so this is purely additive authorization, not a behavior change reviewers need to worry about. If a strict full-PUT-replace is ever desired that's a separate behavioral change. Intakeretrieve/partial_updatedelegate to stock DRF (behavior preserved). Bothintakes/and legacyinboxes/routes map to the same viewset, so both are covered.Tests
New
tests/contract/app/test_undecorated_route_project_scope_app.py— 8 cases (non-member cross-project PUT issue / PUT module / GET intake / PATCH intake → 403; + 4 positive controls). Fail-before verified on the CE docker stack: 4 attack routes returned 200 unpatched → all 8 pass patched. (Fullcontract/appregression: 109 pass; the 8 failures are pre-existing rate-limit flakes intest_authentication.py, unrelated.)Summary by CodeRabbit
Bug Fixes
Tests