fix(embedded): block custom SQL injection in guest user chart payloads - #43111
fix(embedded): block custom SQL injection in guest user chart payloads#43111alexandrusoare wants to merge 13 commits into
Conversation
Code Review Agent Run #7c2c01Actionable Suggestions - 0Review 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43111 +/- ##
==========================================
- Coverage 66.73% 66.50% -0.24%
==========================================
Files 2876 2870 -6
Lines 164218 163612 -606
Branches 37890 37633 -257
==========================================
- Hits 109590 108808 -782
- Misses 52469 52638 +169
- Partials 2159 2166 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #788203Actionable Suggestions - 0Review 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 |
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). 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 |
Code Review Agent Run #2bd1d2Actionable Suggestions - 0Review 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 |
|
|
||
| # SQL predicates (extras.where/having, SQL adhoc filters) must match | ||
| # what was saved on the chart; injected custom SQL is rejected. | ||
| if _sql_filters_modified( |
There was a problem hiding this comment.
This guard is reachable only with a slice_id: omit it and QueryContextFactory leaves slice_ None (query_context_factory.py:67), query_context_modified takes the chartless branch, _native_filter_request_modified returns False with no native-filter marker, and arbitrary extras.where is accepted — and the explicit NATIVE_FILTER path never inspects extras either.
Code Review Agent Run #28bffaActionable Suggestions - 0Review 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 |
| # Chartless non-native-filter requests (drill-to-detail, drill-by, | ||
| # samples) must not carry SQL extras; there is no stored chart to | ||
| # validate them against. Only the empty-filter sentinel is allowed. | ||
| sentinel_only: set[str] = {_EMPTY_FILTER_SENTINEL} |
There was a problem hiding this comment.
The chartless branch now allows only the sentinel, which over-blocks two real guest paths:
- Native Select filters put their "Pre-filter available values" adhoc filters into
extras.where(filters/components/Select/buildQuery.ts→processFilters), so a dashboard whose native filter pre-filters with Custom SQL 403s. - Drill-to-detail is chartless too (
/datasource/samples→get_samplesbuilds the context withform_data={"dashboardId": ...}and callsraise_for_access), andgetDrillPayloadsends the chart'sextras.whereplus drillfilterswhosecolis the raw dimension — an adhoc dict whenever that dimension is custom SQL.
What should these validate against when there is no stored chart — the filter's own config, the dashboard's charts?
Nits while here: _native_filter_request_modified's docstring still says drill-to-detail/samples "are not treated as modified here" and that filter/extras are not constrained, both now untrue; _collect_allowed_sql calls flt.get on stored adhoc_filters without the isinstance(flt, dict) guard used further down; and sanitizeClause appends \n when an expression contains --, so (expr\n) won't match the stored raw expr on charts with no saved query_context.
There was a problem hiding this comment.
Removed the chartless guard — SQL extras validation now applies only to chart payloads (with a slice_id), which is the scope of this PR. Chartless paths (native-filter pre-filtering, drill-to-detail, samples) are left unchanged and remain protected by datasource-access checks in raise_for_access. Also fixed the nits: added isinstance(flt, dict) guard in _collect_allowed_sql, and strip trailing \n from split clauses to handle _sanitize_clause's comment-termination behavior.
Code Review Agent Run #681a3aActionable Suggestions - 0Review 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 |
| """ | ||
| extras = getattr(query, "extras", None) or {} | ||
| for param in ("where", "having"): | ||
| composed = extras.get(param, "") |
There was a problem hiding this comment.
QueryObject._sanitize_filters rewrites extras["where"/"having"] in place and get_payload_result caches the rewritten value into cache_values["queries"], so the GET /api/v1/chart/data/<cache_key> re-validation compares normalized SQL against the chart's raw stored sqlExpression — with GLOBAL_ASYNC_QUERIES on, a saved custom SQL filter containing -- is cached as (a > 0 /* x */) (was (a > 0 -- x\n)) and the guest's result fetch 403s.
There was a problem hiding this comment.
Checked — not an issue. The async cache stores the original form_data, not the sanitized QueryObject.extras. On cache fetch, the QueryContext is rebuilt from that original form_data, so our validation sees the pre-sanitization value.
| # tightening this would break legitimate chartless flows (native-filter | ||
| # pre-filtering, drill-to-detail) that carry SQL extras. These paths | ||
| # are still protected by datasource-access checks in raise_for_access. | ||
| # The _sql_filters_modified check below covers chart payloads only. |
There was a problem hiding this comment.
Datasource-access checks gate which dataset is queried, not what SQL runs against it, so a guest can simply omit slice_id — slice_ stays None, _native_filter_request_modified returns False for any payload without the NATIVE_FILTER/native_filter_id marker, and arbitrary extras.where (including subqueries) executes against any dataset the dashboard grants. Is leaving that path open intentional here?
There was a problem hiding this comment.
Yes, intentional
EnxDev's Review Agent — #43111 · HEAD 5bec901comment — right direction and the vectors it does cover are handled carefully, but the control is bypassed by omitting On the earlier bot comments: the multi-query "predicate transfer" flag is a non-issue (all queries in a context share one datasource and predicates only restrict rows — the docstring at 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
Code Review Agent Run #c19013Actionable Suggestions - 0Review 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 |
|
Two issues on the latest commits ( 1. 2. Scalar column params are silently dropped, in both params = {"x_axis": {"sqlExpression": "DATE_TRUNC('month', ts)", "label": "m"},
"groupby": [{"sqlExpression": "UPPER(country)", "label": "c"}]}
_collect_allowed_sql(...) # -> {'1 = 0', 'UPPER(country)'} x_axis missingSo a cross-filter from a sibling chart whose dimension is an adhoc x-axis — the case these commits were added to fix — still 403s. |
Code Review Agent Run #67f6d8Actionable Suggestions - 0Review 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 |
Code Review Agent Run #184affActionable 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 |
3 participants
SUMMARY
Embedded dashboard users (guest tokens) could modify chart data requests to inject custom SQL that wasn't part of the original chart. This change validates that any SQL in the request matches what's saved on the chart — if it doesn't, the request is rejected.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION