diff --git a/.github/workflows/superset-python-unittest.yml b/.github/workflows/superset-python-unittest.yml index 3d2ee7715bd..b723e6afb65 100644 --- a/.github/workflows/superset-python-unittest.yml +++ b/.github/workflows/superset-python-unittest.yml @@ -75,13 +75,18 @@ jobs: SUPERSET_SECRET_KEY: not-a-secret run: | pytest --durations-min=0.5 --cov-report= --cov=superset ./tests/common ./tests/unit_tests --cache-clear --maxfail=50 --junit-xml=test-results/junit-unit.xml + # COVERAGE_FILE keeps these scoped gates off the default .coverage that + # the step above wrote. pytest-cov starts a fresh data file per run, so + # without it the last gate replaces the full-suite data and the report + # uploaded below contains only that gate's subtree -- every other file + # then reads as uncovered, and patch coverage on an unrelated PR is 0%. - name: Python 100% coverage unit tests env: SUPERSET_TESTENV: true SUPERSET_SECRET_KEY: not-a-secret run: | - pytest --durations-min=0.5 --cov=superset/sql/ ./tests/unit_tests/sql/ --cache-clear --cov-fail-under=100 --junit-xml=test-results/junit-sql-coverage.xml - pytest --durations-min=0.5 --cov=superset/semantic_layers/ ./tests/unit_tests/semantic_layers/ --cache-clear --cov-fail-under=100 --junit-xml=test-results/junit-semantic-layers-coverage.xml + COVERAGE_FILE="${RUNNER_TEMP}/.coverage.sql" pytest --durations-min=0.5 --cov=superset/sql/ ./tests/unit_tests/sql/ --cache-clear --cov-fail-under=100 --junit-xml=test-results/junit-sql-coverage.xml + COVERAGE_FILE="${RUNNER_TEMP}/.coverage.semantic_layers" pytest --durations-min=0.5 --cov=superset/semantic_layers/ ./tests/unit_tests/semantic_layers/ --cache-clear --cov-fail-under=100 --junit-xml=test-results/junit-semantic-layers-coverage.xml - name: Upload code coverage uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py index 064b6305407..a2053db56f7 100644 --- a/superset/mcp_service/server.py +++ b/superset/mcp_service/server.py @@ -811,15 +811,33 @@ def _create_auth_provider(flask_app: Any) -> Any | None: """ 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( "Auth provider created from MCP_AUTH_FACTORY: %s", type(auth_provider).__name__ if auth_provider else "None", ) - except Exception: - # Do not log the exception — it may contain secrets - logger.error("Failed to create auth provider from MCP_AUTH_FACTORY") + except MCPAuthConfigError: + # Operator-facing config guidance raised by the factory itself; + # carries no secret material. Propagate as-is. + raise + except Exception as ex: + # A configured MCP_AUTH_FACTORY that cannot build its provider is a + # misconfiguration that must fail closed: falling through would + # start the service unauthenticated. Unlike the default factory + # below, an operator-supplied factory gives no basis to classify + # any of its failures as benign build errors. The original + # exception is suppressed (from None) rather than chained because + # its message may contain secrets; the type name is enough to + # locate the failure. + raise MCPAuthConfigError( + "MCP_AUTH_FACTORY is configured but raised " + f"{type(ex).__name__} while building the auth provider; " + "refusing to start the MCP service without authentication. " + "Fix the factory or unset MCP_AUTH_FACTORY." + ) from None elif ( flask_app.config.get("MCP_AUTH_ENABLED", False) or flask_app.config.get("MCP_API_KEY_ENABLED", False) diff --git a/tests/unit_tests/mcp_service/test_mcp_server.py b/tests/unit_tests/mcp_service/test_mcp_server.py index db701189b6e..68b90aa844c 100644 --- a/tests/unit_tests/mcp_service/test_mcp_server.py +++ b/tests/unit_tests/mcp_service/test_mcp_server.py @@ -291,6 +291,69 @@ def test_create_auth_provider_propagates_auth_config_error() -> None: _create_auth_provider(flask_app) +def test_create_auth_provider_fails_closed_when_custom_factory_raises() -> None: + """A failing MCP_AUTH_FACTORY must abort startup, not fall through to no auth. + + A custom factory is operator configuration evaluated at startup, so it can + fail for mundane reasons (a missing environment variable, a dependency + moving a symbol, a verifier's signature changing). Swallowing that leaves + auth_provider as None and the service comes up unauthenticated. The + original exception must not appear in the raised message — it may contain + secrets — but its type name should, to point at the failure. + """ + 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": MagicMock( + side_effect=KeyError("secret-bearing-env-var-value") + ), + }.get(key, default) + + with pytest.raises(MCPAuthConfigError) as excinfo: + _create_auth_provider(flask_app) + + assert "KeyError" in str(excinfo.value) + assert "secret-bearing-env-var-value" not in str(excinfo.value) + assert excinfo.value.__cause__ is None + assert excinfo.value.__suppress_context__ + + +def test_create_auth_provider_passes_through_custom_factory_config_error() -> None: + """A custom factory raising MCPAuthConfigError keeps its own message. + + That message is operator-facing config guidance and carries no secret + material by contract, so it must propagate unwrapped rather than being + replaced by the generic type-name-only message. + """ + 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": MagicMock( + side_effect=MCPAuthConfigError("MY_AUDIENCE_SETTING must be set") + ), + }.get(key, default) + + with pytest.raises(MCPAuthConfigError, match="MY_AUDIENCE_SETTING must be set"): + _create_auth_provider(flask_app) + + +def test_create_auth_provider_uses_custom_factory_result() -> None: + """The happy path is unchanged: the factory's provider is returned.""" + from superset.mcp_service.server import _create_auth_provider + + auth_provider = MagicMock() + flask_app = MagicMock() + flask_app.config.get.side_effect = lambda key, default=None: { + "MCP_AUTH_FACTORY": MagicMock(return_value=auth_provider), + }.get(key, default) + + assert _create_auth_provider(flask_app) is auth_provider + + def test_create_auth_provider_fails_closed_on_insecure_guest_secret() -> None: """Guest-only deployment with an insecure GUEST_TOKEN_JWT_SECRET must abort.