fix(embedded): load guest charts with a BASE_AXIS x-axis - #42847
fix(embedded): load guest charts with a BASE_AXIS x-axis#42847luizotavio32 wants to merge 2 commits into
Conversation
A guest loading a chart that has an x-axis and no saved query_context gets a
403 (`Guest user cannot modify chart payload`) and the chart renders as an
error tile. The same chart works for Admin.
Before querying, `normalizeTimeColumn` rewrites the chart's x-axis into a
synthetic column, so the request carries
`{"columnType": "BASE_AXIS", "sqlExpression": "order_date", ...}` while the
chart stores `"order_date"` under its own `x_axis` control. Two independent
mismatches each produce the 403: the shapes differ (dict vs. string, never
equal), and the guard never reads the `x_axis` control at all.
Collapse a synthesized BASE_AXIS column back to the reference it stands for
before comparing, and read the stored `x_axis` as an accepted column value.
Both apply to `columns`/`groupby` only -- `metrics` and `orderby` keep exact
comparison, so a BASE_AXIS marker cannot be smuggled onto a metric or a sort
expression. The collapsed value must still match something stored on the
chart, so tagging an unrelated column or free-form SQL as BASE_AXIS grants no
additional access.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/review |
Code Review Agent Run #6a6ae9Actionable Suggestions - 0Review 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 6.2 #42847 +/- ##
======================================
Coverage ? 64.33%
======================================
Files ? 2554
Lines ? 132908
Branches ? 30792
======================================
Hits ? 85506
Misses ? 45932
Partials ? 1470
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Verifying the previous commit end to end against a real embedded dashboard surfaced a second rejection the unit tests missed. A timeseries chart with an x-axis stores its remaining dimensions under `groupby` and leaves `columns` unset, while `normalizeTimeColumn` sends them in `columns` together with the synthesized axis. Comparing `columns` against only `params_dict["columns"]` therefore still rejected the legitimate load, on the groupby dimension rather than on the axis. `columns` and `groupby` are already treated as equivalent when reading the stored query context, for exactly this reason. Apply the same equivalence to the stored params. The union only adds dimensions the chart already renders, and an extra column is still rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
SUMMARY
On
6.2, a guest loading a chart that has an x-axis gets a 403 —POST /api/v1/chart/datareturnsGuest user cannot modify chart payloadand the chart renders as an error tile. The same chart works for Admin.Before querying,
normalizeTimeColumn(normalizeTimeColumn.ts) rewrites the chart's x-axis into a synthetic column, so the request carries:{"columnType": "BASE_AXIS", "isColumnReference": true, "sqlExpression": "order_date", "label": "order_date", "expressionType": "SQL", "timeGrain": "P1D"}while the chart stores the bare string
"order_date". The anti-tamper guardquery_context_modified()compares withfreeze_value(exactjson.dumps), and two independent mismatches each produce the 403:x_axiscontrol, which the guard never reads. Itsstored_valuescome only fromparams_dict["metrics" | "columns" | "groupby" | "orderby"], so even the un-synthesized string would not be found.Fixing either alone changes nothing; both have to be addressed.
The fix
denormalize_base_axis_column()collapses a synthesizedBASE_AXIScolumn back to the reference it stands for — a physical axis to its column name, an adhoc axis to the underlying adhoc column without the synthetic markers — and the storedx_axiscontrol is read as an accepted column value.Both apply to
columns/groupbyonly.metricsandorderbykeep exact comparison, so aBASE_AXISmarker cannot be smuggled onto a metric or a sort expression; those two comparisons are behaviorally identical to6.2.Scope note
This is a 6.2-targeted fix, not a backport of the
#42150chain. That chain reaches the same outcome via seven commits across three production files (~4,000 lines); of those, only theBASE_AXISunwrap and thex_axisread are load-bearing for this 403. This PR is the minimal change: one production file, +104/−30 lines.Deliberately excluded, none of which this 403 requires:
#39197(user_view_menu_namesguest branch) — on plain6.2this is what makes the guard run at all: guests haveis_anonymous = Falsebut noab_userrow, soChartFilterhides the slice,query_context.slice_isNone, and the guard returns early. A deployment where the guard is dormant cannot be hitting this 403; the ones that are already bypassChartFilterviaall_datasource_access. It broadens guest permission resolution across every caller and wants its own PR.#40979native-filter target validation — independent hardening; replaces the chartless early-out and adds aDashboard.json_metadatalookup inside the guard.#37371sort-by-visible-columns — the reasonmodels/helpers.pyandconnectors/sqla/models.pyappear in the chain, adding a newQueryObjectValidationErrorpath for all users, not just guests.granularity_sqlaas a stored column source —normalizeTimeColumnonly fires whenx_axisis set (isXAxisSet), so it cannot produce this payload.except (ValueError, TypeError)widening in_validate_child_in_parent_multilayer.Security posture
Safe by construction: a collapsed value must still be a member of the set stored on the chart, so tagging an unrelated column or free-form SQL as
BASE_AXISgrants no additional access. The unwrap is narrow — columns only,sqlExpressionmust be astr, and the physical-reference branch requiresisColumnReference.Each of the six new tests was verified against a reverted production file: the three legitimate-load cases fail without the fix and pass with it; the three tamper vectors are rejected both before and after, as is the pre-existing
random()-in-orderbytest...._physical_no_stored_context..._adhoc_no_stored_context..._with_stored_context..._forged_column_referenceBASE_AXIStagging an unrelated column..._forged_adhoc_expressionBASE_AXIS..._non_string_sql_expressionstrsqlExpression..._smuggled_into_metricsOne note on affected charts
The bug is not limited to charts whose
query_contextis NULL.form_datais compared againstparams_dictbefore the storedquery_contextis consulted, so an already-normalized stored context cannot rescue the comparison — some Explore-saved charts are affected too. Re-saving in Explore can still mask the symptom, which is why it looks intermittent from one chart to the next.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
End to end, on an embedded dashboard:
all_datasource_access, then replayPOST /api/v1/chart/datawith the guest token plus an extra column, orrandom()inorderby, and confirm 403. Without this the guard silently no-ops on6.2(see#39197above) and every payload returns 200 — a green result that proves nothing.SELECT id FROM slices WHERE query_context IS NULL AND params LIKE '%x_axis%'. Do not re-save it in Explore first — that repopulatesquery_contextand can hide the bug.Guest user cannot modify chart payload. After: the chart renders.ADDITIONAL INFORMATION
EMBEDDED_SUPERSET