codeant-ai-for-open-source[bot] commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2935237586
##########
superset-core/src/superset_core/mcp/decorators.py:
##########
@@ -37,6 +37,8 @@ def another_tool(value: int) -> str:
from typing import Any, Callable, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
**Suggestion:** This introduces a hard runtime dependency on `mcp` in
`apache-superset-core`, but `superset-core/pyproject.toml` does not include
`mcp`/`fastmcp`. Importing this module will raise `ModuleNotFoundError` in
environments without the MCP extra. Make the import optional and provide a
fallback alias so the abstract decorators remain importable. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ README MCP tool example crashes on import.
- ❌ `superset_core.mcp.decorators` unusable in core-only installs.
- ⚠️ Extension authors blocked without optional MCP extras.
```
</details>
```suggestion
try:
from mcp.types import ToolAnnotations
except ImportError: # MCP extras may not be installed in superset-core-only
environments
ToolAnnotations = Any # type: ignore[assignment]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Install only `apache-superset-core` as documented in
`superset-core/README.md:30-32`
(`pip install apache-superset-core`), which uses dependencies declared in
`superset-core/pyproject.toml:44-51` and does not include `mcp`/`fastmcp`.
2. Follow the documented MCP quick-start import in
`superset-core/README.md:82` (`from
superset_core.mcp.decorators import tool`) or `README.md:92` (`prompt`).
3. Python imports `superset-core/src/superset_core/mcp/decorators.py`, which
executes
`from mcp.types import ToolAnnotations` at line 40 before any function call.
4. In environments without MCP extras, import fails immediately with
`ModuleNotFoundError:
No module named 'mcp'`, preventing use of the abstract decorators that are
intended to be
importable from `superset_core`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/mcp/decorators.py
**Line:** 40:40
**Comment:**
*Possible Bug: This introduces a hard runtime dependency on `mcp` in
`apache-superset-core`, but `superset-core/pyproject.toml` does not include
`mcp`/`fastmcp`. Importing this module will raise `ModuleNotFoundError` in
environments without the MCP extra. Make the import optional and provide a
fallback alias so the abstract decorators remain importable.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=a2b8a2e6a9965ed27249066d4a12586307ebee49e8d2173e0f877094611e223b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=a2b8a2e6a9965ed27249066d4a12586307ebee49e8d2173e0f877094611e223b&reaction=dislike'>👎</a>
##########
superset/mcp_service/explore/tool/generate_explore_link.py:
##########
@@ -39,7 +39,15 @@
from superset.mcp_service.utils.schema_utils import parse_request
-@tool(tags=["explore"], class_permission_name="Explore")
+@tool(
+ tags=["explore"],
+ class_permission_name="Explore",
+ annotations=ToolAnnotations(
+ title="Generate explore link",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
Review Comment:
**Suggestion:** The tool is marked as read-only, but it calls logic that
creates a new `form_data_key` cache entry (`MCPCreateFormDataCommand.run()`),
which is a state mutation. This incorrect annotation can cause MCP
clients/directories to treat a mutating tool as safe read-only behavior. Set
`readOnlyHint` to `False` to match actual side effects. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP metadata mislabels mutating `generate_explore_link` behavior.
- ⚠️ Clients may treat cache-writing tool as read-only.
- ⚠️ Directory safety profile for explore tool becomes inaccurate.
```
</details>
```suggestion
annotations=ToolAnnotations(
title="Generate explore link",
readOnlyHint=False,
destructiveHint=False,
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP service and invoke `generate_explore_link` through MCP client
exactly like
unit tests do at
`tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py:119-122`
(`client.call_tool("generate_explore_link", ...)`).
2. Request enters tool function `generate_explore_link()` in
`superset/mcp_service/explore/tool/generate_explore_link.py:52`; decorator
metadata
currently marks `readOnlyHint=True` at `:47`.
3. The tool calls `generate_url(...)` at
`superset/mcp_service/explore/tool/generate_explore_link.py:141-143`, which
resolves to
`chart_utils.generate_explore_link()` at
`superset/mcp_service/chart/chart_utils.py:152`.
4. `chart_utils.generate_explore_link()` executes
`MCPCreateFormDataCommand(cmd_params).run()` at
`superset/mcp_service/chart/chart_utils.py:205`; underlying
`CreateFormDataCommand.run()`
writes cache entries via `cache_manager.explore_form_data_cache.set(...)` at
`superset/commands/explore/form_data/create.py:67-68`, proving server-side
state mutation
despite read-only annotation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/explore/tool/generate_explore_link.py
**Line:** 45:49
**Comment:**
*Logic Error: The tool is marked as read-only, but it calls logic that
creates a new `form_data_key` cache entry (`MCPCreateFormDataCommand.run()`),
which is a state mutation. This incorrect annotation can cause MCP
clients/directories to treat a mutating tool as safe read-only behavior. Set
`readOnlyHint` to `False` to match actual side effects.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=254f0be6597885d9fb69f71d780904c652371b2f6b3c05f1118b0c9ca3f828df&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=254f0be6597885d9fb69f71d780904c652371b2f6b3c05f1118b0c9ca3f828df&reaction=dislike'>👎</a>
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -25,6 +25,8 @@
import logging
from typing import Any, Callable, Optional, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
**Suggestion:** This unconditional import introduces a hard runtime
dependency on `mcp` during normal Superset startup, but MCP support is optional
(`fastmcp` extra). Since this module is imported from core initialization even
when MCP extras are not installed, `ModuleNotFoundError` can crash app
initialization before the existing fastmcp availability guard runs. Make the
import optional with a fallback type alias so startup still works without MCP
packages. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ create_app() fails without optional MCP dependencies.
- ❌ Celery bootstrap fails via superset/tasks/celery_app.py:32.
- ⚠️ fastmcp guard never executes due earlier import crash.
```
</details>
```suggestion
try:
from mcp.types import ToolAnnotations
except ImportError:
ToolAnnotations = Any # type: ignore[assignment]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset through normal app bootstrap (`superset/app.py:47-55`),
which always
calls `app_initializer.init_app()`.
2. During initialization, `SupersetAppInitializer.init_app()` enters
app-context init
(`superset/initialization/__init__.py:728-740`) and calls
`init_all_dependencies_and_extensions()` (`:626`), which always calls
`init_core_dependencies()` (`:543-554`).
3. `init_core_dependencies()` unconditionally imports
`initialize_core_mcp_dependencies`
from `superset.core.mcp.core_mcp_injection`
(`superset/initialization/__init__.py:547-549`), so Python evaluates module
top-level
imports first.
4. In environments without MCP packages installed, top-level `from mcp.types
import
ToolAnnotations` at `superset/core/mcp/core_mcp_injection.py:28` raises
`ModuleNotFoundError` before the internal `fastmcp` guard
(`core_mcp_injection.py:281-288`) can run, aborting startup.
5. This is realistic because MCP is optional (`pyproject.toml:147` defines
`fastmcp` as an
extra), so non-MCP deployments can hit this on standard startup paths.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/core/mcp/core_mcp_injection.py
**Line:** 28:28
**Comment:**
*Possible Bug: This unconditional import introduces a hard runtime
dependency on `mcp` during normal Superset startup, but MCP support is optional
(`fastmcp` extra). Since this module is imported from core initialization even
when MCP extras are not installed, `ModuleNotFoundError` can crash app
initialization before the existing fastmcp availability guard runs. Make the
import optional with a fallback type alias so startup still works without MCP
packages.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=bb7d97f0b6037f45a6a6b0f5f5282635af7082eb8c054b5890932ffd3330a596&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=bb7d97f0b6037f45a6a6b0f5f5282635af7082eb8c054b5890932ffd3330a596&reaction=dislike'>👎</a>
##########
superset/mcp_service/sql_lab/tool/save_sql_query.py:
##########
@@ -43,7 +43,14 @@
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(
+ tags=["mutate"],
+ annotations=ToolAnnotations(
Review Comment:
**Suggestion:** This tool is registered without `class_permission_name`, so
MCP RBAC defaults to allow and skips Superset view-level permission checks. As
a result, any authenticated user who can access a database can invoke this
mutating endpoint even if they do not have SQL Lab permissions. Add SQL Lab
RBAC metadata to the decorator so permission checks are enforced consistently
with other SQL Lab tools. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab RBAC can be bypassed for save_sql_query.
- ❌ Unauthorized users may create persisted SavedQuery records.
- ⚠️ Inconsistent authorization versus execute_sql SQLLab tool.
```
</details>
```suggestion
class_permission_name="SQLLab",
method_permission_name="write",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP server initialization; `superset/mcp_service/app.py:418-421`
imports
`save_sql_query`, so the decorator in
`superset/mcp_service/sql_lab/tool/save_sql_query.py:46-53` is executed
during tool
registration.
2. In `superset/core/mcp/core_mcp_injection.py:110-121`, RBAC attributes are
only attached
when `class_permission_name` is provided; this tool omits it, so no
`_class_permission_name`/`_method_permission_name` gets set.
3. Invoke MCP `save_sql_query` (documented as a normal workflow tool in
`superset/mcp_service/app.py:73,109`); execution enters `mcp_auth_hook` at
`superset/mcp_service/auth.py:55`.
4. `check_tool_permission()` at `superset/mcp_service/auth.py:103-106`
returns
allow-by-default when class permission metadata is missing (also validated by
`tests/unit_tests/mcp_service/test_auth_rbac.py:103-108`), so SQL Lab
view-level RBAC is
skipped.
5. The function then only enforces database access (`save_sql_query.py:91`)
and proceeds
to mutate state with `SavedQueryDAO.create(...)` at
`save_sql_query.py:102-113`, allowing
saved-query creation without explicit SQLLab permission checks.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/tool/save_sql_query.py
**Line:** 48:48
**Comment:**
*Security: This tool is registered without `class_permission_name`, so
MCP RBAC defaults to allow and skips Superset view-level permission checks. As
a result, any authenticated user who can access a database can invoke this
mutating endpoint even if they do not have SQL Lab permissions. Add SQL Lab
RBAC metadata to the decorator so permission checks are enforced consistently
with other SQL Lab tools.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=6fb90b4b5f82a067fad9c3564e6b7f8c596b663ab2358d809f35ef063be17ef6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=6fb90b4b5f82a067fad9c3564e6b7f8c596b663ab2358d809f35ef063be17ef6&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]
