You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/24 17:59:06 UTC
(superset) branch master updated: fix(mcp): tighten auth and request validation edge cases (#43388)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new c980b3a3615 fix(mcp): tighten auth and request validation edge cases (#43388)
c980b3a3615 is described below
commit c980b3a3615938f49c04b370e8463bab8c508a27
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Mon Aug 24 10:58:59 2026 -0700
fix(mcp): tighten auth and request validation edge cases (#43388)
Co-authored-by: Superset Dev <de...@superset.apache.org>
Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
UPDATING.md | 1 +
docs/admin_docs/configuration/mcp-server.mdx | 7 +-
superset/mcp_service/README.md | 21 ++-
superset/mcp_service/__main__.py | 27 +++-
superset/mcp_service/auth.py | 12 +-
superset/mcp_service/caching.py | 21 +++
superset/mcp_service/chart/tool/restore_chart.py | 47 +++++++
.../dashboard/tool/restore_dashboard.py | 50 +++++++
superset/mcp_service/mcp_config.py | 147 ++++++++++++++++----
superset/mcp_service/middleware.py | 27 +++-
superset/mcp_service/server.py | 36 +++--
superset/mcp_service/sql_lab/tool/execute_sql.py | 7 +-
superset/mcp_service/system/schemas.py | 28 +++-
superset/mcp_service/system/tool/find_users.py | 12 +-
.../mcp_service/chart/tool/test_restore_chart.py | 120 ++++++++++++++++-
.../dashboard/tool/test_restore_dashboard.py | 121 ++++++++++++++++-
.../mcp_service/sql_lab/tool/test_execute_sql.py | 46 +++++++
.../mcp_service/system/tool/test_find_users.py | 65 ++++++++-
.../mcp_service/test_auth_user_resolution.py | 30 +++++
.../mcp_service/test_guest_token_auth.py | 21 +--
.../unit_tests/mcp_service/test_main_entrypoint.py | 123 +++++++++++++++++
tests/unit_tests/mcp_service/test_mcp_caching.py | 74 +++++++++-
tests/unit_tests/mcp_service/test_mcp_config.py | 73 ++++++++--
.../mcp_service/test_mcp_config_hs256_guard.py | 150 +++++++++++++++++++++
tests/unit_tests/mcp_service/test_mcp_server.py | 49 +++++++
tests/unit_tests/mcp_service/test_middleware.py | 101 ++++++++++++++
26 files changed, 1324 insertions(+), 92 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index a4adec96cf3..a948107ef8d 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -58,6 +58,7 @@ the old counter to use the outcome-specific replacements.
- [42930](https://github.com/apache/superset/pull/42930): Dataset import data-URI fetches no longer honor an HTTP(S) proxy when `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is `False` (the default): the connection is now made directly to the destination so the peer-address check validates the real target instead of a proxy's. Deployments that require an egress proxy to reach legitimate external data URLs for dataset import should set `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS = True` or other [...]
- [42935](https://github.com/apache/superset/pull/42935): The MCP service now refuses to start (`MCPAuthConfigError`) when `MCP_JWT_ISSUER` trusts more than one issuer and no `MCP_USER_RESOLVER` is configured, instead of only logging a warning. This was already a documented misconfiguration (the default resolver isn't issuer-scoped, so distinct trusted issuers minting the same username/email would resolve to the same Superset user); deployments trusting multiple issuers must configure an [...]
+- [43388](https://github.com/apache/superset/pull/43388): The MCP service now refuses to start (`MCPAuthConfigError`) if `MCP_DEV_USERNAME` and `MCP_AUTH_ENABLED = True` are both set, and separately if `MCP_AUTH_ENABLED = True` but no usable JWT key material is configured (RSA key/JWKS, or an explicit `MCP_JWT_SECRET` for HMAC) — both previously started with authentication silently weaker than configured. Deployments combining a dev-mode username with JWT auth enabled, or enabling JWT au [...]
- [42393](https://github.com/apache/superset/pull/42393): Exported dataset YAML now carries a `uuid` for each metric and column so that custom folder assignments (which reference metrics/columns by UUID) survive an import into another workspace. This affects any export bundle that contains datasets, not just a dataset export: chart, dashboard, database and full-asset exports all embed the same dataset YAML, so a dashboard exported from this release also fails to import into an older one [...]
- [42300](https://github.com/apache/superset/pull/42300): Timeseries charts (line/area/bar) with a Y-axis bound in effect — either an explicit `yAxisBounds` or one derived from `truncateYAxis` — now clamp out-of-range data points to that bound instead of letting ECharts drop the point (and the line segments around it) entirely. Any existing chart with a configured Y-axis bound and data outside it will look different after upgrading: a gap becomes a point pinned to the boundary. The clamp [...]
- [42087](https://github.com/apache/superset/pull/42087): Stored calculated-column and metric expressions are validated when a query is built, under the same sub-query policy already applied to adhoc expressions. Previously only the dataset update path checked them on save, so expressions written by v1 import, by dataset duplication, or before that check existed were never validated. Since `ALLOW_ADHOC_SUBQUERY` defaults to `False` (see [19242](https://github.com/apache/superset/pull/192 [...]
diff --git a/docs/admin_docs/configuration/mcp-server.mdx b/docs/admin_docs/configuration/mcp-server.mdx
index 9edd893d5f1..c6e6acd4656 100644
--- a/docs/admin_docs/configuration/mcp-server.mdx
+++ b/docs/admin_docs/configuration/mcp-server.mdx
@@ -782,13 +782,18 @@ Enable response caching for read-heavy workloads (dashboards/datasets that don't
```python
MCP_CACHE_CONFIG = {
"enabled": True,
+ # Cache keys don't include the requesting principal and hits are served
+ # ahead of auth/RBAC, so a shared cache can return one caller's response
+ # to another. Required for caching to actually start -- only appropriate
+ # when every request is guaranteed to come from the same principal.
+ "dangerously_share_cache_across_principals": True,
"CACHE_KEY_PREFIX": "mcp_cache_",
"call_tool_ttl": 3600,
}
MCP_STORE_CONFIG = {"enabled": True, "CACHE_REDIS_URL": "redis://redis:6379/0"}
```
-Mutating tools (`generate_chart`, `update_chart`, `execute_sql`, `generate_dashboard`) are always excluded from caching regardless of this setting.
+Every tool with a side effect (create/update/delete/execute) is always excluded from caching regardless of this setting -- see the `excluded_tools` default in `superset/mcp_service/mcp_config.py` for the current list.
---
diff --git a/superset/mcp_service/README.md b/superset/mcp_service/README.md
index 507c3d1ca7c..052b18d681c 100644
--- a/superset/mcp_service/README.md
+++ b/superset/mcp_service/README.md
@@ -672,21 +672,34 @@ kubectl get ingress -n superset
| Variable | Description | Default |
|----------|-------------|---------|
-| `MCP_DEV_USERNAME` | Superset username for MCP authentication | `admin` |
-| `MCP_AUTH_ENABLED` | Enable/disable authentication | `true` |
+| `MCP_DEV_USERNAME` | Superset username for MCP authentication in dev mode. Mutually exclusive with `MCP_AUTH_ENABLED = True`: the server refuses to start if both are set. | - |
+| `MCP_AUTH_ENABLED` | Enable/disable authentication | `false` |
| `MCP_JWT_PUBLIC_KEY` | JWT public key for token validation | - |
| `SUPERSET_WEBSERVER_ADDRESS` | Internal Superset URL | `http://localhost:8088` |
| `WEBDRIVER_BASEURL` | URL for screenshot generation | Same as webserver |
#### superset_config.py Options
+Dev mode (`MCP_DEV_USERNAME`) and JWT authentication (`MCP_AUTH_ENABLED`) are
+mutually exclusive -- the server raises at startup if both are set, since a
+fixed dev-mode identity would defeat the point of requiring real auth. Pick one:
+
```python
-# MCP Service Configuration
+# MCP Service Configuration -- development/testing (no auth)
MCP_DEV_USERNAME = 'admin' # Username for development/testing
+
+# WebDriver for chart screenshots
+WEBDRIVER_BASEURL = 'http://superset:8088/'
+WEBDRIVER_TYPE = 'chrome'
+WEBDRIVER_OPTION_ARGS = ['--headless', '--no-sandbox']
+```
+
+```python
+# MCP Service Configuration -- production with JWT authentication
MCP_AUTH_ENABLED = True # Enable authentication
MCP_JWT_PUBLIC_KEY = 'your-public-key' # For JWT token validation
-# For production with JWT authentication
+# Or, for a fully custom auth setup instead of the built-in JWT verifier:
MCP_AUTH_FACTORY = 'your.custom.auth_factory'
MCP_USER_RESOLVER = 'your.custom.user_resolver'
diff --git a/superset/mcp_service/__main__.py b/superset/mcp_service/__main__.py
index 474d5bcc3f3..cb33e3da1aa 100644
--- a/superset/mcp_service/__main__.py
+++ b/superset/mcp_service/__main__.py
@@ -58,6 +58,7 @@ if os.environ.get("FASTMCP_TRANSPORT", "stdio") == "stdio":
click.secho = secho_to_stderr
from superset.mcp_service.app import init_fastmcp_server, mcp
+from superset.mcp_service.caching import create_response_caching_middleware
from superset.mcp_service.middleware import create_response_size_guard_middleware
from superset.mcp_service.server import build_middleware_list
@@ -67,8 +68,9 @@ def _add_default_middlewares() -> None:
Delegates to ``server.build_middleware_list()`` for the core stack so
the stdio entry point stays in sync with the HTTP server without
- duplicating middleware ordering. The optional response size guard is
- appended separately (innermost position, same as in run_server()).
+ duplicating middleware ordering. The optional response size guard and
+ response caching middleware are appended separately (innermost
+ position, same order as in run_server()).
FastMCP wraps handlers so that the FIRST-added middleware is outermost.
``build_middleware_list()`` already returns middlewares in the correct
@@ -77,12 +79,16 @@ def _add_default_middlewares() -> None:
for middleware in build_middleware_list():
mcp.add_middleware(middleware)
- # Response size guard is innermost (added last)
+ # Response size guard is innermost (added last), then response caching.
if size_guard := create_response_size_guard_middleware():
mcp.add_middleware(size_guard)
limit = size_guard.token_limit
sys.stderr.write(f"[MCP] Response size guard enabled (token_limit={limit})\n")
+ if caching_middleware := create_response_caching_middleware():
+ mcp.add_middleware(caching_middleware)
+ sys.stderr.write("[MCP] Response caching enabled\n")
+
def main() -> None:
"""
@@ -157,8 +163,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)
_add_default_middlewares()
# Run with specified transport
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index b6c0a500bf4..ffd649497bf 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -575,7 +575,8 @@ def _resolve_user_from_jwt_context(app: Any) -> MCPUser | None: # noqa: C901
the corresponding ``GuestUser`` built from the token's resources/RLS.
Raises:
- ValueError: If JWT resolves a username that doesn't exist in the DB
+ ValueError: If JWT resolves a username that doesn't exist in the DB,
+ or a guest-marked token is presented while guest auth is disabled
(fail closed — do NOT fall through to weaker auth sources).
MCPAuthConfigError: If more than one JWT issuer is trusted
(``MCP_JWT_ISSUER`` is a list/tuple/set) and no issuer-aware
@@ -618,7 +619,14 @@ def _resolve_user_from_jwt_context(app: Any) -> MCPUser | None: # noqa: C901
"Guest-marked token presented but embedded guest auth is not "
"enabled; rejecting"
)
- return None
+ # Fail closed, matching the sibling failure branches below: a
+ # guest-marked token is an explicit (rejected) authentication
+ # attempt, not an absent one. Returning None here would let the
+ # request degrade to weaker auth sources (API key,
+ # MCP_DEV_USERNAME, or a middleware-set g.user).
+ raise ValueError(
+ "Guest-marked token presented but embedded guest auth is not enabled"
+ )
logger.debug("Resolving MCP request as embedded guest user")
# Drop the internal marker so it does not leak into GuestUser.guest_token.
guest_claims: dict[str, Any] = {
diff --git a/superset/mcp_service/caching.py b/superset/mcp_service/caching.py
index 7805a7cc9ef..c1d658726c1 100644
--- a/superset/mcp_service/caching.py
+++ b/superset/mcp_service/caching.py
@@ -123,6 +123,27 @@ def create_response_caching_middleware() -> Any | None:
logger.debug("MCP response caching disabled")
return None
+ # ResponseCachingMiddleware keys cache entries on the method/tool
+ # name + arguments only and runs ahead of the per-request auth/RBAC
+ # checks, so a cache hit returns a response computed for a different
+ # caller. Only appropriate when every request is guaranteed to come
+ # from the same principal.
+ # that sends byte-identical arguments within the TTL, skipping every
+ # authorization check. Fail closed unless the operator explicitly
+ # accepts a cache shared across principals -- only safe when every
+ # request is guaranteed to come from the same principal (e.g. a
+ # single-user development deployment).
+ if not cache_config.get("dangerously_share_cache_across_principals", False):
+ logger.warning(
+ "MCP_CACHE_CONFIG['enabled'] is set, but response caching "
+ "stays disabled: cache keys do not include the requesting "
+ "principal, so cached responses would be served across users "
+ "without any authorization checks. Set "
+ "'dangerously_share_cache_across_principals': True only when "
+ "all requests share a single principal."
+ )
+ return None
+
try:
from fastmcp.server.middleware.caching import ResponseCachingMiddleware
except ImportError:
diff --git a/superset/mcp_service/chart/tool/restore_chart.py b/superset/mcp_service/chart/tool/restore_chart.py
index d8886423a80..221ec027e52 100644
--- a/superset/mcp_service/chart/tool/restore_chart.py
+++ b/superset/mcp_service/chart/tool/restore_chart.py
@@ -116,6 +116,53 @@ 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:
+ 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(
+ str(request.identifier), skip_visibility_filter=True
+ )
+ if visible is None:
+ display_id = str(request.identifier)[:200]
+ return RestoreChartResponse(
+ success=False,
+ error=f"No chart found with identifier: {display_id}.",
+ error_type="NotFound",
+ )
+ await ctx.warning("Permission denied restoring chart id=%s" % (chart_id,))
+ return RestoreChartResponse(
+ success=False,
+ permission_denied=True,
+ error=(
+ f"You do not have permission to restore chart id={chart_id}. "
+ "Ask the user to restore it or grant access; do not retry."
+ ),
+ error_type="Forbidden",
+ )
+ except SQLAlchemyError:
+ _rollback()
+ logger.exception("Editorship check failed during restore_chart")
+ return RestoreChartResponse(
+ success=False,
+ error="Chart lookup failed due to a database error.",
+ error_type="LookupFailed",
+ )
+
# Chart names are user-controlled and must remain exact in response text.
chart_name = chart.slice_name
diff --git a/superset/mcp_service/dashboard/tool/restore_dashboard.py b/superset/mcp_service/dashboard/tool/restore_dashboard.py
index 17eb8c0af65..d28b0267a9d 100644
--- a/superset/mcp_service/dashboard/tool/restore_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/restore_dashboard.py
@@ -118,6 +118,56 @@ async def restore_dashboard(
return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound")
dashboard_id = dashboard.id
+
+ # The lookup above deliberately bypasses the RBAC base filter (see
+ # _find_dashboard_for_restore), so enforce the restore audience *before*
+ # composing any response that embeds the dashboard's title: without this
+ # gate, iterating identifiers would disclose the existence and exact title
+ # of dashboards the caller cannot see (the web API answers 404 for those).
+ from superset import security_manager
+ from superset.exceptions import SupersetSecurityException
+
+ try:
+ try:
+ security_manager.raise_for_editorship(dashboard)
+ except SupersetSecurityException:
+ from superset.daos.dashboard import DashboardDAO
+
+ # Distinguish "visible but not an editor" from "outside the
+ # caller's RBAC scope": the latter must be indistinguishable
+ # from a dashboard that does not exist.
+ visible = DashboardDAO.find_by_id_or_uuid(
+ str(request.identifier), skip_visibility_filter=True
+ )
+ if visible is None:
+ display_id = str(request.identifier)[:200]
+ return RestoreDashboardResponse(
+ success=False,
+ error=f"No dashboard found with identifier: {display_id}.",
+ error_type="NotFound",
+ )
+ await ctx.warning(
+ "Permission denied restoring dashboard id=%s" % (dashboard_id,)
+ )
+ return RestoreDashboardResponse(
+ success=False,
+ permission_denied=True,
+ error=(
+ f"You do not have permission to restore dashboard "
+ f"id={dashboard_id}. Ask the user to restore it or grant "
+ "access; do not retry."
+ ),
+ error_type="Forbidden",
+ )
+ except SQLAlchemyError:
+ _rollback()
+ logger.exception("Editorship check failed during restore_dashboard")
+ return RestoreDashboardResponse(
+ success=False,
+ error="Dashboard lookup failed due to a database error.",
+ error_type="LookupFailed",
+ )
+
# Dashboard titles are user-controlled and must remain exact in response text.
dashboard_name = dashboard.dashboard_title
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index beefac1e419..690f31e26e6 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -259,7 +259,9 @@ MCP_FACTORY_CONFIG = {
#
# Configuration Flow:
# -------------------
-# - MCP_CACHE_CONFIG controls whether caching is enabled and its TTL settings
+# - MCP_CACHE_CONFIG controls whether caching is enabled and its TTL settings.
+# Note "enabled" alone is not sufficient -- see
+# "dangerously_share_cache_across_principals" below.
# - MCP_STORE_CONFIG controls the Redis store (optional)
#
# Scenarios:
@@ -270,11 +272,13 @@ MCP_FACTORY_CONFIG = {
#
# 2. Caching with in-memory store:
# MCP_CACHE_CONFIG["enabled"] = True
+# MCP_CACHE_CONFIG["dangerously_share_cache_across_principals"] = True
# MCP_STORE_CONFIG["enabled"] = False (or not configured)
# → Caching uses FastMCP's default in-memory store, no Prefix wrapper used
#
# 3. Caching with Redis store:
# MCP_CACHE_CONFIG["enabled"] = True
+# MCP_CACHE_CONFIG["dangerously_share_cache_across_principals"] = True
# MCP_STORE_CONFIG["enabled"] = True
# MCP_STORE_CONFIG["CACHE_REDIS_URL"] = "redis://..."
# → Caching uses Redis with PrefixKeysWrapper
@@ -322,6 +326,13 @@ MCP_STORE_CONFIG: dict[str, Any] = {
# When enabled with MCP_STORE_CONFIG, uses Redis store.
MCP_CACHE_CONFIG: dict[str, Any] = {
"enabled": False, # Disabled by default
+ # Cache keys are method/tool + arguments only and cache hits are served
+ # ahead of per-request auth/RBAC, so a shared cache can return one
+ # caller's response to another. Response caching refuses to start
+ # unless this is explicitly set -- only appropriate when every request
+ # is guaranteed to come from the same principal (e.g. a single-user
+ # development deployment).
+ "dangerously_share_cache_across_principals": False,
# Base prefix for the shared store. Superset appends an internal response-
# contract namespace so incompatible cached values are not reused.
"CACHE_KEY_PREFIX": None, # Only needed when using the store
@@ -332,12 +343,38 @@ MCP_CACHE_CONFIG: dict[str, Any] = {
"get_prompt_ttl": 60 * 60, # 1 hour
"call_tool_ttl": 60 * 60, # 1 hour
"max_item_size": 1024 * 1024, # 1MB
- "excluded_tools": [ # Tools that should never be cached (side effects, dynamic)
- "execute_sql",
- "generate_dashboard",
+ # Every tool whose ToolAnnotations set readOnlyHint=False, i.e. every tool
+ # with a side effect. A cache hit is served ahead of per-request
+ # auth/RBAC, so caching a mutating tool can replay a stale create/update/
+ # delete result -- including to a caller who repeats an identical call
+ # expecting it to run again. This list is enforced complete by
+ # test_mcp_caching.py::test_excluded_tools_covers_every_mutating_tool,
+ # which fails with the specific missing tool name(s) if a new
+ # non-read-only tool is added without also being added here.
+ "excluded_tools": [
+ "add_chart_to_existing_dashboard",
+ "create_dataset",
+ "create_theme",
+ "create_virtual_dataset",
+ "delete_chart",
+ "delete_dashboard",
"duplicate_dashboard",
+ "execute_sql",
"generate_chart",
+ "generate_dashboard",
+ "generate_explore_link",
+ "manage_dashboard_certification",
+ "manage_dashboard_owners",
+ "manage_dashboard_roles",
+ "manage_native_filters",
+ "remove_chart_from_dashboard",
+ "restore_chart",
+ "restore_dashboard",
+ "save_sql_query",
"update_chart",
+ "update_chart_preview",
+ "update_dashboard",
+ "update_dataset_metric",
],
}
@@ -495,6 +532,16 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
if not (auth_enabled or api_key_enabled or guest_enabled):
return None
+ # MCP_DEV_USERNAME makes user resolution fall back to a fixed user for
+ # requests that carry no resolvable identity, which defeats the point of
+ # having transport auth enabled. Refuse the combination outright.
+ if auth_enabled and app.config.get("MCP_DEV_USERNAME"):
+ raise MCPAuthConfigError(
+ "MCP_DEV_USERNAME must not be set when MCP_AUTH_ENABLED is True: "
+ "it would execute callers without a resolvable identity as that "
+ "user. Unset MCP_DEV_USERNAME (a development-only convenience)."
+ )
+
# When JWT auth is enabled, an audience must be configured so issued tokens
# are bound to this service. Without it the verifier accepts any otherwise
# valid same-issuer token, regardless of which service it was minted for.
@@ -519,22 +566,40 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
secret = app.config.get("MCP_JWT_SECRET")
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
- else:
- try:
- jwt_verifier = _build_jwt_verifier(
- app=app,
- jwks_uri=jwks_uri,
- public_key=public_key,
- secret=secret,
- )
- except Exception:
- # Do not log the exception — it may contain secrets (e.g., key material)
- logger.error("Failed to create MCP JWT verifier")
- if not (api_key_enabled or guest_enabled):
- return None
+ # Fail closed regardless of API-key/guest fallbacks: JWT auth was
+ # explicitly enabled, so silently starting without it would leave
+ # the operator's chosen JWT mode disabled without warning them
+ # via anything louder than a log line.
+ 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')."
+ )
+
+ try:
+ jwt_verifier = _build_jwt_verifier(
+ app=app,
+ jwks_uri=jwks_uri,
+ public_key=public_key,
+ secret=secret,
+ )
+ except MCPAuthConfigError:
+ raise
+ except Exception:
+ # Do not log or chain the exception — it may contain secrets
+ # (e.g., key material)
+ logger.error("Failed to create MCP JWT verifier")
+ # Fail closed regardless of API-key/guest fallbacks: JWT auth
+ # was explicitly enabled, so silently starting without it is
+ # a permissive state the operator did not choose.
+ raise MCPAuthConfigError(
+ "Failed to construct the MCP JWT verifier from the "
+ "configured key material; refusing to start with JWT "
+ "auth silently disabled. Verify MCP_JWT_ALGORITHM "
+ "matches the configured key (HS256 for MCP_JWT_SECRET; "
+ "RS256 needs MCP_JWKS_URI or MCP_JWT_PUBLIC_KEY)."
+ ) from None
# A composite verifier is needed whenever API-key OR guest auth is on, so
# those token types are recognized before (or instead of) the JWT verifier.
@@ -708,15 +773,49 @@ def _build_jwt_verifier(
"required_scopes": app.config.get("MCP_REQUIRED_SCOPES", []),
}
- # For HS256 (symmetric), use the secret as the public_key parameter
- if app.config.get("MCP_JWT_ALGORITHM") == "HS256" and secret:
+ algorithm = app.config.get("MCP_JWT_ALGORITHM", "RS256")
+
+ if algorithm in ("HS256", "HS384", "HS512"):
+ # HMAC algorithms are symmetric: verification MUST be keyed on an
+ # explicit shared secret, never on public-key material (PEM or
+ # JWKS), which isn't confidential. Refuse the contradictory
+ # configuration outright instead of honoring it.
+ if not secret:
+ raise MCPAuthConfigError(
+ f"MCP_JWT_ALGORITHM is '{algorithm}' but MCP_JWT_SECRET is "
+ "not set. Refusing to build an HMAC verifier keyed on "
+ "public-key material. Set MCP_JWT_SECRET, or switch to an "
+ "asymmetric algorithm (e.g. RS256) with MCP_JWT_PUBLIC_KEY "
+ "or MCP_JWKS_URI."
+ )
+ if public_key or jwks_uri:
+ raise MCPAuthConfigError(
+ "MCP_JWT_PUBLIC_KEY/MCP_JWKS_URI are configured alongside "
+ f"MCP_JWT_ALGORITHM='{algorithm}'. This usually indicates "
+ "leftover asymmetric-key configuration; remove the public "
+ "key/JWKS settings, or switch back to an asymmetric "
+ "algorithm."
+ )
+ # For HMAC (symmetric), use the secret as the public_key parameter
common_kwargs["public_key"] = secret
- common_kwargs["algorithm"] = "HS256"
+ common_kwargs["algorithm"] = algorithm
else:
# For RS256 (asymmetric), use public key or JWKS
+ if not (jwks_uri or public_key):
+ # Only a secret is configured but the algorithm is asymmetric: a
+ # keyless verifier cannot validate anything. Name the fix rather
+ # than letting the verifier constructor raise opaquely (it would
+ # still fail closed via the caller's fail-closed exception
+ # handling, but with a less actionable message).
+ raise MCPAuthConfigError(
+ "MCP_JWT_SECRET is set but MCP_JWT_ALGORITHM is not 'HS256' "
+ "and no MCP_JWKS_URI/MCP_JWT_PUBLIC_KEY is configured. Set "
+ "MCP_JWT_ALGORITHM='HS256' to use the secret, or configure "
+ "an asymmetric key."
+ )
common_kwargs["jwks_uri"] = jwks_uri
common_kwargs["public_key"] = public_key
- common_kwargs["algorithm"] = app.config.get("MCP_JWT_ALGORITHM", "RS256")
+ common_kwargs["algorithm"] = algorithm
if debug_errors:
# DetailedJWTVerifier: detailed server-side logging of JWT
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index de47a7c0dc7..2a014a5911a 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -218,18 +218,30 @@ _SENSITIVE_PARAM_KEYS = frozenset(
)
+def _sanitize_value(value: Any) -> Any:
+ """Apply ``_sanitize_params`` recursively to any dict/list container."""
+ if isinstance(value, dict):
+ return _sanitize_params(value)
+ if isinstance(value, list):
+ return [_sanitize_value(item) for item in value]
+ return value
+
+
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, including lists of lists, 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):
- result[k] = _sanitize_params(v)
else:
- result[k] = v
+ result[k] = _sanitize_value(v)
return result
@@ -1005,7 +1017,9 @@ class GlobalErrorHandlerMiddleware(Middleware):
) from error
elif isinstance(error, HTTPException):
# HTTP errors from screenshot endpoints or API calls
- raise ToolError(f"Service error in {tool_name}: {error.detail}") from error
+ raise ToolError(
+ f"Service error in {tool_name}: {_sanitize_error_for_logging(error)}"
+ ) from error
elif isinstance(error, MCPPermissionDeniedError):
# MCP RBAC permission denied — convert to structured ToolError.
# Must come before the generic PermissionError branch because
@@ -1020,7 +1034,8 @@ class GlobalErrorHandlerMiddleware(Middleware):
elif isinstance(error, ValueError):
# Value/parameter errors from tool code
raise ToolError(
- f"Invalid parameter in {tool_name}: {str(error)}"
+ f"Invalid parameter in {tool_name}: "
+ f"{_sanitize_error_for_logging(error)}"
) from error
elif isinstance(error, (ObjectNotFoundError, CommandInvalidError)):
# Superset command: not found (404) or validation (422)
diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py
index a2053db56f7..feef18a71d8 100644
--- a/superset/mcp_service/server.py
+++ b/superset/mcp_service/server.py
@@ -808,11 +808,19 @@ def _create_auth_provider(flask_app: Any) -> Any | None:
when either ``MCP_AUTH_ENABLED`` (JWT auth), ``MCP_API_KEY_ENABLED``, or
``FAB_API_KEY_ENABLED`` (API key auth) is True. The default factory builds a
``CompositeTokenVerifier`` that handles either or both auth modes.
+
+ Fail-closed: when auth has been explicitly configured, any error while
+ building the provider (or a configured factory yielding no provider)
+ raises ``MCPAuthConfigError`` so the service refuses to start rather
+ than coming up as an unauthenticated server.
"""
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
+
auth_provider = None
if auth_factory := flask_app.config.get("MCP_AUTH_FACTORY"):
- from superset.mcp_service.mcp_config import MCPAuthConfigError
-
try:
auth_provider = auth_factory(flask_app)
logger.info(
@@ -838,17 +846,18 @@ def _create_auth_provider(flask_app: Any) -> Any | None:
"refusing to start the MCP service without authentication. "
"Fix the factory or unset MCP_AUTH_FACTORY."
) from None
+ if auth_provider is None:
+ raise MCPAuthConfigError(
+ "MCP_AUTH_FACTORY returned no auth provider; refusing to "
+ "start an unauthenticated MCP server. Return a token "
+ "verifier or unset MCP_AUTH_FACTORY."
+ )
elif (
flask_app.config.get("MCP_AUTH_ENABLED", False)
or flask_app.config.get("MCP_API_KEY_ENABLED", False)
or flask_app.config.get("FAB_API_KEY_ENABLED", False)
or flask_app.config.get("MCP_EMBEDDED_GUEST_AUTH_ENABLED", False)
):
- from superset.mcp_service.mcp_config import (
- create_default_mcp_auth_factory,
- MCPAuthConfigError,
- )
-
try:
auth_provider = create_default_mcp_auth_factory(flask_app)
logger.info(
@@ -862,8 +871,19 @@ def _create_auth_provider(flask_app: Any) -> Any | None:
# no secret material.
raise
except Exception:
- # Do not log the exception — it may contain secrets
+ # Do not log or chain the exception — it may contain secrets.
+ # Auth was explicitly enabled, so a provider that cannot be built
+ # must also fail closed instead of starting unauthenticated.
logger.error("Failed to create auth provider from default factory")
+ raise MCPAuthConfigError(
+ "Failed to build the MCP auth provider from the configured "
+ "auth settings; refusing to start an unauthenticated MCP "
+ "server. Check the MCP auth configuration."
+ ) from None
+ # ``None`` here is deliberate only when the factory itself resolved
+ # every auth mode to disabled (e.g. MCP_API_KEY_ENABLED=False
+ # explicitly overriding FAB_API_KEY_ENABLED); misconfigurations of an
+ # enabled mode raise MCPAuthConfigError inside the factory instead.
return auth_provider
diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py b/superset/mcp_service/sql_lab/tool/execute_sql.py
index 12351827b4b..6ed79c87eaa 100644
--- a/superset/mcp_service/sql_lab/tool/execute_sql.py
+++ b/superset/mcp_service/sql_lab/tool/execute_sql.py
@@ -65,7 +65,12 @@ async def _validate_non_destructive_sql(
with event_logger.log_context(action="mcp.execute_sql.ddl_check"):
try:
sql_to_check: str = request.sql
- if request.template_params:
+ # Render whenever template_params is not None, mirroring the
+ # executor (SQLExecutor._render_sql_template), which also renders
+ # for an empty dict. A truthiness check would let destructive SQL
+ # that only appears after rendering slip past the guard when
+ # template_params={}.
+ if request.template_params is not None:
from superset.jinja_context import get_template_processor
tp = get_template_processor(database=database)
diff --git a/superset/mcp_service/system/schemas.py b/superset/mcp_service/system/schemas.py
index 8b58b84d917..b05d1e68881 100644
--- a/superset/mcp_service/system/schemas.py
+++ b/superset/mcp_service/system/schemas.py
@@ -24,6 +24,7 @@ system-level info.
from __future__ import annotations
+import re
from datetime import datetime
from typing import Annotated, Any, List
@@ -32,6 +33,12 @@ from pydantic import BaseModel, ConfigDict, Field, field_validator
from superset.mcp_service.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE
from superset.subjects.types import SubjectType
+# Shape-only check, not RFC validation: just enough to catch "local@domain.tld"
+# so an email-shaped query can be rejected before it reaches the username
+# column, since usernames are frequently email addresses under OAuth
+# provisioning.
+_EMAIL_SHAPE_RE = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s]+$")
+
class HealthCheckResponse(BaseModel):
"""Response model for health check.
@@ -176,7 +183,7 @@ def serialize_user_object(user: Any) -> UserInfo | None:
class FindUsersRequest(BaseModel):
"""Request schema for find_users tool.
- Resolves a person's name (or partial name, username, or email) to user IDs
+ Resolves a person's name (or partial name or username) to user IDs
so they can be passed to listing tools as filter values for created_by_fk
or changed_by_fk. This is the only sanctioned path for "show me what
<person> is working on" queries.
@@ -191,8 +198,9 @@ class FindUsersRequest(BaseModel):
max_length=200,
description=(
"Substring to match (case-insensitive) against username, "
- "first_name, last_name, and email. Required and non-empty: "
- "this tool does not enumerate the full user directory."
+ "first_name, and last_name (never email; email-shaped "
+ "queries are rejected). Required and non-empty: this tool "
+ "does not enumerate the full user directory."
),
),
]
@@ -217,6 +225,20 @@ class FindUsersRequest(BaseModel):
raise ValueError("query must contain at least one non-whitespace character")
return stripped
+ @field_validator("query")
+ @classmethod
+ def _reject_email_shaped_query(cls, value: str) -> str:
+ # Email isn't a searchable column here, but usernames are commonly
+ # email addresses under OAuth provisioning, so an email-shaped query
+ # would still confirm an account's existence via the username column.
+ # Reject the shape outright rather than relying on the column
+ # exclusion alone.
+ if _EMAIL_SHAPE_RE.match(value):
+ raise ValueError(
+ "query must not be an email address; search by name or username instead"
+ )
+ return value
+
class UserMatch(BaseModel):
"""Minimal user projection returned by find_users.
diff --git a/superset/mcp_service/system/tool/find_users.py b/superset/mcp_service/system/tool/find_users.py
index 1cecc91699c..6e46585a48b 100755
--- a/superset/mcp_service/system/tool/find_users.py
+++ b/superset/mcp_service/system/tool/find_users.py
@@ -50,9 +50,14 @@ async def find_users(request: FindUsersRequest, ctx: Context) -> FindUsersRespon
the value for a created_by_fk or changed_by_fk filter on list_dashboards,
list_charts, or list_datasets.
- Matches case-insensitively against username, first_name, last_name, and
- email. The query is required and non-empty; this tool does not enumerate
- the full user directory.
+ Matches case-insensitively against username, first_name, and last_name.
+ Email is deliberately not matched, and an email-shaped query is rejected
+ outright rather than falling through to the username column: since
+ usernames are frequently email addresses under OAuth provisioning, a
+ plain column exclusion would still let an email lookup confirm whether
+ an address has an account (and resolve it to a person), a directory
+ capability the web API reserves for admins. The query is required and
+ non-empty; this tool does not enumerate the full user directory.
Privacy: returning a user's identity here is sanctioned only for resolving
filter values. Do not use the response to answer "who owns X", "who can
@@ -75,7 +80,6 @@ async def find_users(request: FindUsersRequest, ctx: Context) -> FindUsersRespon
user_model.username.ilike(needle, escape="\\"),
user_model.first_name.ilike(needle, escape="\\"),
user_model.last_name.ilike(needle, escape="\\"),
- user_model.email.ilike(needle, escape="\\"),
)
)
.order_by(user_model.username.asc())
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py b/tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py
index a9c1111a1e9..1a5f795a1e0 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py
@@ -23,6 +23,7 @@ autouse mock_auth fixture, matching the other chart tool test files.
from collections.abc import Iterator
from datetime import datetime
+from typing import Any
from unittest.mock import Mock, patch
from uuid import UUID
@@ -45,11 +46,15 @@ def mcp_server() -> object:
@pytest.fixture(autouse=True)
def mock_auth() -> Iterator[Mock]:
with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
- mock_user = Mock()
- mock_user.id = 1
- mock_user.username = "admin"
- mock_get_user.return_value = mock_user
- yield mock_get_user
+ # The tool's editorship gate calls the real security manager; default
+ # it to a no-op (caller is an editor) so unrelated tests keep passing.
+ # The disclosure regression tests below re-patch it to raise.
+ with patch("superset.security_manager.raise_for_editorship"):
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
def _mock_chart(
@@ -232,6 +237,33 @@ async def test_restore_chart_lookup_db_error_is_structured(
assert "down" not in (content["error"] or "")
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_chart_editorship_check_db_error_is_structured(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """DB failures during the editorship check (not just the initial lookup)
+ must return the structured LookupFailed response instead of escaping the
+ tool as an unhandled error."""
+ from sqlalchemy.exc import OperationalError
+
+ mock_find.return_value = _mock_chart(chart_id=10, slice_name="Sales")
+
+ with patch(
+ "superset.security_manager.raise_for_editorship",
+ side_effect=OperationalError("SELECT ...", {}, Exception("down")),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_chart", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["error_type"] == "LookupFailed"
+ assert "down" not in (content["error"] or "")
+
+
@pytest.mark.asyncio
async def test_restore_chart_rejects_boolean_identifier(mcp_server: object) -> None:
"""bool subclasses int; identifier=true must not coerce to chart ID 1."""
@@ -240,3 +272,81 @@ async def test_restore_chart_rejects_boolean_identifier(mcp_server: object) -> N
async with Client(mcp_server) as client:
with pytest.raises(ToolError):
await client.call_tool("restore_chart", {"request": {"identifier": True}})
+
+
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_chart_inaccessible_chart_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A chart outside the caller's RBAC scope must not leak its existence or
+ title: the unfiltered restore lookup finds it, the base-filtered re-lookup
+ does not, so the tool must answer exactly as if it does not exist.
+
+ The mock's return value is keyed on the actual ``skip_base_filter`` kwarg
+ of each call rather than call order, so a regression that accidentally
+ keeps ``skip_base_filter=True`` on the re-lookup (turning the intended
+ NotFound response into a disclosure) is caught by a call-order-based
+ mock returning the chart on both calls, not masked by it."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ def _find_side_effect(*args: Any, **kwargs: Any) -> Any | None:
+ # Mirrors the real DAO contract: only an explicit skip_base_filter=True
+ # (the initial unfiltered restore lookup) sees the chart; the
+ # re-lookup omits it, so it defaults to False and must see nothing.
+ if kwargs.get("skip_base_filter"):
+ return _mock_chart(chart_id=10, slice_name="Secret KPI")
+ return None
+
+ mock_find.side_effect = _find_side_effect
+ forbidden = SupersetSecurityException(
+ SupersetError(
+ message="forbidden",
+ error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+ with patch("superset.security_manager.raise_for_editorship", side_effect=forbidden):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_chart", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["error_type"] == "NotFound"
+ assert "Secret KPI" not in (content["error"] or "")
+
+
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_chart_visible_non_editor_gets_nameless_forbidden(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A caller who can see the chart but cannot edit it gets a permission
+ error naming the id only, never the title."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ chart = _mock_chart(chart_id=10, slice_name="Secret KPI")
+ mock_find.side_effect = [chart, chart]
+ forbidden = SupersetSecurityException(
+ SupersetError(
+ message="forbidden",
+ error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+ with patch("superset.security_manager.raise_for_editorship", side_effect=forbidden):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_chart", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["permission_denied"] is True
+ assert content["error_type"] == "Forbidden"
+ assert "Secret KPI" not in (content["error"] or "")
+ assert "10" in (content["error"] or "")
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py b/tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py
index dc3688dfb65..576b0d5bd6d 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py
@@ -23,6 +23,7 @@ autouse mock_auth fixture, matching the other dashboard tool test files.
from collections.abc import Iterator
from datetime import datetime
+from typing import Any
from unittest.mock import Mock, patch
from uuid import UUID
@@ -45,11 +46,15 @@ def mcp_server() -> object:
@pytest.fixture(autouse=True)
def mock_auth() -> Iterator[Mock]:
with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
- mock_user = Mock()
- mock_user.id = 1
- mock_user.username = "admin"
- mock_get_user.return_value = mock_user
- yield mock_get_user
+ # The tool's editorship gate calls the real security manager; default
+ # it to a no-op (caller is an editor) so unrelated tests keep passing.
+ # The disclosure regression tests below re-patch it to raise.
+ with patch("superset.security_manager.raise_for_editorship"):
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
def _mock_dashboard(
@@ -236,6 +241,33 @@ async def test_restore_dashboard_slug_conflict(
assert "slug" in (content["error"] or "").lower()
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_dashboard_editorship_check_db_error_is_structured(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """DB failures during the editorship check (not just the initial lookup)
+ must return the structured LookupFailed response instead of escaping the
+ tool as an unhandled error."""
+ from sqlalchemy.exc import OperationalError
+
+ mock_find.return_value = _mock_dashboard(10, "Sales Dashboard")
+
+ with patch(
+ "superset.security_manager.raise_for_editorship",
+ side_effect=OperationalError("SELECT ...", {}, Exception("down")),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_dashboard", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["error_type"] == "LookupFailed"
+ assert "down" not in (content["error"] or "")
+
+
@pytest.mark.asyncio
async def test_restore_dashboard_rejects_boolean_identifier(
mcp_server: object,
@@ -248,3 +280,82 @@ async def test_restore_dashboard_rejects_boolean_identifier(
await client.call_tool(
"restore_dashboard", {"request": {"identifier": True}}
)
+
+
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_dashboard_inaccessible_dashboard_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A dashboard outside the caller's RBAC scope must not leak its
+ existence or title: the unfiltered restore lookup finds it, the
+ base-filtered re-lookup does not, so the tool must answer exactly as if it
+ does not exist.
+
+ The mock's return value is keyed on the actual ``skip_base_filter`` kwarg
+ of each call rather than call order, so a regression that accidentally
+ keeps ``skip_base_filter=True`` on the re-lookup (turning the intended
+ NotFound response into a disclosure) is caught by a call-order-based
+ mock returning the dashboard on both calls, not masked by it."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ def _find_side_effect(*args: Any, **kwargs: Any) -> Any | None:
+ # Mirrors the real DAO contract: only an explicit skip_base_filter=True
+ # (the initial unfiltered restore lookup) sees the dashboard; the
+ # re-lookup omits it, so it defaults to False and must see nothing.
+ if kwargs.get("skip_base_filter"):
+ return _mock_dashboard(dashboard_id=10, title="Secret Board")
+ return None
+
+ mock_find.side_effect = _find_side_effect
+ forbidden = SupersetSecurityException(
+ SupersetError(
+ message="forbidden",
+ error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+ with patch("superset.security_manager.raise_for_editorship", side_effect=forbidden):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_dashboard", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["error_type"] == "NotFound"
+ assert "Secret Board" not in (content["error"] or "")
+
+
+@patch(_FIND)
+@pytest.mark.asyncio
+async def test_restore_dashboard_visible_non_editor_gets_nameless_forbidden(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A caller who can see the dashboard but cannot edit it gets a
+ permission error naming the id only, never the title."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ dashboard = _mock_dashboard(dashboard_id=10, title="Secret Board")
+ mock_find.side_effect = [dashboard, dashboard]
+ forbidden = SupersetSecurityException(
+ SupersetError(
+ message="forbidden",
+ error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+ with patch("superset.security_manager.raise_for_editorship", side_effect=forbidden):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "restore_dashboard", {"request": {"identifier": 10}}
+ )
+
+ content = result.structured_content
+ assert content["success"] is False
+ assert content["permission_denied"] is True
+ assert content["error_type"] == "Forbidden"
+ assert "Secret Board" not in (content["error"] or "")
+ assert "10" in (content["error"] or "")
diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
index e495183281b..8116b5601fc 100644
--- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
+++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
@@ -1399,6 +1399,52 @@ class TestDestructiveDDLBlocking:
assert "Destructive DDL" in data["error"]
ddl_mocks.execute.assert_not_called()
+ @pytest.mark.asyncio
+ async def test_empty_template_params_rendered_before_ddl_check(
+ self, ddl_mocks, mcp_server
+ ):
+ """template_params={} must not skip Jinja rendering in the DDL guard.
+
+ The executor renders templates whenever template_params is not None
+ (including {}), so the guard must parse the same rendered SQL. With a
+ truthiness check, SQL whose destructive statement is hidden inside a
+ Jinja expression in a comment passes the guard unrendered and then
+ renders and executes.
+ """
+ ddl_mocks.execute.return_value = _create_select_result(
+ rows=[{"x": 1}], columns=["x"], original_sql="SELECT 1"
+ )
+ mock_tp = MagicMock()
+ # The raw (unrendered) SQL below parses as a single, non-destructive
+ # SELECT -- the Jinja expression sits inside a `--` comment. Only
+ # after rendering does it become a second, destructive statement.
+ mock_tp.process_template.return_value = (
+ "SELECT 1;\n-- \nDROP TABLE important_table;"
+ )
+
+ with patch(
+ "superset.jinja_context.get_template_processor",
+ return_value=mock_tp,
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "execute_sql",
+ {
+ "request": {
+ "database_id": 1,
+ "sql": (
+ 'SELECT 1 -- {{ "\\nDROP TABLE important_table; --" }}'
+ ),
+ "template_params": {},
+ }
+ },
+ )
+ data = result.structured_content
+ assert data["success"] is False
+ assert "Destructive DDL" in data["error"]
+ mock_tp.process_template.assert_called_once()
+ ddl_mocks.execute.assert_not_called()
+
@pytest.mark.asyncio
async def test_select_allowed(self, ddl_mocks, mcp_server):
"""SELECT queries pass through the DDL check."""
diff --git a/tests/unit_tests/mcp_service/system/tool/test_find_users.py b/tests/unit_tests/mcp_service/system/tool/test_find_users.py
index 805cc306f86..975ccb558f3 100644
--- a/tests/unit_tests/mcp_service/system/tool/test_find_users.py
+++ b/tests/unit_tests/mcp_service/system/tool/test_find_users.py
@@ -141,9 +141,9 @@ async def test_find_users_returns_matches(mcp_server):
# required for filter resolution. Catch regressions on the response shape.
for forbidden in ("email", "active", "roles"):
assert forbidden not in data["users"][0]
- # or_ should have been built across the four matched columns
+ # or_ should have been built across the three matched columns (no email)
assert mock_or.called
- assert len(mock_or.call_args.args) == 4
+ assert len(mock_or.call_args.args) == 3
@pytest.mark.asyncio
@@ -201,6 +201,19 @@ def test_find_users_request_strips_query_whitespace():
assert request.query == "maxime"
+@pytest.mark.parametrize(
+ "email",
+ ["victim@example.com", "Victim@Example.COM", "a.b+tag@sub.example.co"],
+)
+def test_find_users_request_rejects_email_shaped_query(email):
+ # Usernames are frequently email addresses under OAuth provisioning, so
+ # an email-shaped query must be rejected outright rather than relying on
+ # the email-column exclusion alone -- otherwise it still confirms an
+ # account's existence via the username column.
+ with pytest.raises(ValidationError):
+ FindUsersRequest(query=email)
+
+
# ---------------------------------------------------------------------------
# Filter contract: created_by_fk / changed_by_fk filtering on list tools
# ---------------------------------------------------------------------------
@@ -336,3 +349,51 @@ async def test_find_users_escapes_literal_backslash(mcp_server):
assert ilike_call is not None
assert ilike_call.args[0] == "%\\\\%"
assert ilike_call.kwargs.get("escape") == "\\"
+
+
+@pytest.mark.asyncio
+async def test_find_users_does_not_match_on_email(mcp_server):
+ """Email must not be a searchable column: substring or exact email
+ matching would let any MCP credential confirm which addresses have
+ accounts (an email-disclosure oracle the web API reserves for admins).
+ Uses a non-email-shaped query so this exercises the OR-clause contract
+ itself, independent of the email-shape rejection covered below."""
+ session, _ = _patch_user_query([])
+
+ with (
+ patch.object(find_users_module, "db") as mock_db,
+ patch.object(find_users_module, "security_manager") as mock_sm,
+ patch.object(find_users_module, "or_") as mock_or,
+ ):
+ mock_db.session = session
+ user_model = MagicMock()
+ mock_sm.user_model = user_model
+ mock_or.return_value = MagicMock()
+
+ async with Client(mcp_server) as client:
+ await client.call_tool("find_users", {"request": {"query": "victim"}})
+
+ assert user_model.username.ilike.called
+ assert user_model.first_name.ilike.called
+ assert user_model.last_name.ilike.called
+ assert not user_model.email.ilike.called
+
+
+@pytest.mark.asyncio
+async def test_find_users_rejects_email_shaped_query_via_client(mcp_server):
+ """An email-shaped query is rejected before it ever reaches the DB
+ filter: since usernames are frequently email addresses under OAuth
+ provisioning, letting it fall through to the username column would
+ still let an email lookup confirm whether an address has an account,
+ defeating the documented non-enumeration guarantee."""
+ with (
+ patch.object(find_users_module, "db") as mock_db,
+ patch.object(find_users_module, "security_manager"),
+ ):
+ async with Client(mcp_server) as client:
+ with pytest.raises(ToolError):
+ await client.call_tool(
+ "find_users", {"request": {"query": "victim@example.com"}}
+ )
+
+ mock_db.session.query.assert_not_called()
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 a9ff3ab5263..106a3fe863d 100644
--- a/tests/unit_tests/mcp_service/test_auth_user_resolution.py
+++ b/tests/unit_tests/mcp_service/test_auth_user_resolution.py
@@ -233,6 +233,36 @@ def test_raises_when_no_auth_source(app) -> None:
get_user_from_request()
+def test_rejected_guest_token_does_not_fall_through_to_dev_username(app) -> None:
+ """A guest-marked token rejected for disabled guest auth must not degrade
+ to a weaker auth source (MCP_DEV_USERNAME here) via get_user_from_request.
+
+ Before the fix, _resolve_user_from_jwt_context returned None for this
+ case, which get_user_from_request treats identically to "no token
+ present" and falls through to the next priority source -- silently
+ executing the caller as MCP_DEV_USERNAME in a JWT-only deployment with a
+ dev username configured.
+ """
+ from superset.mcp_service.guest_token_verifier import GUEST_TOKEN_CLAIM
+
+ token = MagicMock()
+ token.claims = {GUEST_TOKEN_CLAIM: True, "sub": "attacker"}
+ token.client_id = "guest"
+
+ with app.app_context():
+ app.config["MCP_DEV_USERNAME"] = "dev_admin"
+ app.config["MCP_EMBEDDED_GUEST_AUTH_ENABLED"] = False
+ try:
+ with patch(
+ "fastmcp.server.dependencies.get_access_token", return_value=token
+ ):
+ with pytest.raises(ValueError, match="Guest-marked token"):
+ get_user_from_request()
+ finally:
+ app.config.pop("MCP_DEV_USERNAME", None)
+ app.config.pop("MCP_EMBEDDED_GUEST_AUTH_ENABLED", None)
+
+
def test_no_auth_source_error_message_has_no_config_details(app) -> None:
"""Client-facing auth error must be generic — no server config disclosed.
diff --git a/tests/unit_tests/mcp_service/test_guest_token_auth.py b/tests/unit_tests/mcp_service/test_guest_token_auth.py
index dc99e0d3845..09e5fca9830 100644
--- a/tests/unit_tests/mcp_service/test_guest_token_auth.py
+++ b/tests/unit_tests/mcp_service/test_guest_token_auth.py
@@ -575,7 +575,13 @@ def test_resolve_rejects_guest_marker_when_guest_auth_disabled(
app: SupersetApp,
) -> None:
"""Even with the marker + client_id, a token is not treated as a guest when
- embedded guest auth is disabled (defense against marker forgery)."""
+ embedded guest auth is disabled (defense against marker forgery).
+
+ A guest-marked token is an explicit, rejected authentication attempt, not
+ an absent one, so it must fail closed (raise) rather than return None and
+ let the caller fall through to a weaker auth source (API key,
+ MCP_DEV_USERNAME, or a middleware-set g.user).
+ """
token = MagicMock()
token.claims = {GUEST_TOKEN_CLAIM: True, **_parsed_guest_claims()}
token.client_id = "guest"
@@ -586,15 +592,15 @@ def test_resolve_rejects_guest_marker_when_guest_auth_disabled(
patch("fastmcp.server.dependencies.get_access_token", return_value=token),
patch("superset.mcp_service.auth.is_feature_enabled", return_value=True),
):
- result = _resolve_user_from_jwt_context(app)
-
- assert result is None
+ with pytest.raises(ValueError, match="Guest-marked token"):
+ _resolve_user_from_jwt_context(app)
def test_resolve_rejects_guest_marker_when_embedded_flag_off(app: SupersetApp) -> None:
"""The other half of the gate: with the marker + client_id + the MCP guest
flag on, a token is still not treated as a guest when the EMBEDDED_SUPERSET
- feature flag is off (both gates are required)."""
+ feature flag is off (both gates are required). Same fail-closed
+ requirement as the sibling case above."""
token = MagicMock()
token.claims = {GUEST_TOKEN_CLAIM: True, **_parsed_guest_claims()}
token.client_id = "guest"
@@ -605,9 +611,8 @@ def test_resolve_rejects_guest_marker_when_embedded_flag_off(app: SupersetApp) -
patch("fastmcp.server.dependencies.get_access_token", return_value=token),
patch("superset.mcp_service.auth.is_feature_enabled", return_value=False),
):
- result = _resolve_user_from_jwt_context(app)
-
- assert result is None
+ with pytest.raises(ValueError, match="Guest-marked token"):
+ _resolve_user_from_jwt_context(app)
def test_resolve_ignores_guest_marker_without_guest_client_id(app: SupersetApp) -> None:
diff --git a/tests/unit_tests/mcp_service/test_main_entrypoint.py b/tests/unit_tests/mcp_service/test_main_entrypoint.py
new file mode 100644
index 00000000000..9a57ad3e61f
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_main_entrypoint.py
@@ -0,0 +1,123 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Tests for the ``python -m superset.mcp_service`` entrypoint's non-stdio path.
+
+Network transports (streamable-http, sse, ...) must install the same auth
+provider as the supported CLI path (``superset mcp run`` -> server.run_server()
+-> _create_auth_provider()) instead of starting with no verifier at all.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+def test_main_installs_auth_provider_for_network_transport(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """The non-stdio branch must call _create_auth_provider and pass its
+ result into init_fastmcp_server, mirroring run_server()."""
+ monkeypatch.setenv("FASTMCP_TRANSPORT", "sse")
+
+ from superset.mcp_service import __main__ as main_module
+
+ flask_app = MagicMock()
+ auth_provider = object()
+ mcp_instance = MagicMock()
+
+ with (
+ patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=flask_app,
+ ),
+ patch(
+ "superset.mcp_service.server._create_auth_provider",
+ return_value=auth_provider,
+ ) as mock_create_auth_provider,
+ patch.object(
+ main_module, "init_fastmcp_server", return_value=mcp_instance
+ ) as mock_init,
+ patch.object(main_module, "_add_default_middlewares"),
+ patch.object(main_module.mcp, "run") as mock_run,
+ ):
+ main_module.main()
+
+ mock_create_auth_provider.assert_called_once_with(flask_app)
+ mock_init.assert_called_once_with(auth=auth_provider)
+ mock_run.assert_called_once_with(transport="sse")
+
+
+def test_main_propagates_auth_config_error_for_network_transport(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A verifier-construction failure must abort startup, not fall through to
+ an unauthenticated server.
+
+ Before the fix, the non-stdio branch never called _create_auth_provider
+ at all, so MCP_AUTH_ENABLED was silently discarded by this entrypoint.
+ """
+ monkeypatch.setenv("FASTMCP_TRANSPORT", "streamable-http")
+
+ from superset.mcp_service import __main__ as main_module
+ from superset.mcp_service.mcp_config import MCPAuthConfigError
+
+ flask_app = MagicMock()
+
+ with (
+ patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=flask_app,
+ ),
+ patch(
+ "superset.mcp_service.server._create_auth_provider",
+ side_effect=MCPAuthConfigError("bad auth config"),
+ ),
+ patch.object(main_module, "init_fastmcp_server") as mock_init,
+ patch.object(main_module, "_add_default_middlewares"),
+ patch.object(main_module.mcp, "run") as mock_run,
+ ):
+ with pytest.raises(MCPAuthConfigError):
+ main_module.main()
+
+ mock_init.assert_not_called()
+ mock_run.assert_not_called()
+
+
+def test_add_default_middlewares_installs_response_caching() -> None:
+ """``_add_default_middlewares`` must install response caching when
+ configured, matching ``run_server()``'s default (non-factory) path --
+ otherwise MCP_CACHE_CONFIG has no effect for this entrypoint's transports.
+ """
+ from superset.mcp_service import __main__ as main_module
+
+ caching_middleware = object()
+
+ with (
+ patch.object(main_module, "build_middleware_list", return_value=[]),
+ patch.object(
+ main_module, "create_response_size_guard_middleware", return_value=None
+ ),
+ patch.object(
+ main_module,
+ "create_response_caching_middleware",
+ return_value=caching_middleware,
+ ),
+ patch.object(main_module.mcp, "add_middleware") as mock_add_middleware,
+ ):
+ main_module._add_default_middlewares()
+
+ mock_add_middleware.assert_called_once_with(caching_middleware)
diff --git a/tests/unit_tests/mcp_service/test_mcp_caching.py b/tests/unit_tests/mcp_service/test_mcp_caching.py
index 07343e8fb61..b997e12efee 100644
--- a/tests/unit_tests/mcp_service/test_mcp_caching.py
+++ b/tests/unit_tests/mcp_service/test_mcp_caching.py
@@ -19,6 +19,8 @@
from unittest.mock import MagicMock, patch
+import pytest
+
from superset.mcp_service.caching import (
_build_caching_settings,
_version_cache_prefix,
@@ -106,7 +108,11 @@ def test_create_response_caching_middleware_falls_back_to_memory_when_no_prefix(
"""Caching middleware uses in-memory store when CACHE_KEY_PREFIX is not set."""
mock_flask_app = MagicMock()
mock_configs = {
- "MCP_CACHE_CONFIG": {"enabled": True, "list_tools_ttl": 300},
+ "MCP_CACHE_CONFIG": {
+ "enabled": True,
+ "dangerously_share_cache_across_principals": True,
+ "list_tools_ttl": 300,
+ },
"MCP_STORE_CONFIG": {"enabled": True}, # Store enabled but no CACHE_KEY_PREFIX
}
mock_flask_app.config.get.side_effect = lambda key, default=None: mock_configs.get(
@@ -141,7 +147,11 @@ def test_create_response_caching_middleware_uses_memory_store_when_store_disable
"""Caching middleware uses in-memory store when MCP_STORE_CONFIG is disabled."""
mock_flask_app = MagicMock()
mock_configs = {
- "MCP_CACHE_CONFIG": {"enabled": True, "list_tools_ttl": 300},
+ "MCP_CACHE_CONFIG": {
+ "enabled": True,
+ "dangerously_share_cache_across_principals": True,
+ "list_tools_ttl": 300,
+ },
"MCP_STORE_CONFIG": {"enabled": False},
}
mock_flask_app.config.get.side_effect = lambda key, default=None: mock_configs.get(
@@ -177,6 +187,7 @@ def test_create_response_caching_middleware_creates_middleware():
mock_flask_app = MagicMock()
mock_flask_app.config.get.return_value = {
"enabled": True,
+ "dangerously_share_cache_across_principals": True,
"CACHE_KEY_PREFIX": "mcp_cache_v1_",
"list_tools_ttl": 300,
}
@@ -211,3 +222,62 @@ def test_create_response_caching_middleware_creates_middleware():
call_kwargs = mock_middleware_class.call_args[1]
assert call_kwargs["cache_storage"] is mock_store
assert call_kwargs["list_tools_settings"] == {"ttl": 300}
+
+
+def test_create_response_caching_middleware_fails_closed_without_principal_optin():
+ """Enabling the cache without the explicit cross-principal opt-in is refused.
+
+ The cache key contains no principal and hits are served before any
+ authorization runs, so a shared cache would replay one principal's
+ responses to another (see create_response_caching_middleware).
+ """
+ mock_flask_app = MagicMock()
+ mock_configs = {
+ "MCP_CACHE_CONFIG": {"enabled": True, "call_tool_ttl": 3600},
+ "MCP_STORE_CONFIG": {"enabled": False},
+ }
+ mock_flask_app.config.get.side_effect = lambda key, default=None: mock_configs.get(
+ key, default
+ )
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.caching import (
+ create_response_caching_middleware,
+ )
+
+ assert create_response_caching_middleware() is None
+
+
+@pytest.mark.asyncio
+async def test_excluded_tools_covers_every_mutating_tool():
+ """Every registered tool without readOnlyHint=True must be listed in
+ MCP_CACHE_CONFIG["excluded_tools"].
+
+ Caching is keyed on tool name + arguments and served ahead of
+ per-request auth/RBAC, so a mutating tool left off this list can
+ silently replay a stale create/update/delete result to a caller who
+ repeats an identical call expecting it to actually run again. This
+ list is maintained by hand (FastMCP's caching middleware only accepts
+ a static exclusion list); this test is what keeps it complete as new
+ tools are added, by failing with the specific tool name(s) missing.
+ """
+ from superset.mcp_service.app import mcp
+ from superset.mcp_service.mcp_config import MCP_CACHE_CONFIG
+
+ tools = await mcp.list_tools()
+ mutating_tool_names = {
+ tool.name
+ for tool in tools
+ if tool.annotations is None or tool.annotations.readOnlyHint is not True
+ }
+
+ excluded = set(MCP_CACHE_CONFIG["excluded_tools"])
+ missing = mutating_tool_names - excluded
+ assert not missing, (
+ f"These mutating tools are cacheable because they're missing from "
+ f"MCP_CACHE_CONFIG['excluded_tools']: {sorted(missing)}"
+ )
diff --git a/tests/unit_tests/mcp_service/test_mcp_config.py b/tests/unit_tests/mcp_service/test_mcp_config.py
index ff19c0afd81..cbc4a68a2f9 100644
--- a/tests/unit_tests/mcp_service/test_mcp_config.py
+++ b/tests/unit_tests/mcp_service/test_mcp_config.py
@@ -421,9 +421,12 @@ def test_create_default_mcp_auth_factory_jwt_with_keys():
mock_build.assert_called_once()
-def test_create_default_mcp_auth_factory_jwt_enabled_without_keys_returns_none():
- """MCP_AUTH_ENABLED=True with no keys/secret and no API key auth returns None."""
- from superset.mcp_service.mcp_config import create_default_mcp_auth_factory
+def test_create_default_mcp_auth_factory_jwt_enabled_without_keys_fails_closed():
+ """MCP_AUTH_ENABLED=True with no keys/secret and no fallback must abort."""
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
mock_app = MagicMock()
mock_app.config.get.side_effect = lambda key, default=None: {
@@ -433,16 +436,40 @@ def test_create_default_mcp_auth_factory_jwt_enabled_without_keys_returns_none()
"MCP_JWT_AUDIENCE": "superset-mcp",
}.get(key, default)
- with patch("superset.mcp_service.mcp_config.logger") as mock_logger:
- result = create_default_mcp_auth_factory(mock_app)
+ with pytest.raises(MCPAuthConfigError):
+ create_default_mcp_auth_factory(mock_app)
- assert result is None
- mock_logger.warning.assert_called_once()
+def test_create_default_mcp_auth_factory_jwt_missing_keys_fails_closed_with_api_key():
+ """A missing JWT key must abort startup even when API-key auth is also
+ enabled: silently starting without the operator's requested JWT mode
+ would leave JWT clients unable to authenticate with only a log line to
+ show for it.
+ """
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
-def test_create_default_mcp_auth_factory_jwt_build_failure_returns_none():
- """A JWT verifier build failure with no API key fallback returns None."""
- from superset.mcp_service.mcp_config import create_default_mcp_auth_factory
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_ENABLED": True,
+ "MCP_API_KEY_ENABLED": True,
+ "FAB_API_KEY_ENABLED": False,
+ "FAB_API_KEY_PREFIXES": ["sst_"],
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ }.get(key, default)
+
+ with pytest.raises(MCPAuthConfigError):
+ create_default_mcp_auth_factory(mock_app)
+
+
+def test_create_default_mcp_auth_factory_jwt_build_failure_fails_closed():
+ """A JWT verifier build failure must abort startup, not disable auth."""
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
mock_app = MagicMock()
mock_app.config.get.side_effect = lambda key, default=None: {
@@ -460,12 +487,34 @@ def test_create_default_mcp_auth_factory_jwt_build_failure_returns_none():
),
patch("superset.mcp_service.mcp_config.logger") as mock_logger,
):
- result = create_default_mcp_auth_factory(mock_app)
+ with pytest.raises(MCPAuthConfigError):
+ create_default_mcp_auth_factory(mock_app)
- assert result is None
mock_logger.error.assert_called_once()
+def test_create_default_mcp_auth_factory_refuses_dev_username_with_auth():
+ """MCP_DEV_USERNAME + MCP_AUTH_ENABLED is a standing auth bypass."""
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_ENABLED": True,
+ "MCP_API_KEY_ENABLED": False,
+ "FAB_API_KEY_ENABLED": False,
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ "MCP_JWT_SECRET": "shhh",
+ "MCP_JWT_ALGORITHM": "HS256",
+ "MCP_DEV_USERNAME": "admin",
+ }.get(key, default)
+
+ with pytest.raises(MCPAuthConfigError, match="MCP_DEV_USERNAME"):
+ create_default_mcp_auth_factory(mock_app)
+
+
def test_create_default_mcp_auth_factory_requires_audience_when_jwt_enabled():
"""MCP_AUTH_ENABLED=True without MCP_JWT_AUDIENCE fails closed.
diff --git a/tests/unit_tests/mcp_service/test_mcp_config_hs256_guard.py b/tests/unit_tests/mcp_service/test_mcp_config_hs256_guard.py
new file mode 100644
index 00000000000..caceaafd160
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_mcp_config_hs256_guard.py
@@ -0,0 +1,150 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""HS256 key-confusion guard: never key an HMAC verifier on public material."""
+
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+PEM_PUBLIC_KEY = "-----BEGIN PUBLIC KEY-----\nMFkw...\n-----END PUBLIC KEY-----"
+
+
+def _mock_app(config: dict[str, Any]) -> MagicMock:
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: config.get(key, default)
+ return mock_app
+
+
+def test_build_jwt_verifier_refuses_hs256_without_secret():
+ """HS256 pinned but only public-key material configured must hard-error.
+
+ Before the fix this silently built an HS256 verifier keyed on the PEM
+ public key, letting anyone holding the (public) key forge admin tokens.
+ """
+ from superset.mcp_service.mcp_config import (
+ _build_jwt_verifier,
+ MCPAuthConfigError,
+ )
+
+ app = _mock_app(
+ {
+ "MCP_JWT_ALGORITHM": "HS256",
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ }
+ )
+ with pytest.raises(MCPAuthConfigError, match="MCP_JWT_SECRET"):
+ _build_jwt_verifier(
+ app=app,
+ jwks_uri=None,
+ public_key=PEM_PUBLIC_KEY,
+ secret=None,
+ )
+
+
+@pytest.mark.parametrize("algorithm", ["HS256", "HS384", "HS512"])
+def test_build_jwt_verifier_refuses_hmac_alongside_public_key_material(
+ algorithm: str,
+):
+ """HMAC algorithm plus leftover public key / JWKS config is refused."""
+ from superset.mcp_service.mcp_config import (
+ _build_jwt_verifier,
+ MCPAuthConfigError,
+ )
+
+ app = _mock_app(
+ {
+ "MCP_JWT_ALGORITHM": algorithm,
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ }
+ )
+ with pytest.raises(MCPAuthConfigError, match="MCP_JWT_PUBLIC_KEY"):
+ _build_jwt_verifier(
+ app=app,
+ jwks_uri=None,
+ public_key=PEM_PUBLIC_KEY,
+ secret="shhh", # noqa: S106
+ )
+
+
+def test_build_jwt_verifier_hs256_with_explicit_secret_still_works():
+ """A correct HS256 config (secret only) builds an HS256 verifier."""
+ from superset.mcp_service import mcp_config
+
+ app = _mock_app(
+ {
+ "MCP_JWT_ALGORITHM": "HS256",
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ }
+ )
+ with patch.object(mcp_config, "MCPJWTVerifier") as mock_verifier:
+ mcp_config._build_jwt_verifier(
+ app=app,
+ jwks_uri=None,
+ public_key=None,
+ secret="shhh", # noqa: S106
+ )
+ kwargs = mock_verifier.call_args.kwargs
+ assert kwargs["algorithm"] == "HS256"
+ assert kwargs["public_key"] == "shhh"
+
+
+def test_build_jwt_verifier_refuses_rs256_secret_only():
+ """RS256 (asymmetric) pinned but only a secret configured must hard-error.
+
+ A keyless RS256 verifier cannot validate anything -- name the fix rather
+ than letting the underlying verifier constructor raise opaquely.
+ """
+ from superset.mcp_service.mcp_config import (
+ _build_jwt_verifier,
+ MCPAuthConfigError,
+ )
+
+ app = _mock_app(
+ {
+ "MCP_JWT_ALGORITHM": "RS256",
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ }
+ )
+ with pytest.raises(MCPAuthConfigError, match="MCP_JWT_ALGORITHM"):
+ _build_jwt_verifier(
+ app=app,
+ jwks_uri=None,
+ public_key=None,
+ secret="shhh", # noqa: S106
+ )
+
+
+def test_auth_factory_propagates_hs256_config_error():
+ """The factory must not swallow the misconfiguration into a None provider."""
+ from superset.mcp_service.mcp_config import (
+ create_default_mcp_auth_factory,
+ MCPAuthConfigError,
+ )
+
+ app = _mock_app(
+ {
+ "MCP_AUTH_ENABLED": True,
+ "MCP_API_KEY_ENABLED": False,
+ "FAB_API_KEY_ENABLED": False,
+ "MCP_JWT_AUDIENCE": "superset-mcp",
+ "MCP_JWT_ALGORITHM": "HS256",
+ "MCP_JWT_PUBLIC_KEY": PEM_PUBLIC_KEY,
+ }
+ )
+ with pytest.raises(MCPAuthConfigError):
+ create_default_mcp_auth_factory(app)
diff --git a/tests/unit_tests/mcp_service/test_mcp_server.py b/tests/unit_tests/mcp_service/test_mcp_server.py
index 68b90aa844c..c899c0216d2 100644
--- a/tests/unit_tests/mcp_service/test_mcp_server.py
+++ b/tests/unit_tests/mcp_service/test_mcp_server.py
@@ -383,6 +383,55 @@ def test_create_auth_provider_fails_closed_on_insecure_guest_secret() -> None:
_create_auth_provider(flask_app)
+def test_create_auth_provider_fails_closed_on_default_factory_error() -> None:
+ """A generic error while building the enabled auth provider must abort.
+
+ Verifier-construction failures (bad key material, config typos) used to be
+ swallowed, silently starting an unauthenticated server.
+ """
+ from superset.mcp_service.mcp_config import MCPAuthConfigError
+ from superset.mcp_service.server import _create_auth_provider
+
+ flask_app = MagicMock()
+ flask_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_FACTORY": None,
+ "MCP_AUTH_ENABLED": True,
+ "MCP_API_KEY_ENABLED": False,
+ "FAB_API_KEY_ENABLED": False,
+ }.get(key, default)
+
+ with patch(
+ "superset.mcp_service.mcp_config.create_default_mcp_auth_factory",
+ side_effect=ValueError("bad PEM"),
+ ):
+ with pytest.raises(MCPAuthConfigError):
+ _create_auth_provider(flask_app)
+
+
+def test_create_auth_provider_fails_closed_on_custom_factory_error() -> None:
+ """MCP_AUTH_FACTORY raising (or yielding None) must abort startup."""
+ from superset.mcp_service.mcp_config import MCPAuthConfigError
+ from superset.mcp_service.server import _create_auth_provider
+
+ def broken_factory(app: Any) -> Any:
+ raise ValueError("bad key material")
+
+ flask_app = MagicMock()
+ flask_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_FACTORY": broken_factory,
+ }.get(key, default)
+
+ with pytest.raises(MCPAuthConfigError):
+ _create_auth_provider(flask_app)
+
+ flask_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_FACTORY": lambda app: None,
+ }.get(key, default)
+
+ with pytest.raises(MCPAuthConfigError):
+ _create_auth_provider(flask_app)
+
+
@contextlib.contextmanager
def _run_server_dependencies(
flask_config: dict[str, Any],
diff --git a/tests/unit_tests/mcp_service/test_middleware.py b/tests/unit_tests/mcp_service/test_middleware.py
index 2b1529a3880..251fc9ebd32 100644
--- a/tests/unit_tests/mcp_service/test_middleware.py
+++ b/tests/unit_tests/mcp_service/test_middleware.py
@@ -39,6 +39,7 @@ from superset.mcp_service.constants import DEFAULT_MAX_LIST_ITEMS
from superset.mcp_service.mcp_config import MCP_RESPONSE_SIZE_CONFIG
from superset.mcp_service.middleware import (
_is_user_error,
+ _sanitize_params,
create_response_size_guard_middleware,
GlobalErrorHandlerMiddleware,
RBACToolVisibilityMiddleware,
@@ -1381,6 +1382,106 @@ class TestGlobalErrorHandlerLogLevels:
# Should log at ERROR (both the classification log and the error_id log)
assert mock_logger.error.call_count >= 1
+ @pytest.mark.asyncio
+ async def test_value_error_message_is_sanitized(self) -> None:
+ """A ValueError's text reaches the client through _sanitize_error_for_logging.
+
+ ValueError is deliberately not in that sanitizer's generic-message
+ list (so LLM callers still get parameter feedback), but a connection
+ string embedded in the message must still be redacted, not returned
+ verbatim.
+ """
+ middleware = GlobalErrorHandlerMiddleware()
+
+ context = MagicMock()
+ context.message.name = "execute_sql"
+ context.method = "tools/call"
+
+ call_next = AsyncMock(
+ side_effect=ValueError(
+ "Invalid config: postgresql://admin:hunter2@db.internal/prod"
+ )
+ )
+
+ with (
+ patch("superset.mcp_service.middleware.get_user_id", return_value=1),
+ patch("superset.mcp_service.middleware.event_logger"),
+ patch("superset.mcp_service.middleware.logger"),
+ ):
+ with pytest.raises(ToolError) as exc_info:
+ await middleware.on_message(context, call_next)
+
+ message = str(exc_info.value)
+ assert "hunter2" not in message
+ assert "db.internal" not in message
+ assert "Invalid config" in message
+
+ @pytest.mark.asyncio
+ async def test_http_exception_detail_is_sanitized(self) -> None:
+ """HTTPException.detail reaches the client through
+ _sanitize_error_for_logging instead of being interpolated raw."""
+ from starlette.exceptions import HTTPException
+
+ middleware = GlobalErrorHandlerMiddleware()
+
+ context = MagicMock()
+ context.message.name = "get_chart_preview"
+ context.method = "tools/call"
+
+ call_next = AsyncMock(
+ side_effect=HTTPException(
+ status_code=502,
+ detail="Upstream failed: postgresql://admin:hunter2@db.internal/prod",
+ )
+ )
+
+ with (
+ patch("superset.mcp_service.middleware.get_user_id", return_value=1),
+ patch("superset.mcp_service.middleware.event_logger"),
+ patch("superset.mcp_service.middleware.logger"),
+ ):
+ with pytest.raises(ToolError) as exc_info:
+ await middleware.on_message(context, call_next)
+
+ message = str(exc_info.value)
+ assert "hunter2" not in message
+ assert "db.internal" not in message
+ assert "Upstream failed" in message
+
+
+class TestSanitizeParams:
+ """Tests for _sanitize_params's recursion into nested containers."""
+
+ def test_redacts_top_level_sensitive_key(self) -> None:
+ result = _sanitize_params({"password": "hunter2", "name": "alice"})
+ assert result["password"] == "[REDACTED]" # noqa: S105
+ assert result["name"] == "alice"
+
+ def test_redacts_sensitive_key_nested_under_arguments(self) -> None:
+ result = _sanitize_params({"arguments": {"password": "hunter2"}})
+ assert result["arguments"]["password"] == "[REDACTED]" # noqa: S105
+
+ def test_redacts_sensitive_key_nested_under_request(self) -> None:
+ """Any nested dict wrapper is redacted, not just the literal
+ 'arguments' key -- Pydantic-request tools wrap params under 'request'."""
+ result = _sanitize_params({"request": {"password": "hunter2"}})
+ assert result["request"]["password"] == "[REDACTED]" # noqa: S105
+
+ def test_redacts_sensitive_key_inside_list_of_dicts(self) -> None:
+ result = _sanitize_params({"items": [{"token": "abc123"}, {"name": "x"}]})
+ assert result["items"][0]["token"] == "[REDACTED]" # noqa: S105
+ assert result["items"][1]["name"] == "x"
+
+ def test_redacts_sensitive_key_inside_nested_list_of_lists(self) -> None:
+ """A list nested inside another list must still be recursed into,
+ not copied unchanged -- otherwise a sensitive key inside it would
+ reach the audit log unredacted."""
+ result = _sanitize_params({"items": [[{"password": "hunter2"}]]})
+ assert result["items"][0][0]["password"] == "[REDACTED]" # noqa: S105
+
+ def test_non_dict_passthrough(self) -> None:
+ assert _sanitize_params("not-a-dict") == "not-a-dict" # type: ignore[arg-type]
+
@pytest.mark.asyncio
async def test_event_logger_includes_severity(self) -> None:
"""Event logger payload should include severity field."""