khaa-dv opened a new pull request, #43577:
URL: https://github.com/apache/superset/pull/43577
Fixes #43576 :
Some viz types (e.g. heatmap_v2's 'groupby' control) store a single value as
a bare string rather than a list. _columns_metrics_modified iterated
form_data.get(key)/params_dict.get(key)/getattr(query, key) directly, so a
scalar string was iterated character-by-character, causing every legitimate
guest request for such a chart to be rejected as 'Guest user cannot modify
chart payload'.
Wrap all four iteration sites with a small _ensure_list() helper that treats
a non-list, non-None value as a single-item list.
### SUMMARY
Fixes a bug in the guest-user payload security check
(`_columns_metrics_modified`
in `superset/security/manager.py`) that rejects **every** legitimate embedded
guest request for a chart whose stored `groupby`/`columns`/`metrics` control
value is a single scalar rather than a list.
`heatmap_v2` is the clearest example: its `groupby` form control stores a
single column name as a plain string (e.g. `"territory_name_2"`), not a
one-item list. `_columns_metrics_modified` iterates these values with
`for value in form_data.get(key) or []` (and the equivalent for
`stored_chart.params_dict.get(key)` and `getattr(query, key, [])`) without
checking whether the value is actually a list. Iterating a Python string
yields its individual characters, so the "requested" set ends up as e.g.
`{"t", "e", "r", "r", ...}` instead of `{"territory_name_2"}` — which is of
course never a subset of anything stored, so the guest request is always
rejected with:
```
Guest user cannot modify chart payload
```
even when the guest is requesting exactly the chart's own saved data, with no
modification whatsoever.
This was discovered while debugging why every `heatmap_v2` chart on an
embedded dashboard failed to load for guest users, while charts of other viz
types (whose equivalent controls are always lists) worked fine.
**Note on #42864:** apache/superset#42864 (merged into `6.2`) performs a much
broader rewrite of this same comparator — moving it to
`superset/security/guest_payload.py` and covering many more per-viz-type
control names and structural cases (deck.gl, Cartodiagram, MixedTimeseries
Query B, etc.) — and its description explicitly notes fixing this exact
scalar-iteration case as one of many improvements. As of this writing that
rewrite has not landed in `master` (confirmed via `git log origin/master`),
which is still on the narrower `_STORED_COLUMN_PARAMS`-based comparator this
PR patches. This PR is a minimal, `master`-targeted fix for just the
scalar-value regression, useful on its own regardless of whether/when
`6.2`'s broader rewrite is merged forward into `master`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before:** any embedded dashboard containing a `heatmap_v2` chart (or any
other chart type whose `groupby`/`columns`/`metrics` control happens to be a
scalar rather than a list) shows a permanent `Data error: Guest user cannot
modify chart payload` for guest users, regardless of whether the guest
actually modified anything.
**After:** the same chart loads normally for guest users.
### TESTING INSTRUCTIONS
1. Create a `heatmap_v2` chart on a dataset, using a single column for the
"Group by" control (not multiple).
2. Add it to a dashboard and enable embedding for that dashboard.
3. Load the dashboard via the embedded SDK / guest token.
4. Before this fix: the chart fails to load with `Guest user cannot modify
chart payload`.
5. After this fix: the chart loads normally.
Also added
`tests/unit_tests/security/manager_test.py::test_query_context_modified_scalar_control_value_not_tampered`
(and a companion negative-case test,
`test_query_context_modified_scalar_control_value_tampered`) reproducing the
exact scenario at the `query_context_modified()` level, without needing a
live dashboard/guest token. Both were confirmed to fail against the
pre-patch comparator.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #43576
- [ ] Required feature flags: none
- [ ] Changes UI
- [ ] Includes DB Migration (SQL)
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [x] Confirm DB Migration upgrade and downgrade tested — N/A, no migration
- [x] Introduces new feature or API — no, bugfix only
- [x] Removes existing feature or API — no
--
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]
