rusackas opened a new pull request, #40648:
URL: https://github.com/apache/superset/pull/40648
> **Draft / `hold:testing`** — this changes an authorization path in a
widely-used command. It needs CI + manual validation (especially the
report-execution screenshot flow) before it should be considered for merge.
### SUMMARY
`UpdateChartCommand.validate()` skips the ownership check for
"query_context-only" updates — when the payload is exactly `{query_context,
query_context_generation: true}` (`is_query_context_update()`). This bypass
exists so report workers and the Explore UI's lazy `query_context` backfill can
run as non-owners.
The problem: it skipped **all** authorization, gated only by the coarse
`can_write on Chart` FAB permission. So any authenticated user with `can_write
on Chart` (Gamma has it in many deployments) could PUT `{query_context,
query_context_generation: true}` to **any** chart id and rewrite its
`query_context` — a chart they don't own and may have no access to (CWE-639 /
broken object-level authorization).
### APPROACH
Replace the unconditional skip with
`security_manager.raise_for_access(chart=self._model)` on the query_context
path. `raise_for_access(chart=...)` permits **admins, owners, and users with
access to the chart's datasource**, and rejects everyone else. That preserves
the legitimate non-owner flows:
- the Explore UI backfill — any user who can *view/render* the chart already
has datasource access;
- report workers — the screenshot runs as the executor user, who must be
able to render the chart;
…while closing the "any `can_write Chart` holder can tamper with any chart"
gap.
### WHY DRAFT
The report-execution path renders the chart via a headless browser as the
configured **executor** user, then that session hits the chart PUT. This change
assumes the executor always passes `raise_for_access(chart=...)`. That's
expected (the executor must be able to render the chart), but it depends on the
executor configuration (`THUMBNAIL_EXECUTE_AS` / report executor settings) and
should be validated against the report/alert flow before merge. Hence draft +
`hold:testing`.
### TESTING INSTRUCTIONS
```
pytest tests/integration_tests/charts/commands_tests.py -k query_context
```
- New `test_query_context_update_requires_chart_access`: a non-owner without
datasource access (gamma) is rejected with `ChartForbiddenError`.
- **To validate before merge:** confirm the report/alert execution flow (and
the Explore lazy-backfill flow) still succeed end-to-end for non-owner users
with chart access.
### 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]
