fix(chart): require chart access for query_context-only updates by rusackas · Pull Request #40648 · apache/superset

rusackas · 2026-06-02T00:25:56Z

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.

WORTH NOTING:

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.

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

🤖 Generated with Claude Code

netlify · 2026-06-02T00:27:49Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`f915b46`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a2887bcdb86a50008aae2d2
😎 Deploy Preview	https://deploy-preview-40648--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2026-06-02T00:29:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.11%. Comparing base (d51753d) to head (d796fc3).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40648      +/-   ##
==========================================
- Coverage   64.12%   64.11%   -0.01%     
==========================================
  Files        2654     2654              
  Lines      143723   143767      +44     
  Branches    33153    33161       +8     
==========================================
+ Hits        92157    92180      +23     
- Misses      49951    49971      +20     
- Partials     1615     1616       +1

Flag	Coverage Δ
hive	`39.47% <0.00%> (-0.01%)`	⬇️
mysql	`58.22% <100.00%> (-0.01%)`	⬇️
postgres	`58.29% <100.00%> (-0.01%)`	⬇️
presto	`41.06% <0.00%> (-0.02%)`	⬇️
python	`59.76% <100.00%> (-0.01%)`	⬇️
sqlite	`57.91% <100.00%> (-0.01%)`	⬇️
unit	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR tightens object-level authorization for “query_context-only” chart updates by requiring chart access (via security_manager.raise_for_access(chart=...)) instead of skipping authorization entirely for that update shape. This closes a path where users with broad “can_write Chart” could overwrite query_context on charts they don’t own and shouldn’t be able to access.

Changes:

Enforce raise_for_access(chart=...) for query_context-only updates in UpdateChartCommand.validate().
Add an integration test asserting a non-owner without datasource access is rejected with ChartForbiddenError.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`superset/commands/chart/update.py`	Adds chart-access authorization enforcement on the query_context-only update path.
`tests/integration_tests/charts/commands_tests.py`	Adds a regression test covering the forbidden case for query_context-only updates.

UpdateChartCommand skips the ownership check for "query_context-only" updates (payload == {query_context, query_context_generation:true}) so report workers and the UI's lazy query_context backfill can run as non-owners. But it skipped ALL authorization, so any user with can_write on Chart could rewrite the query_context of a chart they don't own (CWE-639). Replace the unconditional skip with security_manager.raise_for_access(chart=...) on that path. That still permits the legitimate non-owner flows (admins, owners, and any user with access to the chart's datasource — which includes viewers who can render the chart and the report executor), while rejecting users who cannot access the chart at all. DRAFT: needs CI / manual validation that the report-execution screenshot path (executor user) passes raise_for_access in all configurations before merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@patch

…equires_chart_access Three @patch decorators but only two function parameters meant the mock for superset.commands.chart.update.g was never captured and its .user was never set. Added mock_u_g as the third parameter and set mock_u_g.user = gamma so all three g objects are consistent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Rebased on master and updated the tests for the new raise_for_access on the query_context-only update path: - the unit test now mocks raise_for_access (so it no longer hits an unmocked g.user) and asserts access is enforced while ownership is relaxed; adds a forbidden-access case. - the integration test also accepts ChartNotFoundError, since a no-access user is filtered by the DAO access filter before the explicit check is reached. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rusackas · 2026-06-10T00:38:21Z

Lifting the hold — the test failures are resolved (rebased on master; the unit tests now cover that a non-owner with chart access, i.e. the report-worker / Explore-backfill case, still succeeds, while a non-owner without access is rejected). The remaining hold:testing concern was the report-execution flow; report workers run as the report owner, who has chart access, so the new raise_for_access is satisfied for them. Reviewers may still want to spot-check a report screenshot run, but the path is covered by tests. Marking ready for review.

bito-code-review · 2026-06-10T00:41:15Z

Code Review Agent Run #c13a15

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: 2f0e486..f915b46
- superset/commands/chart/update.py
- tests/integration_tests/charts/commands_tests.py
- tests/unit_tests/commands/chart/update_test.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

richardfogaca

Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.

The fix itself looks right — correct layer, correct primitive, and the error translation mirrors the existing pattern in the same method. The main thing worth addressing is that the new integration test doesn't actually prove the fix; the rest are smaller suggestions. All line numbers verified against HEAD f915b46.

Functional — worth a look before merge

tests/integration_tests/charts/commands_tests.py:483

This test passes on master too, so it doesn't regression-test the vulnerability. validate() loads the chart via ChartDAO.find_by_id (superset/commands/chart/update.py:119), which applies the ChartFilter base filter — gamma has no energy-datasource access, so the chart is never found and the command raises ChartNotFoundError at update.py:121 on both branches. The pytest.raises((ChartForbiddenError, ChartNotFoundError)) tuple accepts that, which the docstring acknowledges, but it means the new raise_for_access branch is likely never reached here.

Could we patch ChartDAO.find_by_id to return the chart (bypassing the base filter) and assert ChartForbiddenError only? That would make the test fail on master and deterministically exercise the new check end-to-end. (The unit tests do pin the new behavior well — this is about the integration test earning its keep.)
superset/commands/chart/update.py:144

Related observation: since ChartFilter already blocks no-datasource-access users at find_by_id, the new check is mostly defense-in-depth for callers that bypass the base filter — and the residual surface is that any non-owner with datasource access can still overwrite a sibling chart's stored query_context with arbitrary content (nothing validates it references the chart's own datasource, and report executors later run the stored payload with their broader permissions). That gap predates this PR, so no need to solve it here — but would it be worth a follow-up issue to validate query_context.datasource against the chart's datasource_id on this path? Happy to keep as-is for this PR.

Re: the "WORTH NOTING" question in the PR body

We statically traced the report/alert path: the worker never calls UpdateChartCommand directly — when query_context is missing it takes a headless screenshot (superset/commands/report/execute.py:519-521) rendering Explore as the executor, and the frontend PUT fires with the executor's session. For the screenshot to produce a query_context at all, the executor must already pass raise_for_access(query_context=...) on the same datasource — so the new check should never be the binding constraint for a report that could succeed before. End-to-end validation is still prudent, but the static trace looks safe.

Other suggestions

tests/unit_tests/commands/chart/update_test.py:76

Small suggestion: raise_for_access.assert_called_once_with(chart=find_by_id.return_value) instead of assert_called_once() — pins what gets authorized so a future refactor can't silently switch the resource. Totally optional.
Few small nits, take or leave:
- tests/integration_tests/charts/commands_tests.py:460 — missing -> None on the new test method (the new unit tests have it).
- tests/integration_tests/charts/commands_tests.py:456-461 — mock param names are swapped relative to the decorators (mock_g binds superset.utils.core.g, mock_u_g binds superset.commands.chart.update.g); harmless since all three get the same user, but confusing for the next reader.
- tests/integration_tests/charts/commands_tests.py:470 — db.session.query(Slice).all()[0] is order-dependent; filtering by slice_name like neighboring tests would avoid flakiness if another fixture's chart sorts first.
- tests/unit_tests/commands/chart/update_test.py:86-89 — reusing _ownership_exc() as the raise_for_access side effect labels an access failure as an ownership error; a tiny _access_exc() helper would keep the fixture honest.

Praise

The unit tests at tests/unit_tests/commands/chart/update_test.py:54-94 pin both halves of the contract — ownership still relaxed for the report-worker path (raise_for_ownership.assert_not_called()) and access newly enforced with the right error translation. That's exactly the pair of invariants a future refactor needs to trip over.

…ith access Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- Patch ChartDAO.find_by_id in the integration test to bypass the ChartFilter base filter so the new raise_for_access gate is what denies; assert ChartForbiddenError only (fails on master). - Add -> None, fix mock param names, filter by slice_name. - Pin raise_for_access(chart=...) in unit tests; add _access_exc() helper so the access-denied case uses an access error, not an ownership error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rusackas · 2026-06-10T03:00:50Z

@richardfogaca Thanks for the thorough pass — addressed the actionable items in d796fc3:

Integration test now earns its keep (commands_tests.py:483): patched ChartDAO.find_by_id to return the chart directly, bypassing the ChartFilter base filter, and tightened the assertion to ChartForbiddenError only. It now deterministically exercises the new raise_for_access branch and fails on master.
Unit assertion pinned (update_test.py:76): raise_for_access.assert_called_once_with(chart=...) so a refactor cannot silently switch the authorized resource.
Nits: added -> None, renamed the mock params to match their decorators (mock_core_g / mock_update_g / mock_sm_g), switched the order-dependent .all()[0] to filter_by(slice_name="Energy Sankey").one(), and added an _access_exc() helper so the access-denied path uses a CHART_SECURITY_ACCESS_ERROR instead of mislabeling it as an ownership error.
Also added the positive test Copilot asked for: test_update_chart_query_context_non_owner_with_access_allowed, pinning that a non-owner whose access check passes can complete the backfill.

On the query_context.datasource validation gap at update.py:144 — agreed that lets a non-owner with datasource access overwrite a sibling chart's stored query_context, and that it predates this PR. I'll open a follow-up issue to validate the payload's datasource against the chart's own datasource_id rather than widen this PR's scope. Thanks for the static trace on the report path too — matches what I found.

bito-code-review · 2026-06-10T06:01:24Z

Code Review Agent Run #7a9188

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: f915b46..d796fc3
- tests/unit_tests/commands/chart/update_test.py
- tests/integration_tests/charts/commands_tests.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

rusackas · 2026-06-10T20:07:24Z

Opened the follow-up I promised for the query_context.datasource validation gap: #40953. Keeping that out of this PR's scope as discussed — this PR's chart-access gate stands on its own. Thanks again @richardfogaca for the thorough pass; all the actionable items (deterministic integration test, pinned unit assertion, positive non-owner-with-access test, and the nits) landed in d796fc3.

richardfogaca

LGTM, great work 💯

Two CI integration failures (surfaced once the alembic head-fork was resolved and the test jobs actually ran): - VersioningFlaskPlugin.transaction_args recorded whatever get_user_id() returned, guarding only `is None`. A mocked `g` (master's test_query_context_update_requires_chart_access, apache#40648) yields a Mock g.user.id, which then hit the integer version_transaction.user_id column and blew up the flush with a SQL bind error. Guard on isinstance(int) — production-safe (user_id is always int/None there), fixes the test. - test_list_versions_denies_non_owner (chart/dashboard/dataset) asserted Alpha -> 403 on a "row-level ownership" theory the endpoint never implemented: it gates via raise_for_access, and Alpha has all_datasource_access + can_write (FR-013), so 200 is correct. Renamed to _allows_can_write_non_owner and assert 200; model-level denial stays covered by the Gamma tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

rusackas added the hold:testing! On hold for testing label Jun 2, 2026

pull-request-size Bot added the size/M label Jun 2, 2026

rusackas added this to Superset Review Help Wanted Jun 2, 2026

github-project-automation Bot moved this to Needs Review in Superset Review Help Wanted Jun 2, 2026

rusackas requested review from Copilot, dpgaspar and sha174n June 2, 2026 00:41

Copilot started reviewing on behalf of rusackas June 2, 2026 00:41 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Comment thread tests/integration_tests/charts/commands_tests.py Outdated

Comment thread tests/integration_tests/charts/commands_tests.py

Comment thread superset/commands/chart/update.py

rusackas moved this from Needs Review to Needs Follow-Up Work in Superset Review Help Wanted Jun 2, 2026

sadpandajoe added the review:draft label Jun 2, 2026

claude and others added 3 commits June 9, 2026 14:34

rusackas force-pushed the fix/chart-query-context-access-check branch from 004df4b to f915b46 Compare June 9, 2026 21:38

rusackas added maintain Tracked for ongoing maintenance — keep open and removed hold:testing! On hold for testing review:draft labels Jun 10, 2026

rusackas marked this pull request as ready for review June 10, 2026 00:38

dosubot Bot added the authentication:access-control Rlated to access control label Jun 10, 2026

rusackas moved this from Needs Follow-Up Work to Needs Review in Superset Review Help Wanted Jun 10, 2026

rusackas requested a review from richardfogaca June 10, 2026 01:02

github-actions Bot added the preset-io label Jun 10, 2026

richardfogaca reviewed Jun 10, 2026

View reviewed changes

rusackas and others added 2 commits June 9, 2026 19:57

test(chart): add positive query_context backfill test for non-owner w…

c5a683a

…ith access Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

pull-request-size Bot added size/L and removed size/M labels Jun 10, 2026

eschutho moved this from Needs Review to In Review in Superset Review Help Wanted Jun 10, 2026

rusackas mentioned this pull request Jun 10, 2026

Validate query_context.datasource against the chart's own datasource_id on update #40953

Closed

rusackas requested a review from villebro June 10, 2026 23:22

richardfogaca approved these changes Jun 13, 2026

View reviewed changes

eschutho moved this from In Review to Approved and/or Merged in Superset Review Help Wanted Jun 13, 2026

rusackas merged commit b05fe48 into master Jun 13, 2026
61 checks passed

rusackas deleted the fix/chart-query-context-access-check branch June 13, 2026 11:16

Navigation Menu

Uh oh!

fix(chart): require chart access for query_context-only updates - #40648

fix(chart): require chart access for query_context-only updates#40648
rusackas merged 5 commits into
masterfrom
fix/chart-query-context-access-check

rusackas commented Jun 2, 2026 •

edited

Loading

Uh oh!

netlify Bot commented Jun 2, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

bito-code-review Bot commented Jun 10, 2026 •

edited

Loading

Code Review Agent Run #c13a15

Uh oh!

richardfogaca left a comment

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

bito-code-review Bot commented Jun 10, 2026 •

edited

Loading

Code Review Agent Run #7a9188

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

richardfogaca left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

rusackas commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

APPROACH

WORTH NOTING:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

netlify Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

codecov Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

bito-code-review Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #c13a15

Uh oh!

richardfogaca left a comment

Choose a reason for hiding this comment

Functional — worth a look before merge

Re: the "WORTH NOTING" question in the PR body

Other suggestions

Praise

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

bito-code-review Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #7a9188

Uh oh!

rusackas commented Jun 10, 2026

Uh oh!

richardfogaca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rusackas commented Jun 2, 2026 •

edited

Loading

netlify Bot commented Jun 2, 2026 •

edited

Loading

codecov Bot commented Jun 2, 2026 •

edited

Loading

bito-code-review Bot commented Jun 10, 2026 •

edited

Loading

bito-code-review Bot commented Jun 10, 2026 •

edited

Loading