[WEB-8110] fix: sanitize page list order_by against an allowlist - #9387
[WEB-8110] fix: sanitize page list order_by against an allowlist#9387mguptahub wants to merge 4 commits into
Conversation
…A-2v48) PageViewSet.get_queryset passed the raw order_by query param into .order_by(). In Django 4.2 .order_by() resolves field names at call time, so an unknown field (e.g. order_by=password) raises FieldError → 500 DoS, and a valid relation path (e.g. order_by=owned_by__password) enables ORM relational traversal (GHSA-2v48-qcjw-74ch). Add PAGE_ORDER_BY_ALLOWLIST to utils/order_queryset.py and wrap the param with the existing sanitize_order_by() before it reaches .order_by(), matching the issue/project/view/notification endpoints. Unknown or malformed values fall back to the safe -created_at default. Covers only the app project-pages residual; the 3 external-REST-API sites in the advisory are handled by PR #9348. EE Wiki counterpart: WEB-8111. Add contract regression tests (fail-before verified). Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPage list ordering now uses an explicit allowlist and sanitization before applying query parameters. Contract tests cover malicious, allowlisted, descending, omitted, and effective ChangesPage order_by hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security hardening gap in the app “project pages” list endpoint by adding an allowlisted sanitizer for the order_by query parameter before it reaches Django ORM .order_by(), preventing invalid-field FieldError (500) and relational-path traversal attempts.
Changes:
- Introduces
PAGE_ORDER_BY_ALLOWLISTalongside existing order-by allowlists inutils/order_queryset.py. - Applies
sanitize_order_by()to theorder_byquery param inPageViewSet.get_queryset. - Adds contract regression tests covering malicious and allowlisted
order_byvalues for the pages endpoint.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/order_queryset.py | Adds PAGE_ORDER_BY_ALLOWLIST to support safe page list ordering sanitization. |
| apps/api/plane/app/views/page/base.py | Sanitizes the incoming order_by param before passing it to Django .order_by(). |
| apps/api/plane/tests/contract/app/test_page_order_by_allowlist_app.py | Adds contract regression coverage for injected and allowlisted order_by inputs. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/views/page/base.py`:
- Around line 104-115: Merge the queryset’s duplicate .order_by() calls into one
call, preserving "-is_favorite" as the primary sort and using
sanitize_order_by(self.request.GET.get("order_by", "-created_at"),
PAGE_ORDER_BY_ALLOWLIST, default="-created_at") as the user-controlled fallback;
remove the later overriding .order_by("-is_favorite", "-created_at") call.
In `@apps/api/plane/tests/contract/app/test_page_order_by_allowlist_app.py`:
- Around line 35-69: Add direct unit coverage for sanitize_order_by using
PAGE_ORDER_BY_ALLOWLIST, including valid ascending/descending fields, unknown
and relation paths, double-dash, empty/None, and whitespace inputs with expected
default fallback. Strengthen TestPageOrderByAllowlist integration coverage by
using a fixture that creates multiple distinctly named pages and asserting
response ordering for ascending and descending order_by values, rather than
checking only status_code.
🪄 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
Run ID: 5b5bf425-96c5-4b16-bb63-561bfb68618e
📒 Files selected for processing (3)
apps/api/plane/app/views/page/base.pyapps/api/plane/tests/contract/app/test_page_order_by_allowlist_app.pyapps/api/plane/utils/order_queryset.py
… (review) Address CodeRabbit + Copilot on #9387: the sanitized .order_by(user) was immediately overridden by a later .order_by("-is_favorite", "-created_at"), so the order_by param had no effect on the result (dead code) and cost an extra query-build step. Merge them into one .order_by("-is_favorite", <sanitized>, "id") — matching the EE project-pages viewset — so favourites stay pinned first, the allowlisted user ordering actually applies as the secondary sort, and id is a stable pagination tiebreak. The no-param default is unchanged (-created_at). Add a test asserting order_by=name / -name actually reorders the results. Co-authored-by: Plane AI <noreply@plane.so>
sriramveeraghanta
left a comment
There was a problem hiding this comment.
LGTM. The fix is correct, minimal, and follows the same sanitize_order_by + allowlist pattern as the other endpoints. I verified it end-to-end rather than just reading the diff:
Verification (docker-compose-test stack, this PR's head)
- All 11 new contract tests pass.
- Fail-before confirmed against
origin/preview:order_by=password→ 500,order_by=bogus__field__x→ 500,order_by=owned_by__password→ 200. ruff checkclean on the changed files.
One behavior change worth calling out explicitly
Pre-fix, the order_by param was actually dead: the first .order_by(request.GET.get(...)) was fully replaced by the later .order_by("-is_favorite", "-created_at") (Django's order_by() clears prior ordering). So the user param never reached SQL — it only got validated at call time, which is why bad fields 500'd. I confirmed empirically that ?order_by=name on preview returns pages in -created_at order. Two consequences:
- This PR is not just hardening — it makes
order_byfunctional for the first time (as secondary sort behind-is_favorite, plus a newidtiebreak). That's the right behavior, and the default ordering is unchanged, but it's worth a line in the description since clients may observe the new sorting. - The pre-fix "ORM relational traversal" impact at this specific site was latent — the traversal ordering never made it into the final query. The live impact was the FieldError 500 DoS. Doesn't change the fix, just the severity narrative.
Follow-up needed (out of scope here): same class, still live in CE app API
GET /api/workspaces/<slug>/export-issues/ — apps/api/plane/app/views/exporter/base.py:75 passes request.GET.get("order_by") straight into self.paginate(...), and OffsetPaginator feeds it into F(*key) in .order_by() with no sanitization. Verified against preview: order_by=bogus_field → 500, order_by=initiated_by__password → 200 (ExporterHistory has the initiated_by FK to User, so the traversal resolves). Needs an EXPORTER_HISTORY_ORDER_BY_ALLOWLIST (or central sanitization in the paginator). Could you file a work item for it?
CE/EE divergence to be aware of
plane-ee preview already fixes this same viewset differently: _safe_order_by() with ALLOWED_ORDER_BY = {±created_at, ±updated_at, ±name} — no sort_order, and a case-insensitive name_lower sort via _page_order_expression. This PR allows sort_order and sorts name case-sensitively. Since that file is shared lineage, the next CE→EE merge will conflict on exactly these lines and the two allowlists will disagree. Worth aligning the allowed sets (or deciding EE adopts sanitize_order_by) before this lands, or at least flagging it for whoever does that merge.
Nit (non-blocking): ruff format --check wants to reformat the new test file and order_queryset.py (collapses wrapped calls that fit in 120 cols). CI only runs ruff check, which is green, so purely optional.
…r-by-allowlist # Conflicts: # apps/api/plane/utils/order_queryset.py
|
Rebased on latest Re the earlier review points (@coderabbitai / Copilot) on the double |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Explanations kept unchanged; only the IDs are removed. Co-authored-by: Plane AI <noreply@plane.so>
|
React Doctor found 4 issues in 4 files · 1 error & 3 warnings · score 82 / 100 (Needs work) · vs Errors
3 warnings
|
Fixes the app project-pages residual of WEB-8110 (HIGH).
Problem
PageViewSet.get_querysetpassed the raworder_byquery param straight into.order_by(self.request.GET.get("order_by", "-created_at")). In Django 4.2,.order_by(<field>)resolves field names at call time, so:order_by=password) raisesFieldError→ 500 DoS;order_by=owned_by__password) performs ORM relational traversal.Verified empirically against
origin/preview(500 onpassword/bogus__field; 200 traversal onowned_by__password).Fix
Add
PAGE_ORDER_BY_ALLOWLISTtoutils/order_queryset.pyand wrap the param with the existingsanitize_order_by()before it reaches.order_by()— the same pattern already used by the issue/project/view/notification endpoints. Unknown or malformed values fall back to the safe-created_atdefault.Scope
This covers only the app project-pages residual. The 3 external-REST-API sites in the advisory are handled by #9348. EE Wiki counterpart: WEB-8111 (plane-ee). EE project pages are already protected via
_safe_order_by.Tests
tests/contract/app/test_page_order_by_allowlist_app.py— 10 tests (invalid field / relation-path rejected → 200, allowlisted accepted, default). Fail-before verified (invalid fields return 500 before the fix).ruff+manage.py checkgreen.Summary by CodeRabbit
order_byquery parameter against an allowlist.order_byvalues actually change result ordering (ascending and descending).