Repository navigation
fix(embedded): tighten guest-token scoping for annotations, result types, and channels - #43391
Conversation
Code Review Agent Run #3432a2Actionable 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 |
|
The flagged issue is correct. Deriving the channel ID from the entire guest token, including To resolve this, you should derive the channel ID from a stable claim that persists across token refreshes, such as a unique session identifier or a user-specific claim that remains constant for the duration of the embedded session. You should exclude volatile claims like I have checked the PR comments, and there are no other actionable suggestions in the provided context. Would you like me to implement a fix for this issue? superset/async_events/async_query_manager.py |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43391 +/- ##
=======================================
Coverage 80.65% 80.65%
=======================================
Files 2942 2942
Lines 175692 175730 +38
Branches 40788 40797 +9
=======================================
+ Hits 141705 141740 +35
- Misses 31325 31327 +2
- Partials 2662 2663 +1
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:
|
Code Review Agent Run #6a5989Actionable 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 |
Code Review Agent Run #d4f446Actionable 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 |
Code Review Agent Run #adfcf1Actionable 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 |
ce5e707 to
54d42c9
Compare
Code Review Agent Run #95e833Actionable 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 |
cffbb36 to
39b8693
Compare
Code Review Agent Run #d11154Actionable 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 |
Code Review Agent Run #cf2b35Actionable 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 |
query_context_modified() compared columns/metrics/series-limit/order-by against a chart's stored query context but never inspected annotation_layers, which is accepted on any query object and resolved by AnnotationLayerDAO.find_by_ids with no per-object restriction. Add _annotation_layers_modified(), matching the existing subset-comparator pattern: the union of annotation layers in form_data and every query object must already appear (by sourceType/value identity) on the stored chart's params or saved query_context. Replaying a chart's own layers still passes; referencing any other layer now fails the guest tamper check.
query_context_modified() compared columns/metrics/order-by against the stored chart but never compared result_type, and the samples/drill_detail preparers in query_actions.py rewrite an accepted query to every column on the datasource (dropping metrics/post_processing) after that check has already passed. Add _result_type_modified(), checking the request's top-level and per-query result_type (samples or drill_detail) against the stored chart's own saved query context; unless the chart itself uses one of those result types, requesting it now fails the guest tamper check.
…t auth The Referer and Sec-Fetch-Dest checks on the pre-token embed page are browser-cooperation only; a non-browser client can forge or omit both headers, so anything the page renders is effectively reachable by knowing an embed UUID. Stop passing the dashboard title and description into the template at this stage. The embedded SPA already fetches dashboard metadata through the guest-token-authenticated API once it holds a token, so the pre-auth page falls back to a generic title.
…d query, not the flattened set _result_type_modified previously unioned every stored query's result_type into one set and checked requested result types against that set, so a result type stored on one query (e.g. samples) could be requested for a different query in the same query context. Compare each requested query's effective result type (its own result_type, falling back to the query context's top-level result_type - mirroring how the value is resolved at render time) against the corresponding stored query at the same position instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…uests The samples/drill_detail preparers replace a query's column list with every column on the datasource, bypassing the native-filter target-column allowlist entirely. Reject those result types outright on the chartless native-filter path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s requests Drill By (slice_id sentinel 0 plus a source chart_id) is only supposed to read the chart's configured drillable dimensions, not the whole datasource. The samples/drill_detail preparers ignore groupby entirely and return every column, bypassing that allowlist. Drill to Detail (no slice_id/chart_id at all) is unaffected since it's already meant to expose a full dataset attached to the dashboard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y_context_modified Rebasing onto master pulled in the JSONDecodeError branch, which pushed query_context_modified past ruff's C901 threshold. Collapse the six comparator/warn/return blocks into one loop over (comparator, reason) pairs; behavior and log messages are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ybdwS1GbAy3Pkt6YGKCQp
… chartless guards The Drill By and native-filter guards carried the same any(...) expression over _ROW_EXPANDING_RESULT_TYPES; fold it into _any_row_expanding_result_type so the set and effective-type logic have one call site to keep in sync. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ybdwS1GbAy3Pkt6YGKCQp
…t params A params-only chart update can drop an annotation layer without refreshing the chart's cached query_context, so authorizing a requested layer whenever it appeared in either the current params or that stale snapshot let an embedded guest keep requesting a layer that had since been removed from the chart. Check the requested layer against params only. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f23dc32 to
1d786fa
Compare
Code Review Agent Run #b2fabaActionable Suggestions - 0Additional Suggestions - 4
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 |
_result_type_modified accessed query.result_type directly while its sibling _any_row_expanding_result_type already used getattr, raising AttributeError whenever a query object lacks the attribute. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #78b726Actionable 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 |
The default-result-type read in _any_row_expanding_result_type and _result_type_modified accessed query_context.result_type directly, raising AttributeError on any QueryContext-like object without that attribute set (e.g. a test double), unlike the per-query result_type read next to it which already used getattr. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #d1bc6eActionable 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 |
SUMMARY
annotation_layersandresult_typeagainst the stored chart's query context, closing gaps where a request could reference an annotation layer or result type (e.g.samples/drill_detail) the stored chart didn't itself use.TESTING INSTRUCTIONS
pytest tests/unit_tests/security/manager_test.py tests/unit_tests/async_events/ tests/integration_tests/embedded/test_view.py— new/extended tests per change, each verified to fail pre-fix and pass post-fix.ADDITIONAL INFORMATION