rusackas opened a new pull request, #41218: URL: https://github.com/apache/superset/pull/41218
### 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 the `query_context_modified()` guard in `superset/security/manager.py`. **Root cause:** `orderby` was compared with the same strict subset check as `metrics`, `columns`, and `groupby` — the requested `orderby` had to be a subset of the stored chart's `orderby`. Saved charts almost always store `orderby == []`, 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:** `orderby` is now validated separately. A guest may sort by any column or metric the stored chart already references (collected from the chart's `params_dict` and stored `query_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-form `random()` 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 keep `query_context_modified` within the complexity budget, and a pre-existing variable-shadowing bug in the inner loop (`for key in equivalent` shadowing the outer `key`) is fixed. > ⚠️ **Security-sensitive area — please review carefully.** This relaxes the guest payload-tampering guard. The intent is to permit legitimate re-ordering while continuing to reject any order-by term the chart does not already expose. The existing injection test (`test_query_context_modified_orderby`, sorting by `random()`) still passes. cc anyone who worked on the recent `_native_filter_*` hardening in this module — the new check mirrors that `_native_filter_term_allowed` style. ### 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 ```bash pytest tests/unit_tests/security/manager_test.py -k "query_context_modified" -v ``` New tests (added alongside the existing `test_query_context_modified_orderby` injection test, which continues to pass): - `test_query_context_modified_orderby_sort_by_column` — sort by an existing column → allowed - `test_query_context_modified_orderby_sort_by_metric` — sort by an existing metric → allowed - `test_query_context_modified_orderby_sort_by_adhoc_metric` — sort by an existing adhoc metric definition → allowed - `test_query_context_modified_orderby_unknown_column` — sort by a column not in the chart → rejected - `test_query_context_modified_orderby_empty` — empty order-by → allowed Manual: 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 - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
