[WEB-7813] fix: prevent ORM order_by injection in issue and other endpoints#9292
[WEB-7813] fix: prevent ORM order_by injection in issue and other endpoints#9292mguptahub wants to merge 2 commits into
Conversation
…HSA-2r95, GHSA-w45q) Add field-name allowlists and a sanitize_order_by() utility in order_queryset.py. All allowlists are centralised there; each call site imports the named constant so there are no inline sets scattered across view files. - order_queryset.py: ISSUE_ORDER_BY_ALLOWLIST, INTAKE_ISSUE_ORDER_BY_ALLOWLIST, ACTIVITY_ORDER_BY_ALLOWLIST, PROJECT_ORDER_BY_ALLOWLIST, VIEW_ORDER_BY_ALLOWLIST, NOTIFICATION_ORDER_BY_ALLOWLIST + sanitize_order_by() utility; validation added at the top of order_issue_queryset() — fixes all callers including the unauthenticated ProjectIssuesPublicEndpoint (GHSA-w45q) - api/views/cycle.py, api/views/module.py: cycle/module issue list endpoints - api/views/issue.py: IssueActivity list and detail endpoints - app/views/intake/base.py: IntakeIssue list - app/views/view/base.py: saved-view list - app/views/notification/base.py: notification paginator - app/views/project/base.py: project list paginator - app/views/user/base.py, app/views/workspace/user.py: activity paginators Closes WEB-7813 Co-authored-by: Plane AI <noreply@plane.so>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesORM order_by Injection Prevention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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 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.
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/api/views/cycle.py`:
- Line 858: The sanitize_order_by function in
apps/api/plane/utils/order_queryset.py uses lstrip("-") which removes all
leading dashes, allowing malformed inputs like "--created_at" to pass allowlist
validation. Fix this by updating the normalization logic to strip only a single
leading dash (for descending order) before validating against
ISSUE_ORDER_BY_ALLOWLIST, then append it back if present in the original input.
This ensures only properly formatted sort tokens with at most one leading dash
are accepted.
In `@apps/api/plane/api/views/module.py`:
- Line 601: The sanitize_order_by function uses lstrip("-") which strips all
leading dashes, allowing malformed inputs like "--created_at" to pass through
unchanged when the bare field is in the allowlist. These invalid tokens then
cause FieldError exceptions at runtime instead of safely falling back to the
default value. Update the sanitize_order_by function in
apps/api/plane/utils/order_queryset.py to properly validate the input so that it
rejects fields with multiple leading dashes or other malformed patterns,
returning the default value for any input that doesn't match the exact expected
format (a single optional dash followed by a valid field name from the
allowlist).
🪄 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: e3c8bcee-66d0-452a-aa04-c22ab8b9c9f8
📒 Files selected for processing (10)
apps/api/plane/api/views/cycle.pyapps/api/plane/api/views/issue.pyapps/api/plane/api/views/module.pyapps/api/plane/app/views/intake/base.pyapps/api/plane/app/views/notification/base.pyapps/api/plane/app/views/project/base.pyapps/api/plane/app/views/user/base.pyapps/api/plane/app/views/view/base.pyapps/api/plane/app/views/workspace/user.pyapps/api/plane/utils/order_queryset.py
lstrip("-") stripped all leading dashes, allowing "--created_at" to
pass the allowlist check unchanged and reach .order_by() as a malformed
token (causing FieldError). Now strips only one leading dash; any
remaining dash prefix is rejected to the safe default.
Co-authored-by: Plane AI <noreply@plane.so>
Summary
order_byquery param was passed directly to.order_by()/ paginator without validation, allowing field-name injection (e.g. traversals likeworkspace__secret_keyfor timing-based data inference)ProjectIssuesPublicEndpointin SpacesApproach
All allowlists are centralised in
plane/utils/order_queryset.pyas namedfrozensetconstants. Each call site imports the constant by name — no inline sets in view files.sanitize_order_by(value, allowed_fields, default)strips the leading-before looking up the bare field name; unrecognised values fall back to the safe default silently.Files changed
utils/order_queryset.pysanitize_order_by()+ guard at top oforder_issue_queryset()api/views/cycle.pyapi/views/module.pyapi/views/issue.pyapp/views/intake/base.pyapp/views/view/base.pyapp/views/notification/base.pyapp/views/project/base.pyapp/views/user/base.pyapp/views/workspace/user.pyTest plan
order_by=sequence_idsorts correctlyorder_by=workspace__secret_keysilently falls back to-created_atorder_byreturns-created_atorderingCo-authored-by: Plane AI noreply@plane.so
Summary by CodeRabbit
order_byfields and directions, improving stability and preventing unsafe ordering inputs.