You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "luizotavio32 (via GitHub)" <gi...@apache.org> on 2026/08/06 21:00:58 UTC
[PR] fix(embedded): compare guest chart payloads against every stored control [superset]
luizotavio32 opened a new pull request, #42864:
URL: https://github.com/apache/superset/pull/42864
`query_context_modified` compared a request's `metrics`, `columns`, `groupby` and `orderby` against identically-named keys in the chart's `params`. Charts save those values under whichever control names their viz type uses, so a guest merely loading an embedded dashboard was rejected as a tamperer: big number and pie store `metric`, bubble stores `x`/`y`/`size`, sankey stores `source`/`target`, time series stores `x_axis`.
Each request key is now compared against every equivalent stored control, and `orderby` against any metric or column the chart already reads, since charts sort by their own metric on initial load without user interaction. Values are normalized before comparison so the same underlying column compares equal however it is spelled — adhoc references that only point back at a physical column collapse to its name, synthesized `BASE_AXIS` markers are shed, sort pairs reduce to the expression they sort on, and scalar-valued controls are no longer iterated character by character.
Matching itself stays exact: only the shape of a value is normalized, never its identity, so a collapsed reference still has to match something stored on the chart. Pointing at an unrelated column or wrapping free-form SQL grants no extra access.
The comparison helpers move to `superset/security/guest_payload.py`; `freeze_value` is re-exported from `superset.security.manager` for compatibility.
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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 estimates and downtime expectations provided
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5217119335
Nice fix for the general case, but the two lookup tables aren't complete yet — I found several other real controls that map into `metrics`/`columns` via `buildQuery` but aren't listed, so those chart types will still hit the same false-403 whenever the chart has no cached `query_context`:
**Missing from `STORED_METRIC_PARAMS`:**
- `tooltip_metrics` (Gantt)
- `timeseries_limit_metric_b` (MixedTimeseries, Query B)
- `point_radius_fixed` (deck.gl Scatter/Polygon, when set to a metric)
**Missing from `STORED_COLUMN_PARAMS`:**
- `groupby_b` (MixedTimeseries, Query B)
- `source_category` / `target_category` (Graph/network)
- `dimension`, `line_column`, `js_columns` (deck.gl)
- `geom_column` (Cartodiagram)
**Structural gaps** that adding a name alone won't fix, since the value isn't a flat string/list:
- deck.gl `spatial`/`start_spatial`/`end_spatial` — nested lat/lon/geohash column configs (decomposed today only via `getSpatialColumns()` on the frontend)
- deck.gl `tooltip_contents` — list of tooltip-config objects wrapping column refs
- Cartodiagram's `selected_chart` — a JSON-encoded embedded chart definition with its own `metrics`/`groupby` nested inside a string; the flat key scan never looks inside it
These all fail closed (false 403 for a legit guest), not open, so no new security exposure — but the fix doesn't cover "every stored control" yet as claimed in the description.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5209125876
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 51c9226f783e019b9116bf8026cdba91600e5052 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a74f60bebc6470009676783 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42864--superset-docs-preview.netlify.app](https://deploy-preview-42864--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcrLCYAwDADQXXK2WFRSdQPHaJr4AWtDUVDE3e3tHd4LmlPUE0aACij7I6zF83bXEkmYhY1XNdiYsPp8GrqWEll0T8_EpaJ33YyWhAJ2zlo7oEPXt_D9) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42864--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5217446196
Confirmed — I checked each name against the `buildQuery` that consumes it, and all of them do reach a query. Fixed in cb1aea5.
Added to the lookup tables:
- metrics: `tooltip_metrics`, `timeseries_limit_metric_b`, `point_radius_fixed`, plus `series_limit_metric_b` (Query B's section is generated by `createQuerySection(..., '_b')`, and `retainFormDataSuffix` strips the suffix, so the `_b` twin of every Query A control is live)
- columns: `groupby_b`, `source_category`, `target_category`, `dimension`, `line_column`, `js_columns`, `geom_column`
The three structural gaps are decomposed the way the frontend does, so they compare against the flat names a request carries:
- `spatial`/`start_spatial`/`end_spatial` → the lat/lon/geohash columns they name, mirroring `getSpatialColumns()`. All of `lonCol`/`latCol`/`lonlatCol`/`geohashCol` are collected regardless of declared `type`, since each is a column the chart is saved with.
- `tooltip_contents` → the columns its entries wrap, mirroring `extractTooltipColumns()`. Only `item_type: "column"` entries count; metric entries are read from data already fetched and select nothing, so requesting one as a column is still rejected.
- `selected_chart` → decoded, its `params` decoded again, and recursed into, so the nested chart's own metrics/group-bys are reachable. A malformed value yields no params rather than raising, and grants nothing.
Decomposition only reaches names a control already points at, so nothing opens up: a `fix`-typed `point_radius_fixed` is a radius in pixels and grants no metric. Both of those are covered by tests, along with a tamper case per structural gap.
You're right that the description overclaimed; I've reworded it to say the controls are enumerated from what `buildQuery` feeds into a query, and to name the nested ones explicitly rather than implying exhaustiveness.
One note on verification, since it bears on how much the green suite is worth here: I confirmed each new legitimate-load test fails against the previous comparator (10 of them did), so they exercise the fix rather than restating it. The tamper tests reject in both versions, as they should.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5209096230
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5217534544
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42864?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `52.08333%` with `46 lines` in your changes missing coverage. Please review.
:warning: Please [upload](https://docs.codecov.com/docs/codecov-uploader) report for BASE (`6.2@5687c29`). [Learn more](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#section-missing-base-commit) about missing BASE report.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42864?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/security/guest\_payload.py](https://app.codecov.io/gh/apache/superset/pull/42864?src=pr&el=tree&filepath=superset%2Fsecurity%2Fguest_payload.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvZ3Vlc3RfcGF5bG9hZC5weQ==) | 52.87% | [32 Missing and 9 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/42864?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/42864?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 44.44% | [3 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/42864?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 6.2 #42864 +/- ##
======================================
Coverage ? 63.52%
======================================
Files ? 2555
Lines ? 132984
Branches ? 30811
======================================
Hits ? 84475
Misses ? 47012
Partials ? 1497
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42864/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.86% <23.95%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.88% <23.95%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/42864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.22% <52.08%> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42864/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.10% <52.08%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42864?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5209113143
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #42864:
URL: https://github.com/apache/superset/pull/42864
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): compare guest chart payloads against every stored control [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #42864:
URL: https://github.com/apache/superset/pull/42864#issuecomment-5209112401
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org