fix(embedded): allow guest users to sort table columns in embedded dashboards - #41218
Conversation
query_context_modified() compared a guest request's orderby against the stored chart's orderby with a strict subset check, alongside columns, metrics and groupby. Saved charts usually store an empty orderby, so when a guest clicked a column header to sort a table in an embedded dashboard the new orderby value failed the subset check and the request was rejected with "Guest user cannot modify chart payload", even though sorting only changes result ordering and not which data is read. Handle orderby separately: a guest may sort by any column or metric the stored chart already references, but order-by terms that are not present in the stored chart (e.g. a free-form random() expression) are still rejected, preserving the existing SQL-injection guard. The columns/ metrics/groupby comparison is extracted into a helper to keep query_context_modified within the complexity budget, and the inner loop's variable shadowing of `key` is fixed along the way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
authentication:access-control
Code Review Agent Run #4f3376Actionable 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 #41218 +/- ##
==========================================
- Coverage 64.36% 64.32% -0.05%
==========================================
Files 2651 2652 +1
Lines 144812 145056 +244
Branches 33417 33474 +57
==========================================
+ Hits 93208 93307 +99
- Misses 49935 50057 +122
- Partials 1669 1692 +23
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:
|
…derby in guest sort guard Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #f16dbcActionable 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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Security review — query_context_modified orderby relaxation
HEAD SHA: 7c1fa4a
The three issues raised by codeant-ai are addressed in the final diff (legacy metric singular field added at line 554–555, entry shape validated at lines 584–591, closures left undocumented per author's call). The remainder of this review focuses on what's new.
Automated scan coverage (20 scans, both changed Python files)
| Scan | Result |
|---|---|
| 1. Inline imports without circular-import guard | 0 violations |
2. any() without short-circuit concern |
0 violations |
| 3. Mutable default arguments | 0 violations |
4. Optional[X] vs X | None style |
Matches existing file convention |
5. isinstance(x, type) == True patterns |
0 violations |
6. Bare except / except Exception |
0 violations |
| 7. Hardcoded strings that should be constants | 0 violations (key names are domain model) |
| 8. SQL string interpolation | 0 violations |
| 9. Missing type hints | 0 violations — all three helpers typed |
| 10. Missing return types | 0 violations |
11. print() statements |
0 violations |
| 12. Dead / unreachable code | 0 violations |
13. Overly broad Any in signatures |
add(values: Any) — local closure, acceptable |
14. Missing __init__.py |
N/A |
| 15. N+1 queries | 0 violations — no DB I/O in new code |
16. assert in production code |
0 violations |
| 17. Magic numbers | 0 violations |
| 18. Hardcoded credentials/secrets | 0 violations |
| 19. Missing edge-case tests | See MEDIUM #2 below |
| 20. Missing docstrings on public functions | 0 violations — all three new helpers documented |
Security analysis
freeze_value / _strip_overridable_keys: freeze_value performs deterministic JSON serialization after stripping timeGrain from every nested dict/list/tuple (_strip_overridable_keys is recursive). Both stored and requested values go through the same call so the comparison is symmetric. Tuple-to-list normalization in nested structures is consistent across both sides. ✓
SQL injection: A freeform expression like random() serializes to a string not in the stored chart's allowlist and is rejected by _orderby_modified. The freeze_value comparison requires an exact structural match — approximate or partial strings cannot slip through. ✓
RLS: Not modified. Row-level security is applied at query-execution time by the database layer. Sorting the filtered result set cannot bypass it — ORDER BY is applied to the rows the guest was already entitled to see. ✓
Direction bit: Not validated (any direction is permitted). Correct — direction does not affect which rows are read, only their ordering. Rejecting it would block the primary use case without security benefit. ✓
Entry shape: Validated as (term, bool) pair at lines 584–591 before entry[0] and entry[1] are accessed. Malformed shapes (bare string, zero-length list, non-bool second element) are treated as tampering. ✓
Guest capability boundary: Guests may sort only by columns/metrics the stored chart already references. They cannot introduce new expressions. This is the minimum relaxation needed and does not widen what data is accessible compared to what the chart was designed to expose. ✓
MEDIUM
1. _collect_sortable_identifiers misses all_columns from stored query context (manager.py:562)
From params_dict the function iterates over ("columns", "groupby", "metrics", "all_columns"). From stored_query_context.queries it only iterates over ("columns", "groupby", "metrics") — all_columns is absent:
if stored_query_context:
for query in stored_query_context.get("queries") or []:
for key in ("columns", "groupby", "metrics"): # ← all_columns missing
add(query.get(key))
add_orderby(query.get("orderby"))For table charts configured with "show all columns", QueryObject writes the exposed columns to all_columns in the serialized query context, not columns. If the stored chart's all_columns in the query context includes columns not present in params_dict (e.g., after a chart is resaved with a different column selection), legitimate guest sorts by those columns are silently rejected — the opposite of what this PR intends.
Suggested fix: add all_columns to the inner key list at line 562:
for key in ("columns", "groupby", "metrics", "all_columns"):
add(query.get(key))This is a conservative gap (false positives only — no security regression), but it affects the primary use case.
2. No test independently exercises the stored_query_context path in _collect_sortable_identifiers (manager_test.py:1246–1257)
Every new test uses _table_sort_query_context, which puts the sort column ("gender") in params_dict.groupby. The stored query context entry (columns: ["gender"]) is therefore redundant — removing the entire stored_query_context branch of _collect_sortable_identifiers would leave all the new positive tests green.
A focused test should verify that a column appearing only in stored_query_context.queries[].columns (not in params_dict) is accepted:
def test_query_context_modified_orderby_sort_by_stored_qc_only_column(mocker):
"""A column present only in the stored query context is an allowed sort target."""
query_context = _table_sort_query_context(mocker, orderby=[("age", True)])
# "age" not in params_dict, but present in the stored query context
query_context.slice_.params_dict = {"groupby": ["gender"], "metrics": ["count"]}
query_context.slice_.query_context = json.dumps(
{"queries": [{"columns": ["gender", "age"], "metrics": ["count"]}]}
)
assert not query_context_modified(query_context)Without this the stored_query_context branch has no independent coverage; a silent break there would go undetected by the current suite.
LOW
3. _orderby_modified collects the same entry twice when frontend populates both sources (manager.py:580–584)
requested = list(form_data.get("orderby") or [])
for query in query_context.queries:
requested.extend(getattr(query, "orderby", None) or [])In the common frontend path both form_data["orderby"] and QueryObject.orderby carry the same sort, so each entry is validated twice in the loop below. This is harmless for correctness (the result is the same) but can confuse a future reader who wonders whether the union is intentional or an oversight. A brief comment explaining that both sources must be validated — because either can carry unauthorized terms — would resolve the ambiguity.
NIT
4. Asymmetric stored_values in _columns_metrics_modified lacks an explanatory comment (manager.py:616–636)
The two subset checks inside the loop use stored_values at different stages of construction:
stored_values = {freeze_value(v) for v in stored_chart.params_dict.get(key) or []}
if not requested_values.issubset(stored_values): # params_dict only
return True
...
if stored_query_context:
stored_values.update(...) # augmented here
if not queries_values.issubset(stored_values): # augmented stored_values
return Truerequested_values (from form_data) is checked against params_dict alone; queries_values (from QueryObject) gets the fuller stored_query_context view. This was the pre-existing behavior, not a regression introduced here, but it is now more visible in the extracted helper. A one-line comment noting the intentional asymmetry would help future reviewers understand why the two checks differ.
… sort targets Address review feedback: the stored query context loop in _collect_sortable_identifiers omitted all_columns, silently rejecting legitimate guest sorts on table charts configured with 'show all columns'. Add it, plus a regression test and clarifying comments on the order-by union and the intentional stored_values asymmetry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@aminghadersohi good catches — pushed a fix:
Left the direction-bit and closure-docstring calls as-is per the earlier reasoning. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #9e5f5fActionable 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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
All four prior findings addressed at the new HEAD: all_columns added to the stored-query-context collection path in _collect_sortable_identifiers, stored-QC-only column test added (test_query_context_modified_orderby_sort_by_stored_qc_only_column), double-counting comment added to _orderby_modified, and asymmetric stored_values comment added to _columns_metrics_modified. 20 scans clean. All 4 review threads resolved. CI fully green.
…shboards (apache#41218) Co-authored-by: Claude Code <noreply@anthropic.com>
3 participants
SUMMARY
When viewing an embedded dashboard as a guest user, clicking a table column header to sort raises
Guest user cannot modify chart payload. This is a false positive from thequery_context_modified()guard insuperset/security/manager.py.Root cause:
orderbywas compared with the same strict subset check asmetrics,columns, andgroupby— the requestedorderbyhad to be a subset of the stored chart'sorderby. Saved charts almost always storeorderby == [], so any guest sort (e.g.[["gender", true]]) fails the subset check and the request is rejected, even though sorting only changes the ordering of the result, not which data is read.Fix:
orderbyis now validated separately. A guest may sort by any column or metric the stored chart already references (collected from the chart'sparams_dictand storedquery_context— columns, groupby, metrics, all_columns, plus existing order-by targets). Any order-by term not present in the stored chart — e.g. a free-formrandom()expression — is still rejected, so the existing SQL-injection protection is preserved.While here, the columns/metrics/groupby comparison is extracted into a small helper (
_columns_metrics_modified) to keepquery_context_modifiedwithin the complexity budget, and a pre-existing variable-shadowing bug in the inner loop (for key in equivalentshadowing the outerkey) is fixed.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Guest clicks a table column header →
Data error: Guest user cannot modify chart payload.After: Guest can sort embedded table columns. Sorting by an expression not present in the chart (e.g.
random()) is still blocked.TESTING INSTRUCTIONS
pytest tests/unit_tests/security/manager_test.py -k "query_context_modified" -vNew tests (added alongside the existing
test_query_context_modified_orderbyinjection test, which continues to pass):test_query_context_modified_orderby_sort_by_column— sort by an existing column → allowedtest_query_context_modified_orderby_sort_by_metric— sort by an existing metric → allowedtest_query_context_modified_orderby_sort_by_adhoc_metric— sort by an existing adhoc metric definition → allowedtest_query_context_modified_orderby_unknown_column— sort by a column not in the chart → rejectedtest_query_context_modified_orderby_empty— empty order-by → allowedManual: open an embedded dashboard with a table chart as a guest user and click a column header to sort — it should reorder instead of erroring.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code