fix(embedded): stop rejecting guest chart data built from control-specific params keys

[rusackas]

SUMMARY

Embedded guests get a generic 403 (Guest user cannot modify chart payload) on any chart persisted without a stored query_context — charts created via the REST API and every chart loaded by load_examples. The guest tamper check (_columns_metrics_modified) compares the requested metrics/columns against only the same-named chart params keys, but chart types store what they query under control-specific keys: metric (big number, world map), entity/series/x/y/size (bubble), granularity_sqla (temporal axis), secondary_metric (sunburst), plain groupby vs the payload's columns (even table hits this). The chart's own generated payload reads as tampering.

This is also why playwright-tests-experimental's embedded suite is flaky: the embedded project runs with admin storageState, and a cookie race (the /embedded/<uuid> response rotates the iframe to an anonymous session while the SDK's parallel csrf/guest-token fetches rewrite the admin cookie) decides whether chart data requests are evaluated as admin (tamper check skipped, 200) or as a real guest (403). The suite only passes when admin wins.

The fix reads the stored side across the control names that can hold a metric or column, keeping exact freeze_value matching — only values the chart already references become requestable. No label or expression equivalence is introduced: novel saved metrics, novel columns, adhoc SQL expressions, and label-spoofed adhoc columns replaying a stored column's label are all still rejected (covered by new unit tests).

Verification beyond unit tests: replayed the actual network payloads of all nine world_health example charts (captured from the embedded Playwright trace artifacts) through the check — every one was rejected before this change and passes after, while tampered variants of the same payloads stay rejected.

Security model note per SECURITY.md: this does not grant any principal a new capability. An embedded guest token holder is entitled to view the data of charts on the dashboard shared with them; the values unlocked here are exactly the ones the stored chart already renders to that guest.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: embedded world_health charts show "Data error: Forbidden" for guest-evaluated requests. After: they render.

TESTING INSTRUCTIONS

pytest tests/unit_tests/security/manager_test.py -k query_context_modified — includes the new singular-metric, control-specific-columns, and still-tampered cases. Or embed the world_health dashboard via the SDK and confirm charts load for a guest token.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[bito-code-review]

Code Review Agent Run #474a51

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: a4e9e7d..a4e9e7d

  • superset/security/manager.py
  • tests/unit_tests/security/manager_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ 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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[bito-code-review]

The flagged issue is a misinterpretation of the test's intent. The test test_query_context_modified_singular_metric_param correctly validates that a chart storing a metric under the singular metric key is allowed to generate a payload with a plural metrics list without being flagged as tampering. The setup correctly models the chart's stored parameters, and the test correctly asserts that this is not tampering. No changes are required.

tests/unit_tests/security/manager_test.py

query_context.slice_.params_dict = {
        "metric": "sum__SP_POP_TOTL",
        "groupby": [],
    }

    query_context.form_data = {
        "slice_id": 42,
        "metric": "sum__SP_POP_TOTL",
    }
    query_context.queries = [QueryObject(metrics=["sum__SP_POP_TOTL"])]  # type: ignore
    assert not query_context_modified(query_context)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.15%. Comparing base (cae61b5) to head (f3da847).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #42295   +/-   ##
=======================================
  Coverage   65.15%   65.15%           
=======================================
  Files        2789     2789           
  Lines      157543   157548    +5     
  Branches    35864    35866    +2     
=======================================
+ Hits       102644   102652    +8     
+ Misses      52931    52927    -4     
- Partials     1968     1969    +1     

Flag	Coverage Δ
hive	38.74% <23.07%> (-0.01%)	⬇️
mysql	57.66% <100.00%> (+<0.01%)	⬆️
postgres	57.71% <100.00%> (+<0.01%)	⬆️
presto	40.65% <23.07%> (-0.01%)	⬇️
python	59.09% <100.00%> (+<0.01%)	⬆️
sqlite	57.32% <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.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	ca1eab6
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a6015f8ed24720008aa94ed
😎 Deploy Preview	https://deploy-preview-42295--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.

[bito-code-review]

Code Review Agent Run #a995f1

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: a4e9e7d..f3da847

  • tests/unit_tests/security/manager_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ 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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Reviewed at f3da8478555312f7fee67fd2b432052644e3dfff. The core fix — reading stored chart params across control-specific keys (metric, entity/series/x/y/size, granularity_sqla, groupby/columns) instead of only same-named keys — is the right shape for the flakiness/false-403 problem described, and the three bypass classes the PR claims to still reject genuinely are still rejected: I traced and reproduced each against the exact _columns_metrics_modified/_stored_param_values logic at HEAD:

• Novel metric not referenced anywhere on the chart → still rejected (freeze_value subset check fails).
• Novel column not referenced anywhere → still rejected.
• Label-spoofed adhoc column (payload column with label matching a stored column's label but a different sqlExpression) → still rejected, because freeze_value serializes the whole structure (json.dumps of the dict), not just the label, so a dict with an extra sqlExpression key never equals the stored plain string. No label/expression equivalence is introduced here.

However, I believe _STORED_METRIC_PARAMS in superset/security/manager.py:994-1004 over-expands the allowed set beyond what the chart renders to the guest, specifically by including series_limit_metric and timeseries_limit_metric:

Those two params are the chart's "Sort by"/limit-ranking metric selector, which by design is commonly configured to a different metric than the ones actually displayed (e.g. rank the top 5 regions by sum__cost, but only display sum__sales). Per superset/models/helpers.py (get_sqla_query, ~4189-4327), that metric is used exclusively as an ORDER BY in an internal ranking subquery/prequery — it is never SELECTed as an output column unless it also happens to be in metrics. It's also already guarded by a dedicated, narrower check (_series_limit_metric_modified) that only looks at the series_limit_metric/timeseries_limit_metric request fields.

Because the new _stored_param_values folds it into the same pool consulted for the plain metrics comparison, a guest can now send form_data["metrics"] = [<the chart's series_limit_metric value>] (with series_limit_metric itself left unset) and the tamper check passes — _columns_metrics_modified only checks requested_values.issubset(stored_values), and stored_values now contains that value via the series_limit_metric/timeseries_limit_metric entries. _series_limit_metric_modified doesn't catch this either, since it never inspects metrics. The result: the guest receives aggregate data for a metric the chart never displays.

I reproduced this against the extracted production functions with a minimal repro (chart displaying sum__sales grouped by region, ranked by series_limit_metric = sum__cost; guest request substitutes metrics: ["sum__cost"]) — _columns_metrics_modified returns False (not tampered) post-fix, whereas the pre-fix code (which only compared metrics against the stored metrics key) correctly rejected it. So this is a regression introduced by this diff, not pre-existing behavior. It's also untested — the existing series_limit_metric-related tests (e.g. around _series_limit_metric_query_context) only exercise the dedicated field-level check, not this cross-field path.

Suggested fix: drop series_limit_metric/timeseries_limit_metric from _STORED_METRIC_PARAMS (they're already covered by _series_limit_metric_modified), so the generic metrics equivalence set only includes control keys whose values the chart actually renders.

Nice test coverage otherwise on the control-specific-key cases (test_query_context_modified_singular_metric_param, test_query_context_modified_control_specific_column_params) and the negative cases (test_query_context_modified_novel_values_still_tampered) — I confirmed both would fail against the pre-fix code, so they're real behavioral coverage, not vacuous.

[mistercrunch]

LGTM

[rusackas]

I'll clean up this mess, I promise.