rusackas commented on code in PR #40861:
URL: https://github.com/apache/superset/pull/40861#discussion_r3375302913
##########
superset/mcp_service/auth.py:
##########
@@ -369,14 +371,11 @@ def get_user_from_request() -> User:
"MCP_DEV_USERNAME is not configured",
"g.user was not set by external middleware",
]
- configured_prefixes = current_app.config.get("FAB_API_KEY_PREFIXES",
["sst_"])
- prefix_example = configured_prefixes[0] if configured_prefixes else "sst_"
- raise ValueError(
- "No authenticated user found. Tried:\n"
- + "\n".join(f" - {d}" for d in details)
- + f"\n\nEither pass a valid API key (Bearer {prefix_example}...), "
- "JWT token, or configure MCP_DEV_USERNAME for development."
+ logger.warning(
+ "MCP request could not be authenticated. Tried: %s",
+ "; ".join(details),
)
+ raise ValueError("Authentication required. No valid credentials provided.")
Review Comment:
This does not apply:
`test_auth_user_resolution.py::test_raises_when_no_auth_source` already asserts
`pytest.raises(ValueError, match="Authentication required")`, which matches the
new message — it does not expect `"No authenticated user found"`. The generic
error is raised as `MCPNoAuthSourceError`, a `ValueError` subclass, so that
test passes (verified locally). No call sites depend on the old string.
##########
tests/unit_tests/mcp_service/test_auth_api_key.py:
##########
@@ -113,13 +113,37 @@ def test_api_key_disabled_skips_auth(app) -> None:
# Without API key auth or MCP_DEV_USERNAME, should raise ValueError
# about no authenticated user (not about invalid API key)
- with pytest.raises(ValueError, match="No authenticated user found"):
+ with pytest.raises(ValueError, match="Authentication required"):
get_user_from_request()
# SecurityManager API key methods should never be called
mock_sm.extract_api_key_from_request.assert_not_called()
[email protected]("_disable_api_keys")
+def test_unauthenticated_error_does_not_leak_config(app) -> None:
+ """The error returned to an unauthenticated client must not reveal which
+ auth mechanisms are configured."""
+ app.config["MCP_AUTH_ENABLED"] = True
+ app.config["MCP_JWT_SECRET"] = "super-secret-value" # noqa: S105
+
+ with app.test_request_context():
+ g.user = None
+ with pytest.raises(ValueError, match="Authentication required") as
exc_info:
+ get_user_from_request()
+
+ message = str(exc_info.value)
+ assert message == "Authentication required. No valid credentials provided."
+ for leaked in (
+ "MCP_AUTH_ENABLED",
+ "JWT keys configured",
+ "API key",
+ "MCP_DEV_USERNAME",
+ "super-secret-value",
+ ):
+ assert leaked not in message
+
Review Comment:
Good catches on both points. Fixed: the test now snapshots and restores
`MCP_AUTH_ENABLED`/`MCP_JWT_SECRET` in a `finally` so the module-scoped `app`
fixture cannot leak config into later tests, and it now asserts via `caplog`
that the auth diagnostics (e.g. `MCP_AUTH_ENABLED=True`) are still emitted
server-side — confirming they are kept out of the client-facing message.
--
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]
