sadpandajoe opened a new pull request, #42150:
URL: https://github.com/apache/superset/pull/42150
### SUMMARY
Embedded (guest-token) dashboards return a 403 ("Guest user cannot modify
chart payload") on every chart whose saved `query_context` is NULL or stale.
The dashboard shell, filter bar, and metadata load fine; only the chart tiles
fail, and only for guest users (admin sessions skip the guard, so the same
dashboard looks healthy when tested logged in).
Fixes #31872.
### Root cause
The guest anti-tamper guard in `superset/security/manager.py`
(`query_context_modified` -> `_columns_metrics_modified`) requires the
request's columns/metrics to be a subset of what the chart has stored (`params`
plus the saved `query_context`). For time-series charts, `normalizeTimeColumn`
(superset-ui-core) rewrites the chart's saved x-axis into a synthesized adhoc
column tagged `columnType: "BASE_AXIS"`. A physical x-axis is stored as a bare
string under its own `x_axis` control, not in `columns`, so the wrapped form
never matches the stored value. A chart whose `query_context` is NULL or was
written by an older frontend (imported charts, API-created charts, older saves)
therefore has nothing the synthesized column can match, and every guest load is
rejected.
### Change
- Reduce a synthesized `BASE_AXIS` column back to the reference it stands
for before the subset check: a physical reference collapses to its column name,
an adhoc reference to the underlying adhoc column without the synthesized
markers.
- Treat the chart's stored `x_axis` as an allowed dimension for
`columns`/`groupby` and for order-by, so sorting by the temporal x-axis is also
accepted.
- The collapsed value must still be a subset of the chart's owner-saved
`params`/`query_context`, so tagging an unrelated column or free-form SQL as
`BASE_AXIS` grants no additional access. Metrics, series-limit metrics, and
non-x-axis order-by keep exact-match semantics.
- Log, server-side, which comparator rejected a guest payload (no payload
values), so this class of failure is diagnosable instead of an opaque 403. The
client-facing message is unchanged.
This extends the existing `GUEST_OVERRIDABLE_VALUE_KEYS` / `timeGrain`
carve-out that already handles the same `BASE_AXIS` column.
This touches guest access control, so a security-minded review is
appreciated.
### TESTING INSTRUCTIONS
Added unit tests in `tests/unit_tests/security/manager_test.py`:
- guest load passes for a physical x-axis, an adhoc x-axis, and a stale
non-null `query_context`;
- guest sort by the temporal x-axis passes;
- a `BASE_AXIS`-tagged column or order-by referencing a column the chart
never exposed is still rejected;
- a `BASE_AXIS`-tagged adhoc column whose SQL differs from the saved x-axis
is still rejected.
Run: `pytest tests/unit_tests/security/manager_test.py -k base_axis`
Manual: embed a dashboard whose charts have `query_context = NULL` (for
example `UPDATE slices SET query_context = NULL WHERE id = ...`) with a valid
guest token. Before this change the chart tiles show "Data error / Forbidden";
after, they render.
### ADDITIONAL INFORMATION
- [ ] Has associated issue: Fixes #31872
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime consideration & tested
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
