fix: Drill to Detail for Embedded#39214
Conversation
Code Review Agent Run #7a943aActionable 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 @@
## master #39214 +/- ##
=======================================
Coverage 64.38% 64.38%
=======================================
Files 2550 2550
Lines 132141 132184 +43
Branches 30658 30661 +3
=======================================
+ Hits 85074 85103 +29
- Misses 45584 45595 +11
- Partials 1483 1486 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| .filter(Slice.id == parent_id) | ||
| .one_or_none() | ||
| ) | ||
| and parent_slc in dashboard_.slices |
There was a problem hiding this comment.
Suggestion: The multi-layer child access path validates parent_slice_id and child membership in deck_slices, but it never verifies that the requested datasource actually belongs to that child chart. Because datasource authorization is granted when this branch is true, a forged request can pair a valid parent/child chart combination with an unrelated datasource and bypass datasource access checks. Add an explicit lookup of the child slice and ensure its datasource matches the requested datasource before granting access. [security]
Severity Level: Critical 🚨
- ❌ Embedded guests query unauthorized datasets via ChartDataRestApi.data endpoint.
- ❌ Dashboard RBAC users bypass datasource checks for deck_multi children.
- ⚠️ Inconsistent child datasource validation in SupersetSecurityManager.raise_for_access.| and parent_slc in dashboard_.slices | |
| and ( | |
| child_slc := self.session.query(Slice) | |
| .filter(Slice.id == slice_id) | |
| .one_or_none() | |
| ) | |
| and parent_slc in dashboard_.slices | |
| and child_slc.datasource == datasource |
Steps of Reproduction ✅
1. Configure a dashboard with roles or embedded access so that either DASHBOARD_RBAC or
EMBEDDED_SUPERSET is enabled and used (checked in
`SupersetSecurityManager.raise_for_access` at `superset/security/manager.py:162-179` and
`:269-271` via `is_feature_enabled("DASHBOARD_RBAC")` /
`is_feature_enabled("EMBEDDED_SUPERSET")`).
2. On that dashboard, add a `deck_multi` chart whose parent slice has a `deck_slices`
configuration including a child chart ID (validated by
`_validate_child_in_parent_multilayer` at `superset/security/manager.py:702-724`),
ensuring the child chart uses dataset A; also create another dataset B that the same
user/guest does NOT have `datasource_access` or schema access to.
3. As an embedded guest user (feature flag `EMBEDDED_SUPERSET`) or a RBAC viewer of that
dashboard, send a POST request to `ChartDataRestApi.data` at `/api/v1/chart/data`
(`superset/charts/data/api.py:280-99`) with a JSON body that `ChartDataQueryContextSchema`
accepts, where `datasource` points to dataset B (e.g. `{"id": <B_id>, "type": "table"}`)
and `form_data` contains `{"dashboardId": <dashboard_id>, "type": "DRILL_DETAIL",
"slice_id": <child_slice_id>, "parent_slice_id": <parent_slice_id>, ...}` so that
`slice_id` is the valid child from the parent's `deck_slices`.
4. The request flows through `ChartDataCommand.validate`
(`superset/commands/chart/data/get_data_command.py:39-40`) which calls
`QueryContext.raise_for_access` (`superset/common/query_context.py:18`), which delegates
to `QueryContextProcessor.raise_for_access` and then
`security_manager.raise_for_access(query_context=self._query_context)`
(`superset/common/query_context_processor.py:525-537);` inside
`SupersetSecurityManager.raise_for_access` (`superset/security/manager.py:145-180` and
`:2445-2647`), the user fails the normal schema/datasource/ownership checks, but passes
the multi-layer child branch at lines `2644-35` because `parent_slc` is in
`dashboard_.slices` and `_validate_child_in_parent_multilayer` returns True, and there is
currently no `child_slc.datasource == datasource` validation—so no
`SupersetSecurityException` is raised and the query executes against dataset B, returning
unauthorized data.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 2672:2672
**Comment:**
*Security: The multi-layer child access path validates `parent_slice_id` and child membership in `deck_slices`, but it never verifies that the requested datasource actually belongs to that child chart. Because datasource authorization is granted when this branch is true, a forged request can pair a valid parent/child chart combination with an unrelated datasource and bypass datasource access checks. Add an explicit lookup of the child slice and ensure its datasource matches the requested datasource before granting access.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
this is outside the scope of this PR. I'll tag @msyavuz here as he originally worked on this logic. Let me know if this is a concern, and if I can help with anything
There was a problem hiding this comment.
The child slice is validated against the parent's config via _validate_child_in_parent_multilayer this ensures the slice_id is actually a child of the parent_slice_id in the deck_multi chart's deck_slices param. This is a false positive
There was a problem hiding this comment.
thanks for confirming it!
| ) | ||
| or ( | ||
| # Chart. | ||
| form_data.get("type") != "NATIVE_FILTER" |
There was a problem hiding this comment.
The only real changes to this block are:
- I noticed we were validating
form_data.get("type") != "NATIVE_FILTER"more than once, so I moved it to the top for single validation; - Re-named the method from
has_drill_by_accesstohas_drill_access.
I can undo #1 if we think it's better
Code Review Agent Run #3b26c4Actionable 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 |
| slc := self.session.query(Slice) | ||
| .filter(Slice.id == slice_id) | ||
| .one_or_none() | ||
| # Direct chart access (no parent) |
There was a problem hiding this comment.
holly molly, maybe it's time to refactor this method ...
There was a problem hiding this comment.
I'll try tackling that in a follow up. I believe the logic on its own is correct, but we could move each validation block to their own helper method (like drill validation is now)
`not form_data.get("slice_id")` is True for both absent keys (D2D)
and `0` (Drill By sentinel), which could cause a malformed Drill By
request to be evaluated under the more permissive D2D path. Using
`is None` makes the intent explicit.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mistercrunch
left a comment
mistercrunch
left a comment
There was a problem hiding this comment.
Reviewed the full diff — this correctly fixes D2D for embedded users by:
- Broadening
has_drill_by_access→has_drill_accessto handle both Drill to Detail and Drill By - Plumbing
dashboardIdthrough/samples→get_samples→QueryContextFactorysoform_datacarries the dashboard context needed for embedded access validation - Properly nesting the drill access check under the
type != "NATIVE_FILTER"branch (previously it was a top-levelorthat could theoretically fire for native filter requests)
Test coverage is thorough — both happy paths and negative cases for D2D and Drill By.
I pushed one small fix in 85ac6f7: changed not form_data.get("slice_id") to form_data.get("slice_id") is None (same for chart_id) in has_drill_access. The not check treats 0 (Drill By sentinel) and absent/None (D2D) identically, which could let a malformed Drill By request (slice_id=0, no chart_id) take the more permissive D2D path. Using is None makes the intent explicit.
One suggestion for a follow-up: the boolean expression in raise_for_access (lines ~2589–2690) is now 10+ levels deep with walrus operators. Extracting each access path into named helpers (_check_native_filter_access, _check_chart_access, _check_drill_access, etc.) would make future security reviews much more tractable.
Nice work @Vitor-Avila 👍
|
Bito Automatic Review Skipped – PR Already Merged |
Co-authored-by: Maxime Beauchemin <maximebeauchemin@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit c7955a3)
Co-authored-by: Maxime Beauchemin <maximebeauchemin@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
🍒 6.1.0
5 participants
SUMMARY
#34319 added support for D2D operations (Drill to Detail and Drill By) to Embedded users. This flow was broken after this security fix: #36550.
This PR is fixing this flow, ensuring that required permission validations are applied for Embedded requests. Test coverage was also improved.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
Test coverage added. For manual testing:
can sample on Datasourceperm, as well as other required perms for D2D actions.ADDITIONAL INFORMATION