fix: IDOR Vulnerabilities in Asset & Attachment Endpoints#8644
fix: IDOR Vulnerabilities in Asset & Attachment Endpoints#8644sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughBoth endpoints tighten FileAsset retrieval by adding workspace and project scoping: ProjectAssetEndpoint.patch now filters by workspace slug and project_id; IssueAttachmentEndpoint.delete additionally filters by issue_id to ensure assets belong to the specified context. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 pull request addresses two critical Insecure Direct Object Reference (IDOR) vulnerabilities that could allow authenticated users to access and modify resources across tenant boundaries. The fixes add proper scoping filters to FileAsset queries to ensure workspace, project, and issue ownership verification.
Changes:
- Fixed IDOR vulnerability in
IssueAttachmentEndpoint.delete()by adding workspace, project, and issue scoping to the asset lookup - Fixed IDOR vulnerability in
ProjectAssetEndpoint.patch()by adding workspace and project scoping to the asset lookup
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/app/views/issue/attachment.py | Added workspace__slug, project_id, and issue_id filters to the FileAsset.objects.get() call in the delete() method to prevent cross-tenant attachment deletion |
| apps/api/plane/app/views/asset/v2.py | Added workspace__slug and project_id filters to the FileAsset.objects.get() call in the patch() method to prevent cross-tenant asset modification |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/issue/attachment.py (1)
141-160:⚠️ Potential issue | 🟠 MajorIncomplete IDOR fix —
IssueAttachmentV2Endpoint.deleteis missingissue_idscoping.Line 143 filters only by
workspace__slugandproject_id, without constrainingissue_id. An authenticated user can delete attachments belonging to a different issue within the same project by supplying a validpkfrom that issue. This is the same IDOR class the PR targets inIssueAttachmentEndpoint.delete.The same gap exists in
IssueAttachmentV2Endpoint.patch(line 197) andget(line 166) — both lackissue_idin their lookups.🔒 Proposed fix for `IssueAttachmentV2Endpoint.delete`, `patch`, and `get`
def delete(self, request, slug, project_id, issue_id, pk): - issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id) + issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id, issue_id=issue_id)def patch(self, request, slug, project_id, issue_id, pk): - issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id) + issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id, issue_id=issue_id)if pk: - asset = FileAsset.objects.get(id=pk, workspace__slug=slug, project_id=project_id) + asset = FileAsset.objects.get(id=pk, workspace__slug=slug, project_id=project_id, issue_id=issue_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/issue/attachment.py` around lines 141 - 160, The delete/patch/get handlers in IssueAttachmentV2Endpoint perform FileAsset lookups without scoping by issue_id, allowing cross-issue deletes/edits; update the FileAsset queries in IssueAttachmentV2Endpoint.delete, IssueAttachmentV2Endpoint.patch, and IssueAttachmentV2Endpoint.get to include issue_id as a filter (e.g., include issue_id=issue_id alongside workspace__slug and project_id) so the retrieved FileAsset is constrained to the specified issue before marking deleted, saving, or returning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/app/views/issue/attachment.py`:
- Around line 62-63: The delete(...) method currently calls
FileAsset.objects.get(...) without handling FileAsset.DoesNotExist; wrap the
FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id,
issue_id=issue_id) call in a try/except FileAsset.DoesNotExist block (same
pattern used in WorkspaceAssetDownloadEndpoint.get and
ProjectAssetDownloadEndpoint.get) and return a 404 response (or raise Http404)
when the exception is caught so missing or cross-tenant attachments produce a
404 instead of a 500.
---
Outside diff comments:
In `@apps/api/plane/app/views/issue/attachment.py`:
- Around line 141-160: The delete/patch/get handlers in
IssueAttachmentV2Endpoint perform FileAsset lookups without scoping by issue_id,
allowing cross-issue deletes/edits; update the FileAsset queries in
IssueAttachmentV2Endpoint.delete, IssueAttachmentV2Endpoint.patch, and
IssueAttachmentV2Endpoint.get to include issue_id as a filter (e.g., include
issue_id=issue_id alongside workspace__slug and project_id) so the retrieved
FileAsset is constrained to the specified issue before marking deleted, saving,
or returning it.
* fix: idor issues in project assets and issue attachements * fix: comments
Description
Patched two Insecure Direct Object Reference (IDOR) vulnerabilities that allowed authenticated users to access and modify resources across tenant boundaries.
Vulnerabilities Fixed
1.
ProjectAssetEndpoint.patch()Asset lookup relied solely on the asset ID, with no verification of workspace or project ownership. This allowed any authenticated user to modify asset metadata belonging to a different tenant.
2.
IssueAttachmentEndpoint.delete()Attachment lookup relied solely on the primary key, with no scoping to the workspace, project, or issue. This allowed any authenticated user to delete attachments belonging to a different tenant.
Type of Change
Test Plan
PATCH /api/v1/workspaces/<slug>/projects/<project_id>/assets/<asset_id>/404when the asset belongs to a different workspace or projectDELETE /api/v1/workspaces/<slug>/projects/<project_id>/issues/<issue_id>/attachments/<pk>/404when the attachment belongs to a different workspace, project, or issueSummary by CodeRabbit