codeant-ai-for-open-source[bot] commented on code in PR #37216:
URL: https://github.com/apache/superset/pull/37216#discussion_r2699593405
##########
superset/mcp_service/server.py:
##########
@@ -55,21 +63,106 @@ 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_EVENT_STORE_CONFIG.
+
+ Returns:
+ EventStore instance if Redis is configured, None otherwise.
+ """
+ if config is None:
+ config = MCP_EVENT_STORE_CONFIG
+
+ if not config.get("enabled", False):
+ logging.info("EventStore: Using in-memory storage (single-pod mode)")
+ return None
+
+ redis_url = config.get("redis_url")
+ if not redis_url:
+ logging.warning(
+ "MCP_EVENT_STORE_CONFIG enabled but redis_url not set, "
+ "falling back to in-memory storage"
+ )
+ return None
+
+ try:
+ from fastmcp.server.event_store import EventStore
+ from key_value.aio.stores.redis import RedisStore
+
+ # Parse Redis URL to handle SSL properly
+ parsed = urlparse(redis_url)
+ use_ssl = parsed.scheme == "rediss"
+
+ # Build clean URL for RedisStore
+ # RedisStore from key_value uses different parameters than redis-py
+ clean_url = f"{parsed.scheme}://"
+ if parsed.password:
+ clean_url += f":{parsed.password}@"
+ 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
+ if use_ssl:
+ redis_store = RedisStore(
+ url=clean_url,
+ ssl_cert_reqs="none", # Disable cert verification for
ElastiCache
Review Comment:
**Suggestion:** The code passes the string "none" to `ssl_cert_reqs` when
creating `RedisStore`, which is not the standard SSL constant and may be
rejected by underlying libraries or silently misconfigure SSL handling; also
disabling certificate verification is a security risk. Use the `ssl` module
constant (e.g. `ssl.CERT_NONE`) if you must disable verification, and
preferably make this configurable instead of hardcoding. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Redis TLS connections may be misconfigured.
- ❌ Session data could be exposed over network.
- ⚠️ Insecure hardcoded disabling of certificate checks.
```
</details>
```suggestion
import ssl as _ssl
if use_ssl:
redis_store = RedisStore(
url=clean_url,
ssl_cert_reqs=_ssl.CERT_NONE, # Explicit SSL constant;
consider making this configurable
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure MCP_EVENT_STORE_CONFIG.redis_url to use TLS (scheme
"rediss://...") and
enable the event store.
2. Start the server (superset/mcp_service/server.py -> run_server());
create_event_store()
is invoked at superset/mcp_service/server.py:210 and sets use_ssl when
urlparse detects
"rediss" (line ~99).
3. create_event_store() reaches the RedisStore instantiation at
superset/mcp_service/server.py:111-118 and passes the literal string "none"
for
ssl_cert_reqs rather than the ssl module constant.
4. Depending on the underlying key_value.aio.stores.redis.RedisStore
implementation,
passing the string may either be ignored, misconfigure SSL verification, or
raise a
TypeError when the value is applied to ssl.SSLContext, resulting in insecure
connections
or runtime errors when connecting to ElastiCache.
```
</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:** 112:115
**Comment:**
*Security: The code passes the string "none" to `ssl_cert_reqs` when
creating `RedisStore`, which is not the standard SSL constant and may be
rejected by underlying libraries or silently misconfigure SSL handling; also
disabling certificate verification is a security risk. Use the `ssl` module
constant (e.g. `ssl.CERT_NONE`) if you must disable verification, and
preferably make this configurable instead of hardcoding.
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]
