[SECUR-243] fix(security): scope issue comment + relation endpoints to the URL project (GHSA-hvx3-58mp-5fpx) - #9531
[SECUR-243] fix(security): scope issue comment + relation endpoints to the URL project (GHSA-hvx3-58mp-5fpx)#9531mguptahub wants to merge 3 commits into
Conversation
…he URL project ProjectEntityPermission and allow_permission validate only that the caller is an active member of the URL's project_id. Neither validates that the sibling issue_id path parameter belongs to that project or workspace, and two write handlers used issue_id unscoped: - IssueCommentViewSet.create resolved the issue with Issue.objects.get(pk=issue_id). Any member of any one project could comment on an issue in another project or workspace, and the 201 response serialized that issue's full issue_detail — name, description_json, description_html, priority, dates, sequence_id — back to the caller, making this a cross-tenant read as well as a write. - IssueRelationViewSet.create workspace-scoped the request-body issues list but used the URL issue_id unscoped as the other side of the relation, so the earlier scoping fix covered one side of the relationship and missed the other. Both lookups are now bound to workspace__slug + project_id and 404 otherwise, matching the pattern already used by sub_issue.py get() and PagesDescriptionViewSet. The third vector in the advisory (SubIssuesEndpoint.post) is already addressed by open PR #9466 and is deliberately not touched here. Adds contract regression coverage for both endpoints: cross-workspace and same-workspace-other-project both 404 with nothing written and no detail leaked, plus positive controls for the in-project paths. Advisory: GHSA-hvx3-58mp-5fpx Co-authored-by: Plane AI <noreply@plane.so>
…oject Follow-up from adversarial review of the create-path fix: remove_relation had the same gap on the delete side. It filtered IssueRelation on workspace__slug only and never checked that the URL issue_id belongs to the URL project, so a member of one project could delete relations between two issues of a sibling project in the same workspace — verified 204 with deleted_at set on the sibling project's row. Applies the same participant binding as create(): the URL issue must live in the URL project. The IssueRelation row itself stays workspace-scoped, because relations legitimately span projects and either participant's project may remove them. Also returns 404 instead of a 500 when no relation matches — the handler did .first() and then called .delete() on None (AttributeError -> generic 500). Adds coverage for the sibling-project delete, the missing-relation 404, and a positive control, plus tests pinning the IssueManager boundary: comment.py deliberately uses Issue.objects (intake/triage, archived and draft issues must stay commentable) while relation.py uses Issue.issue_objects (matching the body `issues` filter and SubIssuesEndpoint). That axis was previously untested, so "tidying" the two to match would have silently broken commenting on intake items. 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)
📝 WalkthroughWalkthroughThe API now scopes issue comment and relation operations to the URL workspace and project. Invalid issue bindings and missing relations return 404 responses. Contract tests cover cross-scope requests, valid operations, deletion behavior, issue states, and malformed input. ChangesIssue subresource scope
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 addresses SECUR-243 / GHSA-hvx3-58mp-5fpx by hard-binding issue sub-resource handlers (comments + issue relations) to the URL workspace + project, preventing cross-project / cross-workspace IDOR via an unscoped issue_id.
Changes:
- Scope
IssueCommentViewSet.createissue lookup to(pk, workspace__slug, project_id)and return 404 when mismatched. - Scope
IssueRelationViewSet.createandremove_relationby validating the URLissue_idbelongs to(workspace__slug, project_id), and return 404 when missing relations would previously 500. - Add contract tests to cover cross-workspace and same-workspace-other-project cases, plus manager-boundary behavior for archived/draft issues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/app/views/issue/comment.py | Scopes comment creation to the URL project/workspace to prevent IDOR and leakage via the 201 response payload. |
| apps/api/plane/app/views/issue/relation.py | Adds URL-issue binding for relation create/remove and returns 404 instead of 500 when no matching relation exists. |
| apps/api/plane/tests/contract/app/test_issue_subresource_scope_app.py | Introduces contract coverage for the IDOR regression and documents the intentional Issue.objects vs Issue.issue_objects behavior boundary. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/plane/app/views/issue/relation.py (2)
220-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the issue-binding check into a shared helper.
The
Issue.issue_objects.filter(pk=issue_id, workspace__slug=slug, project_id=project_id).exists()check at Line 226 duplicates the identical check inremove_relationat Line 289. Extract this into one private method both call. A single implementation reduces the risk that a future edit updates one check but not the other, which would reopen the scoping gap this PR fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/issue/relation.py` around lines 220 - 228, Extract the repeated issue-binding query into a private helper on the surrounding view class, preserving the workspace slug, project ID, and issue ID filters. Update both the shown relation-creation path and remove_relation to call this helper and return the existing 404 response when it fails.
283-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood fix for the missing-relation 500; extract the duplicated binding check.
The
.first()plusis Nonecheck correctly converts the previousNone.delete()crash into a 404, matchingtest_missing_relation_404s_rather_than_500.The issue-binding check at Line 289 duplicates the one in
createat Line 226. Extract both into a shared private method to keep this security-relevant check in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/issue/relation.py` around lines 283 - 300, Extract the duplicated issue-binding validation from the create and delete flows into a shared private method on the view, using the existing issue ID, workspace slug, and project ID inputs. Replace both inline checks with calls to that method while preserving the current 404 response behavior, and leave the missing-relation `.first()`/`None` handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/plane/app/views/issue/relation.py`:
- Around line 220-228: Extract the repeated issue-binding query into a private
helper on the surrounding view class, preserving the workspace slug, project ID,
and issue ID filters. Update both the shown relation-creation path and
remove_relation to call this helper and return the existing 404 response when it
fails.
- Around line 283-300: Extract the duplicated issue-binding validation from the
create and delete flows into a shared private method on the view, using the
existing issue ID, workspace slug, and project ID inputs. Replace both inline
checks with calls to that method while preserving the current 404 response
behavior, and leave the missing-relation `.first()`/`None` handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 211393e6-0511-48be-bc68-83ad107a4866
📒 Files selected for processing (3)
apps/api/plane/app/views/issue/comment.pyapps/api/plane/app/views/issue/relation.pyapps/api/plane/tests/contract/app/test_issue_subresource_scope_app.py
Addresses Copilot review on #9531. IssueRelationViewSet.list had the same unscoped issue_id as the write handlers: it filtered relations on workspace__slug only, so a member of project A could list the relations of an issue in project B of the same workspace and receive that issue's name, priority, sequence_id, assignee_ids and label_ids. Verified 200 with the foreign issue's name in the body before the fix. Extracts the binding into a module-level issue_in_project() helper (per CodeRabbit) now that list, create and remove_relation all need it — one place to get right, and the next handler added to this file has an obvious thing to call. Adds contract coverage for the list path (cross-project 404 with no name leak, plus an in-project positive control), extends the IssueManager boundary tests to remove_relation and list so all handlers are pinned to the same manager, and pins that a non-UUID related_issue is a 400 rather than a 500 — Copilot flagged that as a 500 risk, but Django raises ValidationError on UUID coercion and BaseViewSet.handle_exception already converts it. The test locks that in. Co-authored-by: Plane AI <noreply@plane.so>
|
Thanks — one of these was a real hole I'd missed, one doesn't reproduce. Addressed in 9fd1d53. 1.
|
Summary
Closes a cross-tenant IDOR (CWE-639) on three issue sub-resource write handlers.
ProjectEntityPermissionand theallow_permissiondecorator validate only that the caller is an active member of the URL'sproject_id— neither validates that the siblingissue_idpath parameter belongs to that project or workspace. Any authenticated user who is a member of one project on the instance could therefore reach issues in other projects and other workspaces by passing their UUID in the URL.Advisory: GHSA-hvx3-58mp-5fpx (HIGH,
CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:L/A:N), reported by @Avinash9102. Ticket: SECUR-243.What was wrong
IssueCommentViewSet.createIssue.objects.get(pk=issue_id)— no workspace/project filterissue_detail—name,description_json,description_html,priority,start_date,target_date,sequence_id— back to the caller, and plants a comment on an issue in a workspace they never joinedIssueRelationViewSet.createissueswere workspace-scoped, but the URLissue_idused as the other side of the relation was notIssueRelationViewSet.remove_relationIssueRelationonworkspace__slugonlyThe fix
All three bind the lookup to
pk=issue_id, workspace__slug=slug, project_id=project_idand return 404 otherwise — the pattern already established bysub_issue.py'sget()andPagesDescriptionViewSet.retrieve.For
remove_relation, only the URL issue is project-bound; theIssueRelationrow stays workspace-scoped on purpose, because relations legitimately span projects and either participant's project must be able to remove them.Also fixes a pre-existing 500 in
remove_relation: it did.first()and then called.delete()onNone(AttributeError→ generic 500) when no relation matched. Now a 404.Deliberately not in this PR
SubIssuesEndpoint.post— the advisory's third vector. Already fixed by open PR [WEB-8352] fix(security): scope SubIssuesEndpoint to the URL project (GHSA-gxhv-fw9x-2pg3) #9466 (WEB-8352, GHSA-gxhv-fw9x-2pg3); no point duplicating it.reaction.py,link.py,subscriber.py,CommentReactionViewSet.create,attachment.py. All verified reachable cross-workspace, but impact is materially lower: their responses carry no victim content and rows land with the attacker'sproject_id, so the victim's project-scoped querysets andnotification_taskhide them (pollution, not disclosure). Tracked in SECUR-244.Behaviour change worth reviewing
comment.pyusesIssue.objectsandrelation.pyusesIssue.issue_objects. That asymmetry is intentional, not an oversight —IssueManageradditionally excludes triage-state, archived, draft, and archived-project issues:Issue.objects— exactly behaviour-preserving. Intake/triage, archived and draft issues must remain commentable (intake depends on it).issue_objects— matches the file's existing convention (the bodyissuesfilter) andSubIssuesEndpoint. This narrows behaviour: creating or removing a relation whose URL issue is triage/archived/draft now 404s where it previously succeeded. No first-party regression — the web client renders no relations UI in intake, passesdisabled={isArchived}on archived detail, and drafts have no relation UI — but a non-web app-API consumer doing this would see the change. Flagging explicitly in case reviewers preferIssue.objectshere for strict parity.New tests pin this boundary in both directions, so a future "tidy these to match" change can't silently break commenting on intake items.
Testing
12 new contract tests in
plane/tests/contract/app/test_issue_subresource_scope_app.py, covering each handler with a cross-workspace case, a same-workspace-other-project case, and a positive control.Fail-before verified by reverting each fix and re-running:
"Confidential victim issue"and"Secret roadmap detail"present in the response bodyIssueRelationrow written against the victim issuedeleted_atset on the sibling project's relationAttributeError: 'NoneType' object has no attribute 'delete'Positive controls stayed green throughout, so the tests exercise the scoping rather than the plumbing — and the 404s are attributable to the new lookups, not to a permission denial that would have happened anyway (the URL project is always one the attacker is a member of).
Full suite: 526 passed (
plane/tests/contract+plane/tests/unit).ruff checkandruff format --checkclean.Follow-ups
ProjectEntityPermissionignoresissue_id. Worth validating issue-to-project binding at the permission layer rather than per view. Also noted in SECUR-244:permissions/base.py'screator=Truebranch filters oncreated_bywith no workspace/project scope, which is safe only because today's callers re-scope themselves.The advisory stays in triage until this ships in a release — it is not eligible for the v1.4.0 CVE batch.
Summary by CodeRabbit
Bug Fixes
Tests