codeant-ai-for-open-source[bot] commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3641285286
##########
superset/mcp_service/middleware.py:
##########
@@ -132,6 +138,29 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+def _invoke_error_hook(error: Exception, hook_context: dict[str, Any]) -> None:
+ """Invoke the operator-configured ``MCP_ERROR_HOOK``, if any.
+
+ Kept vendor-neutral (no ``sentry_sdk`` import here) so the OSS repo has
+ no hard dependency on any particular error tracker — operators wire
+ their own hook (e.g. calling ``sentry_sdk.capture_exception``) via
+ ``MCP_ERROR_HOOK`` in ``superset_config.py``. Hook failures are logged
+ and swallowed; they must never affect the MCP response.
+ """
+ try:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ hook = get_flask_app().config.get("MCP_ERROR_HOOK")
+ except Exception: # noqa: BLE001
+ return
+ if hook is None:
+ return
+ try:
+ hook(error, hook_context)
Review Comment:
**Suggestion:** The configured error hook is invoked synchronously from the
async request path, so any slow or blocking hook implementation will block the
event loop and stall concurrent tool calls. Execute hook calls off the event
loop (or support async hooks with timeout/isolation) so observability side
effects cannot degrade request throughput. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slow MCP_ERROR_HOOK blocks FastMCP async event loop.
- ⚠️ Concurrent MCP tool calls experience increased latency under errors.
- ⚠️ Error bursts can degrade MCP service throughput.
- ⚠️ Affects GlobalErrorHandler and fallback StructuredContentStripper paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an MCP error hook via Flask config as used in
`_invoke_error_hook` (lines
141–162 in `superset/mcp_service/middleware.py`): in `superset_config.py` or
tests, set
`get_flask_app().config["MCP_ERROR_HOOK"]` to a function that performs
blocking work, e.g.
`def slow_hook(error, ctx): import time; time.sleep(5)`.
2. Trigger the hook from the primary error handler by causing a system-class
error in a
tool: make a tool implementation raise a database `OperationalError` so
`GlobalErrorHandlerMiddleware._handle_error` (starting around line 773)
classifies it as
non-user and calls `_invoke_error_hook(error, {...})` at lines 792–806.
3. Run this flow under an async event loop (the MCP server path uses `async
def`
middleware): while invoking the failing tool call from step 2, concurrently
schedule
another quick MCP tool call or async task in the same loop.
4. Observe that when `_invoke_error_hook` executes `hook(error,
hook_context)`
synchronously at line 159, the event loop is blocked for the duration of
`slow_hook` (e.g.
~5 seconds), delaying the concurrent tool call and demonstrating that a slow
or blocking
MCP_ERROR_HOOK can stall other in-flight MCP requests.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=36c9affab9f147478df61b42242acc1b&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=36c9affab9f147478df61b42242acc1b&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/middleware.py
**Line:** 159:159
**Comment:**
*Possible Bug: The configured error hook is invoked synchronously from
the async request path, so any slow or blocking hook implementation will block
the event loop and stall concurrent tool calls. Execute hook calls off the
event loop (or support async hooks with timeout/isolation) so observability
side effects cannot degrade request throughput.
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%2F41921&comment_hash=c1892d01da304f0ecc93b5dda9c2a707b6997443acd128c95315cafdd26fd53f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=c1892d01da304f0ecc93b5dda9c2a707b6997443acd128c95315cafdd26fd53f&reaction=dislike'>👎</a>
##########
superset/mcp_service/middleware.py:
##########
@@ -456,8 +615,38 @@ async def on_call_tool(
# GlobalErrorHandlerMiddleware, ValueError, TypeError, etc. —
# will cause encoding failures on the wire.
mcp_call_id = _mcp_call_id_var.get(None)
+ # This is the documented "must never propagate" point, but
+ # formatting/sanitizing both call str(e) — a pathological
+ # __str__ would make this handler itself raise past the
+ # middleware chain. Fall back to the exception class name.
+ try:
+ error_text = f"Error: {e}"
+ sanitized_message = _sanitize_error_for_logging(e)
Review Comment:
**Suggestion:** This returns raw exception text to the client in the
fallback error path, which can leak sensitive internal details (SQL fragments,
connection info, tokens) when unexpected exceptions bypass the primary handler.
Use the sanitized message for client-facing content and keep raw exception
details only for internal logging/hook capture. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP tool clients see raw internal exception messages.
- ⚠️ Sensitive SQL or connection details leaked in responses.
- ⚠️ Bypasses existing _sanitize_error_for_logging protections for fallback.
- ⚠️ Affects StructuredContentStripperMiddleware fallback error responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a test module, import `StructuredContentStripperMiddleware` and
`_sanitize_error_for_logging` from `superset/mcp_service/middleware.py` (see
`async def
on_call_tool` starting at line 601 and the `except Exception as e:` block
around lines
617–627).
2. Define an async `call_next` stub that always raises a non-ToolError
exception with
sensitive text, for example:
`async def call_next(context): raise ValueError("SELECT secret_token FROM
users WHERE
password='raw'")`.
3. Construct a minimal `MiddlewareContext` (from
`fastmcp.server.middleware.MiddlewareContext` imported at line 27) and
invoke `await
StructuredContentStripperMiddleware().on_call_tool(context, call_next)` so
the exception
from step 2 is caught by the `except Exception as e:` block at lines 617–627.
4. Observe that the returned `ToolResult` (constructed at lines 648–651) has
`result.content[0].text` equal to `f"Error: {e}"`, which includes the full
unsanitized
exception message (including the SQL and raw secret text), while
`_sanitize_error_for_logging(e)` (line 624) is only used for the hook
context and not for
the client-facing error string.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9169da6ab9644ecba8cca82703c9e1d5&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=9169da6ab9644ecba8cca82703c9e1d5&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/middleware.py
**Line:** 623:624
**Comment:**
*Security: This returns raw exception text to the client in the
fallback error path, which can leak sensitive internal details (SQL fragments,
connection info, tokens) when unexpected exceptions bypass the primary handler.
Use the sanitized message for client-facing content and keep raw exception
details only for internal logging/hook capture.
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%2F41921&comment_hash=b5f5600d2cffaee3415f5b1f4bf5fa3a4c8cf67d8f3daac5525c6eead056c19f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=b5f5600d2cffaee3415f5b1f4bf5fa3a4c8cf67d8f3daac5525c6eead056c19f&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]
