fix(embedded): tighten guest-token scoping for annotations, result types, and channels - #43391
fix(embedded): tighten guest-token scoping for annotations, result types, and channels#43391rusackas wants to merge 6 commits into
Conversation
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.
get_guest_user_channel_id() HMACed an enumerated subset of guest-token claims (user/resources/iat/exp/aud/datasets/rev), omitting rls_rules. Two guest tokens differing only in their RLS clauses -- the documented pattern for embedding one dashboard across multiple tenants -- could derive the same async channel and collide on job-status polling and cancellation. HMAC the complete decoded claim set instead of an enumerated subset, so any claim difference (including future ones) yields a distinct channel.
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 AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
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. |
…ed447e8f5a7bf # Conflicts: # superset/security/manager.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43391 +/- ##
==========================================
- Coverage 66.81% 66.78% -0.04%
==========================================
Files 2876 2876
Lines 164237 164451 +214
Branches 37917 37956 +39
==========================================
+ Hits 109743 109828 +85
- Misses 52310 52424 +114
- Partials 2184 2199 +15
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:
|
rusackas
added
the
merge-if-green
…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>
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 AI Code Review powered by |
2 participants
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