[SECUR-247] fix(security): scope ProjectMemberPermission POST to the URL project - #9596
[SECUR-247] fix(security): scope ProjectMemberPermission POST to the URL project#9596mguptahub wants to merge 2 commits into
Conversation
The SAFE_METHODS branch and the trailing branch both bind project_id; only POST did not, checking workspace membership alone. Any workspace member could therefore create sub-resources in a project they do not belong to. The reported impact is deploy-board creation: publishing a Secret project returns the public anchor, which Space then serves to anonymous callers — work item list and detail including description_html. LabelListCreateAPIEndpoint runs through the same branch and is closed by the same change. ProjectMemberListCreateAPIEndpoint is unaffected: it overrides get_permissions() to use ProjectAdminPermission for non-GET. ProjectBasePermission has a similar-looking POST branch and is deliberately left alone — there the workspace-only check is correct, since it guards project creation itself. Also validates project_id against the URL slug in DeployBoardViewSet.create; get_or_create lookup keys are unchanged to avoid matching differently against existing rows. Contract tests cover the denied publish, that no anchor leaks and no board is created on denial, and cross-workspace project ids — plus a positive control that a project member can still publish. Fail-before verified: 3 failed / 3 passed unpatched, 6 passed patched. 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDeployBoard publishing now requires project membership and validates that the project belongs to the requested workspace. Contract tests cover denied access, side effects, successful publishing, and cross-workspace project IDs. ChangesDeployBoard project scoping
Estimated code review effort: 2 (Simple) | ~15 minutes Mergeability Score: ⚪ Minimal · up to The change restricts project-member POST actions to the project in the URL and validates the project identifier, preventing unauthorized sub-resource creation. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull request overview
This PR fixes a privilege-escalation in the API where ProjectMemberPermission.has_permission previously allowed POST based on workspace membership only, enabling workspace members to create project-scoped sub-resources (e.g., deploy boards/labels) in projects they do not belong to. It also adds a defensive validation in deploy-board creation and extends contract tests to cover the reported vectors.
Changes:
- Scope
ProjectMemberPermission’sPOSTauthorization to the URLproject_idby checkingProjectMembermembership (not justWorkspaceMember). - Add a
project_id↔slugvalidation guard inDeployBoardViewSet.createbeforeget_or_create. - Extend deploy-board contract tests to cover negative/positive publish behavior and cross-workspace
slug/project_idmismatch.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/app/permissions/project.py | Updates ProjectMemberPermission POST checks to require project membership for the URL project_id. |
| apps/api/plane/app/views/project/base.py | Adds an explicit Project existence check to prevent cross-workspace slug + foreign project_id publishes. |
| apps/api/plane/tests/contract/app/test_deploy_board_project_scope_app.py | Adds regression tests for deploy-board publish scoping and cross-tenant slug/project_id mismatch behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…h too Addresses review on #9596. plane/utils/permissions/project.py holds a second ProjectMemberPermission that, comments aside, was byte-identical to the one in plane/app/permissions. Its POST branch still checked workspace membership alone. It is imported (api/views/member.py) but its POST branch is currently unreachable: ProjectMemberListCreateAPIEndpoint.get_permissions() routes non-GET to ProjectAdminPermission, and the other consumer is GET-only. So this is a latent hazard rather than a second live vector — but two same-named classes that have already drifted make reintroduction easy, and this repo has previously had to patch the same duplication in the page permission classes. Both copies now carry a comment saying they must not drift. Also aligns the deploy-board 404 string with the module's existing wording ("Project does not exist", cf. base.py:230) rather than introducing a second phrasing for clients to handle. Co-authored-by: Plane AI <noreply@plane.so>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/api/plane/app/permissions/project.py:74
- The POST query is now identical to the generic write query immediately below, so keeping this special case leaves two authorization paths that can drift again—the failure mode this patch addresses. Remove the POST branch and let the shared non-safe-method check handle POST.
if request.method == "POST":
return ProjectMember.objects.filter(
apps/api/plane/utils/permissions/project.py:74
- The POST query is now identical to the generic write query immediately below, so keeping this special case leaves two authorization paths that can drift again—the failure mode this patch addresses. Remove the POST branch and let the shared non-safe-method check handle POST.
if request.method == "POST":
return ProjectMember.objects.filter(
apps/api/plane/tests/contract/app/test_deploy_board_project_scope_app.py:190
- This permits a 403, which
ProjectMemberPermissionreturns beforeDeployBoardViewSet.createruns for this fixture. Therefore the new slug/project guard can be removed entirely while this test still passes. Add focused coverage that bypasses or overrides the permission layer and expects the view's 404, while retaining the no-board assertion.
assert response.status_code in (
status.HTTP_403_FORBIDDEN,
status.HTTP_404_NOT_FOUND,
), f"Got {response.status_code}: {getattr(response, 'data', None)!r}"
Summary
ProjectMemberPermission.has_permissionscopes its SAFE_METHODS branch and its trailing (write) branch toproject_id=view.project_id. The POST branch checked workspace membership only. Any workspace MEMBER could therefore create sub-resources in a project they do not belong to.Verified against
preview@1c8a60f858. Closes 2 HIGH advisories (SECUR-247 carries the mapping).Impact
The reported vector is deploy-board creation — the publish action:
ProjectMemberrow for anetwork=0(Secret) projectPOST .../project-deploy-boards/returns200and the public anchorAllowAnyendpoints serve that anchor to unauthenticated callers — work-item list and detail, includingdescription_htmlA second report notes
project_idfrom the URL was never checked againstslug, so a user who owns any workspace on the instance could aim their own slug at another tenant's project id.The change
Plus
DeployBoardViewSet.createnow validatesproject_idagainstslugbeforeget_or_create.Blast radius — please read
ProjectMemberPermissionis shared. I audited every consumer:DeployBoardViewSetLabelListCreateAPIEndpoint.postLabelDetailAPIEndpointProjectMemberListCreateAPIEndpointget_permissions()toProjectAdminPermissionfor non-GETProjectMemberLiteAPIEndpointSo the diff closes label creation too. That is the same root cause, not scope creep — but it is more than the two advisories name, and worth knowing when reviewing.
ProjectBasePermissionis deliberately untouched. It has a near-identical POST branch atproject.py:25, but there the workspace-only check is correct — it genuinely guards project creation. Its misuse by the archive endpoint is tracked separately (SECUR-249).ProjectMemberPermissionis not used for project creation anywhere; the stale comment claiming otherwise is likely how this survived review, and is now replaced.Update after review
Two Copilot findings, both valid and both fixed in
f494bfd76d:A duplicate permission class.
plane/utils/permissions/project.pyholds a secondProjectMemberPermissionwhich — comments aside — was byte-identical, including the unscoped POST branch. It is imported (api/views/member.py:23), but its POST branch is currently unreachable:ProjectMemberListCreateAPIEndpoint.get_permissions()routes non-GET toProjectAdminPermission, and the other consumer is GET-only. So it was a latent hazard rather than a second live vector. Scoped anyway — two same-named classes that have already drifted make reintroduction easy, and this repo has previously had to patch the same duplication in the page permission classes. Both copies now note they must not drift.Worth flagging that my original blast-radius audit grepped for the class name and never checked import sources, which is how I missed it.
404 wording. The new guard said
"Project not found"; the module already uses"Project does not exist"(base.py:230). Aligned.Consolidating the two permission modules is the real fix but is wider than a security patch should carry — worth its own ticket.
I did not add
workspace_idtoget_or_create's lookup keys — that changes matching semantics and risks creating duplicate boards against existing rows whereworkspaceis null. The validation is a separate guard.Tests
Extended
test_deploy_board_project_scope_app.py, which already covered the SAFE_METHODS sibling of this same class. Four new tests:DeployBoardrow (a 403 that still created the board would leave the project published)Fail-before verified: 3 failed / 3 passed unpatched → 6 passed patched. The positive control and both pre-existing tests pass in both runs, confirming they are independent of the change.
Summary by CodeRabbit