fix(plugin-chart-ag-grid-table): validate filter values/operators in state converter#40623
Conversation
Code Review Agent Run #2c72bdActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Hardens the AG Grid table's chart-state → ownState converter (stateConversion.ts) against SQL injection via unquoted filter values and operators, and adds the first unit tests for convertFilterModel.
Changes:
- Coerce number-filter values via
Number()and skip the clause when the result is not finite. - Restrict compound-filter join operators to
AND/OR(upper-cased), skipping the clause otherwise. - Use
Object.create(null)for the column-id → SQL clause map to prevent prototype-key collisions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts | Validates numeric filter values, allowlists join operators, and uses a null-prototype map for SQL clauses. |
| superset-frontend/plugins/plugin-chart-ag-grid-table/test/stateConversion.test.ts | New Jest tests covering numeric validation, operator allowlist, and prototype-safe key handling. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40623 +/- ##
==========================================
+ Coverage 63.45% 63.47% +0.02%
==========================================
Files 2662 2662
Lines 143254 143260 +6
Branches 32941 32943 +2
==========================================
+ Hits 90899 90941 +42
+ Misses 50763 50727 -36
Partials 1592 1592
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sha174n
added
the
merge-if-green
…state converter The chart-state converter builds SQL filter clauses that are interpolated without quoting. Harden three spots in stateConversion.ts: - Number filter values are coerced with Number() and the filter is skipped when the value is not finite (previously interpolated as-is). - Compound filter join operators are restricted to AND/OR (normalized to upper case); anything else skips the clause. - The column-id-keyed clause map is created with Object.create(null) so user-influenced column ids can't reach prototype keys. Adds the first test coverage for convertFilterModel (numeric validation, operator allowlist, null-prototype map). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/ag-grid-stateconversion-filter-validation
branch
from
eda7371 to
d844ed4
Compare
github-project-automation
Bot
moved this from Needs Review
to Approved and/or Merged
in Superset Review Help Wanted
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
stateConversion.tsis the chart-state → ownState converter for the AG Grid table (registered viaregisterChartStateConverter, used on dashboards/Explore/embedded). It builds SQL filter clauses that are interpolated without quoting, so the inputs need validation. This hardens three spots:Number()and the filter is skipped when the value is not finite (they were previously interpolated as-is, unlike the text branch which already escapes).AND/OR(normalized to upper case); any other value skips the clause (previouslyfilter.operatorwas interpolated raw into the join).Object.create(null)so user-influenced column ids can't reach prototype keys.This converter previously had no test coverage; this adds the first tests for
convertFilterModelcovering numeric validation, the operator allowlist, and the null-prototype map.TESTING INSTRUCTIONS
5/5 pass.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code