codeant-ai-for-open-source[bot] commented on code in PR #37216:
URL: https://github.com/apache/superset/pull/37216#discussion_r2706545626
##########
superset/mcp_service/storage.py:
##########
@@ -109,15 +116,67 @@ def _create_redis_store(
return None
try:
+ # Parse URL to handle SSL properly
+ parsed = urlparse(redis_url)
+ use_ssl = parsed.scheme == "rediss"
+
+ # RedisStore doesn't handle SSL from URL - it parses URL manually
+ # and ignores the scheme. We must create the Redis client ourselves.
+
+ db = 0
+ if parsed.path and parsed.path != "/":
+ try:
+ db = int(parsed.path.strip("/"))
+ except ValueError:
+ db = 0
+
+ redis_client: Redis[str]
+ if use_ssl:
+ # For ElastiCache with self-signed certs, disable cert
verification.
+ # NOTE: ssl_cert_reqs="none" disables certificate verification.
+ # Do not use Python None - that would default to CERT_REQUIRED.
+ redis_client = Redis(
+ host=parsed.hostname or "localhost",
+ port=parsed.port or 6379,
+ db=db,
+ username=parsed.username, # Support Redis 6+ ACLs
+ password=parsed.password,
+ decode_responses=True,
+ ssl=True,
+ ssl_cert_reqs="none",
Review Comment:
**Suggestion:** Passing the string "none" to `ssl_cert_reqs` is the wrong
type and will not be interpreted correctly; also disabling certificate
verification by default is unsafe. Compute `ssl_cert_reqs` using the `ssl`
module (e.g., `ssl.CERT_NONE` or `ssl.CERT_REQUIRED`) and make insecure
verification opt-in via configuration. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ TLS validation disabled for Redis connections.
- ❌ EventStore sessions exposed to MitM attacks.
- ⚠️ Multi-pod Redis setup insecure by default.
```
</details>
```suggestion
# For ElastiCache with self-signed certs, allow opting out of
verification via config.
allow_insecure = store_config.get("ALLOW_INSECURE_REDIS", False)
ssl_cert_reqs = ssl.CERT_NONE if allow_insecure else
ssl.CERT_REQUIRED
redis_client = Redis(
host=parsed.hostname or "localhost",
port=parsed.port or 6379,
db=db,
username=parsed.username, # Support Redis 6+ ACLs
password=parsed.password,
decode_responses=True,
ssl=True,
ssl_cert_reqs=ssl_cert_reqs,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure MCP to use TLS Redis: set MCP_STORE_CONFIG with `"enabled":
True` and
`"CACHE_REDIS_URL": "rediss://my-redis-host:6379/1"` in your Flask app
config (the storage
factory reads this at `superset/mcp_service/storage.py:_get_store` around
lines 68-76).
2. Call `superset/mcp_service/storage.get_mcp_store()` from application code
(the
top-level `get_mcp_store` at `superset/mcp_service/storage.py:37` enters
`_create_redis_store` at `superset/mcp_service/storage.py:86`).
3. Execution reaches the SSL branch in `_create_redis_store` (lines
~133-148) which
constructs a Redis client with `ssl=True` and `ssl_cert_reqs="none"`. This
both uses an
untyped string value where SSL constants (from the `ssl` module) are
expected and
effectively disables certificate verification by default.
4. Observed outcomes: depending on redis-py version, this either (a)
silently disables TLS
verification (security risk for cloud deployments) or (b) raises a TypeError
/ runtime
error if the client expects an int/constant for `ssl_cert_reqs`. Either way,
TLS handling
is incorrect and risky.
```
</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:** 135:146
**Comment:**
*Security: Passing the string "none" to `ssl_cert_reqs` is the wrong
type and will not be interpreted correctly; also disabling certificate
verification by default is unsafe. Compute `ssl_cert_reqs` using the `ssl`
module (e.g., `ssl.CERT_NONE` or `ssl.CERT_REQUIRED`) and make insecure
verification opt-in via configuration.
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]
