diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 00b8d70e348..d72a30b5587 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -487,26 +487,17 @@ def _setup_user_context() -> User | None: _cleanup_session_on_error() continue logger.error("DB connection failed on retry during user setup: %s", e) + _cleanup_session_on_error() raise except ValueError as e: - # JWT user resolution failed (e.g. SAML subject not in DB). - # Log a security warning but fall back to middleware-provided - # g.user if available. This handles cases where the JWT - # resolver username format doesn't match the DB username - # (e.g., SAML subject vs email). A separate story should - # investigate whether any deployments hit this path and - # migrate them before removing the fallback entirely. - if has_request_context() and hasattr(g, "user") and g.user: - logger.warning( - "SECURITY: JWT user resolution failed (%s), falling " - "back to middleware-provided g.user=%s. This fallback " - "should be investigated and removed once all " - "deployments use consistent username resolution.", - e, - g.user.username, - ) - user = g.user - break + # User resolution failed — fail closed. Do not fall back to + # g.user from middleware, as that could allow a request to + # proceed as a different user in multi-tenant deployments. + # Clear g.user so error/audit logging doesn't attribute + # the denied request to the middleware-provided identity. + logger.error("MCP user resolution failed, denying request: %s", e) + if has_request_context(): + g.pop("user", None) raise g.user = user diff --git a/tests/unit_tests/mcp_service/test_auth_user_resolution.py b/tests/unit_tests/mcp_service/test_auth_user_resolution.py index 9779b1717c2..658e131f18b 100644 --- a/tests/unit_tests/mcp_service/test_auth_user_resolution.py +++ b/tests/unit_tests/mcp_service/test_auth_user_resolution.py @@ -427,3 +427,30 @@ def test_default_resolver_preferred_username_takes_priority() -> None: } ) assert default_user_resolver(None, token) == "alice" + + +def test_setup_user_context_propagates_valueerror(app) -> None: + """ValueError from get_user_from_request propagates — no g.user fallback. + + This is a security-critical test: when JWT resolution explicitly fails + (user not in DB), the request must be denied. The auth layer must NOT + silently fall back to g.user from middleware. + + Uses test_request_context() so g.user is preserved (not cleared by + the no-request-context guard), validating that the ValueError still + propagates even when middleware has set g.user. + """ + from superset.mcp_service.auth import _setup_user_context + + fallback_user = _make_mock_user("middleware_user") + + with app.test_request_context(): + g.user = fallback_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + side_effect=ValueError("User 'ghost' not found"), + ): + with pytest.raises(ValueError, match="User 'ghost' not found"): + _setup_user_context() + # g.user should be cleared after ValueError (no misleading audit) + assert not hasattr(g, "user") or g.user is None