feat(mcp): browser hello page with working middleware and config-driven content#40469
Closed
aminghadersohi wants to merge 5 commits into
Closed
Conversation
When a browser opens the MCP endpoint (Accept: text/html without application/json or text/event-stream), return a 200 HTML page explaining what the endpoint is and how to configure it in Claude Desktop, Claude Code, or Cursor. API and SSE clients continue to receive the existing JSON 401 response unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…HEAD - Introduce MCPJWTVerifier(JWTVerifier) base class that registers _auth_error_handler as the Starlette on_error callback; previously the callback was only wired inside DetailedJWTVerifier (MCP_JWT_DEBUG_ERRORS=True), so the HTML page was never shown in the default configuration - mcp_config.py non-debug path now uses MCPJWTVerifier instead of bare JWTVerifier; DetailedJWTVerifier inherits MCPJWTVerifier - Add _prefers_browser_html() helper: checks method (GET/HEAD only) and Accept header (case-insensitive); prevents POST/OPTIONS with text/html from incorrectly receiving a 200 HTML response - Rename _json_auth_error_handler -> _auth_error_handler, return type narrowed to Response (Starlette base class, matching on_error signature) - Add tests: POST+text/html -> 401, HEAD+text/html -> 200, uppercase Accept Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The PR renamed _json_auth_error_handler to _auth_error_handler in jwt_verifier.py (to reflect that it now returns HTML for browsers rather than always JSON), but test_jwt_verifier.py still imported the old name, causing a collection-time ImportError that failed all unit tests.
…ig-driven page - Replace on_error hook approach (dead code — BearerAuthBackend never raises AuthenticationError) with a Starlette BaseHTTPMiddleware that intercepts browser GET/HEAD requests before FastMCP's router returns 405 - Works regardless of MCP_AUTH_ENABLED — no longer requires auth to be on - Auth-aware: omits Authorization header from config snippet when auth is off - Config-driven via MCP_HELLO_PAGE dict: override title, server_key, show_transport, and clients list per deployment
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+728
to
+735
| auth_enabled = flask_app.config.get("MCP_AUTH_ENABLED", False) | ||
| page_config = flask_app.config.get("MCP_HELLO_PAGE", None) | ||
| return [ | ||
| StarletteMiddleware( | ||
| BrowserHelloMiddleware, | ||
| auth_enabled=auth_enabled, | ||
| page_config=page_config, | ||
| ) |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Browser hello page infers whether auth is required solely from MCP_AUTH_ENABLED, but auth can be enabled via MCP_AUTH_FACTORY even when that flag is False. In such valid custom-auth deployments, the hello page omits the Authorization header in the config snippet even though the MCP endpoint requires a Bearer token, causing users to copy a non-working client config.
Suggestion: Derive the hello-page auth behavior from the actual auth provider in use (e.g., based on whether _create_auth_provider returned a non-None provider or by passing an explicit auth_enabled flag from auth-provider creation) instead of relying only on MCP_AUTH_ENABLED.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/server.py
**Line:** 728:735
**Comment:**
*HIGH: Browser hello page infers whether auth is required solely from `MCP_AUTH_ENABLED`, but auth can be enabled via `MCP_AUTH_FACTORY` even when that flag is `False`. In such valid custom-auth deployments, the hello page omits the `Authorization` header in the config snippet even though the MCP endpoint requires a Bearer token, causing users to copy a non-working client config.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
Contributor
|
The provided pr_comments.csv file appears to be empty or contains only a header row without any actual review comments. Without specific content to analyze, I cannot validate the correctness of the flagged issue or provide a fix. Please ensure the file contains the expected review comments data. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## revert-40309-research-mcp-hello-page #40469 +/- ##
=====================================================================
Coverage 64.15% 64.15%
=====================================================================
Files 2592 2592
Lines 139256 139319 +63
Branches 32334 32342 +8
=====================================================================
+ Hits 89342 89385 +43
- Misses 48386 48399 +13
- Partials 1528 1535 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| " }" | ||
| ) | ||
| inner = ",\n".join(inner_parts) | ||
| return f'{{\n "mcpServers": {{\n "{server_key}": {{\n{inner}\n }}\n }}\n}}' |
Contributor
There was a problem hiding this comment.
Suggestion: The generated JSON config snippet inserts server_key directly into a JSON string literal; if the configured key contains quotes or backslashes, the snippet becomes invalid JSON and setup instructions will fail for users. Serialize or escape server_key as a JSON string before interpolation. [logic error]
Severity Level: Major ⚠️
- ⚠️ Generated MCP client snippet may be invalid JSON.
- ⚠️ Users copying config see confusing parse-time failures.Steps of Reproduction ✅
1. Configure the MCP hello page via Flask config so that
`flask_app.config["MCP_HELLO_PAGE"]` contains a `server_key` value with characters that
must be escaped in JSON, for example: `MCP_HELLO_PAGE = {"server_key": "my\"superset"}`;
this config is later read by `_build_starlette_middleware()` at
`superset/mcp_service/server.py:79`.
2. Start the MCP HTTP server via `run_server()` in
`superset/mcp_service/server.py:90-151`, which invokes
`_build_starlette_middleware(flask_app)` at `server.py:167-169`; that constructs
`BrowserHelloMiddleware` whose `__init__()` (`jwt_verifier.py:251-260`) calls
`_build_browser_hello_html(auth_enabled, page_config)` (`jwt_verifier.py:153-215`).
3. `_build_browser_hello_html()` calls `_build_config_snippet(auth_enabled, server_key,
show_transport)` at `jwt_verifier.py:163`, passing the configured `server_key` string
containing an embedded quote.
4. `_build_config_snippet()` builds the JSON snippet string using `return f'{{\n
"mcpServers": {{\n "{server_key}": {{\n{inner}\n }}\n }}\n}}'` at `jwt_verifier.py:150`
without escaping `server_key`; when the browser loads the hello page and the operator
copies the `<code>` block into an MCP client config, the resulting JSON contains a key
like `"my"superset": { ... }`, which is syntactically invalid and will cause JSON parsing
failures in the client.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/jwt_verifier.py
**Line:** 150:150
**Comment:**
*Logic Error: The generated JSON config snippet inserts `server_key` directly into a JSON string literal; if the configured key contains quotes or backslashes, the snippet becomes invalid JSON and setup instructions will fail for users. Serialize or escape `server_key` as a JSON string before interpolation.
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
Comment on lines
+189
to
+196
| <style>{_HTML_STYLES} | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="card"> | ||
| <div class="badge">MCP API Endpoint</div> | ||
| <h1>{title}</h1> | ||
| <p> |
Contributor
There was a problem hiding this comment.
Suggestion: Config-driven page values are interpolated directly into HTML without escaping, so a crafted MCP_HELLO_PAGE["title"] value can inject markup/script into the response. Escape dynamic HTML content before rendering to prevent XSS and broken markup. [security]
Severity Level: Major ⚠️
- ❌ Admin-defined hello page config can inject arbitrary HTML.
- ⚠️ XSS possible if config built from untrusted inputs.Steps of Reproduction ✅
1. Configure the Flask app used by the MCP service so that
`flask_app.config["MCP_HELLO_PAGE"]` includes an untrusted HTML string for `title`, for
example in deployment config: `MCP_HELLO_PAGE = {"title": "<img src=x onerror=alert(1)>",
"server_key": "superset"}`; this config is later read in `_build_starlette_middleware()`
at `superset/mcp_service/server.py:79`.
2. Start the MCP HTTP server via `run_server()` in `superset/mcp_service/server.py:90-151`
(e.g. `superset mcp run --port 5008`), which obtains a Flask app from `get_flask_app()`
(`superset/mcp_service/flask_singleton.py:99-106`) and then calls
`_build_starlette_middleware(flask_app)` at `server.py:167-169`.
3. `_build_starlette_middleware()` reads `page_config =
flask_app.config.get("MCP_HELLO_PAGE", None)` at `server.py:79` and constructs
`BrowserHelloMiddleware` with this `page_config`; `BrowserHelloMiddleware.__init__()`
(`jwt_verifier.py:251-260`) calls `_build_browser_hello_html(auth_enabled, page_config)`
at `jwt_verifier.py:153-215`.
4. `_build_browser_hello_html()` merges defaults and config into `cfg` and interpolates
`cfg["title"]` directly into the HTML template at `jwt_verifier.py:188-196` — specifically
`<title>{title}</title>` and `<h1>{title}</h1>` — without escaping; when a browser
requests `GET /mcp` with a browser-style `Accept: text/html` header (triggering
`BrowserHelloMiddleware.dispatch()` at `jwt_verifier.py:262-267`), the response contains
`<img src=x onerror=alert(1)>` in the title and heading, and the `onerror` JavaScript
executes in the MCP server's origin context.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/jwt_verifier.py
**Line:** 189:196
**Comment:**
*Security: Config-driven page values are interpolated directly into HTML without escaping, so a crafted `MCP_HELLO_PAGE["title"]` value can inject markup/script into the response. Escape dynamic HTML content before rendering to prevent XSS and broken markup.
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
Comment on lines
+265
to
+266
| if request.method in ("GET", "HEAD") and _prefers_browser_html(request): | ||
| return HTMLResponse(content=self._html, status_code=200) |
Contributor
There was a problem hiding this comment.
Suggestion: The hello middleware currently returns the HTML page for any GET/HEAD request that advertises text/html, without checking the request path. That makes unrelated or unknown routes return 200 with the MCP page instead of their real response (for example, expected 404/405), which can mask routing errors and break endpoint behavior. Restrict this branch to the MCP endpoint path only. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Unknown MCP HTTP paths return 200 hello instead 404.
- ⚠️ Masks routing errors when debugging MCP reverse proxies.Steps of Reproduction ✅
1. Start the MCP HTTP server via `run_server()` in
`superset/mcp_service/server.py:90-151`, e.g. using the documented CLI `superset mcp run
--port 5008`, which constructs an HTTP app and passes `middleware=starlette_middleware` at
`server.py:179-185`.
2. `_build_starlette_middleware()` in `superset/mcp_service/server.py:72-87` wraps the
FastMCP ASGI app with `BrowserHelloMiddleware` (from
`superset/mcp_service/jwt_verifier.py:239-267`) as Starlette middleware, so it sees all
incoming HTTP requests regardless of path.
3. From a browser (or with curl mimicking a browser header), send a GET request to an
undefined path on the MCP server, for example: `GET /nonexistent HTTP/1.1` with header
`Accept: text/html,*/*` against `http://localhost:5008` (the host/port used in
`run_server()`).
4. `BrowserHelloMiddleware.dispatch()` at `jwt_verifier.py:262-267` evaluates
`request.method in ("GET", "HEAD")` and calls `_prefers_browser_html(request)` at
`jwt_verifier.py:222-236`, which returns True because `Accept` contains `text/html` and
not `application/json`/`text/event-stream`; it then immediately returns
`HTMLResponse(content=self._html, status_code=200)` for `/nonexistent`, so the underlying
FastMCP router never runs and the client sees a 200 MCP hello page instead of the expected
404/405 for an unknown route.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/jwt_verifier.py
**Line:** 265:266
**Comment:**
*Incorrect Condition Logic: The hello middleware currently returns the HTML page for any `GET`/`HEAD` request that advertises `text/html`, without checking the request path. That makes unrelated or unknown routes return `200` with the MCP page instead of their real response (for example, expected `404`/`405`), which can mask routing errors and break endpoint behavior. Restrict this branch to the MCP endpoint path only.
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
- Escape server_key via superset.utils.json.dumps() to prevent invalid JSON in the config snippet when the key contains quotes or backslashes - HTML-escape title and client names to prevent XSS from crafted MCP_HELLO_PAGE config - Restrict BrowserHelloMiddleware to the MCP path only (default /mcp) so unrelated routes return their real 404/405 instead of the hello page - Derive auth_enabled from the actual auth provider returned by _create_auth_provider() so MCP_AUTH_FACTORY deployments show the correct Authorization header even when MCP_AUTH_ENABLED is False
Antonio-RiveroMartnez
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds a browser-friendly hello page for the MCP endpoint. When a user opens the MCP URL in a browser, they see a styled HTML page with setup instructions instead of a
405 Method Not Allowed.Implementation:
BrowserHelloMiddleware(BaseHTTPMiddleware) interceptsGET/HEADrequests with a browserAccept: text/htmlheader before they reach FastMCP's router. Wired inserver.pyfor both single-pod (mcp.run()) and multi-pod (mcp.http_app()+ uvicorn) paths.Features:
Authorizationheader when auth is off, includes it when auth is onAPP_NAME; logo shown ifAPP_ICONis an absolute URLMCP_HELLO_PAGEdict — deployments can overridetitle,server_key,show_transport,clients,logo_url, andapp_nameBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
405 Method Not Allowedfor any browserGETto/mcpAfter: 200 HTML page with setup instructions. Config snippet adapts to auth state:
{"url": "<this-url>", "transport": "streamable-http"}"Authorization": "Bearer <your-api-key>"TESTING INSTRUCTIONS
superset mcp run --port 5008 --debughttp://localhost:5008/mcp— should show the HTML hello pagecurl -s -H "Accept: text/html,*/*" http://localhost:5008/mcp— should return 200 HTMLcurl -s -H "Accept: application/json" http://localhost:5008/mcp— should return 405 (unaffected)MCP_AUTH_ENABLED = Truein config, restart — config snippet should includeAuthorizationheaderMCP_HELLO_PAGE = {"title": "My MCP", "server_key": "my-superset"}— page should reflect overridesADDITIONAL INFORMATION