fix(embedded): allow guest users to sort table columns in embedded dashboards#41218
fix(embedded): allow guest users to sort table columns in embedded dashboards#41218rusackas wants to merge 2 commits into
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.35% -0.01%
==========================================
Files 2651 2651
Lines 144812 144850 +38
Branches 33417 33430 +13
==========================================
+ Hits 93208 93222 +14
- Misses 49935 49951 +16
- Partials 1669 1677 +8
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 |
2 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