codeant-ai-for-open-source[bot] commented on code in PR #40471:
URL: https://github.com/apache/superset/pull/40471#discussion_r3312566194
##########
superset/mcp_service/server.py:
##########
@@ -717,6 +718,52 @@ def build_middleware_list() -> list[Middleware]:
]
+def _build_starlette_middleware(
+ flask_app: Any | None = None, auth_provider: Any | None = None
+) -> list[Any]:
+ from starlette.middleware import Middleware as StarletteMiddleware
+
+ if flask_app is None:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ flask_app = get_flask_app()
+ # Auth is active if a non-None provider was returned (covers both
+ # MCP_AUTH_ENABLED and MCP_AUTH_FACTORY paths).
+ if auth_provider is not None:
+ auth_enabled = True
+ else:
+ auth_enabled = bool(
+ flask_app.config.get("MCP_AUTH_FACTORY")
+ or flask_app.config.get("MCP_AUTH_ENABLED", False)
+ )
Review Comment:
**Suggestion:** The hello-page auth hint is derived from Flask config flags
instead of the actual auth object used by the running MCP app. If
`MCP_AUTH_FACTORY` is configured but returns `None`, or if
`use_factory_config=True` supplies `auth` directly in factory config, the page
can incorrectly instruct users to include (or omit) `Authorization` headers.
Derive `auth_enabled` from the effective runtime auth configuration (the
instantiated provider/factory config auth), not only from config presence.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP hello page misleads on authentication header requirements.
- ⚠️ Users may misconfigure MCP clients, causing avoidable auth failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Flask with `MCP_AUTH_FACTORY` pointing to a callable but no
`MCP_AUTH_ENABLED` flag; start the MCP server via `run_server()` in
`superset/mcp_service/server.py:128-189` with `use_factory_config=False`.
`_create_auth_provider(flask_app)` at `server.py:26-57` invokes the factory
and assigns
its return value to `auth_provider`.
2. In a misconfigured or conditional setup where the factory returns `None`,
`_create_auth_provider()` logs `"Auth provider created from
MCP_AUTH_FACTORY: None"` and
returns `auth_provider = None` (still at `server.py:33-39,57`).
`init_fastmcp_server()` is
then called with `auth=auth_provider` at `server.py:185-188`, and in
`superset/mcp_service/app.py:55-57` the global `mcp.auth` is left unset
because `auth` is
`None`, so HTTP auth is effectively disabled.
3. After creating the EventStore, `run_server()` builds the Starlette
middleware list at
`superset/mcp_service/server.py:205-208` via
`_build_starlette_middleware(flask_app=flask_app,
auth_provider=auth_provider)`. Inside
`_build_starlette_middleware()` at `server.py:91-99`, `auth_provider` is
`None` but
`flask_app.config.get("MCP_AUTH_FACTORY")` is a truthy callable, so
`auth_enabled` is
computed as `True` and passed into `BrowserHelloMiddleware` via
`StarletteMiddleware(BrowserHelloMiddleware, auth_enabled=auth_enabled,
page_config=page_config)` at `server.py:120-124`.
4. A browser `GET` to `/mcp` hits `BrowserHelloMiddleware.dispatch()` in
`superset/mcp_service/jwt_verifier.py:279-284`. Because `auth_enabled=True`
was computed
from config flags, `_build_browser_hello_html()` (called from
`BrowserHelloMiddleware.__init__` at `jwt_verifier.py:268-277`) generates a
config snippet
including `"headers": { "Authorization": "Bearer <your-api-key>" }` in
`_build_config_snippet()` at `jwt_verifier.py:141-158`, even though the
FastMCP server is
running with no auth provider. The hello page therefore incorrectly tells
users that an
Authorization header is required when it is not.
5. Conversely, when `run_server()` is invoked with `use_factory_config=True`
(factory-based deployments), `factory_config = get_mcp_factory_config()` at
`server.py:156-160` can include an `auth` provider passed into
`create_mcp_app()` at
`app.py:513-71`. In this branch `auth_provider` and `flask_app` are not
passed into
`_build_starlette_middleware()` (both `None` at `server.py:205-208`), so
`_build_starlette_middleware()` falls back solely to
`flask_app.config.get("MCP_AUTH_FACTORY")` / `MCP_AUTH_ENABLED` at
`server.py:96-99`. If
those flags are unset while an `auth` object is provided via factory config,
the hello
page renders with `auth_enabled=False` and omits the Authorization header,
even though the
MCP app actually enforces auth. This demonstrates the logic error: the hello
page's auth
hint is derived from config presence rather than the effective runtime auth
provider.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=503d0296d7674902a94af1eb75cde6f6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=503d0296d7674902a94af1eb75cde6f6&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/server.py
**Line:** 732:738
**Comment:**
*Logic Error: The hello-page auth hint is derived from Flask config
flags instead of the actual auth object used by the running MCP app. If
`MCP_AUTH_FACTORY` is configured but returns `None`, or if
`use_factory_config=True` supplies `auth` directly in factory config, the page
can incorrectly instruct users to include (or omit) `Authorization` headers.
Derive `auth_enabled` from the effective runtime auth configuration (the
instantiated provider/factory config auth), not only from config presence.
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%2F40471&comment_hash=06ba6cd57c3adf19c408f8318a023643ae0a96fe49d36604a358d09a5713996f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40471&comment_hash=06ba6cd57c3adf19c408f8318a023643ae0a96fe49d36604a358d09a5713996f&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]
