[WEB-8017] fix(security): sanitize order_by on external REST API list endpoints#9348
Open
mguptahub wants to merge 1 commit into
Open
[WEB-8017] fix(security): sanitize order_by on external REST API list endpoints#9348mguptahub wants to merge 1 commit into
mguptahub wants to merge 1 commit 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>
Contributor
📝 WalkthroughWalkthroughThis PR sanitizes the ChangesOrder-by injection sanitization
Estimated code review effort: 2 (Simple) | ~15 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
This PR closes a security gap where two external REST API list endpoints accepted a raw order_by query parameter and passed it directly to Django’s .order_by(), enabling relational-path traversal attempts (ordering oracle) and unhandled FieldError → HTTP 500. The fix routes both endpoints through the existing sanitize_order_by() helper with the appropriate allowlists, and adds unit + contract regressions to prevent recurrence.
Changes:
- Sanitize
order_byin the external project list endpoint usingPROJECT_ORDER_BY_ALLOWLIST. - Sanitize
order_byin the external issue list endpoint usingISSUE_ORDER_BY_ALLOWLISTbefore its ordering branch logic. - Add unit tests for sanitizer allowlist behavior and contract tests ensuring injected/invalid
order_byvalues return 200.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/api/views/project.py | Applies sanitize_order_by() to the external project list ordering. |
| apps/api/plane/api/views/issue.py | Applies sanitize_order_by() to the external issue list ordering before branch logic. |
| apps/api/plane/tests/unit/utils/test_order_by_sanitize.py | Adds unit regressions ensuring allowlists default unsafe/malformed values and preserve legitimate orderings. |
| apps/api/plane/tests/contract/api/test_projects.py | Adds contract regressions that invalid/relational order_by values don’t trigger 500s. |
| apps/api/plane/tests/contract/api/test_issues.py | Adds contract regressions for the issue list endpoint covering invalid/relational/legitimate order_by. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/plane/tests/contract/api/test_projects.py (1)
194-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-query after create.
Project.objects.create(...)return value is discarded, then re-fetched viaProject.objects.get(identifier="OP")on the next line forProjectMember.objects.create. Capture the created instance directly instead of issuing an extra query.♻️ Proposed fix
- Project.objects.create( + project = Project.objects.create( name="Ordered Project", identifier="OP", workspace=workspace, created_by=create_user, ) - ProjectMember.objects.create(project=Project.objects.get(identifier="OP"), member=create_user, role=20) + ProjectMember.objects.create(project=project, member=create_user, role=20)🤖 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/api/test_projects.py` around lines 194 - 200, The Project creation flow in test_projects.py does an unnecessary re-query right after Project.objects.create, then uses Project.objects.get(identifier="OP") for ProjectMember.objects.create. Capture the instance returned by Project.objects.create in the existing project setup and pass that object directly to ProjectMember.objects.create, avoiding the extra database hit.apps/api/plane/tests/unit/utils/test_order_by_sanitize.py (1)
99-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing coverage for
issue_module__module__name.
ISSUE_ORDER_BY_ALLOWLISTalso includesissue_module__module__name, which isn't exercised in this parametrize list. Not a correctness bug, but adding it would make the test fully mirror the allowlist contract.♻️ Suggested addition
"state__name", "state__group", "assignees__first_name", "labels__name", + "issue_module__module__name", ], )The unit tests validate that injection payloads fall back to endpoint defaults and that allowlisted (including descending variants) values pass; these allowlists are the contract inputs those tests use.
🤖 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/unit/utils/test_order_by_sanitize.py` around lines 99 - 114, Add test coverage for the missing allowlisted order-by field by including issue_module__module__name in the parametrize list in test_order_by_sanitize.py. Keep the existing sanitize/allowlist assertions in the same test so the Order By validation continues to exercise every value in ISSUE_ORDER_BY_ALLOWLIST, including this nested field.
🤖 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/api/test_projects.py`:
- Around line 194-200: The Project creation flow in test_projects.py does an
unnecessary re-query right after Project.objects.create, then uses
Project.objects.get(identifier="OP") for ProjectMember.objects.create. Capture
the instance returned by Project.objects.create in the existing project setup
and pass that object directly to ProjectMember.objects.create, avoiding the
extra database hit.
In `@apps/api/plane/tests/unit/utils/test_order_by_sanitize.py`:
- Around line 99-114: Add test coverage for the missing allowlisted order-by
field by including issue_module__module__name in the parametrize list in
test_order_by_sanitize.py. Keep the existing sanitize/allowlist assertions in
the same test so the Order By validation continues to exercise every value in
ISSUE_ORDER_BY_ALLOWLIST, including this nested field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54aac86d-1c60-4eab-82e2-01826b31eef2
📒 Files selected for processing (5)
apps/api/plane/api/views/issue.pyapps/api/plane/api/views/project.pyapps/api/plane/tests/contract/api/test_issues.pyapps/api/plane/tests/contract/api/test_projects.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
Closes a partial bypass of WEB-7813 (PR #9292, GHSA-2r95 / GHSA-w45q). Two external REST API list endpoints passed a raw
order_byquery parameter straight to Django's.order_by():GET /api/v1/workspaces/<slug>/projects/—ProjectListCreateAPIEndpoint.getGET /api/v1/workspaces/<slug>/projects/<project_id>/issues/—IssueListCreateAPIEndpoint.getBecause Django resolves
__-separated relational paths, an attacker with an API token could:created_by__password,created_by__token,created_by__email,workspace__owner__password) to build an ordering oracle.order_by=not_a_field) → unhandledFieldError→ HTTP 500.Advisory: GHSA-p885-6jpg-cr2p (medium).
Fix
Route both endpoints through the existing
sanitize_order_by()helper with the appropriate allowlist:PROJECT_ORDER_BY_ALLOWLIST(defaultsort_order) for the project listISSUE_ORDER_BY_ALLOWLIST(default-created_at) for the work-item listThis mirrors how
order_issue_queryset()already sanitizes at its top. Non-allowlisted values collapse to the safe default; every legitimate ordering (priority,state__group,labels__name,assignees__first_name,-created_at,name,network, …) is unchanged.Tests
tests/unit/utils/test_order_by_sanitize.py): the two allowlists neutralise the disclosed payloads (incl.created_by__password,--created_at,id; drop table) to the default, and preserve every legitimate value.tests/contract/api/test_projects.py,test_issues.py): both endpoints return 200 (not 500) for injected fields, and legit ordering still works.git stash— both endpoint tests return 500 without the fix. 52 passed with it.Verification
ruff checkcleanpython manage.py checkcleanSummary by CodeRabbit
order_byvalues more gracefully.