[INFRA-777] fix(security): scope ProjectArchiveUnarchiveAPIEndpoint to the URL project - #9706
[INFRA-777] fix(security): scope ProjectArchiveUnarchiveAPIEndpoint to the URL project#9706mguptahub wants to merge 1 commit into
Conversation
…o the URL project ProjectArchiveUnarchiveAPIEndpoint used ProjectBasePermission, whose POST branch is written for project creation and checks only workspace-level role (ADMIN/MEMBER) with no project_id binding at all. Since archive is POST but is not creation, any active workspace member could archive (and delete every UserFavorite row on) any project in the workspace, including a fully private one they have no ProjectMember row on and cannot otherwise read. DELETE (unarchive) was never affected by this — it falls through to ProjectBasePermission's non-POST branch, which does check project membership, so this was an asymmetry between the two verbs of the same endpoint (and that branch was itself stricter than intended: it required project ADMIN or workspace-ADMIN-plus-project-membership, not just an active project MEMBER). Give the endpoint a dedicated ProjectArchiveUnarchivePermission, scoped to project_id, mirroring the app-layer twin's gate (active ProjectMember with role ADMIN or MEMBER, or any active ProjectMember plus a workspace ADMIN) on both post and delete — instead of touching ProjectBasePermission itself, which is correctly relied on by the actual project-create endpoint and by ProjectDetailAPIEndpoint's patch/delete. 3 new tests, fail-before verified. Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe archive and unarchive endpoint now uses project-scoped authorization. Workspace members without project membership receive 403 responses. Active project members retain archive and unarchive access. ChangesProject archive authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR scopes archive and unarchive actions to active membership in the selected project and preserves the existing endpoint contract. 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 explains the vulnerability, fix, authorization behavior, scope, and test results. It omits some template headings, but it provides the required change and test information in equivalent sections.
✨ 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 |
|
/code-review: clean, no findings. Verified the permission class's query logic against both ProjectBasePermission's non-POST branch and the app-layer allow_permission([ROLE.ADMIN, ROLE.MEMBER]) decorator, confirmed view.project_id/view.workspace_slug are valid BaseAPIView properties, and confirmed ProjectBasePermission/ProjectMemberPermission are correctly left untouched. |
There was a problem hiding this comment.
Pull request overview
Scopes the external API’s project archive/unarchive endpoint authorization to the target project_id, closing a workspace-member authorization bypass and aligning archive/unarchive behavior between POST and DELETE.
Changes:
- Introduces
ProjectArchiveUnarchivePermissionto enforce project-scoped membership checks for both archive and unarchive. - Updates
ProjectArchiveUnarchiveAPIEndpointto use the new permission instead ofProjectBasePermission. - Adds contract regression tests covering both the bypass case and a positive-control member case.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/api/plane/tests/contract/api/test_project_archive_scope.py | Adds regression/positive-control tests for archive/unarchive project scoping. |
| apps/api/plane/app/permissions/project.py | Adds ProjectArchiveUnarchivePermission with project-scoped checks for POST/DELETE. |
| apps/api/plane/app/permissions/init.py | Exports the new permission class for app-wide import usage. |
| apps/api/plane/api/views/project.py | Switches the archive/unarchive endpoint to the new permission class. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
ProjectArchiveUnarchiveAPIEndpointusedProjectBasePermission, whose POST branch is written for project creation and checks only workspace-level role (ADMIN/MEMBER) with noproject_idbinding at all.UserFavoriterow on) any project in the workspace — including a fully private one they have noProjectMemberrow on and cannot otherwise read.ProjectBasePermission's non-POST branch, which does check project membership. So this was specifically an asymmetry between the two verbs of the same endpoint. That branch also turned out to be stricter than intended (project ADMIN, or workspace-ADMIN-plus-project-membership — not just an active project MEMBER), which this fix corrects too.ProjectArchiveUnarchivePermission, scoped toproject_id, mirroring the app-layer twin's gate (allow_permission([ROLE.ADMIN, ROLE.MEMBER])) on bothpostanddelete— instead of touchingProjectBasePermissionitself, which is correctly relied on by the actual project-create endpoint and byProjectDetailAPIEndpoint's patch/delete.ProjectMemberPermissionhas the same POST-branch shape but is a different class, already tracked and being fixed by the openf63ccluster's PR [SECUR-247] fix(security): scope ProjectMemberPermission POST to the URL project #9596 — confirmed by reading that PR's actual diff, which explicitly leavesProjectBasePermissionuntouched.Test plan
apps/api/plane/tests/contract/api/test_project_archive_scope.py: a workspace member with no project membership cannot archive or unarchive a private project; an active project member (not just admin) can do bothgit stash— the archive-bypass test fails against pre-fix code, and the positive-control test also caught a genuine pre-existing over-restriction (a plain project member couldn't unarchive their own project before this fix)ruff check/ruff format --checkcleanSummary by CodeRabbit