fix(embedded): compare guest chart payloads against every stored control - #42864
Draft
luizotavio32 wants to merge 1 commit into
Draft
fix(embedded): compare guest chart payloads against every stored control#42864luizotavio32 wants to merge 1 commit into
luizotavio32 wants to merge 1 commit into
Conversation
`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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
Contributor
Author
|
/review |
Contributor
Code Review Agent Run #4752b5Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
query_context_modifiedcompared a request'smetrics,columns,groupbyandorderbyagainst identically-named keys in the chart'sparams. 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 storemetric, bubble storesx/y/size, sankey storessource/target, time series storesx_axis.Each request key is now compared against every equivalent stored control, and
orderbyagainst any metric or column the chart already reads — charts sort by their own metric on initial load, with no user interaction. Values are normalized before comparison so the same underlying column compares equal however it is spelled:BASE_AXISmarkers (added bynormalizeTimeColumn) are shedMatching 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.
series_limit_metricandtimeseries_limit_metricare included as stored metric controls, so a guest cannot rank top-N groups by arbitrary adhoc SQL.The comparison helpers move to
superset/security/guest_payload.py;freeze_valueis re-exported fromsuperset.security.managerfor compatibility.TESTING INSTRUCTIONS
pytest tests/unit_tests/security/manager_test.py— 52 tests, covering per-viz-type control names, each normalization rule, and tamper vectors that must still be rejected.Manual verification against a real embedded dashboard, which is what this fix is about — synthetic
POST /api/v1/chart/datapayloads exercise the comparator but not what the frontend actually sends:EMBEDDED_SUPERSETand a guest role, and an embedded dashboard with a spread of viz types.series_limit_metricwith adhoc SQL) and assert a 403 carrying the guard's own message,Guest user cannot modify chart payload. A genericForbiddenmeans an earlier gate objected and the guard never ran, so a 200 on the legitimate load would prove nothing.Note that
query_context_modifiedreturns early whenquery_context.slice_ is None. On 6.2user_view_menu_names()returns nothing for guests, soChartFiltercan hide the slice and the guard no-ops, accepting every payload — step 3 is what distinguishes "allowed" from "never ran".ADDITIONAL INFORMATION