From b33a0524f52e4c1d6389b4e9e7cafc4c5afd8af5 Mon Sep 17 00:00:00 2001 From: blackax Date: Wed, 30 Sep 2026 07:28:18 -0700 Subject: [PATCH 1/2] fix(config): reject non-list groups instead of coercing --- AGENTS.md | 2 +- CHANGELOG.md | 6 ++++++ src/ssh_mcp/config.py | 7 +------ src/ssh_mcp/models.py | 10 ++++++++++ tests/test_config.py | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 78634d4..2cafb46 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Command sources: `.github/workflows/ci.yml` (the gates), `.github/workflows/rele ```bash uv sync --locked --extra dev # setup. --locked is required; see Gotchas -uv run pytest # full suite (850 tests, seconds) +uv run pytest # full suite (853 tests, seconds) uv run pytest tests/test_ssh.py::TestRedactSecrets -v # one class uv run pytest 'tests/test_ssh.py::TestRedactSecrets::' -v # one test uv run pytest -k "confinement" -v # by keyword diff --git a/CHANGELOG.md b/CHANGELOG.md index 7da5d5a..2705d80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +> **The next release is 0.9.0, not 0.8.2:** the `groups` fix under Fixed makes two configurations that started on 0.8.1 refuse to start, and by the rule recorded under 0.8.1 below that takes the minor. + ### Security **PyJWT is held at 2.14.0 or later, clearing ten advisories that turned the `pip-audit` gate red on `main`.** 2.13.0, locked transitively through `mcp`, carries CVE-2026-101917, CVE-2026-102265 to CVE-2026-102269 and CVE-2026-102271 to CVE-2026-102274: four algorithm-confusion variants, an empty-HMAC-key bypass, lenient Base64URL signature decoding, a followed JWKS redirect, and three denial-of-service paths in JWKS fetching and token/JWKS parsing. None is reachable here: the only `import jwt` in mcp 2.2.0 is `mcp/client/auth/extensions/client_credentials.py`, an OAuth *client* extension ssh-mcp never imports; its HTTP transport authenticates with its own bearer-token middleware. @@ -15,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **Stateful HTTP sessions now expire after 30 idle minutes, and at most 10,000 are held at once.** Both limits are mcp 2.2.0 defaults (#64), which the lockfile and container image now ship, and they apply only to stateful sessions on the legacy (2025-11-25 and earlier) protocol. A session with no request in flight and no open GET stream is closed after 1800 s; its next request gets `404` `Session not found`, which a client that does not re-initialize surfaces as an error. An open GET stream prevents expiry but still counts toward the cap, and a session opened beyond the cap gets `503`. Stateless mode (`SSH_MCP_HTTP_STATELESS=true`) avoids both limits. ssh-mcp deliberately exposes no knob for either: both bound server-side memory on an endpoint that executes shell commands. The declared floor stays `mcp>=2.1.1`, so an existing environment that already has 2.1.1 keeps the old unbounded behaviour until it upgrades. +### Fixed + +**A non-list `groups` is now rejected at load instead of being coerced.** `groups = "web"` used to load as `('w', 'e', 'b')`: the config started with three undefined-group warnings and the server dropped out of group `web`, so `execute_on_group` skipped it. The loader's `tuple()` pre-conversion is gone, so Pydantic validates the raw value and still converts a TOML list to a tuple; `groups = 5` is now a `ConfigError` instead of a raw `TypeError`. **Two shapes that used to start now refuse to start:** a string `groups`, and an inline table (`groups = { web = 1 }`, which used to load as `('web',)`). The stdio `ssh-mcp healthcheck` reports the same configs `UNHEALTHY`, so a container healthcheck in the default stdio mode that passed will now fail. The fix for both is a list: `groups = ["web"]`, which the refusal message now names. + ## [0.8.1] - 2026-09-07 > **Why 0.8.1 and not 0.9.0:** nothing here changes a configuration that started successfully on 0.8.0 into one that refuses to start, which is the test 0.8.0 itself recorded for taking the minor. The one behaviour an operator could notice is that four more command shapes are now blocked by the dangerous-command tripwire — and that list has always been documented as advisory and subject to widening, `force=true` still bypasses it, and no *configuration* becomes invalid. Everything else is a hang becoming an error, a log line becoming honest, an image gaining an architecture, and prose corrections. diff --git a/src/ssh_mcp/config.py b/src/ssh_mcp/config.py index cacc324..173c787 100644 --- a/src/ssh_mcp/config.py +++ b/src/ssh_mcp/config.py @@ -12,7 +12,6 @@ import os import tomllib from pathlib import Path -from typing import Any from pydantic import ValidationError @@ -251,14 +250,10 @@ def _load(self) -> None: raise ConfigError(f"{detail}. Valid keys: {valid}") from e for server_name, server_data in config_data.get("servers", {}).items(): - # Convert groups list to tuple before Pydantic sees it - data: dict[str, Any] = dict(server_data) - if "groups" in data: - data["groups"] = tuple(data["groups"]) try: self._servers[server_name] = ServerConfig( name=server_name, - **data, + **dict(server_data), ) except ValidationError as e: detail = _format_validation_error("servers", server_name, e) diff --git a/src/ssh_mcp/models.py b/src/ssh_mcp/models.py index 85a069b..b3206da 100644 --- a/src/ssh_mcp/models.py +++ b/src/ssh_mcp/models.py @@ -182,6 +182,16 @@ class ServerConfig: default_dir: str | None = None timeout: int | None = Field(default=None, ge=1, le=3600) + @field_validator("groups", mode="before") + @classmethod + def _require_group_list(cls, value: object) -> object: + # TOML has no tuple type, so Pydantic's own "Input should be a valid + # tuple" gives a servers.toml author nothing to act on. Name the shape + # the file needs instead. + if not isinstance(value, (list, tuple)): + raise ValueError('must be a list of group names, e.g. groups = ["web"]') + return value + @field_validator("identity_file") @classmethod def _expand_identity_file(cls, value: str | None) -> str | None: diff --git a/tests/test_config.py b/tests/test_config.py index 7406bb9..c50ca47 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -398,6 +398,39 @@ def test_invalid_settings_value_propagates_as_config_error( ServerRegistry(str(f)) assert "command_timeout" in str(exc_info.value) + @pytest.mark.parametrize("groups_value", ['"web"', "5", "{ web = 1 }"]) + def test_non_list_groups_rejected_not_coerced( + self, tmp_path: Path, groups_value: str + ) -> None: + """A non-list `groups` must raise ConfigError, not be coerced. + + The loader used to call tuple() on the raw value before Pydantic saw + it. `groups = "web"` loaded as ('w', 'e', 'b') with three + undefined-group warnings, and the server silently dropped out of + group `web`; `groups = 5` escaped as a raw TypeError; and + `groups = { web = 1 }` loaded as ('web',) by iterating the table's + keys. + """ + config_content = f""" +[groups] +web = {{ description = "Web" }} + +[servers.a] +hostname = "h" +user = "u" +description = "d" +groups = {groups_value} +""" + f = tmp_path / "non-list-groups.toml" + f.write_text(config_content) + with pytest.raises(ConfigError) as exc_info: + ServerRegistry(str(f)) + # Every [servers] ConfigError ends with "Valid keys: ..., groups, ...", + # so a bare "groups" substring can never fail; the quoted form with a + # colon appears only when `groups` itself is the rejected field. + assert "'groups':" in str(exc_info.value) + assert 'groups = ["web"]' in str(exc_info.value) + def test_config_error_is_valueerror_subclass(self) -> None: """ConfigError must remain a ValueError subclass for backward compat.""" assert issubclass(ConfigError, ValueError) From 1e67148080fa44db8141bca8e99e3d3f9b7c4784 Mon Sep 17 00:00:00 2001 From: blackax Date: Wed, 30 Sep 2026 07:28:18 -0700 Subject: [PATCH 2/2] docs: correct mcp 2.2.0 citations and _build_http_app signature --- AGENTS.md | 2 +- src/ssh_mcp/server.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2cafb46..ecd1b4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -165,7 +165,7 @@ Most of these will break a build, a release, or production if ignored. The last - **Never lower the `asyncssh` log levels — that suppression *is* a credential control.** `server.py::_configure_logging` raises the `asyncssh`, `asyncssh.sftp` and `asyncssh.connection` loggers to WARNING because asyncssh logs the full raw command, credentials included, at INFO via its own channel logger. That is "Production incident 2026-04-11 (round 2)", and `_redact_secrets` did **not** fix it: redaction only sanitizes the ssh-mcp logger, so the password leaked anyway. Turning those loggers back down to INFO to debug an SSH problem reopens the leak. Note also that `_configure_logging()` is called at *import* time and clears root handlers, so anything configuring logging around it will be overwritten. -- **`mcp>=2` removed the `mcp.settings` transport fields, and `_build_http_app`'s three keyword arguments have no defaults, deliberately.** v1's `Settings` carried `host`/`port`/`stateless_http`/`transport_security`; v2's does not, so transport config is passed straight to `streamable_http_app()` instead. `_build_transport_security(raw_allowed_hosts, host) -> TransportSecuritySettings` and `_build_http_app(token, *, host, stateless, transport_security)` in `server.py` both require every argument — no `transport_security=None` shortcut. `mcp/server/transport_security.py:48` turns a `None` settings object into `TransportSecuritySettings(enable_dns_rebinding_protection=False)`, so a default would silently fail open on a non-loopback bind. Do not add one to "simplify" a callsite. +- **`mcp>=2` removed the `mcp.settings` transport fields, and `_build_http_app`'s two keyword arguments have no defaults, deliberately.** v1's `Settings` carried `host`/`port`/`stateless_http`/`transport_security`; v2's does not, so transport config is passed straight to `streamable_http_app()` instead. `_build_transport_security(raw_allowed_hosts, host) -> TransportSecuritySettings` and `_build_http_app(token, *, stateless, transport_security)` in `server.py` both require every argument — no `transport_security=None` shortcut. `mcp/server/transport_security.py:48` turns a `None` settings object into `TransportSecuritySettings(enable_dns_rebinding_protection=False)`, so a default would silently fail open on a non-loopback bind. Do not add one to "simplify" a callsite. - **`mcp>=2` logs a tool-failure message at INFO before converting it to a client-visible error.** `mcp/server/mcpserver/server.py:444` (mcp 2.2.0) does `logger.info("Tool %r failed: %r", params.name, str(exc))` on the `mcp.server.mcpserver.server` logger before turning a `ToolError` into `CallToolResult(is_error=True)`. `upload_file`/`download_file` are the tools that raise, and their messages carry local and remote paths. `_configure_logging` now raises that logger to WARNING for the same reason it already raises the `asyncssh` loggers above — "Production incident 2026-04-11 (round 2)" was a third-party library logging sensitive text at INFO, and this is the same class of leak on a different logger. diff --git a/src/ssh_mcp/server.py b/src/ssh_mcp/server.py index 3b6bfea..ed38330 100644 --- a/src/ssh_mcp/server.py +++ b/src/ssh_mcp/server.py @@ -722,7 +722,7 @@ def _build_http_app( # (Starlette itself went 0.52.1 -> 1.3.1 during this release cycle, # unconstrained by this project). `streamable_http_app()` above wires # its own inner Starlette app with `lifespan=lambda app: - # self.session_manager.run()` (mcp/server/lowlevel/server.py:840 in mcp 2.2.0) — + # session_manager.run()` (mcp/server/lowlevel/server.py:840 in mcp 2.2.0) — # calling `lifespan_ctx(inner_app)` therefore did nothing but reach `.run()` # through a private indirection. `MCPServer.session_manager` is the # SDK-documented public accessor for the exact same object, and @@ -748,7 +748,7 @@ def _build_http_app( "streamable_http_app() — this used to also populate " "Starlette's private router.lifespan_context, which " "ssh-mcp no longer depends on. The MCP SDK may have changed " - "its internal wiring (as of mcp 2.1.1 it lives in " + "its internal wiring (as of mcp 2.2.0 it lives in " "mcp/server/lowlevel/server.py); update the lifespan wiring " "in server.py._build_http_app or pin a known-good mcp." ) from exc @@ -975,7 +975,7 @@ def _build_transport_security( # two independent validators plus a direct read of the installed # SDK). Until 0.7.0 this gate permitted "*.internal.example.com" # and three docs sites advertised it, but the SDK never - # implemented suffix matching: mcp 2.1.1's + # implemented suffix matching: mcp 2.2.0's # TransportSecurityMiddleware._validate_host # (mcp/server/transport_security.py:50-70) does an exact-set # lookup and then ONE trailing ":*" port-wildcard pass, and there