[SECUR-248] fix(security): guard ProjectViewSet PUT — routed verb falls through to DRF - #9603
[SECUR-248] fix(security): guard ProjectViewSet PUT — routed verb falls through to DRF#9603mguptahub wants to merge 2 commits into
Conversation
… DRF urls/project.py maps "put": "update", but ProjectViewSet never defined update, so PUT fell through to DRF's ModelViewSet.update. The class sets no permission_classes, so that ran under the default IsAuthenticated while partial_update on the identical URL requires workspace or project admin. A workspace MEMBER with no membership of a Secret project could PUT it to network=2 (Public) and then join via the public-project API — read plus member-level write on a project they were never in. Confirmed: an unpatched non-member PUT returns 200 and flips network. PUT now routes through partial_update so both verbs enforce the same rule; a full PUT-replace is not a meaningful operation for projects. Contract tests cover the denied PUT, that a denial writes nothing, that PUT and PATCH reach the same verdict (the defect was that they disagreed), and a positive control that an admin PUT still works. Fail-before verified: 3 failed / 1 passed unpatched, 4 passed patched. Note the payload must be complete — DRF's generic update runs the serializer with partial=False, so an incomplete body 400s on validation before authorization differs, and the test would pass either way. Co-authored-by: Plane AI <noreply@plane.so>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesProject PUT authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change routes project PUT requests through the same authorization path as PATCH and adds coverage for denied writes and administrator success; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed context, impact, implementation details, test coverage, fail-before and patched results, and related findings. It omits the template's explicit Type of Change and Screenshots and Media headings, but the required technical information is substantially complete.
✨ Finishing Touches 💡 1📝 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull request overview
Fixes an authorization gap where PUT /workspaces/<slug>/projects/<id>/ was routed to update but ProjectViewSet did not implement update, causing DRF’s generic ModelViewSet.update (with only default IsAuthenticated) to run instead of the stricter admin checks enforced in partial_update.
Changes:
- Add
ProjectViewSet.updatethat delegates topartial_updateso PUT and PATCH enforce identical authorization rules. - Add contract tests covering non-member denial, no-write-on-deny, PUT/PATCH verdict consistency, and an admin positive control.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/api/plane/app/views/project/base.py | Adds update() to route PUT through the existing partial_update admin checks. |
| apps/api/plane/tests/contract/app/test_project_put_authz.py | Adds contract/regression coverage for the PUT authorization defect and ensures denied PUTs do not mutate project state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The test module's docstring embedded the internal advisory ticket reference directly in source. This repo is public, and an ID tying a docstring straight to the exact authorization gap it covers hands anyone on an unpatched fork a precise map from advisory to vulnerable code path. Reworded the docstring to describe the invariant under test (PUT must enforce the same authorization as PATCH) instead. Co-authored-by: Plane AI <noreply@plane.so>
|
Addressed the review blocker: the test module's docstring embedded the internal ticket reference directly in source. Reworded it to describe the invariant under test instead — no ticket ID anywhere in the file now (double-checked with a repo-wide grep restricted to files this branch touches). Old: Ran |
|
React Doctor found 26 new issues in 25 files · 26 warnings · score 68 / 100 (Needs work) · 1 fixed · vs 26 warnings
Reviewed by React Doctor for commit |
|
Closing in favour of #9652, which supersedes this — it covers the same ProjectViewSet PUT fall-through plus every other undecorated routed verb across the URLconf (225 actions audited, 27 refused vs. this PR's single-viewset point-fix), from the same structural guard. No functionality here is lost; it's a strict subset of #9652's diff. Thanks for the fix — the audit approach is what's landing, not the specific patch. |
Summary
app/urls/project.py:40maps"put": "update", butProjectViewSetnever definedupdate— so PUT fell through to DRF'sModelViewSet.update. The class sets nopermission_classes, so that generic handler ran under the project default ofIsAuthenticated, whilepartial_updateon the identical URL does an inline workspace-admin / project-admin check.PATCH was authorised. PUT was not.
Verified against
preview@1c8a60f858. Closes 2 HIGH advisories (SECUR-248 carries the mapping).Impact — reproduced
A workspace MEMBER with no
ProjectMemberrow for anetwork=0(Secret) project:PUT .../projects/<id>/withnetwork: 2→200 OK, project becomes PublicProjectMember(role=15)Step 1 confirmed in the fail-before run: an unpatched non-member PUT returns
200and flipsnetwork.The change
Both verbs now enforce the same rule. A full PUT-replace is not a meaningful operation for projects — the client only ever sends PATCH.
This follows the pattern #9461 established for the work-item / module / intake instances of the same defect. It does not need that PR's
partial_update.__wrapped__trick:ProjectViewSet.partial_updatecarries no decorator (it authorises inline), so calling it directly does not double-authorise.Tests
Four contract tests:
networkwould leave the project published, which is the actual damageFail-before verified: 3 failed / 1 passed unpatched → 4 passed patched. The positive control passes in both runs, confirming it is independent of the change.
updateruns the serializer withpartial=False, so an incomplete body 400s on validation before authorization differs at all — with a partial body the tests passed with and without the fix and proved nothing.full_put_body()exists for that reason and the docstring says so.Related — a broader finding
The ticket asked for a sweep for other instances. Checking every
as_view({...})mapping inapp/urls/against whether the viewset defines that action, then filtering to classes with no class-levelpermission_classes:permission_classesProjectViewSetPUT (this PR)Examples, all on classes whose defined actions carry
@allow_permission/@canwhile the undefined one gets nothing:CycleViewSetPUT,IssueViewViewSetPOST,WorkspaceViewViewSetPOST,NotificationViewSetDELETE,StateViewSetGET,ModuleIssueViewSetPUT/PATCH/GET.I have not verified each is exploitable — some may be protected by
get_querysetscoping, andcreate/destroyon a generic mixin may fail for unrelated reasons. That verification is the work. Tracked separately rather than expanding this PR.Summary by CodeRabbit
Bug Fixes
Tests