[WEB-8352] fix(security): scope SubIssuesEndpoint to the URL project - #9466
[WEB-8352] fix(security): scope SubIssuesEndpoint to the URL project#9466mguptahub wants to merge 3 commits into
Conversation
…(GHSA-gxhv-fw9x-2pg3) SubIssuesEndpoint is guarded only by ProjectEntityPermission, which verifies the caller belongs to the URL project_id but not that the path issue_id lives in that project. Both handlers then resolved issues without a project scope: - GET filtered sub-issues by parent_id + workspace__slug only, leaking the names/priorities/assignees/dates of another project's sub-issues (read IDOR). - POST loaded the parent by bare pk (no workspace/project scope) and filtered the moved sub-issues by workspace__slug only, letting any project member re-parent issues from other projects/workspaces (write IDOR). Scope the parent lookup and both sub-issue querysets to the URL project_id (and bind the parent to the workspace), returning 404 when the parent is not in the caller's project. Adds 5 contract tests (3 security, 2 positive controls); fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Linked to Plane Work Item(s) References 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ChangesSub-issue project scoping
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 an IDOR (CWE-639) in SubIssuesEndpoint by ensuring all issue/sub-issue lookups are scoped to the URL project_id (and workspace), preventing cross-project access within the same workspace.
Changes:
- Scope
GETsub-issue queryset byproject_idin addition toworkspace__slug. - Scope
POSTparent issue lookup andsub_issue_idsupdates/response queryset byworkspace__slug + project_id, returning 404 when the parent is not in the URL project. - Add contract tests covering cross-project read/write attempts plus same-project positive 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/issue/sub_issue.py | Adds project/workspace scoping to sub-issue read/write paths to prevent cross-project IDOR. |
| apps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py | Adds regression/contract coverage for cross-project sub-issue access and re-parenting scenarios. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@apps/api/plane/app/views/issue/sub_issue.py`:
- Around line 230-242: Update the activity-dispatch loop in the sub-issue update
flow to iterate only the scoped `sub_issues` or `updated_sub_issues` queryset,
not raw `sub_issue_ids`. Preserve the existing activity payload while ensuring
events are emitted only for issues matching the workspace and project filters.
In `@apps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py`:
- Around line 162-180: Extend test_write_cross_project_sub_issue_ids_ignored to
assert the successful response excludes orphan_b from both the returned
sub_issues collection and state_distribution, while retaining the existing
database-parent assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aab54f34-8991-43a5-a214-5a0cda32abc5
📒 Files selected for processing (2)
apps/api/plane/app/views/issue/sub_issue.pyapps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py
…ssues (CodeRabbit/Copilot #9466) The DB update + response were scoped to the URL project, but the activity loop still iterated the raw caller-supplied sub_issue_ids. A cross-project id (excluded from the re-parent) would still fire issue_activity.delay, whose task does an unscoped Issue.objects.get and bumps updated_at — touching a foreign issue and creating a bogus activity row. Dispatch from the project-scoped sub_issues (scoped_sub_issue_ids) instead. Strengthened the test to assert the foreign issue is absent from the response body (sub_issues / state_distribution) and that no activity is dispatched for it (mock). Fail-before verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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/sub_issue.py (1)
253-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord the actual parent ID in activity metadata.
Line 259 writes each child ID as
parent, whilerequested_datacorrectly uses the URL parent (issue_id). Activity consumers will therefore see a self-parent relation rather than the applied re-parenting.Proposed fix
- current_instance=json.dumps({"parent": sub_issue_id}), + current_instance=json.dumps({"parent": str(issue_id)}),🤖 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/sub_issue.py` around lines 253 - 260, Update the issue_activity.delay call in the sub-issue reparenting flow so current_instance records the actual parent issue_id, matching requested_data, instead of the child sub_issue_id. Keep the existing activity payload structure and other arguments 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.
Outside diff comments:
In `@apps/api/plane/app/views/issue/sub_issue.py`:
- Around line 253-260: Update the issue_activity.delay call in the sub-issue
reparenting flow so current_instance records the actual parent issue_id,
matching requested_data, instead of the child sub_issue_id. Keep the existing
activity payload structure and other arguments unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f2ae175-1939-4677-81f9-96d17d55b82a
📒 Files selected for processing (2)
apps/api/plane/app/views/issue/sub_issue.pyapps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py
Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so>
|
React Doctor found 4 issues in 4 files · 1 error & 3 warnings · score 82 / 100 (Needs work) · vs Errors
3 warnings
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/api/plane/app/views/issue/sub_issue.py:258
issue_activityusescurrent_instanceto infer the previous parent (seetrack_parent()readingcurrent_instance.get("parent")). Passing the sub-issue’s own id here ({"parent": sub_issue_id}) will make activity logs compute the old parent as the sub-issue itself, producing incorrect old/new identifiers (and potentially misleading audit history). Capture each sub-issue’sparent_idbefore mutating it and pass that ascurrent_instance.parent.
requested_data=json.dumps({"parent": str(issue_id)}),
actor_id=str(request.user.id),
issue_id=sub_issue_id,
project_id=str(project_id),
current_instance=json.dumps({"parent": sub_issue_id}),
Summary
SubIssuesEndpoint(/workspaces/<slug>/projects/<project_id>/issues/<issue_id>/sub-issues/) is guarded only byProjectEntityPermission, which verifies the caller belongs to the URLproject_idbut not that the pathissue_idlives in that project. Both handlers then resolved issues without a project scope, so any member of one project could reach another project's issues in the same workspace.Fixes WEB-8352 (CWE-639). Confirmed vulnerable against
origin/preview@a8e53b6ac7by reading the canonical code.Vulnerability
parent_id+workspace__slugonly. With a cross-projectissue_id, it returned that issue's sub-issues (names, priorities, assignees, labels, dates).pk(no workspace/project scope) and filtered the moved sub-issues byworkspace__slugonly, letting a member re-parent issues from other projects — even other workspaces — under an arbitrary parent.Fix
Scope every lookup to the URL
project_id:+ project_id.+ workspace__slug + project_id, returning 404 when the parent isn't in the caller's project (replaces the bare.get()that would otherwise 500).sub_issue_idsfilter and the response-builder queryset →+ project_id.Defense in depth: even with an in-project parent,
sub_issue_idsfrom another project are now excluded.Tests
apps/api/plane/tests/contract/app/test_sub_issue_cross_project_scope_app.py— 5 contract tests:sub_issue_idsignored (in-project parent, foreign child untouched)Fail-before verified: with the fix reverted, the 3 security tests fail and the 2 positive controls pass; with the fix, all 5 pass.
ruff+manage.py checkclean.EE
plane-ee vendors its own copy of the CE backend; an EE port will be assessed separately under Epic WEB-8293.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests