security: enforce user ownership on global tasks (Fix IDOR)#1065
security: enforce user ownership on global tasks (Fix IDOR)#1065Adraca wants to merge 2 commits intodfir-iris:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdded ownership/assignee filtering to GlobalTasks GET/EDIT/DELETE handlers so queries and deletions only match tasks where Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/app/blueprints/rest/dashboard_routes.py (1)
157-164:⚠️ Potential issue | 🟠 MajorIDOR still present on the view endpoint.
The PR description states that global tasks should only be viewed, updated, or deleted by the owner or assignee. However,
view_gtask(GET/global/tasks/<cur_id>) returns any task by id viaget_global_task(task_id=cur_id)regardless of ownership. Meanwhile, the update and delete endpoints (edit_gtaskandgtask_delete) both enforce the required filter:or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)Any authenticated user can currently read any global task's details—the same IDOR class being fixed for update/delete. Since
get_global_taskis only called fromview_gtask, you can either apply the filter inline (as sketched below) or push it into the helper for consistency with the other endpoints.In-route fix (inline filter)
`@dashboard_rest_blueprint.route`('/global/tasks/<int:cur_id>', methods=['GET']) `@ac_api_requires`() def view_gtask(cur_id): - task = get_global_task(task_id=cur_id) - if not task: + task = get_global_task(task_id=cur_id) + if not task or (task.task_userid_open != current_user.id + and task.task_assignee_id != current_user.id): return response_error(f'Global task ID {cur_id} not found')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/blueprints/rest/dashboard_routes.py` around lines 157 - 164, view_gtask currently returns any global task by id causing an IDOR; enforce the same owner/assignee restriction used by edit_gtask and gtask_delete by filtering so only tasks where GlobalTasks.task_userid_open == current_user.id OR GlobalTasks.task_assignee_id == current_user.id are returned. Fix by updating view_gtask to call get_global_task with that filter (or add the filter logic into get_global_task itself) so the GET /global/tasks/<cur_id> only returns the task when the current_user is owner or assignee.
🧹 Nitpick comments (1)
source/app/blueprints/rest/dashboard_routes.py (1)
288-293: Redundant query in delete path; factor out the duplicated ownership filter.
gtask_deleteruns the ownership-scoped query twice — once to fetch (line 288) and once to delete (line 292). Sincedatais already loaded, just delete that instance; this also makes the intent clearer and removes a class of subtle bugs (e.g., if the two filters ever drift). While here, the sameor_(...)expression appears three times in this file — extracting it into a small helper or scoping it on a query object reduces the risk of an inconsistent fix later.♻️ Proposed refactor
- data = GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).first() - if not data: - return response_error("Invalid global task ID") - - GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).delete() - db.session.commit() + data = GlobalTasks.query.filter( + GlobalTasks.id == cur_id, + or_(GlobalTasks.task_userid_open == current_user.id, + GlobalTasks.task_assignee_id == current_user.id), + ).first() + if not data: + return response_error("Invalid global task ID") + + db.session.delete(data) + db.session.commit()For the duplication, consider a small helper next to the model/data-access layer, e.g.:
def _user_can_access_gtask_filter(): return or_( GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/blueprints/rest/dashboard_routes.py` around lines 288 - 293, The delete path duplicates the ownership filter and re-queries instead of deleting the already loaded instance; update gtask_delete to reuse the previously fetched data (variable data) when deleting rather than issuing a second filtered delete, and extract the repeated or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id) into a small helper (e.g., _user_can_access_gtask_filter) or a query-scoping function so both the initial lookup (GlobalTasks.query.filter(...).first()) and any future checks use the same filter helper to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/app/blueprints/rest/dashboard_routes.py`:
- Line 245: Import sqlalchemy.or_ into the module and apply ownership filtering
wherever tasks are fetched: update the imports to include or_ and change queries
that fetch GlobalTasks (e.g., the one in GlobalTasks.query.filter(...) used in
the update and delete flows) to use or_(GlobalTasks.task_userid_open ==
current_user.id, GlobalTasks.task_assignee_id == current_user.id). Also fix the
view_gtask/get_global_task flow by either adding the same ownership filters
inside get_global_task or by applying the ownership filter in view_gtask before
returning the task (ensure get_global_task(task_id=cur_id) cannot return tasks
the current_user does not own), and simplify delete to use that filtered query
instead of re-querying redundantly.
---
Outside diff comments:
In `@source/app/blueprints/rest/dashboard_routes.py`:
- Around line 157-164: view_gtask currently returns any global task by id
causing an IDOR; enforce the same owner/assignee restriction used by edit_gtask
and gtask_delete by filtering so only tasks where GlobalTasks.task_userid_open
== current_user.id OR GlobalTasks.task_assignee_id == current_user.id are
returned. Fix by updating view_gtask to call get_global_task with that filter
(or add the filter logic into get_global_task itself) so the GET
/global/tasks/<cur_id> only returns the task when the current_user is owner or
assignee.
---
Nitpick comments:
In `@source/app/blueprints/rest/dashboard_routes.py`:
- Around line 288-293: The delete path duplicates the ownership filter and
re-queries instead of deleting the already loaded instance; update gtask_delete
to reuse the previously fetched data (variable data) when deleting rather than
issuing a second filtered delete, and extract the repeated
or_(GlobalTasks.task_userid_open == current_user.id,
GlobalTasks.task_assignee_id == current_user.id) into a small helper (e.g.,
_user_can_access_gtask_filter) or a query-scoping function so both the initial
lookup (GlobalTasks.query.filter(...).first()) and any future checks use the
same filter helper to avoid drift.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13f794b5-9c6c-4389-81ba-6ffce94f8607
📒 Files selected for processing (1)
source/app/blueprints/rest/dashboard_routes.py
| def edit_gtask(cur_id, caseid): | ||
| form = CaseGlobalTaskForm() | ||
| task = GlobalTasks.query.filter(GlobalTasks.id == cur_id).first() | ||
| task = GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).first() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm `or_` is not imported in the file and that the new code references it.
rg -nP '\bor_\b' source/app/blueprints/rest/dashboard_routes.py
rg -nP '^\s*(from|import)\b.*\bor_\b' source/app/blueprints/rest/dashboard_routes.pyRepository: dfir-iris/iris-web
Length of output: 607
🏁 Script executed:
# Get the view_gtask function and surrounding context (lines 157-164)
sed -n '155,170p' source/app/blueprints/rest/dashboard_routes.py
# Get the gtask_delete function and surrounding context (lines 280-295)
sed -n '278,296p' source/app/blueprints/rest/dashboard_routes.py
# Also check imports at the top of the file to confirm no or_ import
sed -n '1,60p' source/app/blueprints/rest/dashboard_routes.pyRepository: dfir-iris/iris-web
Length of output: 3926
🏁 Script executed:
# Find and inspect the get_global_task function to see if it includes ownership filters
fd -n 'dashboard_db.py' source/Repository: dfir-iris/iris-web
Length of output: 292
🏁 Script executed:
# Find dashboard_db.py
fd dashboard_db.py source/
# Then inspect the get_global_task functionRepository: dfir-iris/iris-web
Length of output: 107
🏁 Script executed:
# Search for get_global_task function definition and its implementation
rg -A 10 'def get_global_task' source/app/datamgmt/dashboard/dashboard_db.pyRepository: dfir-iris/iris-web
Length of output: 456
🏁 Script executed:
# Get more context of get_global_task function
rg -A 25 'def get_global_task' source/app/datamgmt/dashboard/dashboard_db.pyRepository: dfir-iris/iris-web
Length of output: 739
Critical: or_ is not imported — endpoints will raise NameError at runtime.
The new ownership filters at lines 245, 288, and 292 all reference or_, but sqlalchemy.or_ is not imported anywhere in this module. As soon as a user calls POST /global/tasks/update/<id> or POST /global/tasks/delete/<id>, Flask will raise NameError: name 'or_' is not defined and return a 500 — so the IDOR fix not only doesn't land, it also breaks legitimate edit/delete flows.
Additionally, the view_gtask endpoint (lines 157-164) is still vulnerable to IDOR. It calls get_global_task(task_id=cur_id), which only filters by task ID with no ownership check. Any user can retrieve details of any global task regardless of whether they created it or are assigned to it.
🛠️ Proposed fixes
Add missing import:
import marshmallow
from datetime import datetime
from datetime import timedelta
from oic.oauth2.exception import GrantError
+from sqlalchemy import or_
+
from flask import BlueprintApply ownership filter in view_gtask:
def view_gtask(cur_id):
- task = get_global_task(task_id=cur_id)
+ task = GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).first()
if not task:
return response_error(f'Global task ID {cur_id} not found')Minor refactor — avoid redundant query in delete:
data = GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).first()
if not data:
return response_error("Invalid global task ID")
- GlobalTasks.query.filter(GlobalTasks.id == cur_id).filter(or_(GlobalTasks.task_userid_open == current_user.id, GlobalTasks.task_assignee_id == current_user.id)).delete()
+ db.session.delete(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/app/blueprints/rest/dashboard_routes.py` at line 245, Import
sqlalchemy.or_ into the module and apply ownership filtering wherever tasks are
fetched: update the imports to include or_ and change queries that fetch
GlobalTasks (e.g., the one in GlobalTasks.query.filter(...) used in the update
and delete flows) to use or_(GlobalTasks.task_userid_open == current_user.id,
GlobalTasks.task_assignee_id == current_user.id). Also fix the
view_gtask/get_global_task flow by either adding the same ownership filters
inside get_global_task or by applying the ownership filter in view_gtask before
returning the task (ensure get_global_task(task_id=cur_id) cannot return tasks
the current_user does not own), and simplify delete to use that filtered query
instead of re-querying redundantly.
|
Updated the PR to include missing imports and addressed the IDOR in the view endpoint as flagged by the review. Ready for re-audit. |
2 participants
Ensures that global tasks can only be viewed, updated, or deleted by the task owner or assignee. Linked to issue #1034.
Summary by CodeRabbit