fix(mcp): harden the embedded-guest chart data-read path - #43066
fix(mcp): harden the embedded-guest chart data-read path#43066gabotorresruiz wants to merge 1 commit into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
393151d to
c695220
Compare
c695220 to
cc2c4af
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43066 +/- ##
==========================================
- Coverage 66.67% 57.24% -9.43%
==========================================
Files 2866 2866
Lines 162798 162777 -21
Branches 37507 37504 -3
==========================================
- Hits 108546 93185 -15361
- Misses 52130 68708 +16578
+ Partials 2122 884 -1238
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:
|
dosubot
Bot
added
authentication:access-control
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Automated multi-reviewer pass (correctness + adversarial security + test coverage). Posting the one blocker and the medium items. The security model itself held up — no boundary break, and the guest filter refactor actually narrows scope. Lower-severity nits omitted here.
The blocker was verified end-to-end against validate_chart_dataset → DatasetDAO.find_by_id → DatasourceFilter (no guest carve-out), cross-checked against the sibling get_chart_info, which still skips the pre-check for guests.
cc2c4af to
a0f9872
Compare
|
@rebenitez1802 Thanks for the thorough pass, genuinely useful. Fixed the blocker at the root in |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
69ad36a to
cad8a7b
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a1a7e2
Actionable Suggestions - 1
-
superset/dashboards/filters.py - 1
- Dead method post-refactor · Line 204-241
Additional Suggestions - 1
-
tests/unit_tests/subjects/test_filters.py - 1
-
Redundant inline import · Line 77-77This import is redundant — `DashboardAccessFilter` is already imported at the module level (lines 50, 63). Per rule [12745], inline imports should only be used when a circular dependency exists.
-
Review Details
-
Files reviewed - 13 · Commit Range:
cad8a7b..cad8a7b- superset/daos/dashboard.py
- superset/dashboards/filters.py
- superset/mcp_service/auth.py
- superset/mcp_service/chart/chart_utils.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- superset/mcp_service/mcp_config.py
- superset/utils/filters.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/test_guest_token_auth.py
- tests/unit_tests/subjects/test_filters.py
- tests/unit_tests/utils/filters_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
LGTM once CI is green
cad8a7b to
009022e
Compare
009022e to
e85ed21
Compare
Code Review Agent Run #fbd25fActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
e85ed21 to
9e40ff5
Compare
9e40ff5 to
efaca68
Compare
Follow-up to the embedded-guest MCP data-read work (apache#41753): a batch of lower-severity correctness, robustness, and coverage gaps deferred from that review. None change behavior for non-guest principals, and none is a data bypass — guest chart data stays gated by raise_for_access on every path. - Route a dashboard slug resource id to Dashboard.slug instead of binding it to the integer id column (avoids a cast error/500 on strict backends), in the shared guest_embedded_dashboard_filter. - Make the dashboard-list guest filter reuse guest_embedded_dashboard_filter so it matches the chart-list filter (both OR uuid/int/slug ids); a mixed-id token no longer hides int-id dashboards. Removes the duplicated branch. - Coerce row_limit to int on the using_unsaved_state and _query_from_form_data paths of get_chart_data (a string row_limit could 500 in apply_max_row_limit). - Deny a guest cleanly on the form_data_key-only path (the unsaved-chart cache is not scoped to the token's dashboards) instead of failing opaquely. - Run the dataset existence pre-check for guests with check_access=False so a deleted dataset returns the clean DatasetNotAccessible contract rather than a raw error (get_chart_data and get_chart_preview). - Forward MCP_RESTRICTED_TOOL_POLICY through get_mcp_config so a policy set at the mcp_config layer is not dropped in standalone MCP mode. - Derive the auth.py guest allow-list from the single mcp_config source instead of a duplicated literal; drop the now-redundant sync test. - Add tool-level guest tests for get_chart_data (dashboard-pinned query, existence-only dataset check, out-of-scope denial, form_data_key denial).
efaca68 to
63b3c18
Compare
3 participants
SUMMARY
Follow-up to the embedded-guest MCP data-read work (#41753). The max-effort review of that PR fixed the high-severity findings in-line and deferred a batch of lower-severity correctness, robustness, and coverage gaps; this PR clears that batch.
Two things up front, because these are intentionally small:
raise_for_accesson every path. These are about not returning a raw 500/opaque error and about internal consistency, not about what a guest can access.Grouped by area:
Guest dashboard scoping (
utils/filters.py,dashboards/filters.py)Dashboard.getaccepts it), but it was routed toDashboard.id.in_([slug]), which raises a cast error/500 on strict backends. It now routes toDashboard.slug.guest_embedded_dashboard_filter, so visibility is consistent and the duplicate branch is gone.get_chart_data/get_chart_previewrobustnessrow_limitcan arrive as a string (cached form_data). The no-context path already coerced it; theusing_unsaved_stateand_query_from_form_datapaths did not, and a string could 500 inapply_max_row_limit. Now coerced on all paths.form_data_key-only path had no guest handling: the unsaved-chart cache is not scoped to a guest's dashboards, so a guest failed opaquely downstream. Guests are now denied cleanly there.DatasetNotAccessiblecontract. It now runs for guests withcheck_access=False— existence check kept (clean error), RBAC access check skipped (guests are authorized via the dashboard context, not dataset RBAC).MCP config / cleanup (
mcp_config.py,auth.py)get_mcp_configdid not forwardMCP_RESTRICTED_TOOL_POLICY, so a policy set at themcp_configlayer was dropped in standalone MCP mode. Now forwarded.auth.pynow derives it from the singlemcp_configsource; the now-redundant sync test is removed.Tests
get_chart_data(the highest-value guest tool): the data query is pinned to the token's dashboard, the dataset check runs existence-only for guests, an out-of-scope chart is denied, and theform_data_key-only path is denied.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend/MCP correctness).
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py tests/unit_tests/mcp_service/test_guest_token_auth.py tests/unit_tests/mcp_service/test_guest_scope.pypytest tests/unit_tests/utils/filters_test.py tests/unit_tests/dashboards/ruffandruff formatclean.ADDITIONAL INFORMATION