[WEB-8019] fix(security): scope CycleIssue reassignment lookup to workspace/project#9349
Open
mguptahub wants to merge 2 commits into
Open
[WEB-8019] fix(security): scope CycleIssue reassignment lookup to workspace/project#9349mguptahub wants to merge 2 commits into
mguptahub wants to merge 2 commits into
Conversation
… endpoints Close a partial bypass of WEB-7813 (GHSA-2r95 / GHSA-w45q): the external REST API project-list and work-item-list endpoints passed a raw order_by query parameter to Django's .order_by(). Because Django resolves __-separated relational paths, an attacker could order by sensitive columns on related tables (created_by__password / token / email) to build a blind ordering oracle, or crash the endpoint (HTTP 500) with an unknown field. Route both endpoints through the existing sanitize_order_by() helper with the appropriate allowlist (PROJECT_ORDER_BY_ALLOWLIST, default sort_order; ISSUE_ORDER_BY_ALLOWLIST, default -created_at), mirroring how order_issue_queryset() already sanitizes. Non-allowlisted values collapse to the safe default; legitimate orderings are unchanged. Adds unit tests (allowlist neutralisation + passthrough) and contract tests asserting both endpoints return 200 (not 500) for injected fields; fail-before verified via git stash. Advisory: GHSA-p885-6jpg-cr2p Co-authored-by: Plane AI <noreply@plane.so>
…kspace/project CycleIssueViewSet.create looked up "issues already in another cycle" with CycleIssue.objects.filter(~Q(cycle_id=cycle_id), issue_id__in=issues) — without scoping to the caller's workspace/project. An ADMIN/MEMBER of their own project could pass a work-item UUID from a different tenant and have that foreign CycleIssue row reassigned to their cycle, silently evicting the victim's work item from the victim's cycle (cross-tenant write / BOLA). Scope the lookup to workspace__slug + project_id, mirroring the adjacent create-path guard. Foreign-tenant rows are excluded from reassignment and already dropped from the create path by the scoped new_issues query. Adds a contract regression test proving a foreign-tenant CycleIssue row is not reassigned (fail-before verified via git stash) plus a same-project reassignment test to confirm the legitimate flow is unaffected. Advisory: GHSA-4w5x-wc9w-f47x Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
Contributor
📝 WalkthroughWalkthroughThis PR adds order_by sanitization via allowlists to issue and project list endpoints to prevent unsafe/injection-style ordering, and scopes the CycleIssue lookup query in ChangesOrder-by injection sanitization
Cycle issue cross-tenant scoping fix
Estimated code review effort: 3 (Moderate) | ~25 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens Plane’s REST API against two security issues by (1) preventing cross-tenant CycleIssue reassignment during cycle-issue creation and (2) sanitizing user-supplied order_by parameters for project and issue listing endpoints to block ORM relational-traversal ordering injection. Adds unit + contract regression tests to ensure both fixes remain enforced.
Changes:
- Scope
CycleIssueViewSet.create’s “already in another cycle” lookup to the caller’s workspace + project to prevent cross-tenant write/BOLA reassignment. - Sanitize
order_byquery params for project and issue list endpoints usingsanitize_order_by()+ endpoint-specific allowlists. - Add unit tests for
sanitize_order_by()behavior and contract tests covering both the order-by injection regression and the cross-tenant cycle-issue reassignment regression.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/app/views/cycle/issue.py | Scopes reassignment lookup by workspace__slug + project_id to prevent cross-tenant CycleIssue updates. |
| apps/api/plane/api/views/project.py | Sanitizes order_by for project listing using PROJECT_ORDER_BY_ALLOWLIST. |
| apps/api/plane/api/views/issue.py | Sanitizes order_by for work-item listing using ISSUE_ORDER_BY_ALLOWLIST before any ordering logic runs. |
| apps/api/plane/tests/unit/utils/test_order_by_sanitize.py | Adds unit-level regression coverage ensuring allowlists reject payloads and preserve legitimate fields (including descending variants). |
| apps/api/plane/tests/contract/api/test_projects.py | Adds contract-level regression coverage ensuring malicious/invalid order_by no longer triggers HTTP 500 for projects list. |
| apps/api/plane/tests/contract/api/test_issues.py | Adds contract-level regression coverage ensuring malicious/invalid order_by no longer triggers HTTP 500 for issues list. |
| apps/api/plane/tests/contract/app/test_cycle_issue_app.py | Adds contract regression tests validating cross-tenant reassignment is blocked while same-tenant reassignment still works. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/app/test_cycle_issue_app.py (1)
139-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame-tenant reassignment coverage looks good.
Confirms the scope guard doesn't break legitimate same-project reassignment.
One gap: only cross-workspace isolation is tested; consider adding a same-workspace-but-different-project case, since the fix also scopes by
project_idand that boundary isn't currently exercised.🤖 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/tests/contract/app/test_cycle_issue_app.py` around lines 139 - 176, Add a contract test for the project boundary in the cycle reassignment flow: the current `test_same_tenant_reassignment_still_works` covers same-project success, but you should also exercise the `CycleIssue`/`CycleIssueView` path with an issue in the same workspace and a different project to ensure the `project_id` scope guard blocks reassignment across projects while still allowing the legitimate same-project case.
🤖 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/tests/contract/app/test_cycle_issue_app.py`:
- Around line 139-176: Add a contract test for the project boundary in the cycle
reassignment flow: the current `test_same_tenant_reassignment_still_works`
covers same-project success, but you should also exercise the
`CycleIssue`/`CycleIssueView` path with an issue in the same workspace and a
different project to ensure the `project_id` scope guard blocks reassignment
across projects while still allowing the legitimate same-project case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77919783-a866-4ed8-ac23-a7175fdfe7d9
📒 Files selected for processing (7)
apps/api/plane/api/views/issue.pyapps/api/plane/api/views/project.pyapps/api/plane/app/views/cycle/issue.pyapps/api/plane/tests/contract/api/test_issues.pyapps/api/plane/tests/contract/api/test_projects.pyapps/api/plane/tests/contract/app/test_cycle_issue_app.pyapps/api/plane/tests/unit/utils/test_order_by_sanitize.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a cross-tenant write BOLA in
CycleIssueViewSet.create(GHSA-4w5x-wc9w-f47x, medium).POST /api/workspaces/<slug>/projects/<project_id>/cycles/<cycle_id>/cycle-issues/looked up work items "already in another cycle" with:This lookup was not scoped to the caller's workspace/project. An ADMIN/MEMBER of their own project could pass a work-item UUID belonging to a different workspace/project and have that foreign
CycleIssuerow reassigned to their own cycle — silently evicting the victim's work item from the victim's cycle and orphaning the row (project_id= victim,cycle_id= attacker).The adjacent create path in the same handler was already scoped (with an explicit
# prevent cross-tenant IDORcomment, added for GHSA-933r); the reassignment path above it was missed. Same class as GHSA-933r-rxg8-f3h2.Fix
Scope the lookup to
workspace__slug=slug+project_id=project_id, mirroring the adjacent create-path guard. Foreign-tenant rows are excluded from reassignment and are already dropped from the create path by the scopednew_issuesquery — so foreign issue IDs no longer produce any write.Tests
tests/contract/app/test_cycle_issue_app.py:CycleIssuestays in the victim's cycle, and noCycleIssueis created under the attacker's cycle. Fail-before verified viagit stash(row was reassigned without the fix).Verification
ruff checkcleanpython manage.py checkcleanSummary by CodeRabbit
order_byvalues no longer cause errors.