rusackas commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3832235766
##########
superset/mcp_service/__main__.py:
##########
@@ -157,8 +157,19 @@ def main() -> None:
sys.stderr.write(f"[MCP] Client disconnected: {e}\n")
sys.exit(0)
else:
- # For other transports, use normal initialization
- init_fastmcp_server()
+ # For other transports (network listeners), install the same auth
+ # provider as the supported entry point (`superset mcp run` ->
+ # server.run_server()) instead of starting with no verifier at all.
+ # _create_auth_provider fails closed (raises MCPAuthConfigError) when
+ # auth is configured but a verifier could not be built, so letting
+ # that propagate here refuses to start rather than silently running
+ # this transport unauthenticated.
+ from superset.mcp_service.flask_singleton import get_flask_app
+ from superset.mcp_service.server import _create_auth_provider
+
+ flask_app = get_flask_app()
+ auth_provider = _create_auth_provider(flask_app)
+ init_fastmcp_server(auth=auth_provider)
Review Comment:
Good catch, fixed. The stdio entrypoint already skipped this; the
network-transport branch did too. Both now call
create_response_caching_middleware() same as run_server().
##########
superset/mcp_service/middleware.py:
##########
@@ -219,15 +219,23 @@ def _is_user_error(error: Exception) -> bool:
def _sanitize_params(params: dict[str, Any]) -> dict[str, Any]:
- """Remove sensitive fields from params before logging."""
+ """Remove sensitive fields from params before logging.
+
+ Recurses into nested containers so sensitive keys are redacted no matter
+ which wrapper they arrive under (``arguments``, ``request``, etc.).
+ """
if not isinstance(params, dict):
return params
result: dict[str, Any] = {}
for k, v in params.items():
if k.lower() in _SENSITIVE_PARAM_KEYS:
result[k] = "[REDACTED]"
- elif k == "arguments" and isinstance(v, dict):
+ elif isinstance(v, dict):
result[k] = _sanitize_params(v)
+ elif isinstance(v, list):
+ result[k] = [
+ _sanitize_params(item) if isinstance(item, dict) else item for
item in v
Review Comment:
Good catch, fixed. Recursing into list elements now too, not just dict
elements, so a list nested inside a list gets sanitized.
##########
superset/mcp_service/chart/tool/restore_chart.py:
##########
@@ -116,6 +116,44 @@ async def restore_chart(
return RestoreChartResponse(success=False, error=msg,
error_type="NotFound")
chart_id = chart.id
+
+ # The lookup above deliberately bypasses the RBAC base filter (see
+ # _find_chart_for_restore), so enforce the restore audience *before*
+ # composing any response that embeds the chart's name: without this gate,
+ # iterating identifiers would disclose the existence and exact title of
+ # charts the caller cannot see (the web API answers 404 for those).
+ from superset import security_manager
+ from superset.exceptions import SupersetSecurityException
+
+ try:
+ security_manager.raise_for_editorship(chart)
+ except SupersetSecurityException:
+ from superset.daos.chart import ChartDAO
+
+ # Distinguish "visible but not an editor" from "outside the caller's
+ # RBAC scope": the latter must be indistinguishable from a chart that
+ # does not exist.
+ visible = ChartDAO.find_by_id_or_uuid(
Review Comment:
Good catch, fixed. Wrapped the editorship check and its re-lookup in the
same SQLAlchemyError handling as the initial lookup, with rollback. Did the
same for restore_dashboard.py since it has the identical pattern.
##########
superset/mcp_service/mcp_config.py:
##########
@@ -521,7 +539,14 @@ def create_default_mcp_auth_factory(app: Flask) ->
Optional[Any]:
if not (jwks_uri or public_key or secret):
logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret
configured")
if not (api_key_enabled or guest_enabled):
- return None
+ # Fail closed: the surrounding bootstrap would otherwise turn
+ # a None provider into an unauthenticated server.
+ raise MCPAuthConfigError(
+ "MCP_AUTH_ENABLED is True but no JWT verification key is "
+ "configured; refusing to start an unauthenticated MCP "
+ "server. Set MCP_JWKS_URI, MCP_JWT_PUBLIC_KEY, or "
+ "MCP_JWT_SECRET (with MCP_JWT_ALGORITHM='HS256')."
+ )
Review Comment:
Good catch, fixed. Missing JWT keys now raises MCPAuthConfigError
unconditionally when MCP_AUTH_ENABLED is True, same fail-closed rule already
applied a few lines down for a verifier build failure.
--
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]
