[WEB-8066] fix: scope workspace asset get/patch/delete to project membership#9372
Conversation
…bership WorkspaceFileAssetEndpoint is authorized at the WORKSPACE level, so any workspace member/guest could reach get/patch/delete for a project-bound asset (issue attachment/description, comment description, page description) of a project they are not a member of — an incomplete fix of the GHSA-qw87 asset-IDOR cluster (GHSA-h7mc-p9mm-2r4w / GHSA-cjph-cgm5-8pw8). Add project_membership_denied(): for project-bound assets (project_id set) require an active ProjectMember of the asset's project, else 403. Workspace- level entity types (WORKSPACE_LOGO, USER_AVATAR, USER_COVER) have project_id NULL and remain accessible to any workspace member. Mirrors ProjectAssetEndpoint (level=PROJECT). Guard runs before the is_uploaded check / mutation so a non-member gets a uniform 403 and cannot probe upload state. Contract regression tests cover denied get/patch/delete for a non-project member, the positive project-member path, and the workspace-level exemption; 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds project-level authorization to ChangesWorkspace file asset project scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkspaceFileAssetEndpoint
participant FileAsset
participant ProjectMember
Client->>WorkspaceFileAssetEndpoint: GET/PATCH/DELETE asset
WorkspaceFileAssetEndpoint->>FileAsset: lookup workspace-scoped asset
WorkspaceFileAssetEndpoint->>ProjectMember: check active membership for project-bound asset
alt access denied
WorkspaceFileAssetEndpoint-->>Client: 403 Forbidden
else access granted or workspace-level asset
WorkspaceFileAssetEndpoint-->>Client: continue operation or redirect
end
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 |
There was a problem hiding this comment.
Pull request overview
This PR closes an IDOR gap in WorkspaceFileAssetEndpoint by adding a project-membership authorization guard for workspace-scoped asset get/patch/delete operations when the asset is project-bound, and adds contract tests to prevent regressions tied to GHSA-h7mc-p9mm-2r4w / GHSA-cjph-cgm5-8pw8 (WEB-8066).
Changes:
- Add a
project_membership_denied()helper and invoke it inget/patch/deleteto return403for non-project-members accessing project-bound assets. - Preserve workspace-level asset access (assets with
project_id = NULL) for any active workspace member/guest. - Add contract tests covering denied access for non-project-members, plus positive/exemption controls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/api/plane/app/views/asset/v2.py | Adds project-membership authorization guard to workspace-scoped asset fetch/mutation endpoints. |
| apps/api/plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py | Adds regression/contract coverage ensuring non-project-members receive 403 and no URL/mutations occur. |
…s workspace Address Copilot review: filter ProjectMember by workspace_id=asset.workspace_id in addition to project_id, mirroring allow_permission's PROJECT branch. Prevents a member of the same project in a different workspace from passing the check if an asset row is ever inconsistent (asset.workspace_id != project.workspace_id). Co-authored-by: Plane AI <noreply@plane.so>
| else: | ||
| return | ||
|
|
||
| def project_membership_denied(self, request, asset): |
There was a problem hiding this comment.
we should make this helper function return a boolean value and return Response in the view itself based on the value
There was a problem hiding this comment.
Done in the latest commit — renamed to has_project_asset_access(request, asset) which now returns a boolean (True = allowed), and each of get/patch/delete builds the 403 Response itself based on the returned value. Behaviour is unchanged (same 403 + message; workspace-level assets with project_id=None still allowed). Existing 5 contract tests still pass.
…onse in views Address review (Saurabhkmr98): rename project_membership_denied -> has_project_asset_access, returning a boolean (True = allowed) instead of a Response. Each of get/patch/delete now builds the 403 Response based on the returned value. Behaviour is unchanged (same 403 + message; workspace-level assets with project_id=None still allowed). Co-authored-by: Plane AI <noreply@plane.so>
Summary
WorkspaceFileAssetEndpoint(apps/api/plane/app/views/asset/v2.py) is authorized at the WORKSPACE level. Itsget,patch, anddeletefetchFileAsset.objects.get(id=asset_id, workspace__slug=slug)with no project-membership check, so any workspace member or guest could download, mark-uploaded, or delete a project-bound asset (issue attachment/description, comment description, page description) belonging to a project they are not a member of.This is an incomplete fix of the GHSA-qw87 asset-IDOR cluster.
Advisories: GHSA-h7mc-p9mm-2r4w (dup: GHSA-cjph-cgm5-8pw8) · MEDIUM.
Fix
project_membership_denied(request, asset)helper: for project-bound assets (asset.project_id is not None), require an activeProjectMemberofasset.project_id, otherwise403.WORKSPACE_LOGO,USER_AVATAR,USER_COVER) haveproject_id = NULLand remain accessible to any workspace member.get,patch, anddeleteimmediately after the asset fetch — before theis_uploadedcheck / any mutation — so a non-member receives a uniform403and cannot probe upload state.ProjectAssetEndpoint(level="PROJECT").Tests
plane/tests/contract/app/test_workspace_file_asset_project_scope_app.py:get/patch/deleteon a project asset by a non-project-member →403(and no presigned URL minted / no mutation).Fail-before verified: with the fix stashed, the three denied cases return
302/204/204(the vulnerable behavior); positive + exemption controls still pass.Gates: ruff ✅ ·
manage.py check✅ · siblingtest_generic_asset.pyregression ✅.Follow-up (out of scope)
Sibling workspace-scoped endpoints share the same shape and warrant a separate audit:
AssetRestoreEndpoint,AssetCheckEndpoint,DuplicateAssetEndpoint,WorkspaceAssetDownloadEndpoint.🤖 Generated with Claude Code
Summary by CodeRabbit
403and are blocked before any download link is generated or any asset changes occur.