[WEB-7769] fix(security): scope EstimatePoint create/destroy to workspace and project#9286
[WEB-7769] fix(security): scope EstimatePoint create/destroy to workspace and project#9286mguptahub wants to merge 1 commit into
Conversation
…pace and project GHSA-933r-rxg8-f3h2 — EstimatePointEndpoint.create trusted the estimate_id URL parameter without verifying it belonged to the caller's workspace and project. An authenticated user in project A could inject estimate points into any other workspace's estimate by supplying a foreign estimate_id. Fix: added a workspace+project scoped Estimate ownership check before EstimatePoint.objects.create(). GHSA-933r-rxg8-f3h2 (destroy) — old_estimate_point was fetched with pk only (unscoped), allowing cross-tenant key disclosure and manipulation during the key-rearrangement step. Fix: scoped the old_estimate_point lookup to estimate_id + project_id + workspace__slug; added 404 guard for missing/foreign points. Note: BulkEstimatePointEndpoint.partial_update (GHSA-vm3j-5j49-gwrf) was already correctly scoped at lines 116 and 125-130 — no change needed. Co-authored-by: Plane AI <noreply@plane.so>
📝 WalkthroughWalkthrough
ChangesTenant-scoped EstimatePoint IDOR Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
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/estimate/base.py (1)
203-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the scoped estimate point before mutating issues.
The new 404 guard runs after issue updates/activity enqueueing. A mismatched
estimate_idcan still mutate issues or emit activity before returning"Estimate point not found".Proposed fix
def destroy(self, request, slug, project_id, estimate_id, estimate_point_id): new_estimate_id = request.data.get("new_estimate_id", None) + + # Validate the target before any issue updates or activity side effects. + old_estimate_point = EstimatePoint.objects.filter( + pk=estimate_point_id, + estimate_id=estimate_id, + project_id=project_id, + workspace__slug=slug, + ).first() + if not old_estimate_point: + return Response( + {"error": "Estimate point not found"}, + status=status.HTTP_404_NOT_FOUND, + ) + estimate_points = EstimatePoint.objects.filter( estimate_id=estimate_id, project_id=project_id, workspace__slug=slug ) @@ - # delete the estimate point — scope to this estimate/project/workspace to prevent cross-tenant key manipulation - old_estimate_point = EstimatePoint.objects.filter( - pk=estimate_point_id, - estimate_id=estimate_id, - project_id=project_id, - workspace__slug=slug, - ).first() - if not old_estimate_point: - return Response( - {"error": "Estimate point not found"}, - status=status.HTTP_404_NOT_FOUND, - ) - # rearrange the estimate pointsAlso applies to: 241-252
🤖 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/estimate/base.py` around lines 203 - 239, The code validates the scoped estimate point after mutating issues and enqueueing activity logs, allowing invalid or mismatched estimate IDs to cause unintended mutations. Move the validation logic that checks whether the estimate point exists and belongs to the project and workspace to execute before the conditional blocks that filter issues and call issue_activity.delay, ensuring that a 404 response is returned before any issue updates or activity logs are created when an invalid estimate_point_id or new_estimate_id is provided.
🤖 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/estimate/base.py`:
- Around line 203-239: The code validates the scoped estimate point after
mutating issues and enqueueing activity logs, allowing invalid or mismatched
estimate IDs to cause unintended mutations. Move the validation logic that
checks whether the estimate point exists and belongs to the project and
workspace to execute before the conditional blocks that filter issues and call
issue_activity.delay, ensuring that a 404 response is returned before any issue
updates or activity logs are created when an invalid estimate_point_id or
new_estimate_id is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16adca86-0c7f-4ec4-a504-a535f667000e
📒 Files selected for processing (1)
apps/api/plane/app/views/estimate/base.py
Summary
EstimatePointEndpoint.create: added a workspace+project scopedEstimateownership check before creating an estimate point. Previously theestimate_idURL parameter was trusted directly, allowing injection into any workspace's estimate.old_estimate_pointlookup toestimate_id + project_id + workspace__slug; added 404 guard. Previously fetched bypkonly — enabled cross-tenant key disclosure and manipulation during key-rearrangement.BulkEstimatePointEndpoint.partial_update: already correctly scoped (lines 116, 125–130). No change needed.Files changed
apps/api/plane/app/views/estimate/base.pyTest plan
estimate_idin same workspace → succeedsestimate_idfrom a different workspace → 404Co-authored-by: Plane AI noreply@plane.so
Summary by CodeRabbit