AurimasNav opened a new issue, #43327:
URL: https://github.com/apache/superset/issues/43327

   ### Bug description
   
   `_create_auth_provider` in `superset/mcp_service/server.py` swallows any 
exception raised by a configured `MCP_AUTH_FACTORY` and returns `None`. The MCP 
server then starts and serves **with no authentication at all**.
   
   
https://github.com/apache/superset/blob/master/superset/mcp_service/server.py#L813-L822
   
   ```python
       if auth_factory := flask_app.config.get("MCP_AUTH_FACTORY"):
           try:
               auth_provider = auth_factory(flask_app)
               ...
           except Exception:
               # Do not log the exception — it may contain secrets
               logger.error("Failed to create auth provider from 
MCP_AUTH_FACTORY")
   ```
   
   `auth_provider` stays `None`, is returned, and is passed straight to 
`init_fastmcp_server(auth=auth_provider, ...)` — which means an unauthenticated 
server, not a failed startup.
   
   **The same function already treats this as unacceptable one branch below.** 
The default-factory path re-raises `MCPAuthConfigError`, with a comment stating 
the intent explicitly:
   
   ```python
           except MCPAuthConfigError:
               # A misconfiguration that must fail closed: re-raise so the 
service
               # refuses to start rather than falling through to an 
unauthenticated
               # server.
               raise
   ```
   
   So the fail-closed principle is accepted; the custom-factory branch is 
simply inconsistent with it.
   
   **Why this is more than theoretical.** A factory is ordinary Python 
evaluated at startup, so it fails for mundane reasons that have nothing to do 
with the operator's intent:
   
   - a missing environment variable → `KeyError` (e.g. reading a tenant id or 
audience from the environment)
   - a dependency moving a symbol → `ImportError` 
(`fastmcp.server.auth.providers.jwt.JWTVerifier`)
   - a verifier's signature changing → `TypeError` on an unexpected keyword 
argument
   
   The last two need no change by the operator at all. We hit exactly that: an 
unpinned `fastmcp` moved 3.4.6 → 3.4.7 between pod restarts. Nothing in our 
configuration changed, and a breaking change in `JWTVerifier` would have 
silently produced an unauthenticated endpoint fronting a data warehouse.
   
   The failure is also close to invisible: the process starts, the port 
listens, health checks pass, and the only signal is a single `logger.error` 
line with no exception attached. An unauthenticated MCP server is 
indistinguishable from a healthy one unless something explicitly asserts that 
an anonymous request is rejected.
   
   ### How to reproduce
   
   1. Configure `MCP_AUTH_ENABLED = True` and an `MCP_AUTH_FACTORY` in 
`superset_config.py`.
   2. Make the factory raise — for example have it read an environment variable 
that is not set.
   3. Start the MCP server and send an unauthenticated request to `/mcp`.
   
   Expected: startup fails, or requests are rejected.
   Actual: the server starts and the request succeeds.
   
   ### Suggested fix
   
   Re-raise from the `MCP_AUTH_FACTORY` branch, matching the 
`MCPAuthConfigError` handling below it. If a configured factory cannot produce 
a provider, that is a misconfiguration, and refusing to start is safer than 
serving without auth.
   
   A narrower alternative, if failing startup is considered too blunt: keep the 
catch but treat `auth_provider is None` as fatal when `MCP_AUTH_FACTORY` was 
configured — i.e. never silently downgrade an explicit auth configuration to 
none.
   
   Re-raising need not leak secrets. The exception type and traceback are what 
matter for diagnosis, and the existing comment's concern applies to the message 
text, which can be omitted while still failing closed.
   
   ### Superset version
   
   master / latest-dev
   
   ### Python version
   
   3.11
   
   ### Node version
   
   Not applicable
   
   ### Browser
   
   Not applicable
   
   ### Additional context
   
   Observed on 6.1.0 and confirmed present on `master` at the time of filing. 
We have worked around it deployment-side by asserting at startup that the 
factory resolves to the expected verifier type, and with a readiness probe that 
requires an unauthenticated request to return 401 — but that only protects our 
own deployment, and the default is unsafe for anyone using `MCP_AUTH_FACTORY`.
   
   Happy to open a PR if the maintainers agree with the re-raise approach.
   
   *Investigated and drafted with AI assistance; the behaviour described was 
verified against a running instance and against the code on `master`.*
   


-- 
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]

Reply via email to