fix: prevent ORM field injection via segment parameter in analytics (GHSA-93x3-ghh7-72j3)#8864
Conversation
…GHSA-93x3-ghh7-72j3) Centralize analytics field allowlists into VALID_ANALYTICS_FIELDS and VALID_YAXIS constants in analytics_plot.py. Add defense-in-depth validation in build_graph_plot() and extract_axis() so no caller can pass arbitrary field references to Django F() expressions. Add missing segment validation to SavedAnalyticEndpoint. Also fixes ExportAnalytics using "estimate_point" instead of "estimate_point__value".
📝 WalkthroughWalkthroughThe changes consolidate analytics field validation by introducing centralized validation constants ( Changes
Sequence DiagramsequenceDiagram
actor Client
participant Endpoint as Analytics Endpoint
participant Utils as analytics_plot Utils
participant DB as Database
Client->>Endpoint: Request with x_axis, y_axis, segment
Endpoint->>Endpoint: Validate params against VALID_ANALYTICS_FIELDS<br/>and VALID_YAXIS
alt Invalid Parameters
Endpoint-->>Client: Return validation error
else Valid Parameters
Endpoint->>Utils: extract_axis(queryset, x_axis)
Utils->>Utils: Validate x_axis in VALID_ANALYTICS_FIELDS
alt Invalid x_axis
Utils-->>Endpoint: Raise ValueError
Endpoint-->>Client: Return error
else Valid x_axis
Utils->>DB: Extract axis data
DB-->>Utils: Return data
Endpoint->>Utils: build_graph_plot(..., y_axis, segment)
Utils->>Utils: Validate y_axis & segment
alt Invalid y_axis or segment
Utils-->>Endpoint: Raise ValueError
Endpoint-->>Client: Return error
else Valid
Utils->>Utils: Build plot data
Utils-->>Endpoint: Return graph data
Endpoint-->>Client: Return analytics response
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/analytic/base.py (1)
191-214:⚠️ Potential issue | 🟡 MinorSecurity fix for segment is correct, but stored x_axis/y_axis values are not validated.
The segment validation at lines 208-212 correctly addresses the security vulnerability. However,
x_axisandy_axisare retrieved from storedquery_dict(lines 197-198) and only checked for truthiness (lines 200-204), not againstVALID_ANALYTICS_FIELDS/VALID_YAXIS.If legacy saved analytics contain field values that are no longer in the allowlist (or were never valid),
build_graph_plot()will raiseValueError, resulting in a 500 error instead of a 400.Proposed fix to validate stored x_axis/y_axis
x_axis = analytic_view.query_dict.get("x_axis", False) y_axis = analytic_view.query_dict.get("y_axis", False) - if not x_axis or not y_axis: + if not x_axis or not y_axis or x_axis not in VALID_ANALYTICS_FIELDS or y_axis not in VALID_YAXIS: return Response( - {"error": "x-axis and y-axis dimensions are required"}, + {"error": "x-axis and y-axis dimensions are required and the values should be valid"}, status=status.HTTP_400_BAD_REQUEST, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/analytic/base.py` around lines 191 - 214, The stored x_axis and y_axis from AnalyticView.query_dict are only checked for truthiness but not validated against allowed fields, causing build_graph_plot to raise ValueError for legacy/invalid saved values; update the get method to validate x_axis is in VALID_ANALYTICS_FIELDS and y_axis is in VALID_YAXIS (or the appropriate allowlist constants) before calling build_graph_plot, and if either is invalid return a Response with a 400 status and a clear error message; keep existing segment validation that uses VALID_ANALYTICS_FIELDS and ensure you reference AnalyticView.query_dict, the get method, and build_graph_plot when making the change.
🧹 Nitpick comments (2)
apps/api/plane/bgtasks/analytic_plot_export.py (2)
23-23: Unused imports:VALID_ANALYTICS_FIELDSandVALID_YAXISare not used in this file.Only
build_graph_plotis actually used in this file (line 359). The constants are imported but never referenced directly since the validation happens internally withinbuild_graph_plot().Proposed fix to remove unused imports
-from plane.utils.analytics_plot import build_graph_plot, VALID_ANALYTICS_FIELDS, VALID_YAXIS +from plane.utils.analytics_plot import build_graph_plot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/bgtasks/analytic_plot_export.py` at line 23, Remove the two unused imports VALID_ANALYTICS_FIELDS and VALID_YAXIS and keep only build_graph_plot imported from plane.utils.analytics_plot; locate the import line that currently reads from plane.utils.analytics_plot import build_graph_plot, VALID_ANALYTICS_FIELDS, VALID_YAXIS and delete the trailing constants so the module only imports build_graph_plot (validation is handled inside build_graph_plot).
349-406: Consider the impact ofValueErrorfrombuild_graph_plot()on task behavior.If
build_graph_plot()raises aValueErrorfor invalid inputs, the broadExceptionhandler (line 404) catches it and logs the error, but the task silently returns without sending any notification to the user. The user would have received a success message fromExportAnalyticsEndpoint.post()saying the export will be emailed, but no email ever arrives.Since inputs are already validated in
ExportAnalyticsEndpoint.post()before dispatching the task, this is unlikely to happen in normal operation. However, for defense-in-depth, you may want to send a failure notification email or consider more specific exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/plane/app/views/analytic/base.py`:
- Around line 191-214: The stored x_axis and y_axis from AnalyticView.query_dict
are only checked for truthiness but not validated against allowed fields,
causing build_graph_plot to raise ValueError for legacy/invalid saved values;
update the get method to validate x_axis is in VALID_ANALYTICS_FIELDS and y_axis
is in VALID_YAXIS (or the appropriate allowlist constants) before calling
build_graph_plot, and if either is invalid return a Response with a 400 status
and a clear error message; keep existing segment validation that uses
VALID_ANALYTICS_FIELDS and ensure you reference AnalyticView.query_dict, the get
method, and build_graph_plot when making the change.
---
Nitpick comments:
In `@apps/api/plane/bgtasks/analytic_plot_export.py`:
- Line 23: Remove the two unused imports VALID_ANALYTICS_FIELDS and VALID_YAXIS
and keep only build_graph_plot imported from plane.utils.analytics_plot; locate
the import line that currently reads from plane.utils.analytics_plot import
build_graph_plot, VALID_ANALYTICS_FIELDS, VALID_YAXIS and delete the trailing
constants so the module only imports build_graph_plot (validation is handled
inside build_graph_plot).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36b9b0dd-a2d1-45e7-bee1-8ac9c011e573
📒 Files selected for processing (3)
apps/api/plane/app/views/analytic/base.pyapps/api/plane/bgtasks/analytic_plot_export.pyapps/api/plane/utils/analytics_plot.py
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability in analytics plotting by preventing ORM field reference injection via unvalidated axis/segment inputs, and centralizes axis allowlists to a single source of truth.
Changes:
- Introduces centralized allowlists (
VALID_ANALYTICS_FIELDS,VALID_YAXIS) and uses them across analytics endpoints. - Adds defense-in-depth validation in
extract_axis()/build_graph_plot()to block unsafe field references. - Updates analytics endpoints to use the centralized allowlists and adds missing
segmentvalidation for saved analytics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/analytics_plot.py | Adds centralized allowlists and validates x/y/segment inputs to prevent unsafe F() usage. |
| apps/api/plane/bgtasks/analytic_plot_export.py | Updates imports to include centralized allowlists (currently unused). |
| apps/api/plane/app/views/analytic/base.py | Replaces inline allowlists with centralized constants and adds segment validation for saved analytics. |
| segment = request.GET.get("segment", False) | ||
|
|
||
| if segment and (segment not in VALID_ANALYTICS_FIELDS or x_axis == segment): | ||
| return Response( | ||
| {"error": "Both segment and x axis cannot be same and segment should be valid"}, |
There was a problem hiding this comment.
build_graph_plot() now raises ValueError for invalid x_axis/y_axis, but this endpoint takes those values from analytic_view.query_dict (not the request) and doesn’t validate/catch errors before calling build_graph_plot(). A malformed/legacy/malicious saved analytic could now produce a 500. Consider validating x_axis/y_axis against the allowlists here and/or catching ValueError and returning a 400 response.
| # Check for x-axis and y-axis as thery are required parameters | ||
| if not x_axis or not y_axis or x_axis not in valid_xaxis_segment or y_axis not in valid_yaxis: | ||
| if not x_axis or not y_axis or x_axis not in VALID_ANALYTICS_FIELDS or y_axis not in VALID_YAXIS: | ||
| return Response( | ||
| {"error": "x-axis and y-axis dimensions are required and the values should be valid"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
PR description says ExportAnalyticsEndpoint allowlist was fixed to use estimate_point instead of estimate_point__value, but VALID_ANALYTICS_FIELDS (used here for validation) includes estimate_point__value (and not estimate_point). Please reconcile the PR description/test plan with the actual intended accepted field name to avoid confusion for reviewers and release notes.
…ery params Remove unused VALID_ANALYTICS_FIELDS and VALID_YAXIS imports from analytic_plot_export.py. Add x_axis/y_axis allowlist validation in SavedAnalyticEndpoint for stored query_dict values to prevent 500 errors from malformed saved analytics.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/app/views/analytic/base.py (1)
45-52: Extract duplicated axis/segment validation into one helper.The same validation and error payload shape is repeated in three endpoints, which increases maintenance drift risk.
♻️ Proposed refactor
+def _validate_analytics_dimensions(x_axis, y_axis, segment): + if ( + not x_axis + or not y_axis + or x_axis not in VALID_ANALYTICS_FIELDS + or y_axis not in VALID_YAXIS + ): + return Response( + {"error": "x-axis and y-axis dimensions are required and the values should be valid"}, + status=status.HTTP_400_BAD_REQUEST, + ) + + if segment and (segment not in VALID_ANALYTICS_FIELDS or x_axis == segment): + return Response( + {"error": "Both segment and x axis cannot be same and segment should be valid"}, + status=status.HTTP_400_BAD_REQUEST, + ) + return None + class AnalyticsEndpoint(BaseAPIView): @@ - if not x_axis or not y_axis or x_axis not in VALID_ANALYTICS_FIELDS or y_axis not in VALID_YAXIS: - return Response( - {"error": "x-axis and y-axis dimensions are required and the values should be valid"}, - status=status.HTTP_400_BAD_REQUEST, - ) - - # If segment is present it cannot be same as x-axis - if segment and (segment not in VALID_ANALYTICS_FIELDS or x_axis == segment): - return Response( - {"error": "Both segment and x axis cannot be same and segment should be valid"}, - status=status.HTTP_400_BAD_REQUEST, - ) + validation_error = _validate_analytics_dimensions(x_axis, y_axis, segment) + if validation_error: + return validation_errorApply the same helper in
SavedAnalyticEndpoint.getandExportAnalyticsEndpoint.post.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/analytic/base.py` around lines 45 - 52, Extract the duplicated axis/segment validation into a single helper (e.g., validate_axes_and_segment) in apps/api/plane/app/views/analytic/base.py that accepts x_axis, y_axis, segment and returns either None for success or the Response instance (same payload and status) on error; replace the inline checks currently using VALID_ANALYTICS_FIELDS and VALID_YAXIS in the current view and update SavedAnalyticEndpoint.get and ExportAnalyticsEndpoint.post to call this helper and early-return the Response when it indicates an error, preserving the existing error message and HTTP_400_BAD_REQUEST status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/plane/app/views/analytic/base.py`:
- Around line 45-52: Extract the duplicated axis/segment validation into a
single helper (e.g., validate_axes_and_segment) in
apps/api/plane/app/views/analytic/base.py that accepts x_axis, y_axis, segment
and returns either None for success or the Response instance (same payload and
status) on error; replace the inline checks currently using
VALID_ANALYTICS_FIELDS and VALID_YAXIS in the current view and update
SavedAnalyticEndpoint.get and ExportAnalyticsEndpoint.post to call this helper
and early-return the Response when it indicates an error, preserving the
existing error message and HTTP_400_BAD_REQUEST status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c52d97a5-3308-4969-95aa-c5db5f7dd6ac
📒 Files selected for processing (1)
apps/api/plane/app/views/analytic/base.py
Summary
SavedAnalyticEndpointwhere thesegmentquery parameter was passed directly to Django'sF()expression without validation, allowing authenticated workspace members to extract arbitrary related model fields (e.g. password hashes, emails, tokens)VALID_ANALYTICS_FIELDS,VALID_YAXIS) intoanalytics_plot.pyas single source of truth, replacing duplicate inline lists across 3 endpointsbuild_graph_plot()andextract_axis()so no current or future caller can pass unsafe field referencesExportAnalyticsEndpointallowlist usingestimate_pointinstead ofestimate_point__valueSecurity Advisory
https://github.com/makeplane/plane/security/advisories/GHSA-93x3-ghh7-72j3
Test plan
AnalyticsEndpointaccepts valid segment values and rejects invalid onesSavedAnalyticEndpointnow rejects arbitrary segment values likeworkspace__owner__passwordExportAnalyticsEndpointacceptsestimate_point__valueas a valid fieldbuild_graph_plot()raisesValueErrorfor invalid x_axis/y_axis/segment inputsSummary by CodeRabbit