codeant-ai-for-open-source[bot] commented on code in PR #37216:
URL: https://github.com/apache/superset/pull/37216#discussion_r2700626709
##########
superset/mcp_service/server.py:
##########
@@ -17,13 +17,23 @@
"""
MCP server for Apache Superset
+
+Supports both single-pod (in-memory) and multi-pod (Redis) deployments.
+For multi-pod deployments, configure MCP_EVENT_STORE_CONFIG with Redis URL.
"""
import logging
import os
+from typing import Any
+
+import uvicorn
from superset.mcp_service.app import create_mcp_app, init_fastmcp_server
-from superset.mcp_service.mcp_config import get_mcp_factory_config
+from superset.mcp_service.mcp_config import (
+ get_mcp_factory_config,
+ MCP_STORE_CONFIG,
Review Comment:
**Suggestion:** Wrong configuration symbol imported: code imports
`MCP_STORE_CONFIG` but the new setting introduced in PR is
`MCP_EVENT_STORE_CONFIG`; this will cause a NameError or import mismatch if
only the new symbol exists. Import the correct name exported by the config
module. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP service fails starting due ImportError on config import.
- ⚠️ CI jobs importing server will error immediately.
- ❌ Any runtime path that imports server is blocked.
```
</details>
```suggestion
MCP_EVENT_STORE_CONFIG,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset process or otherwise import
`superset.mcp_service.server` which
executes the import block at `superset/mcp_service/server.py:32-35`.
2. Python tries to resolve `MCP_STORE_CONFIG` from
`superset.mcp_service.mcp_config`; if
the PR/other changes only define `MCP_EVENT_STORE_CONFIG`, this import
raises ImportError
at module import time.
3. The ImportError aborts import, preventing any use of
`create_event_store()` or
`run_server()` (the module never loads).
4. This is not a hypothetical mismatch — the PR description names
`MCP_EVENT_STORE_CONFIG`, so keeping `MCP_STORE_CONFIG` is a concrete
mismatch with the
introduced symbol.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 34:34
**Comment:**
*Possible Bug: Wrong configuration symbol imported: code imports
`MCP_STORE_CONFIG` but the new setting introduced in PR is
`MCP_EVENT_STORE_CONFIG`; this will cause a NameError or import mismatch if
only the new symbol exists. Import the correct name exported by the config
module.
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>
##########
superset/mcp_service/server.py:
##########
@@ -55,21 +65,79 @@ def configure_logging(debug: bool = False) -> None:
logging.info("🔍 SQL Debug logging enabled")
+def create_event_store(config: dict[str, Any] | None = None) -> Any | None:
+ """
+ Create an EventStore for MCP session management.
+
+ For multi-pod deployments, uses Redis-backed storage to share session state
+ across pods. For single-pod deployments, returns None (uses in-memory).
+
+ Args:
+ config: Optional config dict. If None, reads from MCP_STORE_CONFIG.
+
+ Returns:
+ EventStore instance if Redis URL is configured, None otherwise.
+ """
+ if config is None:
+ config = MCP_STORE_CONFIG
+
+ redis_url = config.get("CACHE_REDIS_URL")
+ if not redis_url:
+ logging.info("EventStore: Using in-memory storage (single-pod mode)")
+ return None
+
+ try:
+ from fastmcp.server.event_store import EventStore
+
+ # Reuse _create_redis_store with wrap=False for raw RedisStore
+ redis_store = _create_redis_store(config, wrap=False)
+ if redis_store is None:
+ logging.warning("Failed to create Redis store, falling back to
in-memory")
+ return None
+
+ # Create EventStore with Redis backend
+ event_store = EventStore(
+ storage=redis_store,
+ max_events_per_stream=config.get("event_store_max_events", 100),
+ ttl=config.get("event_store_ttl", 3600),
Review Comment:
**Suggestion:** Configuration key mismatch: `create_event_store` reads
`CACHE_REDIS_URL` and expects `event_store_max_events`/`event_store_ttl`, but
the PR and examples use `redis_url`, `max_events_per_stream`, and `ttl`; this
will prevent Redis from being detected/configured and produce incorrect
EventStore parameters. Use the canonical MCP event-store keys. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Multi‑pod Redis sessions not used despite configuration.
- ❌ Session sharing across pods broken in Kubernetes.
- ⚠️ EventStore limits not applied from config.
```
</details>
```suggestion
redis_url = config.get("redis_url")
if not redis_url:
logging.info("EventStore: Using in-memory storage (single-pod mode)")
return None
try:
from fastmcp.server.event_store import EventStore
# Reuse _create_redis_store with wrap=False for raw RedisStore
redis_store = _create_redis_store(config, wrap=False)
if redis_store is None:
logging.warning("Failed to create Redis store, falling back to
in-memory")
return None
# Create EventStore with Redis backend (use canonical keys from
MCP_EVENT_STORE_CONFIG)
event_store = EventStore(
storage=redis_store,
max_events_per_stream=config.get("max_events_per_stream", 100),
ttl=config.get("ttl", 3600),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure MCP event store using the example in the PR (set
`MCP_EVENT_STORE_CONFIG`
with keys `redis_url`, `max_events_per_stream`, `ttl`) in
`superset/mcp_service/mcp_config.py`.
2. Start the MCP service (call `run_server()`), which calls
`create_event_store()` at
`superset/mcp_service/server.py` and sets `config = MCP_STORE_CONFIG` when
`event_store_config` is None (`server.py:82` then uses keys at
`server.py:84`).
3. `create_event_store()` executes `config.get("CACHE_REDIS_URL")` at
`superset/mcp_service/server.py:84`; because the configured key is
`redis_url`, this
returns None.
4. The function logs "Using in-memory storage" and returns None, causing the
server to run
in single‑pod in‑memory mode despite Redis being configured.
5. Additionally, even if redis store is created, the parameters passed are
read from
`event_store_max_events` / `event_store_ttl` (lines `server.py:101-103`)
instead of the
PR's `max_events_per_stream` / `ttl`, so desired EventStore limits are
ignored.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 84:102
**Comment:**
*Logic Error: Configuration key mismatch: `create_event_store` reads
`CACHE_REDIS_URL` and expects `event_store_max_events`/`event_store_ttl`, but
the PR and examples use `redis_url`, `max_events_per_stream`, and `ttl`; this
will prevent Redis from being detected/configured and produce incorrect
EventStore parameters. Use the canonical MCP event-store keys.
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>
##########
superset/mcp_service/storage.py:
##########
@@ -109,15 +115,51 @@ def _create_redis_store(
return None
try:
+ # Parse URL to handle SSL properly
+ parsed = urlparse(redis_url)
+ use_ssl = parsed.scheme == "rediss"
+
+ # Build clean URL without query params that may cause issues
+ clean_url = f"{parsed.scheme}://"
+ if parsed.username or parsed.password:
+ auth = parsed.username if parsed.username else ""
+ if parsed.password:
+ auth += f":{parsed.password}"
+ clean_url += f"{auth}@"
+ clean_url += f"{parsed.hostname or 'localhost'}"
+ clean_url += f":{parsed.port or 6379}"
+ clean_url += f"/{parsed.path.strip('/') or '0'}"
+
+ # Create Redis store with SSL support for cloud deployments
+ # Note: RedisStore uses redis.asyncio under the hood which handles
+ # SSL automatically via the rediss:// URL scheme
+ if use_ssl:
+ # For ElastiCache with self-signed certs, append ssl_cert_reqs
param
+ # redis.asyncio understands this as a query param in the URL
+ ssl_url = f"{clean_url}?ssl_cert_reqs=none"
+ redis_store = RedisStore(url=ssl_url)
+ logger.info("Created RedisStore with SSL at %s", parsed.hostname)
+ else:
+ redis_store = RedisStore(url=clean_url)
Review Comment:
**Suggestion:** Security issue: appending "ssl_cert_reqs=none" to the Redis
URL disables TLS certificate validation and allows MITM attacks; do not inject
an insecure parameter—use the original URL and let the Redis client/RedisStore
handle SSL, or provide a properly configured SSLContext instead. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ EventStore sessions vulnerable to network MITM.
- ❌ Redis connections accept invalid certificates.
- ⚠️ Multi-pod session sharing compromised in cloud.
```
</details>
```suggestion
# Create Redis store and let the underlying client handle SSL from
the URL;
# avoid disabling certificate verification by appending insecure
query params.
redis_store = RedisStore(url=redis_url)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable MCP Redis storage in Flask config: set
MCP_STORE_CONFIG['enabled']=True and
MCP_STORE_CONFIG['CACHE_REDIS_URL']='rediss://example.amazonaws.com:6379/1'
in your Flask
app configuration (superset/mcp_service/storage.py calls into this at
_get_store:
storage.py:67-75).
2. Call get_mcp_store(prefix=...) from application code
(superset/mcp_service/storage.py:get_mcp_store -> inner _get_store which
calls
_create_redis_store at storage.py:85-166).
3. Inside _create_redis_store (storage.py:85-166), the code detects rediss
scheme and
executes the branch at storage.py:136-141 which constructs ssl_url by
appending
?ssl_cert_reqs=none and passes it to RedisStore(url=ssl_url)
(storage.py:139-140).
4. Observe that RedisStore is created with a URL that disables TLS
certificate validation,
effectively allowing MITM attacks between the process and the Redis endpoint.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/storage.py
**Line:** 136:143
**Comment:**
*Security: Security issue: appending "ssl_cert_reqs=none" to the Redis
URL disables TLS certificate validation and allows MITM attacks; do not inject
an insecure parameter—use the original URL and let the Redis client/RedisStore
handle SSL, or provide a properly configured SSLContext instead.
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>
##########
superset/mcp_service/server.py:
##########
@@ -55,21 +65,79 @@ def configure_logging(debug: bool = False) -> None:
logging.info("🔍 SQL Debug logging enabled")
+def create_event_store(config: dict[str, Any] | None = None) -> Any | None:
+ """
+ Create an EventStore for MCP session management.
+
+ For multi-pod deployments, uses Redis-backed storage to share session state
+ across pods. For single-pod deployments, returns None (uses in-memory).
+
+ Args:
+ config: Optional config dict. If None, reads from MCP_STORE_CONFIG.
+
+ Returns:
+ EventStore instance if Redis URL is configured, None otherwise.
+ """
+ if config is None:
+ config = MCP_STORE_CONFIG
Review Comment:
**Suggestion:** Default config variable name mismatch: `create_event_store`
falls back to `MCP_STORE_CONFIG` when `config is None`, but the PR introduced
`MCP_EVENT_STORE_CONFIG`; update the fallback to use the correct exported
config variable name to avoid NameError. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Default EventStore config lookup fails on service start.
- ⚠️ Multi‑pod configuration ignored due to wrong fallback.
- ❌ Service startup may raise NameError.
```
</details>
```suggestion
config = MCP_EVENT_STORE_CONFIG
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Do not pass `event_store_config` to `run_server()` so
`create_event_store()` is called
with `config=None` (this is the default path in
`superset/mcp_service/server.py`).
2. `create_event_store()` executes `if config is None:` at `server.py:81`
and then `config
= MCP_STORE_CONFIG` at `server.py:82`.
3. If the actual configuration symbol in
`superset/mcp_service/mcp_config.py` is
`MCP_EVENT_STORE_CONFIG` (as the PR describes), `MCP_STORE_CONFIG` may be
undefined,
causing NameError at import time or leading to incorrect fallback behavior.
4. Because the PR introduced `MCP_EVENT_STORE_CONFIG`, updating the fallback
avoids a
concrete mismatch between exported config name and the one used here.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 82:82
**Comment:**
*Possible Bug: Default config variable name mismatch:
`create_event_store` falls back to `MCP_STORE_CONFIG` when `config is None`,
but the PR introduced `MCP_EVENT_STORE_CONFIG`; update the fallback to use the
correct exported config variable name to avoid NameError.
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>
##########
superset/mcp_service/storage.py:
##########
@@ -109,15 +115,51 @@ def _create_redis_store(
return None
try:
+ # Parse URL to handle SSL properly
+ parsed = urlparse(redis_url)
+ use_ssl = parsed.scheme == "rediss"
+
+ # Build clean URL without query params that may cause issues
+ clean_url = f"{parsed.scheme}://"
+ if parsed.username or parsed.password:
+ auth = parsed.username if parsed.username else ""
+ if parsed.password:
+ auth += f":{parsed.password}"
+ clean_url += f"{auth}@"
+ clean_url += f"{parsed.hostname or 'localhost'}"
+ clean_url += f":{parsed.port or 6379}"
+ clean_url += f"/{parsed.path.strip('/') or '0'}"
+
+ # Create Redis store with SSL support for cloud deployments
+ # Note: RedisStore uses redis.asyncio under the hood which handles
+ # SSL automatically via the rediss:// URL scheme
+ if use_ssl:
+ # For ElastiCache with self-signed certs, append ssl_cert_reqs
param
+ # redis.asyncio understands this as a query param in the URL
+ ssl_url = f"{clean_url}?ssl_cert_reqs=none"
+ redis_store = RedisStore(url=ssl_url)
+ logger.info("Created RedisStore with SSL at %s", parsed.hostname)
+ else:
+ redis_store = RedisStore(url=clean_url)
+ logger.info("Created RedisStore at %s", parsed.hostname)
+
+ # Return raw store if wrapping not requested
+ if not wrap:
+ return redis_store
+
+ # Wrap with prefix
+ if prefix is None:
+ logger.error("prefix is required when wrap=True")
+ return None
+
wrapper_type = store_config.get("WRAPPER_TYPE")
if not wrapper_type:
logger.error("MCP store WRAPPER_TYPE not configured")
return None
wrapper_class = _import_wrapper_class(wrapper_type)
- redis_store = RedisStore(url=redis_url)
store = wrapper_class(key_value=redis_store, prefix=prefix)
Review Comment:
**Suggestion:** If `prefix` is a callable (allowed by the public API),
passing it directly into the wrapper may be incorrect; evaluate callable
prefixes before handing them to the wrapper so the wrapper always receives a
string/prefix value. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Cache keys prefixed incorrectly, breaking lookups.
- ❌ EventStore streams mis-scoped across features.
- ⚠️ Docstring examples (callable prefixes) produce bugs.
```
</details>
```suggestion
if callable(prefix):
final_prefix = prefix()
else:
final_prefix = prefix
wrapper_class = _import_wrapper_class(wrapper_type)
store = wrapper_class(key_value=redis_store, prefix=final_prefix)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the documented example in storage.py's get_mcp_store: pass a callable
prefix (e.g.,
cache_prefix_lambda) into get_mcp_store (function entry and inner _get_store
at
storage.py:67-75).
2. get_mcp_store calls _create_redis_store (storage.py:85-166) and reaches
wrapper
instantiation at storage.py:160-163 where prefix is forwarded unchanged into
wrapper_class(..., prefix=prefix).
3. Because the code does not evaluate callables, the wrapper receives a
callable object
instead of a string; wrappers expecting strings use that callable as a
literal prefix
leading to incorrect key names and lookup failures.
4. Observe broken cache keys or EventStore streams using the callable
object's repr
instead of the intended string prefix.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/storage.py
**Line:** 160:161
**Comment:**
*Logic Error: If `prefix` is a callable (allowed by the public API),
passing it directly into the wrapper may be incorrect; evaluate callable
prefixes before handing them to the wrapper so the wrapper always receives a
string/prefix value.
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>
--
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]
