feat(mcp): scope embedded-guest data reads to the token's dashboards#41753
Conversation
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
1c19f36 to
07da628
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41753 +/- ##
==========================================
- Coverage 64.60% 64.57% -0.04%
==========================================
Files 2712 2713 +1
Lines 151168 151269 +101
Branches 34769 34793 +24
==========================================
+ Hits 97665 97677 +12
- Misses 51680 51764 +84
- Partials 1823 1828 +5
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:
|
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
07da628 to
00837c5
Compare
dosubot
Bot
added
authentication
|
| Language | Invalidated translations |
|---|---|
fr |
89 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (fr):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
00837c5 to
6fa7814
Compare
Code Review Agent Run #bea91aActionable Suggestions - 0Additional Suggestions - 2
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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Security-focused review of the embedded-guest chart-data scoping mechanism (get_chart_data/get_chart_info/get_chart_preview + ChartFilter/guest_scope.py). Not approving — security-boundary PR, approval held for a human.
Design verdict for the 3 tools this PR touches: sound. Verified (not assumed) that resolution is WHERE-scoped at the SQL layer — ChartDAO.base_filter = ChartFilter, and BaseDAO.find_by_id/_find_by_column always apply base_filter unless explicitly skipped (find_chart_by_identifier never skips it) — so an out-of-scope chart never resolves for a guest; it isn't a fetch-then-drop pattern. The guest_scope.authorize_query() dashboard-context hand-off is correct defense-in-depth on top of that: core's raise_for_access() guest branch (superset/security/manager.py) re-derives the dashboard from form_data["dashboardId"] and re-validates via can_access_dashboard() → has_guest_access(), so a forged/stale dashboardId can't grant access to a dashboard outside the token. Non-guest behavior confirmed unaffected in all 4 touched files (each guest branch short-circuits via is_guest_user()/get_current_guest_user_if_guest() returning falsy for real users).
HIGH — gate completeness gap outside this PR's 3 tools (no diff line; files not touched by this PR). superset/mcp_service/dataset/tool/query_dataset.py, get_dataset_info.py, and list_datasets.py resolve via DatasetDAO (base_filter = DatasourceFilter) and never touch guest_scope or attach dashboardId/slice_id. They currently deny guests only incidentally — DatasourceFilter falls through to schema_perm/catalog_perm RBAC matching, which an ephemeral guest role happens to match nothing against — not via any explicit guest-dashboard check. query_dataset.py additionally logs the dataset name and diffs requested columns/metrics against the real schema before any raise_for_access call, so if resolution ever succeeds for a guest (a guest/Public role with schema-level grants is a realistic embedded-analytics config, or any future loosening of DatasourceFilter), this tool discloses schema ahead of the auth check this PR's design otherwise routes everything through. Recommend applying the same guest_scope pattern (or an explicit is_guest_user() guard returning a clean 403) to these three tools before/alongside #41003 landing. (list_charts.py is fine — it goes through ChartDAO/ChartFilter and is fixed by this PR as a side effect.)
MEDIUM — get_chart_sql.py wasn't given the same treatment (superset/mcp_service/chart/tool/get_chart_sql.py:413, not touched by this PR). Chart resolution there is already guest-scoped via the same ChartFilter, but it still unconditionally calls validate_chart_dataset(chart, check_access=True) — the exact dataset-RBAC pre-check this PR bypasses for guests elsewhere. A guest requesting SQL for a chart genuinely on their dashboard gets DatasetNotAccessible instead. Fails closed (no leak), but looks like an oversight vs. an intentional exclusion.
Line-specific items inline below. Scan coverage: manual systematic pass (injection/authz-bypass/error-oracle/DRY/test-coverage/secrets/boolean-traps) across all 6 non-test changed files plus the unchanged core methods this PR depends on and the other 14 guest-reachable read tools under superset/mcp_service/**/tool/.
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
6fa7814 to
87a8a18
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
87a8a18 to
ec8d907
Compare
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
2 times, most recently
from
497e171 to
0e932c4
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 2 re-review at HEAD 0e932c41 (rebased/force-pushed since the last review at 6fa78143). CI green. Not approving — security-boundary PR, sign-off held for a human.
Resolved from round 1 (verified, not just re-read):
- The
ChartFilterguest branch now has direct test coverage — newtests/unit_tests/charts/test_filters.pyexercisesChartFilter.apply()itself (not just the leaf helpers): confirms the guest branch is taken and short-circuits before the datasource-access join, confirms non-guests reach the original join path, confirms admin/can_access_all_datasourcesbypasses everything. Deleting the guest branch now fails a real test. - The "skip dataset RBAC pre-check for guests" predicate, previously reimplemented 3 different ways, is now a single
guest_scope.is_guest_read()helper used consistently acrossget_chart_data.py,get_chart_info.py, andget_chart_preview.py. Traced through: this only controls a redundant pre-check skip, not the actual gate — the real authorization (raise_for_accessviaauthorize_query()/command.validate()) is unchanged and still keyed off the per-chartguest_dashboard_id(chart)value, so this simplification doesn't weaken anything. - The
row_limitint-coercion is now a named_coerce_row_limit()helper with a dedicated parametrized test (6 cases). No longer scope creep — it's a tested utility.
Still open from round 1 (unaddressed by this push — files untouched):
- HIGH:
query_dataset.py/get_dataset_info.py/list_datasets.pyundersuperset/mcp_service/dataset/tool/still have no guest-aware scoping (confirmed — no reference toguest_scope,is_guest_user, orraise_for_accesscontext-attachment in any of them at this HEAD). - MEDIUM:
get_chart_sql.pystill unconditionally callsvalidate_chart_dataset(chart, check_access=True)(line 414) without theguest_scope.is_guest_read()skip the other three chart tools now share — an embedded guest still can't read SQL for a chart genuinely on their own dashboard.
New, independently confirmed: codeant's finding on superset/charts/filters.py (mixed UUID + integer dashboard IDs in a guest token push non-UUID values into EmbeddedDashboard.uuid.in_(...), a UUID-typed column) is correct — traced guest_embedded_dashboard_filter() and confirmed it routes the entire mixed ids list through the UUID branch whenever any single ID looks like a UUID. That's a real runtime bind/type-conversion error for a mixed-ID guest token (fails closed/500, not a leak, but a genuine availability bug during the uuid-migration window). Already an open thread from codeant — not re-filing, just corroborating with attribution so it doesn't get lost.
![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 #341acc
Actionable Suggestions - 2
-
superset/mcp_service/chart/tool/get_chart_info.py - 1
- Missing guest-skip test coverage · Line 109-115
-
superset/mcp_service/chart/tool/get_chart_preview.py - 1
- Missing guest authorization tests · Line 303-303
Review Details
-
Files reviewed - 10 · Commit Range:
0e932c4..0e932c4- superset/charts/filters.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_info.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- superset/mcp_service/guest_scope.py
- superset/utils/filters.py
- tests/unit_tests/charts/test_filters.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/test_guest_scope.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
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
0e932c4 to
1d5ed2e
Compare
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
94ff233 to
4a42d82
Compare
There was a problem hiding this comment.
Code Review Agent Run #0a1301
Actionable Suggestions - 3
-
superset/mcp_service/guest_scope.py - 2
- Replace Any type annotation in guest_dashboard_id · Line 44-44
- Replace Any type annotations in authorize_query · Line 57-57
-
superset/utils/filters.py - 1
- Imprecise return type annotation · Line 46-46
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/chart/tool/get_chart_data.py - 1
- CWE-285: Missing guest RBAC test coverage · Line 398-430
-
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py - 2
- Missing async test return type · Line 614-614
- Mock variable missing type annotation · Line 619-619
Review Details
-
Files reviewed - 20 · Commit Range:
4a42d82..4a42d82- superset/charts/filters.py
- superset/mcp_service/auth.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_info.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- superset/mcp_service/chart/tool/get_chart_sql.py
- superset/mcp_service/guest_scope.py
- superset/mcp_service/mcp_config.py
- superset/mcp_service/rls/tool/get_rls_filter_info.py
- superset/mcp_service/rls/tool/list_rls_filters.py
- superset/utils/filters.py
- tests/unit_tests/charts/test_filters.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
- tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py
- tests/unit_tests/mcp_service/test_guest_scope.py
- tests/unit_tests/mcp_service/test_guest_token_auth.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
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 4 re-review at HEAD 4a42d828 (rebased again, and this push is substantially larger — 10 → 20 files, +561/-33 → +736/-42). CI green. Not approving — security-boundary PR, sign-off held for a human.
Scope note: this push pulls in real machinery from #41003 (superset/mcp_service/auth.py, mcp_config.py — the MCP_GUEST_DENIED_TOOLS deny-list + check_tool_permission's class_permission_name RBAC gate) that wasn't part of my earlier rounds. I read it in full this round rather than treating it as out-of-scope background, since it materially changes the answer to "is a given MCP tool actually guest-reachable."
Resolved this round:
- MEDIUM from round 1 (
get_chart_sql.pynot guest-scoped) — now explicitly denies guests (if security_manager.is_guest_user(): return ChartError(..., error_type="Forbidden")), matching the same reasoning asget_dataset_info("exposes tables/columns/joins"). Simpler than scoping, and safe. Backed by a real end-to-end test (test_guest_denied, via an actualClient(mcp_server).call_tool(...)). - Two new RLS tools (
get_rls_filter_info.py,list_rls_filters.py) are explicitly guest-denied the same way, each with an end-to-end test. - The default guest tool deny-list (
_DEFAULT_GUEST_DENIED_TOOLS/MCP_GUEST_DENIED_TOOLS) grew from 2 to 6 entries (get_role_info,get_user_info,list_roles,list_usersadded), with a new test asserting the two lists stay in sync and a parametrized test confirming every entry is actually blocked. - Both NITs bito raised last round (missing direct tests for the
is_guest_read()/guest_dashboard_id()skips inget_chart_info.pyandget_chart_preview.py) are now covered by dedicated tests.
This is a genuinely systematic hardening pass across the guest-reachable tool surface — good pattern, applied consistently 3 times this round alone.
Still open, and now more urgent given what I learned about the gate architecture — HIGH: query_dataset.py / get_dataset_info.py / list_datasets.py remain the only unclosed items in this sweep. All three declare class_permission_name="Dataset", so their only guest gate is check_tool_permission's generic RBAC check: security_manager.can_access("can_read", "Dataset") against whatever role GuestUser.roles resolves to — which is GUEST_ROLE_NAME (default "Public", per superset/config.py). If PUBLIC_ROLE_LIKE is configured to grant that role broad read access — a common, arguably expected pattern for embedded-analytics deployments, since without it Public/guest gets zero menu access — this RBAC check passes, the tool body runs, and a guest can read any dataset by id, completely unscoped to their token's dashboards. That defeats this PR's stated purpose. Given the exact fix pattern was just applied 3 times in this same push (inline is_guest_user() → Forbidden, or add the 3 tool names to MCP_GUEST_DENIED_TOOLS), closing this should be quick — it's the last gap in an otherwise complete pass, not a new design problem.
Still escalating to Amin for final sign-off regardless of how this resolves, per this author's established MCP-PR pattern.
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
4a42d82 to
1c3e66d
Compare
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
1c3e66d to
1819c18
Compare
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 5 re-review at HEAD 1819c18b (10→27 files across rounds, +736/-42→+1030/-98). CI green, no new bot/reviewer activity since round 4. Not approving — security-boundary PR, sign-off held for a human (policy unchanged regardless of how clean this is).
The round-4 HIGH is resolved, and resolved better than what I asked for. Rather than adding query_dataset/get_dataset_info/list_datasets to a per-tool deny-list, this round flips the entire guest gate from deny-list to default-deny allow-list: MCP_GUEST_ALLOWED_TOOLS (auth.py's _tool_denied_for_principal + mcp_config.py) now permits a guest to call only get_dashboard_info / get_dashboard_layout / list_dashboards / list_charts / get_chart_info / get_chart_data / get_chart_preview — everything else, including the 3 dataset tools and any tool added in the future, is denied unless explicitly allow-listed. This structurally closes the class of bug ("a new/overlooked tool falls open to guests") rather than just the 3 instances I named. Verified with a parametrized test (test_non_allow_listed_tools_denied_to_guest) that explicitly covers get_dataset_info, list_datasets, query_dataset, plus 19 other tools across data-model/mutating/enumeration categories — this is exactly the regression test I'd have asked for.
Two more things I'd flagged (one as an open HIGH's underlying risk, one from round 3) are also independently closed, at the right layer:
superset/mcp_service/privacy.py'suser_can_view_data_model_metadata()now denies guests unconditionally, before the RBACcan_accesscheck — with a comment stating the exact scenario I raised in round 4 ("a PUBLIC_ROLE_LIKE=Gamma guest carries can_get_drill_info on Dataset, which would otherwise pass"). Backed by a test (test_user_can_view_data_model_metadata_denies_guest) that mockscan_accessto returnTrueand asserts the guest is still denied — proving the short-circuit actually overrides a permissive RBAC grant, not just that it runs first.superset/security/manager.py'svalidate_guest_token_resources()now rejects a raw integer dashboard id at token issuance if that dashboard isn't actually embedded (elif not dashboard.embedded: raise EmbeddedDashboardNotFoundError()). This closes the round-3 finding (int-id branch not checking.embedded, mirrored from the pre-existingDashboardAccessFilter) at its root — a guest token can no longer be minted against a non-embedded dashboard via the int-id path in the first place, which is a better fix than patching every read site individually.
I don't have any open findings from my side at this point across all 5 rounds. Escalating to Amin for final sign-off per this author's established MCP-security-PR policy — not a reflection on this round's quality, just standing policy for this class of change.
There was a problem hiding this comment.
Code Review Agent Run #721a05
Actionable Suggestions - 1
-
superset/mcp_service/chart/tool/get_chart_data.py - 1
- Missing guest scope integration test · Line 414-430
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/security/manager_test.py - 1
- Inline imports in test function · Line 2896-2899
-
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py - 1
- Duplicate module-level import · Line 1039-1039
Review Details
-
Files reviewed - 26 · Commit Range:
1819c18..1819c18- superset/charts/filters.py
- superset/mcp_service/auth.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_info.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- superset/mcp_service/chart/tool/get_chart_sql.py
- superset/mcp_service/guest_scope.py
- superset/mcp_service/mcp_config.py
- superset/mcp_service/privacy.py
- superset/mcp_service/rls/tool/get_rls_filter_info.py
- superset/mcp_service/rls/tool/list_rls_filters.py
- superset/security/manager.py
- superset/utils/filters.py
- tests/unit_tests/charts/test_filters.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
- tests/unit_tests/mcp_service/chart/tool/test_list_charts.py
- tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py
- tests/unit_tests/mcp_service/system/tool/test_get_current_user.py
- tests/unit_tests/mcp_service/test_guest_scope.py
- tests/unit_tests/mcp_service/test_guest_token_auth.py
- tests/unit_tests/mcp_service/test_privacy.py
- tests/unit_tests/security/manager_test.py
- tests/unit_tests/utils/filters_test.py
-
Files skipped - 2
- superset/mcp_service/CLAUDE.md - Reason: Filter setting
- superset/mcp_service/SECURITY.md - Reason: Filter setting
-
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
| # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read). | ||
| if not guest_scope.is_guest_read(): | ||
| validation_result = validate_chart_dataset(chart, check_access=True) | ||
| if not validation_result.is_valid: | ||
| await ctx.warning( | ||
| "Chart found but dataset is not accessible: %s" | ||
| % (validation_result.error,) | ||
| ) | ||
| return ChartError( | ||
| error=validation_result.error | ||
| or "Chart's dataset is not accessible. " | ||
| "Dataset may have been deleted.", | ||
| error_type="DatasetNotAccessible", | ||
| ) | ||
| # Log any warnings (e.g., virtual dataset warnings) | ||
| for warning in validation_result.warnings: | ||
| await ctx.warning("Dataset warning: %s" % (warning,)) |
There was a problem hiding this comment.
The new guest-scoped bypass of validate_chart_dataset (line 414-430) and the authorize_query call (line 628-629) lack test coverage in tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py. An async integration test that mocks guest_scope.is_guest_read() to return True would verify the end-to-end guest flow: chart lookup → guest_dashboard_id capture → RBAC skip → authorize_query with dashboard context.
Code Review Run #721a05
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
1819c18 to
bc82880
Compare
| uuid_ids = [id_ for id_ in ids if is_uuid(id_)] | ||
| int_ids = [id_ for id_ in ids if not is_uuid(id_)] |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the newly introduced local lists that separate UUID and integer dashboard identifiers. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new local variables uuid_ids and int_ids are list-valued and can be explicitly annotated (for example, as list[Any] or a more specific list type). Since the custom rule requires type hints for new or modified Python code where relevant variables can be annotated, omitting annotations here is a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/filters.py
**Line:** 76:77
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced local lists that separate UUID and integer dashboard identifiers.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 6 re-review at HEAD bc82880c. This PR's own authored diff is unchanged from round 5 (+1030/-98, identical files/hunks) — this push is a rebase onto a newer master that happens to include a large, unrelated upstream refactor (Owner/Editor/Viewer + a new "Subject" abstraction unifying users/roles/groups, RLS filters keyed by subjects instead of roles, Group API changes). None of that is this PR's content; confirmed by diffing this round's scoped gh pr diff against round 5's byte-for-byte — the only difference is context-line shifts and one mechanical rebase artifact (see below). All 5 rounds of findings remain resolved — 0 new security findings.
However, CI is now red on a required check (unit-tests-required), and it's worth understanding why before merging: tests/unit_tests/charts/test_filters.py::test_chart_filter_non_guest_skips_guest_branch fails with AttributeError: user at flask/ctx.py:54. Root cause: the rebase's unrelated upstream refactor changed ChartFilter.apply()'s non-guest tail from an inline query.join(...) call to return self._apply_viewers(query), and _apply_viewers() now calls get_user_id() (which touches g.user) before it does anything resembling a join. This test's technique — making query.join raise a sentinel RuntimeError to prove the non-guest path was reached — no longer works, because the refactored code hits the unmocked g.user access and raises AttributeError before ever calling query.join().
This is not a security regression — the guest branch itself (if (guest_dashboards := guest_embedded_dashboard_filter()) is not None: return query.filter(...)) is untouched and still unconditionally short-circuits before _apply_viewers() is ever reached, for guests and non-guests alike; the test in question exists precisely to prove that early-return, and its intent is unaffected. But it's a genuine, required-check-blocking failure that needs a real fix, not just a rebase artifact to shrug off: the test needs get_user_id() (or g.user) mocked so execution reaches a real query.join() call (or another observable point) before the sentinel is meaningful again — as currently written it will keep failing on every future run regardless of how many more times this PR gets rebased.
Two new bot NITs since round 5 (type-hint suggestions on uuid_ids/int_ids in utils/filters.py and a test-local variable) are trivial style items, not blocking. Bito's suggestion for a dedicated end-to-end guest-flow test in test_get_chart_data.py is a reasonable coverage addition but lower priority than the CI break.
Still not approving — security-boundary PR, escalating to Amin for final sign-off per standing policy — and CI needs to be green before this is mergeable regardless of sign-off.
| filt: ChartFilter = ChartFilter.__new__(ChartFilter) | ||
| filt.model = Slice | ||
|
|
||
| with pytest.raises(RuntimeError, match="datasource-access join"): |
There was a problem hiding this comment.
CI is currently failing here: AttributeError: user (flask/ctx.py:54), not the expected RuntimeError sentinel. ChartFilter.apply()'s non-guest tail was refactored upstream to return self._apply_viewers(query), which calls get_user_id() (touching g.user) before anything resembling query.join(...). Since this test never sets up g.user, the AttributeError fires first and the sentinel never gets a chance to prove the guest branch was skipped. Not a security issue (the guest branch above is untouched and still short-circuits first) but this needs a fix — e.g. mocker.patch("superset.charts.filters.get_user_id", return_value=1) — or it will keep failing CI on every future push.
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
bc82880 to
bc154ac
Compare
Code Review Agent Run #985a7eActionable Suggestions - 0Additional Suggestions - 5
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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 7 re-review at HEAD bc154ac71 (rebase/squash of round 6's bc82880c; diff content unchanged). Not approving yet — two housekeeping threads are still open, see below.
All of our own findings across rounds 1–6 are resolved, re-verified at this HEAD (not just re-read):
- HIGH (dataset tools unscoped for guests) — closed via the default-deny
MCP_GUEST_ALLOWED_TOOLSallow-list (auth.py:243,mcp_config.py:161):get_dataset_info/list_datasets/query_datasetare absent from the list and explicitly asserted denied intest_non_allow_listed_tools_denied_to_guest(test_guest_token_auth.py:388, dataset tools listed at 368-370). Intact post-rebase. - HIGH (
ChartFilter.apply()guest branch untested) — resolved;test_filters.py::test_chart_filter_scopes_guest_to_token_dashboardsexercisesapply()directly and asserts the compiled SQL clause (EXISTS/IN (1, 2)) rather than mocking around it. Thread marked resolved. - MEDIUM (
get_chart_sql.pynot guest-scoped) — resolved (explicit guest denial). - Mixed-uuid/int guest-token bug,
PUBLIC_ROLE_LIKEdata-model-metadata bypass, non-embedded-dashboard token issuance — all resolved at the root (manager.py:4396rejects a raw int id whose dashboard isn't.embeddedat token-issuance time;utils/filters.py:76-82ORs uuid/int branches instead of routing everything through one). - Our CI-breaking-test inline comment — confirmed fixed:
test_chart_filter_non_guest_skips_guest_branchnow patches_apply_viewersdirectly instead of relying onquery.join, so it no longer trips over the upstream_apply_viewers→get_user_id()→g.userrefactor.unit-tests-requiredis green.
Guest-scoping enforcement re-confirmed at this HEAD: ChartFilter.apply() (superset/charts/filters.py:114-120) returns query.filter(self.model.dashboards.any(guest_dashboards)) as an early return — a server-side WHERE, not a fetch-then-drop — so an out-of-scope chart never resolves for a guest via ChartDAO. The MCP-tool allow-list is independent defense-in-depth on top of that (default-deny, fails closed on a misconfigured policy per auth.py's _tool_denied_for_principal). Non-guest path is unaffected (can_access_all_datasources / _apply_viewers unchanged, guest branch short-circuits before it).
Only two things left open, neither security:
- bito's
get_chart_data.py:430"missing guest scope integration test" — independently verified real: there's no end-to-end test that calls the tool as a guest against an out-of-scope chart and asserts denial (only theChartFilter.apply()unit test and the tool-allow-list test exist). Worth adding, but not blocking — the primary gate (ChartFilter) and the allow-list are each already directly tested; this would only add belt-and-suspenders coverage. Not re-filing, just corroborating so it isn't lost. - codeant's
utils/filters.py:77type-annotation nit — trivial, not security-relevant.
Once those two threads are closed (or explicitly dismissed), this is ready for sign-off from a security standpoint — the guest boundary holds at this HEAD.
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
bc154ac to
87fe93f
Compare
| # Embedded guests are scoped to their token's dashboards first. A guest | ||
| # is never entitled to all charts, regardless of what its role grants, | ||
| # and an empty token scope denies all charts (a deny-all clause). | ||
| if (guest_dashboards := guest_embedded_dashboard_filter()) is not None: |
There was a problem hiding this comment.
Suggestion: Replace the inline walrus assignment with a separate local variable declaration and add an explicit type annotation for that variable before the conditional check. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new walrus assignment introduces a local variable without any type annotation, and this is exactly the kind of newly added Python code the type-hints rule targets. The suggestion correctly identifies a real omission in the added line.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/filters.py
**Line:** 117:117
**Comment:**
*Custom Rule: Replace the inline walrus assignment with a separate local variable declaration and add an explicit type annotation for that variable before the conditional check.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| # A guest token may mix uuid and int dashboard ids during the uuid rollout. | ||
| # Route each id kind to its own column and OR them — a plain int sent to the | ||
| # uuid-typed column would raise a bind/type error. | ||
| uuid_ids = [id_ for id_ in ids if is_uuid(id_)] |
There was a problem hiding this comment.
Suggestion: Add a concrete type hint for uuid_ids instead of leaving this derived list unannotated. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
uuid_ids is a newly introduced derived list and is left unannotated even though its type can be inferred and annotated. This is a real instance of missing type hints in new Python code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/filters.py
**Line:** 80:80
**Comment:**
*Custom Rule: Add a concrete type hint for `uuid_ids` instead of leaving this derived list unannotated.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # Route each id kind to its own column and OR them — a plain int sent to the | ||
| # uuid-typed column would raise a bind/type error. | ||
| uuid_ids = [id_ for id_ in ids if is_uuid(id_)] | ||
| int_ids = [id_ for id_ in ids if not is_uuid(id_)] |
There was a problem hiding this comment.
Suggestion: Add a concrete type hint for int_ids instead of leaving this derived list unannotated. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
int_ids is also a new list variable without an explicit type annotation. Since the type is evident, this is a valid type-hint omission under the rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/filters.py
**Line:** 81:81
**Comment:**
*Custom Rule: Add a concrete type hint for `int_ids` instead of leaving this derived list unannotated.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| viewers = mocker.patch.object( | ||
| ChartFilter, "_apply_viewers", side_effect=AssertionError("role path") | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation for this local mock object assignment so the new code consistently uses type hints. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a real omission of a type hint in newly added Python code: viewers is assigned a mock object and could be annotated, but no explicit type is present.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_filters.py
**Line:** 170:172
**Comment:**
*Custom Rule: Add a concrete type annotation for this local mock object assignment so the new code consistently uses type hints.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| assert item["subjects"][0]["label"] == "Alpha" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Suggestion: Add explicit type hints to this async test function signature, including the fixture parameter type and a return type annotation. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The added async test function has no type hints for its fixture argument and no return annotation, which matches the rule requiring type hints on new or modified Python functions where applicable.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py
**Line:** 249:249
**Comment:**
*Custom Rule: Add explicit type hints to this async test function signature, including the fixture parameter type and a return type annotation.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
87fe93f to
6548563
Compare
gabotorresruiz
force-pushed
the
feat/mcp-embedded-guest-data-read
branch
from
6548563 to
d443dea
Compare
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Lets an embedded guest (an anonymous dashboard viewer authenticated by a guest token) read chart data over the MCP service, scoped to the dashboards in their guest token, so an embedded assistant can analyze the dashboard the viewer is looking at, and nothing else.
Before this change a guest could read dashboard structure but
get_chart_data,get_chart_info, andget_chart_previewfailed ("Chart not found" / dataset not accessible):ChartFilterhad no embedded-guest branch (onlyDashboardAccessFilterdid), and the data path needs the embedded dashboard context forraise_for_access.Design (mirrors the existing embedded-dashboard pattern, no new bespoke security path):
guest_embedded_dashboard_filter()(superset/utils/filters.py) builds a SQLAlchemy condition matching the guest token's embedded dashboards, mirroringDashboardAccessFilter.ChartFiltergains a guest branch that scopes chart resolution to charts on those dashboards.raise_for_accessbranch. Newsuperset/mcp_service/guest_scope.pyattaches the embedded dashboard context (dashboardId/slice_id) to the query, exactly as the embedded frontend does, and pinsslice_so the guest payload tamper-guard (query_context_modified) still compares requested columns/metrics against the resolved chart. The three read tools skip the dataset RBAC pre-check for guests (guests read via the dashboard context, not dataset RBAC;raise_for_accessremains the real gate).guest_dashboard_id()short-circuits onis_guest_user(), so behavior for regular authenticated users is unchanged.raise_for_access; guests only reach their token's resources.This is the data-read scoping layer. The guest-token authentication that resolves the
GuestUserover MCP lands in #41003, this PR depends on #41003 for end-to-end function (the code is dormant until that merges; the diff itself isindependent and reviews/merges standalone).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A. Backend-only change (MCP read-tool authorization); no UI
TESTING INSTRUCTIONS
Requires
EMBEDDED_SUPERSETand (for the guest to authenticate over MCP)MCP_EMBEDDED_GUEST_AUTH_ENABLEDfrom #41003.Authorization: Bearer <guest_token>.get_chart_data/get_chart_info/get_chart_previewfor a chart on that dashboard --> returns the chart's data/metadata/preview.ADDITIONAL INFORMATION
EMBEDDED_SUPERSET