[WEB-8012] fix: prevent ORM group_by/sub_group_by injection in issue endpoints#9347
Open
mguptahub wants to merge 1 commit into
Open
[WEB-8012] fix: prevent ORM group_by/sub_group_by injection in issue endpoints#9347mguptahub wants to merge 1 commit into
mguptahub wants to merge 1 commit into
Conversation
…endpoints Add ISSUE_GROUP_BY_ALLOWLIST and validate group_by_field_name/ sub_group_by_field_name in BasePaginator.paginate() — the single chokepoint all GroupedOffsetPaginator/SubGroupedOffsetPaginator callers funnel through (the unauthenticated public deploy-board endpoint plus 5 GUEST-reachable authenticated endpoints). Invalid fields now raise ParseError (HTTP 400) instead of reaching F()/.values()/.order_by()/Window partition_by as a raw ORM field name, which previously let an anonymous caller crash the endpoint or force a blind relational-traversal oracle (GHSA-wwgj-929g-42cm). Same field-name-injection class as the order_by fix (GHSA-2r95/GHSA-w45q, WEB-7813), which never extended to group_by/sub_group_by. Closes WEB-8012 Co-authored-by: Plane AI <noreply@plane.so>
Contributor
📝 WalkthroughWalkthroughAdds an ISSUE_GROUP_BY_ALLOWLIST frozenset constant and enforces it in BasePaginator.paginate for group_by_field_name and sub_group_by_field_name, raising ParseError on invalid values. Adds unit tests covering the allowlist, sanitize_order_by, and paginator validation behavior. ChangesGroup-by allowlist enforcement
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BasePaginator
participant ISSUE_GROUP_BY_ALLOWLIST
Client->>BasePaginator: paginate(group_by_field_name, sub_group_by_field_name)
BasePaginator->>ISSUE_GROUP_BY_ALLOWLIST: check group_by_field_name membership
ISSUE_GROUP_BY_ALLOWLIST-->>BasePaginator: valid/invalid
alt invalid
BasePaginator-->>Client: raise ParseError
else valid
BasePaginator->>ISSUE_GROUP_BY_ALLOWLIST: check sub_group_by_field_name membership
ISSUE_GROUP_BY_ALLOWLIST-->>BasePaginator: valid/invalid
BasePaginator-->>Client: paginated response
end
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 |
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
Contributor
There was a problem hiding this comment.
Pull request overview
Closes an ORM field-name injection vector in issue listing endpoints by validating group_by/sub_group_by before those values can reach Django ORM APIs (F(), .values(), .order_by(), Window(partition_by=...)) through the grouped paginator implementations.
Changes:
- Added an explicit
ISSUE_GROUP_BY_ALLOWLISTand enforced it inBasePaginator.paginate()forgroup_byandsub_group_byinputs (returning HTTP 400 viaParseErroron invalid values). - Added unit tests to lock the allowlist contents and verify invalid grouped fields are rejected while non-grouped pagination remains unaffected.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/api/plane/utils/paginator.py | Validates group_by/sub_group_by against a central allowlist before constructing grouped paginator instances. |
| apps/api/plane/utils/order_queryset.py | Introduces ISSUE_GROUP_BY_ALLOWLIST alongside existing ordering allowlists to prevent ORM field-name injection. |
| apps/api/plane/tests/unit/utils/test_paginator.py | Adds regression tests ensuring invalid grouped fields raise ParseError and do not instantiate the paginator. |
| apps/api/plane/tests/unit/utils/test_order_queryset.py | Adds tests asserting allowlist contents and basic rejection of injection-style values; confirms sanitize_order_by behavior unchanged. |
![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/unit/utils/test_order_queryset.py (1)
21-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest hardcodes the expected set instead of validating against the actual safe-fields source.
test_allowlist_matches_known_safe_group_fieldsduplicates the allowlist contents as a literal set. IfISSUE_GROUP_BY_ALLOWLISTandissue_group_values()ingrouper.pydrift apart in the future, this test won't catch it since it only compares the constant against itself (restated), not against the actual grouping fields used by the paginator's data source.Consider deriving
expectedfrom the grouper module's known safe fields (or adding a companion test that cross-checksISSUE_GROUP_BY_ALLOWLISTagainstissue_group_values()output) to keep the allowlist and paginator in sync automatically.🤖 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_queryset.py` around lines 21 - 35, The test `test_allowlist_matches_known_safe_group_fields` is duplicating the safe group-by values as a hardcoded set, so it won’t detect drift between `ISSUE_GROUP_BY_ALLOWLIST` and the real source of truth. Update this test to derive the expected values from the grouper module, ideally by comparing `ISSUE_GROUP_BY_ALLOWLIST` against the output of `issue_group_values()` in `grouper.py` (or another shared safe-fields source) so the allowlist and paginator stay aligned. Keep the assertion anchored on the `ISSUE_GROUP_BY_ALLOWLIST` constant and the `issue_group_values` helper rather than a literal copy of the values.apps/api/plane/utils/paginator.py (1)
690-699: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe the two allowlist checks.
The
not in ISSUE_GROUP_BY_ALLOWLISTcheck is repeated forgroup_by_field_nameandsub_group_by_field_name. Could extract into a tiny helper for readability, though the duplication is minimal.♻️ Optional dedup
+def _validate_group_by_field(field_name, kind): + if field_name not in ISSUE_GROUP_BY_ALLOWLIST: + raise ParseError(detail=f"Invalid {kind} field: {field_name}") + ... if group_by_field_name: - if group_by_field_name not in ISSUE_GROUP_BY_ALLOWLIST: - raise ParseError(detail=f"Invalid group_by field: {group_by_field_name}") + _validate_group_by_field(group_by_field_name, "group_by") paginator_kwargs["group_by_field_name"] = group_by_field_name paginator_kwargs["group_by_fields"] = group_by_fields paginator_kwargs["count_filter"] = count_filter if sub_group_by_field_name: - if sub_group_by_field_name not in ISSUE_GROUP_BY_ALLOWLIST: - raise ParseError(detail=f"Invalid sub_group_by field: {sub_group_by_field_name}") + _validate_group_by_field(sub_group_by_field_name, "sub_group_by")🤖 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/utils/paginator.py` around lines 690 - 699, The allowlist validation for ISSUE_GROUP_BY_ALLOWLIST is duplicated in the paginator logic handling group_by_field_name and sub_group_by_field_name. Refactor the checks in the paginator flow (near the group_by/sub_group_by ParseError handling) into a small shared helper or reusable validation function so both fields use the same validation path while preserving the existing ParseError messages.
🤖 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/unit/utils/test_order_queryset.py`:
- Around line 21-35: The test `test_allowlist_matches_known_safe_group_fields`
is duplicating the safe group-by values as a hardcoded set, so it won’t detect
drift between `ISSUE_GROUP_BY_ALLOWLIST` and the real source of truth. Update
this test to derive the expected values from the grouper module, ideally by
comparing `ISSUE_GROUP_BY_ALLOWLIST` against the output of
`issue_group_values()` in `grouper.py` (or another shared safe-fields source) so
the allowlist and paginator stay aligned. Keep the assertion anchored on the
`ISSUE_GROUP_BY_ALLOWLIST` constant and the `issue_group_values` helper rather
than a literal copy of the values.
In `@apps/api/plane/utils/paginator.py`:
- Around line 690-699: The allowlist validation for ISSUE_GROUP_BY_ALLOWLIST is
duplicated in the paginator logic handling group_by_field_name and
sub_group_by_field_name. Refactor the checks in the paginator flow (near the
group_by/sub_group_by ParseError handling) into a small shared helper or
reusable validation function so both fields use the same validation path while
preserving the existing ParseError messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e192acb0-86bf-4d6b-9213-38ab492636c7
📒 Files selected for processing (4)
apps/api/plane/tests/unit/utils/test_order_queryset.pyapps/api/plane/tests/unit/utils/test_paginator.pyapps/api/plane/utils/order_queryset.pyapps/api/plane/utils/paginator.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
ProjectIssuesPublicEndpoint,plane/space/views/issue.py) readsgroup_by/sub_group_byquery params raw and feeds them intoGroupedOffsetPaginator/SubGroupedOffsetPaginator, which use the value directly as an ORM field name inF(),.values(),.order_by(), and Windowpartition_by— an anonymous caller can crash the endpoint (HTTP 500 DoS) or force a blind relational-traversal oracle (e.g.sub_group_by=created_by__password).order_byfix (GHSA-2r95/GHSA-w45q, WEB-7813), which never extended togroup_by/sub_group_by. The identical unvalidated pattern also exists on 5 authenticated GUEST-reachable endpoints (app/views/issue/base.py,issue/archive.py,cycle/issue.py,module/issue.py,workspace/user.py) — all funnel through the same two paginator classes, so this fixes all 6 in one place.ISSUE_GROUP_BY_ALLOWLISTtoplane/utils/order_queryset.pyand validatesgroup_by_field_name/sub_group_by_field_nameinsideBasePaginator.paginate()— the single chokepoint all 6 call sites funnel through — raisingParseError(→ HTTP 400) on an invalid field, matching the existing idiom already used in that function for badper_page/cursor values.issue_group_values()in bothplane/utils/grouper.pyandplane/space/utils/grouper.py— the exact fields both already resolve safely.Test plan
apps/api/plane/tests/unit/utils/test_order_queryset.py— allowlist contents + rejection of injection-style valuesapps/api/plane/tests/unit/utils/test_paginator.py— invalidgroup_by/sub_group_byraiseParseError; valid values still pass through; plain (non-grouped) pagination unaffectedgit stashon thepaginator.pyfix only) and pass with the fixdocker compose -f docker-compose-test.yml run --rm api-tests pytest plane/tests/unit -m unitruff checkclean on all changed filespython manage.py check— no issueshttps://claude.ai/code/session_01PsfB3aMwtYHFWrDqJTr1aW
Summary by CodeRabbit
Bug Fixes
Tests