fix(auth): treat empty / whitespace-only credentials as absent (closes #35)

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixed
 
+- **Auth ignores empty / whitespace-only credentials at every read site** (#66) — closes #35. `_resolve_credentials()` in `core/auth.py` previously trusted that env / config / keyring values, if present, contained real credentials. Empty strings already fell through (Python truthiness handled them), but **whitespace-only** values like `auth: {username: "   "}` mid-edit slipped through and reached `login()` as `('   ', '\t', None)`, surfacing as a generic DSM 400 that pointed neither at the config nor at the empty values. New `_present_or_none()` helper returns the value unchanged when it has any non-whitespace content, otherwise `None`. Applied at all nine read sites (3 env vars × 3 storage tiers — env / plaintext config / keyring). Meaningful padding is preserved (e.g. a real password `"  pwd  "` keeps its spaces); only purely empty/whitespace-only inputs are filtered. Six regression tests cover whitespace at each strategy level (env, config, keyring), the empty-string regression, the whitespace-doesn't-shadow-valid-keyring case, and the preserve-internal-padding guarantee.
 - **CLI catches `pydantic.ValidationError` and emits a clean `Error:` line** (#65) — closes #34. The four `load_config()` call sites (`cli/check.py`, `cli/main.py` for `serve`, `cli/setup.py` discovery, `cli/setup.py:_setup_with_config` for `-c`) previously caught only `(FileNotFoundError, ValueError, yaml.YAMLError)`. Pydantic's `ValidationError` IS a `ValueError` subclass, so it WAS caught — but `str(e)` rendered Pydantic's raw multi-line traceback-style block, defeating the clean-error pattern PR #26 established for malformed YAML. New helper `format_validation_error()` in `core/config.py` renders `exc.errors()` as a one-line header (`Configuration validation failed (N error(s)):`) plus per-error `<dotted.location>: <message> (got <input>)` lines. The catch order at each site is now `except ValidationError` BEFORE `except (FileNotFoundError, ValueError, yaml.YAMLError)` since pydantic's class subclasses ValueError. Four regression tests added (one per call site) reproducing the gap by triggering AppConfig's strict-top-level `extra="forbid"` with an unknown field, asserting `Error: Configuration validation failed` lands and no `Traceback` leaks.
 - **`search_files` MCP tool exposes `mtime_from` / `mtime_to`** (#64) — closes #33. The underlying `search_files()` handler in `modules/filestation/search.py` accepted both parameters and forwarded them to DSM, but the FastMCP tool registration in `modules/filestation/__init__.py:tool_search_files` omitted them entirely — the documented modification-date filter was inaccessible from MCP clients. The parameters now appear on the tool surface and the description names ISO-8601 / `YYYY-MM-DD` / Unix-epoch as accepted formats. While fixing the surface gap, also added a `parse_mtime()` helper in `modules/filestation/helpers.py` so the documented format actually round-trips: ISO-8601 datetimes (with or without offset), bare calendar dates, and numeric epoch strings now all convert to the integer epoch seconds DSM's `SYNO.FileStation.Search` API expects. Naive datetimes are treated as UTC for stable cross-host behavior. New tests: 8 `TestParseMtime` cases in `tests/modules/filestation/test_helpers.py` plus a `test_search_with_mtime_filter` round-trip test that asserts the converted epoch lands in the DSM `start` request via respx (508 tests pass total at 96.14% coverage).
 - **Auto-CHANGELOG workflow now inserts `### Changed` in Keep-a-Changelog order** (#63) — the workflow's inline-Python composer previously inserted a newly-created `### Changed` block at `unreleased_idx + 1`, which placed it ABOVE any existing `### Added` (or other earlier-sorting subsection). Per Keep a Changelog v1.1.0 the canonical order is Added → Changed → Deprecated → Removed → Fixed → Security; the bug surfaces when `## Unreleased` already contains `### Added` (or any non-Changed subsection) and the next Dependabot PR creates a fresh `### Changed`. Currently dormant on this repo because `## Unreleased` happens to have both subsections; would manifest after the next release ships and a feature PR adds `### Added` to the fresh Unreleased section before any Dependabot bump. Fix: walk forward from `## Unreleased` looking for the first `###` subsection that should sort AFTER `### Changed` (Deprecated / Removed / Fixed / Security) OR the next `## ` release heading; insert immediately before whichever comes first. Default insertion point is the end of the Unreleased section. Smoke-tested locally against five CHANGELOG arrangements (empty Unreleased, Added-only, existing Changed, Added+Fixed, Fixed-only) — all produce KaC-ordered output. Surfaced by [`cmeans/pypi-winnow-downloads#26`](https://github.com/cmeans/pypi-winnow-downloads/issues/26) during QA review of the cascade PR there.

src/mcp_synology/core/auth.py

@@ -27,6 +27,17 @@
 _ERROR_2FA_REQUIRED = 403
 
 
+def _present_or_none(value: str | None) -> str | None:
+    """Return value unchanged if it contains non-whitespace content; else None.
+
+    Used to filter empty / whitespace-only credentials at every read site
+    (env vars, plaintext config, OS keyring) so they fall through the
+    resolution chain instead of flowing into `login()` as bogus credentials
+    that surface as a generic DSM 400.
+    """
+    return value if (value and value.strip()) else None
+
+
 class AuthManager:
     """Manages DSM authentication and session lifecycle."""
 
@@ -62,25 +73,27 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]:
         device_id: str | None = None
 
         # 1. Environment variables (highest priority — explicit override)
-        username = os.environ.get("SYNOLOGY_USERNAME")
+        # _present_or_none normalizes empty / whitespace-only to None so they
+        # fall through to the next strategy instead of becoming bogus creds.
+        username = _present_or_none(os.environ.get("SYNOLOGY_USERNAME"))
         if username:
             logger.debug("Username from env var SYNOLOGY_USERNAME: %s", username)
-        password = os.environ.get("SYNOLOGY_PASSWORD")
+        password = _present_or_none(os.environ.get("SYNOLOGY_PASSWORD"))
         if password:
             logger.debug("Password from env var SYNOLOGY_PASSWORD")
-        device_id = os.environ.get("SYNOLOGY_DEVICE_ID")
+        device_id = _present_or_none(os.environ.get("SYNOLOGY_DEVICE_ID"))
         if device_id:
             logger.debug("Device ID from env var SYNOLOGY_DEVICE_ID")
 
         # 2. Config file (explicit, if present)
-        if not username and self._config.auth.username:
-            username = self._config.auth.username
+        if not username and (cfg_user := _present_or_none(self._config.auth.username)):
+            username = cfg_user
             logger.debug("Username from config file: %s", username)
-        if not password and self._config.auth.password:
-            password = self._config.auth.password
+        if not password and (cfg_pass := _present_or_none(self._config.auth.password)):
+            password = cfg_pass
             logger.debug("Password from config file (plaintext)")
-        if not device_id and self._config.auth.device_id:
-            device_id = self._config.auth.device_id
+        if not device_id and (cfg_dev := _present_or_none(self._config.auth.device_id)):
+            device_id = cfg_dev
             logger.debug("Device ID from config file")
 
         # 3. OS keyring (implicit default — set by 'mcp-synology setup')
@@ -102,9 +115,9 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]:
             try:
                 service = f"mcp-synology/{self._config.instance_id or 'default'}"
                 logger.debug("Trying keyring service: %s", service)
-                kr_user = kr.get_password(service, "username")
-                kr_pass = kr.get_password(service, "password")
-                kr_device = kr.get_password(service, "device_id")
+                kr_user = _present_or_none(kr.get_password(service, "username"))
+                kr_pass = _present_or_none(kr.get_password(service, "password"))
+                kr_device = _present_or_none(kr.get_password(service, "device_id"))
                 if kr_user and not username:
                     username = kr_user
                     logger.debug("Username from keyring: %s", username)

tests/core/test_auth.py

@@ -132,6 +132,117 @@ def test_no_credentials_raises(self) -> None:
         ):
             auth._resolve_credentials()
 
+    # --- Empty / whitespace-only credentials at each strategy level (closes #35) ---
+    #
+    # The bug was that whitespace-only credentials (e.g. `auth: {username: "   "}`
+    # mid-edit) flowed straight into login() as bogus values, surfacing as a
+    # generic DSM 400. Empty strings already fell through correctly; whitespace
+    # did not. The fix normalizes empty + whitespace at every read site
+    # (env / config / keyring) via _present_or_none.
+
+    def test_whitespace_config_credentials_fall_through(self) -> None:
+        """Whitespace-only plaintext config credentials fall through, raise clean error."""
+        config = _make_config(auth={"username": "   ", "password": "\t"})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch("mcp_synology.core.auth.kr", _no_keyring()),
+            pytest.raises(AuthenticationError, match="No credentials"),
+        ):
+            auth._resolve_credentials()
+
+    def test_whitespace_env_credentials_fall_through(self) -> None:
+        """Whitespace-only env credentials are ignored, fall through to next strategy."""
+        config = _make_config()
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        env = {
+            **_clean_env(),
+            "SYNOLOGY_USERNAME": "   ",
+            "SYNOLOGY_PASSWORD": "\t\n",
+        }
+        with (
+            patch.dict(os.environ, env, clear=True),
+            patch("mcp_synology.core.auth.kr", _no_keyring()),
+            pytest.raises(AuthenticationError, match="No credentials"),
+        ):
+            auth._resolve_credentials()
+
+    def test_whitespace_keyring_credentials_fall_through(self) -> None:
+        """Whitespace-only keyring values are ignored, raise clean error."""
+        config = _make_config()
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch("mcp_synology.core.auth.kr", _keyring_with("   ", "\t", "  ")),
+            pytest.raises(AuthenticationError, match="No credentials"),
+        ):
+            auth._resolve_credentials()
+
+    def test_empty_string_env_credentials_fall_through(self) -> None:
+        """Empty-string env credentials fall through (regression coverage)."""
+        config = _make_config(auth={"username": "", "password": ""})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        env = {
+            **_clean_env(),
+            "SYNOLOGY_USERNAME": "",
+            "SYNOLOGY_PASSWORD": "",
+        }
+        with (
+            patch.dict(os.environ, env, clear=True),
+            patch("mcp_synology.core.auth.kr", _no_keyring()),
+            pytest.raises(AuthenticationError, match="No credentials"),
+        ):
+            auth._resolve_credentials()
+
+    def test_whitespace_env_falls_through_to_valid_keyring(self) -> None:
+        """Whitespace-only env doesn't shadow a valid keyring entry."""
+        config = _make_config()
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        env = {
+            **_clean_env(),
+            "SYNOLOGY_USERNAME": "   ",
+        }
+        with (
+            patch.dict(os.environ, env, clear=True),
+            patch(
+                "mcp_synology.core.auth.kr",
+                _keyring_with("kr_user", "kr_pass", "kr_device"),
+            ),
+        ):
+            username, password, device_id = auth._resolve_credentials()
+
+        assert username == "kr_user"
+        assert password == "kr_pass"
+        assert device_id == "kr_device"
+
+    def test_valid_credentials_with_internal_padding_preserved(self) -> None:
+        """Credentials with leading/trailing spaces but real content are NOT stripped."""
+        # If a user's actual password has padding, _present_or_none keeps it.
+        config = _make_config(auth={"username": "  alice  ", "password": "  pwd123  "})
+        client = _make_client()
+        auth = AuthManager(config, client)
+
+        with (
+            patch.dict(os.environ, _clean_env(), clear=True),
+            patch("mcp_synology.core.auth.kr", _no_keyring()),
+        ):
+            username, password, _ = auth._resolve_credentials()
+
+        # Original padding is preserved — we filter out empty/whitespace-only,
+        # we don't strip meaningful values.
+        assert username == "  alice  "
+        assert password == "  pwd123  "
+
 
 class TestLogin:
     @respx.mock