codeant-ai-for-open-source[bot] commented on code in PR #41753:
URL: https://github.com/apache/superset/pull/41753#discussion_r3546657824
##########
superset/charts/filters.py:
##########
@@ -111,6 +114,10 @@ def apply(self, query: Query, value: Any) -> Query:
if security_manager.can_access_all_datasources():
return query
+ # Embedded guests: scope to charts on the dashboards in their token.
+ if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
+ return query.filter(self.model.dashboards.any(guest_dashboards))
Review Comment:
**Suggestion:** This guest branch is fail-open when a guest token has no
dashboard resources (or produces no filter): it falls through to the regular
viewer/dataset logic instead of denying access. For embedded guests, missing
scoped dashboards should return no charts, otherwise guest visibility can
expand based on role/dataset permissions rather than token scope. [security]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Embedded guest can access charts beyond token dashboards scope.
- β MCP chart tools leak data via dataset-based access.
- β οΈ Guest token misconfig grants broader chart visibility than intended.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure an embedded guest token with EMBEDDED_SUPERSET enabled but no
dashboard
resources, mirroring tests/unit_tests/utils/filters_test.py:28-37 where
guest.resources
contains only a dataset entry and guest_embedded_dashboard_filter() returns
None.
2. Run the MCP service so embedded guest chart tools resolve charts via
ChartDAO, which
uses ChartFilter as its base_filter (superset/daos/chart.py:6-8 and
get_by_id_or_uuid at
lines 41-48).
3. For this embedded guest, when ChartFilter.apply executes in
superset/charts/filters.py:112-121,
security_manager.can_access_all_datasources() is False
and guest_embedded_dashboard_filter() returns None
(superset/utils/filters.py:65-71 for
empty ids), so the embedded-guest branch at lines 117-119 is skipped.
4. ChartFilter.apply falls through to _apply_viewers() at lines 123-172,
which applies
subject-based viewer/editor filters and dataset access filters
(get_dataset_access_filters), allowing charts reachable via dataset or
subject permissions
β including charts not on any dashboard in the guest token β to be listed or
resolved by
MCP tools such as list_charts
(superset/mcp_service/chart/tool/list_charts.py:170-176) and
get_chart_info (superset/mcp_service/chart/tool/get_chart_info.py:62-71),
expanding
embedded guest visibility beyond the tokenβs dashboard scope.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab7828b0f81c47a0b033f3defb9391f8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ab7828b0f81c47a0b033f3defb9391f8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/filters.py
**Line:** 118:119
**Comment:**
*Security: This guest branch is fail-open when a guest token has no
dashboard resources (or produces no filter): it falls through to the regular
viewer/dataset logic instead of denying access. For embedded guests, missing
scoped dashboards should return no charts, otherwise guest visibility can
expand based on role/dataset permissions rather than token scope.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=25cc4f204074064ea8c7b98cb4ef2b58c2831187460b125c9a772afe47a11b29&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=25cc4f204074064ea8c7b98cb4ef2b58c2831187460b125c9a772afe47a11b29&reaction=dislike'>π</a>
##########
superset/mcp_service/auth.py:
##########
@@ -236,32 +236,72 @@ def _log_scope_denial(
)
-# Guest deny-list default (when MCP_GUEST_DENIED_TOOLS is unset); blocks tools
-# with no RBAC class that would otherwise fall open. Sync with mcp_config.py.
-_DEFAULT_GUEST_DENIED_TOOLS: frozenset[str] = frozenset(
- {"find_users", "get_instance_info"}
+# Default-deny allow-list for embedded guests: a guest may call only these
tools,
+# regardless of MCP_RBAC_ENABLED or how the guest role (PUBLIC_ROLE_LIKE) is
+# configured. Everything else is denied, including newly added tools until
listed
+# here. Sync with mcp_config.py.
+_DEFAULT_GUEST_ALLOWED_TOOLS: frozenset[str] = frozenset(
+ {
+ # Dashboard structure, scoped to the token's embedded dashboards.
+ "get_dashboard_info",
+ "get_dashboard_layout",
+ "list_dashboards",
+ # Chart read + data, scoped by ChartFilter; data-model fields redacted.
+ "list_charts",
+ "get_chart_info",
+ "get_chart_data",
+ "get_chart_preview",
+ }
)
-def _tool_denied_for_guest(func: Callable[..., Any]) -> bool:
- """True when the current user is a guest and ``func`` is on the guest
- deny-list β barring enumeration tools (user listing, instance metadata)
that
- may declare no RBAC class. ``isinstance`` (not ``is_guest_user``) keeps
this
- cheap and off the feature-flag path."""
- if not isinstance(getattr(g, "user", None), GuestUser):
- return False
- denied = current_app.config.get("MCP_GUEST_DENIED_TOOLS")
- # A str would make ``in`` do substring matching and corrupt the decision;
- # require a real collection, else fall back to the default.
- if not isinstance(denied, (set, frozenset, list, tuple)):
- if denied is not None:
+def _guest_allowed_tools() -> frozenset[str]:
+ """The tool allow-list for embedded guests (``MCP_GUEST_ALLOWED_TOOLS`` or
the
+ default). A str config is rejected β it would make ``in`` do substring
+ matching β and falls back to the safe default."""
+ allowed = current_app.config.get("MCP_GUEST_ALLOWED_TOOLS")
+ if not isinstance(allowed, (set, frozenset, list, tuple)):
+ if allowed is not None:
logger.warning(
- "MCP_GUEST_DENIED_TOOLS must be a set/list of tool names, got
%s; "
- "using the default deny-list",
- type(denied).__name__,
+ "MCP_GUEST_ALLOWED_TOOLS must be a set/list of tool names, got
%s; "
+ "using the default allow-list",
+ type(allowed).__name__,
)
- denied = _DEFAULT_GUEST_DENIED_TOOLS
- return getattr(func, "__name__", None) in denied
+ return _DEFAULT_GUEST_ALLOWED_TOOLS
+ return frozenset(allowed)
+
+
+def _default_restricted_tool_policy(user: Any) -> frozenset[str] | None:
+ """Map a principal to its MCP tool allow-list, or None when the principal
is
+ not allow-list-restricted (RBAC governs instead). Embedded guests are the
+ built-in restricted principal; override ``MCP_RESTRICTED_TOOL_POLICY`` to
add
+ others (e.g. a future embedded principal type)."""
+ if isinstance(user, GuestUser):
+ return _guest_allowed_tools()
+ return None
+
+
+def _tool_denied_for_principal(func: Callable[..., Any]) -> bool:
+ """True when the current principal is allow-list-restricted and ``func`` is
+ not on its allow-list. Default-deny for restricted principals (embedded
guests
+ by default), holds even with MCP_RBAC_ENABLED off; unrestricted principals
are
+ governed by RBAC and never blocked here."""
+ policy = (
+ current_app.config.get("MCP_RESTRICTED_TOOL_POLICY")
+ or _default_restricted_tool_policy
+ )
+ allowed = policy(getattr(g, "user", None))
Review Comment:
**Suggestion:** The restricted-policy config is invoked without verifying it
is callable, so a misconfigured `MCP_RESTRICTED_TOOL_POLICY` (for example a
list/dict/string) raises a `TypeError` and can break tool permission checks at
runtime. Validate callability before invocation and fail closed with a
controlled denial/log path instead of crashing. [type error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Misconfigured policy crashes MCP tool permission evaluation.
- β Tools/list endpoint fails when restricted policy misconfigured.
- β οΈ Operators lose predictable fail-closed behavior for restricted
principals.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Misconfigure MCP_RESTRICTED_TOOL_POLICY in the Flask app config as a
non-callable
object (for example a list or string of tool names), even though it is
intended to be a
callable per superset/mcp_service/mcp_config.py:175.
2. Trigger MCP tool visibility or permission evaluation so
is_tool_visible_to_current_user
in superset/mcp_service/auth.py:21-50 runs and calls
_tool_denied_for_principal(tool_func)
at line 29, or check_tool_permission in auth.py:88-132 calls
_tool_denied_for_principal(func) at line 112.
3. Inside _tool_denied_for_principal (superset/mcp_service/auth.py:65-85),
policy is set
from current_app.config.get("MCP_RESTRICTED_TOOL_POLICY") or
_default_restricted_tool_policy at lines 70-72; because the configured value
is a truthy
non-callable, policy becomes that object rather than the default function.
4. The next line, allowed = policy(getattr(g, "user", None)) at line 74,
attempts to call
the non-callable policy object, raising a TypeError that is not caught by
_tool_denied_for_principal or by the surrounding try/except blocks in
check_tool_permission (which only catches AttributeError, ValueError,
RuntimeError at
lines 220-222) and is_tool_visible_to_current_user (which catches
AttributeError,
RuntimeError, ValueError at lines 52-54), causing MCP tool permission checks
and
tools/list visibility evaluation to crash with an internal error instead of
failing closed
for the misconfigured principal.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8abbbb3db2d84090b5f8674a36f32487&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8abbbb3db2d84090b5f8674a36f32487&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 289:293
**Comment:**
*Type Error: The restricted-policy config is invoked without verifying
it is callable, so a misconfigured `MCP_RESTRICTED_TOOL_POLICY` (for example a
list/dict/string) raises a `TypeError` and can break tool permission checks at
runtime. Validate callability before invocation and fail closed with a
controlled denial/log path instead of crashing.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=25bd6888e61376e7d7472d3059b1e2947f1540b837283e5ff8c6094f2ff3e60d&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=25bd6888e61376e7d7472d3059b1e2947f1540b837283e5ff8c6094f2ff3e60d&reaction=dislike'>π</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
