fix(space): close cross-project comment IDOR and fix broken vote/reaction queryset kwargs - #9498
Conversation
VULN-01 — Cross-project comment list IDOR (AllowAny endpoint)
The IssueCommentPublicViewSet.get_queryset() filtered only by
workspace_id and the URL-supplied issue_id, with no project_id guard.
Any unauthenticated caller who knew a private issue UUID could read
all its EXTERNAL comments through any public board in the workspace.
Fix: add .filter(project_id=project_deploy_board.project_id).
VULN-02 — Cross-project comment injection
IssueCommentPublicViewSet.create() accepted the URL issue_id without
confirming it belonged to the board's project. A caller could POST a
comment referencing an issue from a different private project, creating
a semantically inconsistent row (comment.project_id ≠ issue.project_id)
and contaminating private project data.
Fix: validate issue existence in project_id before serializer.save().
VULN-04 — IssueVotePublicViewSet.get_queryset() wrong lookup kwarg
The queryset used workspace__slug=self.kwargs.get("anchor") but
"anchor" is an opaque token, never a workspace slug. This caused
DeployBoard.DoesNotExist on every list request, silently returning
an empty queryset and making vote listing permanently broken.
Fix: use anchor=self.kwargs.get("anchor") to match the URL pattern.
BONUS — IssueReactionPublicViewSet.get_queryset() wrong lookup kwargs
Same class of bug: the queryset looked up the DeployBoard via
workspace__slug=self.kwargs.get("slug") and
project_id=self.kwargs.get("project_id"), but the URL pattern
/anchor/<str:anchor>/issues/<uuid:issue_id>/reactions/ provides
neither "slug" nor "project_id" kwargs. Both resolved to None,
causing DeployBoard.DoesNotExist on every list request.
Fix: use anchor=self.kwargs.get("anchor") and derive project/workspace
from the resolved board object.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
|
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 (1)
📝 WalkthroughWalkthroughPublic Space issue endpoints now enforce board project/workspace scoping for comment, reaction, and vote operations. Reaction and vote listings resolve boards by URL anchor, and regression tests cover isolation, validation, lookup, and listing behavior. ChangesPublic issue endpoint scope
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…reate Adversarial review (Phase 3) found that IssueVotePublicViewSet.create() and IssueReactionPublicViewSet.create() suffered the same class of bug as VULN-02: both accepted a URL-supplied issue_id without verifying it belonged to the board's project. An authenticated caller could POST a vote or reaction referencing an issue from a private project, creating DB records with vote.project_id != vote.issue.project_id. Fix: add the same Issue ownership check (Issue.objects.filter( pk=issue_id, project_id=..., workspace_id=...).exists()) before saving each object, returning HTTP 404 on mismatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![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
🤖 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/space/views/issue.py`:
- Around line 273-277: Update the Issue ownership query inside
_issue_belongs_to_board() to use Issue.issue_objects instead of Issue.objects,
preserving the existing project_id and workspace_id filters so deleted,
archived, and draft issues are rejected.
🪄 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: eb125563-9442-46aa-b694-d23e84293b52
📒 Files selected for processing (2)
apps/api/plane/space/views/issue.pyapps/api/tests/test_space_issue_security.py
| if not Issue.objects.filter( | ||
| pk=issue_id, | ||
| project_id=project_deploy_board.project_id, | ||
| workspace_id=project_deploy_board.workspace_id, | ||
| ).exists(): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Issue model's manager definitions to confirm delta between `objects` and `issue_objects`.
ast-grep run --pattern 'class Issue($$$) {
$$$
}' --lang python apps/api/plane/db/models
rg -n "issue_objects" apps/api/plane/db/models -A3 -B3Repository: makeplane/plane
Length of output: 847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
git ls-files | rg '(^apps/api/plane/db/models/issue\.py$|^apps/api/plane/space/views/issue\.py$)'
echo
echo "== Issue model manager section =="
sed -n '1,230p' apps/api/plane/db/models/issue.py | nl -ba | sed -n '1,230p'
echo
echo "== views relevant sections =="
sed -n '250,285p' apps/api/plane/space/views/issue.py | nl -ba
sed -n '620,635p' apps/api/plane/space/views/issue.py | nl -ba
echo
echo "== Search for IssueManager/issue_objects uses =="
rg -n "class IssueManager|issue_objects = IssueManager|Issue\.issue_objects|Issue\.objects\.filter\(pk=issue_id|Issue.objects\.filter\(.*issue_id" apps/api -g '*.py'Repository: makeplane/plane
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Issue model manager section =="
sed -n '1,230p' apps/api/plane/db/models/issue.py
echo
echo "== views relevant sections =="
sed -n '250,285p' apps/api/plane/space/views/issue.py
sed -n '620,635p' apps/api/plane/space/views/issue.py
echo
echo "== Search for IssueManager/issue_objects uses =="
rg -n "class IssueManager|issue_objects = IssueManager|Issue\.issue_objects|Issue\.objects\.filter\(pk=issue_id|Issue\.objects\.filter\(.*issue_id" apps/api -g '*.py'Repository: makeplane/plane
Length of output: 32829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== _issue_belongs_to_board helper definition =="
sed -n '1,120p' apps/api/plane/space/views/issue.py
echo
echo "== SoftDeletionManager definition =="
rg -n "class SoftDeletionManager|SoftDeletionManager" apps/api/plane/db apps/api/plane/utils -g '*.py' -A8 -B3
echo
echo "== ProjectBaseModel soft deletion fields =="
rg -n "class ProjectBaseModel|archived_at|deleted_at" apps/api/plane/db/models -g '*.py' -A10 -B5Repository: makeplane/plane
Length of output: 50371
Use Issue.issue_objects for the board ownership check.
Issue.issue_objects is the soft-delete-aware manager and also excludes archived/draft issues, while _issue_belongs_to_board() currently uses plain Issue.objects. Use Issue.issue_objects here to reject comments on deleted or archived issues in the scope that is used for public issue listing and retrieval.
🤖 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/space/views/issue.py` around lines 273 - 277, Update the Issue
ownership query inside _issue_belongs_to_board() to use Issue.issue_objects
instead of Issue.objects, preserving the existing project_id and workspace_id
filters so deleted, archived, and draft issues are rejected.
There was a problem hiding this comment.
Actionable comments posted: 2
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/space/views/issue.py (1)
758-758: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse reaction actor fields for
reaction_items.avatar_url.Line 758 fixes the raw avatar, but the adjacent
avatar_urlCasestill usesvotes__actor. A reaction can therefore display a voter’s avatar URL, or no asset URL when the issue has no votes.🤖 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/space/views/issue.py` at line 758, Update the adjacent avatar_url Case expression to reference the reaction actor relationship, matching the avatar field’s issue_reactions__actor source instead of votes__actor. Preserve the existing fallback behavior while ensuring reaction_items.avatar_url is derived from the reaction actor’s avatar data.
🤖 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/space/views/issue.py`:
- Around line 278-282: The destructive mutation lookups in partial_update() and
destroy() must be scoped to the resolved board. In
apps/api/plane/space/views/issue.py lines 278-282, fetch comments using pk,
actor, route issue_id, board project_id, board workspace_id, and
access="EXTERNAL"; in lines 401-407, add project_deploy_board.project_id to the
reaction lookup so mutations cannot cross project or board boundaries.
- Around line 584-592: Update the vote creation flow in the affected view’s
create method to validate the board’s is_votes_enabled setting before persisting
a vote. When voting is disabled, return the established appropriate error
response and do not create the vote; preserve the existing project-ownership
validation and normal creation behavior when enabled.
---
Outside diff comments:
In `@apps/api/plane/space/views/issue.py`:
- Line 758: Update the adjacent avatar_url Case expression to reference the
reaction actor relationship, matching the avatar field’s issue_reactions__actor
source instead of votes__actor. Preserve the existing fallback behavior while
ensuring reaction_items.avatar_url is derived from the reaction actor’s avatar
data.
🪄 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: bec31edc-e8b6-4a89-976c-70023ef56a72
📒 Files selected for processing (1)
apps/api/plane/space/views/issue.py
| # FIX VULN-02: reject writes when issue_id does not belong to the board's | ||
| # project. Without this check a caller can attach a comment to any issue in | ||
| # the system — including issues from private projects — by supplying an | ||
| # arbitrary issue_id while using a public board as a proxy. | ||
| if not _issue_belongs_to_board(issue_id, project_deploy_board): |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope destructive mutations to the resolved board as well. The new create-side checks do not protect existing update/delete paths, whose object lookups remain insufficiently scoped.
apps/api/plane/space/views/issue.py#L278-L282: makepartial_update()anddestroy()fetch comments bypk,actor, routeissue_id, boardproject_id, boardworkspace_id, andaccess="EXTERNAL". Currently an owner can use another board anchor to modify or delete their comment and bypass the target board’s comments toggle.apps/api/plane/space/views/issue.py#L401-L407: addproject_id=project_deploy_board.project_idto the reaction lookup indestroy(). Its current workspace-only scope lets an actor delete their reaction on another project in the same workspace through this board.
📍 Affects 1 file
apps/api/plane/space/views/issue.py#L278-L282(this comment)apps/api/plane/space/views/issue.py#L401-L407
🤖 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/space/views/issue.py` around lines 278 - 282, The destructive
mutation lookups in partial_update() and destroy() must be scoped to the
resolved board. In apps/api/plane/space/views/issue.py lines 278-282, fetch
comments using pk, actor, route issue_id, board project_id, board workspace_id,
and access="EXTERNAL"; in lines 401-407, add project_deploy_board.project_id to
the reaction lookup so mutations cannot cross project or board boundaries.
|
|
||
| # FIX Phase-3: same cross-project injection risk as VULN-02 — reject votes | ||
| # for issues that do not belong to the board's project. | ||
| if not _issue_belongs_to_board(issue_id, project_deploy_board): | ||
| return Response( | ||
| {"error": "Issue not found in this project."}, | ||
| status=status.HTTP_404_NOT_FOUND, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce is_votes_enabled before creating a vote.
get_queryset() hides votes when disabled, but create() still persists one. This bypasses the board’s vote feature toggle.
Proposed fix
def create(self, request, anchor, issue_id):
project_deploy_board = DeployBoard.objects.get(anchor=anchor, entity_name="project")
+
+ if not project_deploy_board.is_votes_enabled:
+ return Response(
+ {"error": "Votes are not enabled for this project board"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
if not _issue_belongs_to_board(issue_id, project_deploy_board):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # FIX Phase-3: same cross-project injection risk as VULN-02 — reject votes | |
| # for issues that do not belong to the board's project. | |
| if not _issue_belongs_to_board(issue_id, project_deploy_board): | |
| return Response( | |
| {"error": "Issue not found in this project."}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) | |
| # FIX Phase-3: same cross-project injection risk as VULN-02 — reject votes | |
| # for issues that do not belong to the board's project. | |
| if not project_deploy_board.is_votes_enabled: | |
| return Response( | |
| {"error": "Votes are not enabled for this project board"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| if not _issue_belongs_to_board(issue_id, project_deploy_board): | |
| return Response( | |
| {"error": "Issue not found in this project."}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) | |
🤖 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/space/views/issue.py` around lines 584 - 592, Update the vote
creation flow in the affected view’s create method to validate the board’s
is_votes_enabled setting before persisting a vote. When voting is disabled,
return the established appropriate error response and do not create the vote;
preserve the existing project-ownership validation and normal creation behavior
when enabled.
…reate ownership checks Final security review identified two gaps in the test suite: 1. IssueVotePublicViewSet.create() and IssueReactionPublicViewSet.create() both received _issue_belongs_to_board() ownership guards in the Phase-3 hardening commit, but no regression tests were added for those paths. A future refactor could silently remove the guards with no test failure. 2. test_reaction_list_returns_results_when_enabled contained a vacuous assertion (`!= uuid4()`) that always evaluates True regardless of actual code behaviour, providing no security value. Changes: - Add TestIssueVoteCreateProjectIsolation (2 tests: 404 on foreign issue, 201 on own issue) - Add TestIssueReactionCreateProjectIsolation (2 tests: 404 on foreign issue, 201 on own issue) - Fix vacuous uuid4() assertion in TestIssueReactionGetQuerysetKwarg Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔐 Post-Merge Security Operations — Maintainer NotesThis comment documents all post-merge actions for the security team's records. ✅ Merge Checklist
🟡 Non-Blocking Follow-Up IssuesThree pre-existing issues were identified during the audit that do not block this merge but should be addressed in follow-up PRs:
🔍 Vulnerability Pattern DetectedPattern: Inconsistent kwarg usage between Pattern: Public board endpoints that filter by 📋 Commits in this PR
|
Summary
This PR fixes two confirmed security vulnerabilities and two functional bugs discovered during a focused audit of
plane/space/views/issue.py.🔴 VULN-01 — Cross-project comment list IDOR (
AllowAnyendpoint)File:
apps/api/plane/space/views/issue.pyClass:
IssueCommentPublicViewSet.get_queryset()Root cause: The queryset filtered by
workspace_idand a URL-suppliedissue_id, but never byproject_id. Because comment listing usesAllowAny(no authentication required), any caller who knew a private issue UUID could read all itsEXTERNALcomments by routing requests through any public board in the same workspace.Fix: Add
.filter(project_id=project_deploy_board.project_id)before theissue_idfilter.🔴 VULN-02 — Cross-project comment injection
File:
apps/api/plane/space/views/issue.pyClass:
IssueCommentPublicViewSet.create()Root cause:
create()accepted the URL-suppliedissue_idwithout verifying it belonged to the board's project. An authenticated caller could POST a comment referencing any issue in the system, creating a semantically inconsistent database record (comment.project_id ≠ issue.project_id) and contaminating private project data.Fix: Validate
Issue.objects.filter(pk=issue_id, project_id=..., workspace_id=...).exists()beforeserializer.save(). Returns HTTP 404 on mismatch.🟡 VULN-04 —
IssueVotePublicViewSet.get_queryset()wrong lookup kwargFile:
apps/api/plane/space/views/issue.pyClass:
IssueVotePublicViewSet.get_queryset()Root cause: The
DeployBoardlookup usedworkspace__slug=self.kwargs.get("anchor")but the URL pattern/anchor/<str:anchor>/…/votes/never provides aslugkwarg —anchoris an opaque token, not a workspace slug. The lookup always raisedDoesNotExist, silently returning an empty queryset and making vote listing permanently broken on every public board. Thecreatemethod in the same class correctly usedanchor=anchor.Fix: Change to
anchor=self.kwargs.get("anchor")to match both the URL pattern and the workingcreatemethod.🟡 BONUS —
IssueReactionPublicViewSet.get_queryset()wrong lookup kwargsFile:
apps/api/plane/space/views/issue.pyClass:
IssueReactionPublicViewSet.get_queryset()Root cause: Same class of bug — the lookup used
workspace__slug=self.kwargs.get("slug")andproject_id=self.kwargs.get("project_id"), but the URL pattern/anchor/<str:anchor>/issues/<uuid:issue_id>/reactions/provides neither"slug"nor"project_id"kwargs. Both resolved toNone,DoesNotExistwas always raised, and reaction listing was permanently broken on every public board.Fix: Change to
anchor=self.kwargs.get("anchor")and derive workspace/project from the resolved board.Changes
apps/api/plane/space/views/issue.pyapps/api/tests/test_space_issue_security.pyRegression tests
TestIssueCommentGetQuerysetProjectIsolation— assertsproject_idfilter is present in queryset chain (VULN-01)TestIssueCommentCreateProjectIsolation::test_returns_404_for_foreign_issue— exploit scenario, expects HTTP 404 (VULN-02)TestIssueCommentCreateProjectIsolation::test_allows_comment_on_board_project_issue— legitimate path still returns HTTP 201 (VULN-02)TestIssueCommentCreateProjectIsolation::test_returns_400_when_comments_disabled— board gate still blocks before ownership check (VULN-02)TestIssueVoteGetQuerysetKwarg::test_queryset_resolves_board_by_anchor_not_slug— assertsanchor=kwarg is used, notworkspace__slug=(VULN-04)TestIssueReactionGetQuerysetKwarg::test_queryset_resolves_board_by_anchor— same for reactions (BONUS)TestIssueReactionGetQuerysetKwarg::test_reaction_list_returns_results_when_enabled— asserts non-empty queryset is returned (BONUS)Summary by CodeRabbit
Bug Fixes
404for foreign issues.Tests