From 661460bfd9698fa6fdd91a0d49accf5c51c1e6fe Mon Sep 17 00:00:00 2001 From: Marcus van Elst Date: Mon, 18 May 2026 09:07:00 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20v0.2.9=20=E2=80=94=20review=20welle=201?= =?UTF-8?q?=20(C-1,=20C-2,=20M-3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-1: __version__ now derived from package metadata via importlib.metadata.version() so pyproject.toml is the single source of truth. Previously stuck at "0.1.0" since the initial release. C-2: Backfill CHANGELOG entries for 0.2.7 and 0.2.8 (both releases had shipped without changelog updates) and add a 0.2.9 entry covering this welle. M-3: Reject project names containing path separators or other unsafe characters before they reach _find_compose_path. Previously a name like "../../etc" could traverse out of compose_base_path when the project was not yet registered with Container Manager. Adds _validate_project_name (regex ^[a-zA-Z0-9_-]+$, applied in read_compose, update_compose, update_image_tag, update_env_var) plus parametrized tests for valid and unsafe names and one rejection test per tool. 236 tests pass. Also: ruff format autofix on three pre-existing files (cli.py, config.py, test_config.py) — cosmetic only. Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 87 +++++++++++++ pyproject.toml | 2 +- src/mcp_synology_container/__init__.py | 7 +- src/mcp_synology_container/cli.py | 1 + src/mcp_synology_container/config.py | 5 +- src/mcp_synology_container/modules/compose.py | 29 +++++ tests/test_config.py | 1 - tests/test_modules/test_compose.py | 120 ++++++++++++++++++ uv.lock | 2 +- 9 files changed, 246 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 563b99d..351d414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,93 @@ All notable changes to this project will be documented in this file. +## [0.2.9] - 2026-05-18 + +### Fixed + +- `__version__` is now derived from package metadata via + `importlib.metadata.version()`, eliminating the drift between + `pyproject.toml` and `src/mcp_synology_container/__init__.py` (was stuck + at `0.1.0` since the initial release). +- `compose.py`: reject project names that contain path separators or other + unsafe characters before they reach `_find_compose_path`. Previously a + name like `"../../etc"` could traverse out of `compose_base_path` when + the project was not yet registered with Container Manager. The new + `_validate_project_name` helper enforces `^[a-zA-Z0-9_-]+$` and is + applied to `read_compose`, `update_compose`, `update_image_tag`, and + `update_env_var`. Addresses M-3 from the v0.2.8 review. + +### Docs + +- `CHANGELOG.md` backfilled for releases 0.2.7 and 0.2.8 (entries had been + missed during those releases). + +## [0.2.8] - 2026-04-21 + +### Added + +- `tests/test_dsm_client.py` — comprehensive offline test suite for + `DsmClient`: + - `_scrub_url` and `_error_message` pure helpers. + - `request()` happy-path, API-not-cached, `_sid` scrubbing in + `HTTPStatusError`, sensitive-param log masking. + - Session re-auth retry: single-retry semantics, auth-manager-absent + path, re-auth failure path, thundering-herd (login called once under + concurrent 106 responses). + - `trigger_build_stream`: SSE fire-and-forget, JSON error detection, + `ReadTimeout` swallowing, HTTP-error scrubbing. + - `upload_text` and `download_text` happy-path + error-response branches. + - `_ensure_initialized` double-checked locking and negative-cache + cooldown behavior. + +### Fixed + +- `DsmClient._ensure_initialized`: cache failed init outcomes for 60 s so + that repeated tool calls during a credential outage (wrong password, + IP-blocked 407, DNS failure) do not keep hammering DSM. Each caller + receives the cached exception until the cooldown window expires, after + which a fresh attempt is made. Adds `INIT_ERROR_COOLDOWN` module + constant and `_init_error` / `_init_error_until` state. Addresses M4 + from the 0.2.7 review. + +## [0.2.7] - 2026-04-21 + +### Fixed + +- `DsmClient`: scrub `_sid` query-parameter values from URLs embedded in + `httpx.HTTPStatusError` messages so the raw DSM session ID never reaches + log output or MCP tool responses (C1). +- `DsmClient`: re-auth lock now snapshots `_sid` before entering the lock + and skips the redundant login if another task has already refreshed the + session, eliminating duplicate logins on concurrent 106/107/119 + responses (M3). +- `DsmClient.trigger_build_stream`: re-instates immediate JSON-error + detection (regression from 0.2.6). Inspects the `Content-Type` header + and reads a small capped prefix of the body for `application/json` + responses to surface DSM error codes without forcing the caller into a + multi-minute polling timeout. SSE responses remain fire-and-forget + (C2/M8). +- `compose.update_env_var`: parenthesise the apply-branch match condition + so `(isinstance AND startswith) OR (entry == var_name)` no longer + evaluates the equality branch for non-string entries — aligns the apply + side with the preview-side detection logic (M1). + +### Changed + +- All 23 `@mcp.tool()` functions: strip `-> str` return annotations and + trim docstrings to a single line (≤100 chars). FastMCP generates an + `outputSchema` entry for every annotated tool, which roughly doubles + the `tools/list` payload size; multi-line docstrings with + `Args:`/`Returns:` sections add further bulk that Claude Desktop must + parse on every connection. + +### Chore + +- `uv.lock` resynced (was stale at `0.2.2`). +- `.gitignore`: exclude `.claude/` per-user Claude Code settings. +- Mechanical `ruff check --fix` + `ruff format` cleanup (import sorting, + unused-import removal). No functional change. + ## [0.2.6] - 2026-04-21 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index b1082bf..9f8bb76 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "mcp-synology-container" -version = "0.2.8" +version = "0.2.9" description = "MCP server for Synology Container Manager" requires-python = ">=3.12" dependencies = [ diff --git a/src/mcp_synology_container/__init__.py b/src/mcp_synology_container/__init__.py index f054651..c9a0d3a 100644 --- a/src/mcp_synology_container/__init__.py +++ b/src/mcp_synology_container/__init__.py @@ -1,3 +1,8 @@ """MCP server for Synology Container Manager.""" -__version__ = "0.1.0" +from importlib.metadata import PackageNotFoundError, version + +try: + __version__ = version("mcp-synology-container") +except PackageNotFoundError: + __version__ = "0.0.0+unknown" diff --git a/src/mcp_synology_container/cli.py b/src/mcp_synology_container/cli.py index 7520513..7471040 100644 --- a/src/mcp_synology_container/cli.py +++ b/src/mcp_synology_container/cli.py @@ -291,6 +291,7 @@ def serve(config_path: str | None) -> None: # and anyio.run() ensures the correct backend context is set up. # asyncio.run() can cause issues on Windows (ProactorEventLoop + anyio). import anyio + anyio.run(_run_serve, config_path) diff --git a/src/mcp_synology_container/config.py b/src/mcp_synology_container/config.py index 63e628a..ac21abd 100644 --- a/src/mcp_synology_container/config.py +++ b/src/mcp_synology_container/config.py @@ -132,10 +132,7 @@ def _validate_config(raw: dict[str, Any]) -> AppConfig: """ schema_version = raw.get("schema_version") if schema_version != CURRENT_SCHEMA_VERSION: - msg = ( - f"Config schema_version is {schema_version!r}, " - f"expected {CURRENT_SCHEMA_VERSION}." - ) + msg = f"Config schema_version is {schema_version!r}, expected {CURRENT_SCHEMA_VERSION}." raise ValueError(msg) conn_raw = raw.get("connection", {}) diff --git a/src/mcp_synology_container/modules/compose.py b/src/mcp_synology_container/modules/compose.py index 16c0449..bc41f61 100644 --- a/src/mcp_synology_container/modules/compose.py +++ b/src/mcp_synology_container/modules/compose.py @@ -25,6 +25,11 @@ logger = logging.getLogger(__name__) _VOLUME_PREFIX_RE = re.compile(r"^/volume\d+") +# Project names are used as path components for FileStation lookups when the +# project is not yet registered with Container Manager. Restrict to a safe +# subset so a malicious name like "../../etc" cannot escape compose_base_path. +_PROJECT_NAME_RE = re.compile(r"^[a-zA-Z0-9_-]+$") + def _to_filestation_path(path: str) -> str: """Strip /volumeN prefix so paths work with the FileStation API. @@ -35,6 +40,22 @@ def _to_filestation_path(path: str) -> str: return _VOLUME_PREFIX_RE.sub("", path) +def _validate_project_name(project_name: str) -> str | None: + """Return an error message if project_name is unsafe, else None. + + Allowed characters: letters, digits, underscore, hyphen. Anything else + (including '/', '\\', '..', whitespace, empty string) is rejected because + the name flows into FileStation path construction when the project is + not yet known to Container Manager. + """ + if not project_name or not _PROJECT_NAME_RE.match(project_name): + return ( + f"Error: invalid project name {project_name!r}. " + "Allowed: letters, digits, '_' and '-' (no slashes, dots, or spaces)." + ) + return None + + # Recognized compose file names (in priority order) _COMPOSE_FILENAMES = [ "docker-compose.yml", @@ -68,6 +89,8 @@ def register_compose(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None @mcp.tool() async def read_compose(project_name: str): """Read the compose file (YAML) for a project.""" + if (err := _validate_project_name(project_name)) is not None: + return err path = await _find_compose_path(client, config, project_name) if path is None: project = await _find_project(client, project_name) @@ -97,6 +120,8 @@ def register_compose(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None confirmed: bool = False, ): """Update a service's image tag in the compose file. Requires confirmed=True.""" + if (err := _validate_project_name(project_name)) is not None: + return err path = await _find_compose_path(client, config, project_name) if path is None: return f"No compose file found for project '{project_name}'." @@ -216,6 +241,8 @@ def register_compose(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None confirmed: bool = False, ): """Add or update an env var in a service's compose definition. Requires confirmed=True.""" + if (err := _validate_project_name(project_name)) is not None: + return err path = await _find_compose_path(client, config, project_name) if path is None: return f"No compose file found for project '{project_name}'." @@ -311,6 +338,8 @@ def register_compose(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None confirmed: bool = False, ): """Replace the entire compose file with new YAML content. Requires confirmed=True.""" + if (err := _validate_project_name(project_name)) is not None: + return err # Validate YAML before anything else try: parsed = yaml.safe_load(new_content) diff --git a/tests/test_config.py b/tests/test_config.py index 2dff42c..09de077 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,6 +1,5 @@ """Tests for config.py.""" - import pytest import yaml diff --git a/tests/test_modules/test_compose.py b/tests/test_modules/test_compose.py index 83c25e5..9fba9f8 100644 --- a/tests/test_modules/test_compose.py +++ b/tests/test_modules/test_compose.py @@ -5,6 +5,8 @@ from unittest.mock import AsyncMock import pytest import yaml +from mcp_synology_container.modules.compose import _validate_project_name + def make_mock_mcp(): tools: dict = {} @@ -345,3 +347,121 @@ def test_extract_version_prefix(): assert _extract_version_prefix("1.24") is None # no suffix assert _extract_version_prefix("") is None assert _extract_version_prefix("v2.0-rc1") is None # starts with 'v' + + +# ────────────────────────────────────────────────────────────────────── +# _validate_project_name — path-traversal guard +# ────────────────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize( + "name", + [ + "myapp", + "MyApp", + "my-app", + "my_app", + "app123", + "A", + "1", + "snake_case-and-dash_42", + ], +) +def test_validate_project_name_accepts_safe_names(name: str) -> None: + assert _validate_project_name(name) is None + + +@pytest.mark.parametrize( + "name", + [ + "", # empty + "../etc", # parent traversal + "../../etc/passwd", # multi-level traversal + "foo/../bar", # embedded traversal + "foo/bar", # forward slash + "foo\\bar", # backslash + ".", # bare dot + "..", # bare dotdot + ".hidden", # leading dot + "foo.bar", # dot inside (.yaml extensions, etc.) + "foo bar", # whitespace + " foo", # leading space + "foo ", # trailing space + "foo\tbar", # tab + "foo\nbar", # newline + "foo;rm", # shell metachar + "foo|bar", + "foo&bar", + "foo*bar", + "foo?bar", + "foo:bar", + "foo'bar", + 'foo"bar', + "foo$bar", + "foo`bar", + "café", # non-ASCII letter + ], +) +def test_validate_project_name_rejects_unsafe_names(name: str) -> None: + msg = _validate_project_name(name) + assert msg is not None + assert "invalid project name" in msg + + +@pytest.mark.asyncio +async def test_read_compose_rejects_traversal_name() -> None: + """Traversal name must be rejected before any DSM call.""" + from mcp_synology_container.modules.compose import register_compose + + client = AsyncMock() + mcp, tools = make_mock_mcp() + register_compose(mcp, make_config(), client) + + result = await tools["read_compose"]("../../etc") + assert "invalid project name" in result + client.request.assert_not_called() + client.download_text.assert_not_called() + + +@pytest.mark.asyncio +async def test_update_compose_rejects_traversal_name() -> None: + """update_compose with a traversal name must not validate YAML or upload.""" + from mcp_synology_container.modules.compose import register_compose + + client = AsyncMock() + mcp, tools = make_mock_mcp() + register_compose(mcp, make_config(), client) + + result = await tools["update_compose"]( + "foo/../bar", "services:\n web:\n image: nginx\n", confirmed=True + ) + assert "invalid project name" in result + client.upload_text.assert_not_called() + + +@pytest.mark.asyncio +async def test_update_image_tag_rejects_traversal_name() -> None: + from mcp_synology_container.modules.compose import register_compose + + client = AsyncMock() + mcp, tools = make_mock_mcp() + register_compose(mcp, make_config(), client) + + result = await tools["update_image_tag"]("foo/bar", "web", "1.25", confirmed=True) + assert "invalid project name" in result + client.upload_text.assert_not_called() + client.download_text.assert_not_called() + + +@pytest.mark.asyncio +async def test_update_env_var_rejects_traversal_name() -> None: + from mcp_synology_container.modules.compose import register_compose + + client = AsyncMock() + mcp, tools = make_mock_mcp() + register_compose(mcp, make_config(), client) + + result = await tools["update_env_var"]("..", "web", "FOO", "bar", confirmed=True) + assert "invalid project name" in result + client.upload_text.assert_not_called() + client.download_text.assert_not_called() diff --git a/uv.lock b/uv.lock index 0287a4d..ad244f4 100644 --- a/uv.lock +++ b/uv.lock @@ -362,7 +362,7 @@ wheels = [ [[package]] name = "mcp-synology-container" -version = "0.2.8" +version = "0.2.9" source = { editable = "." } dependencies = [ { name = "click" },