From ec630b38e8a97cc69ba4ad3d6a8101e136b1e50a Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 09:35:16 -0500 Subject: [PATCH] fix(auth): narrow keyring exception handler + log root cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #38. core/auth.py:147-148 was a bare `except Exception:` with a flat `logger.debug("Keyring not available.")` that hid every keyring failure mode behind one line: locked macOS keychain, NoKeyringError on a headless host, InitError, OS-level reach errors, and library bugs. Operators running `mcp-synology check -v` saw the same useless "Keyring not available." regardless of why. Narrows to two typed handlers: - `except KeyringError as e` for the typed-error case (covers KeyringLocked, NoKeyringError, InitError, PasswordSetError, PasswordDeleteError, and any other keyring.errors.* class) - `except OSError as e` for D-Bus reach errors and OS-level permission failures on the keychain backend Both log at DEBUG with exc_info=True so verbose-mode surfaces the actual exception type, message, and traceback. Genuine bugs are no longer caught — they propagate. Bumped the pre-keyring D-Bus socket pre-check log from DEBUG to INFO and rewrote it with three concrete remediations: - run `mcp-synology setup` from a real desktop session - wrap with `dbus-run-session` - set credentials via SYNOLOGY_* env vars to bypass keyring The Linux + headless-service path is the most common keyring failure mode and previously needed -v to surface the actual cause. Updated tests/core/test_auth.py::_no_keyring() fixture to raise `keyring.errors.NoKeyringError` instead of bare Exception so existing tests exercise the production-shaped error path. New TestKeyringErrorHandling (3 cases): - KeyringError logged with exc_info and message text at DEBUG - OSError logged separately at DEBUG with exc_info - Defense-in-depth: keyring blow-up doesn't block credential resolution from config/env Strengthened TestDbusSocketMissing:: test_dbus_not_set_when_socket_missing_on_linux to assert the new INFO-level remediation hint contains the socket path AND all three remediation strings. 553 unit tests pass at 96.13% coverage; ruff/mypy clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + src/mcp_synology/core/auth.py | 34 ++++++- tests/core/test_auth.py | 162 ++++++++++++++++++++++++++++++++-- 3 files changed, 186 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70d9931..b5960d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - **publish.yml: bump pinned `mcp-publisher` v1.5.0 → v1.7.6 to match the new registry OIDC audience** (#79) — the v0.5.1 release ran with `mcp-publisher v1.5.0` (the pin in `.github/actions/install-mcp-publisher/action.yml`); PyPI publish succeeded but the `publish-registry` job failed at GitHub OIDC login with `invalid audience: expected https://registry.modelcontextprotocol.io, got [mcp-registry]` (HTTP 401). Root cause: the registry deployed [`modelcontextprotocol/registry#1229`](https://github.com/modelcontextprotocol/registry/pull/1229) ("auth: bind GitHub OIDC token exchange to a per-deployment audience") in `v1.7.6` on 2026-04-30 — one day before our 2026-05-01 release. v1.5.0's `login github-oidc` flow sends audience `mcp-registry`; v1.7.6's flow sends audience `https://registry.modelcontextprotocol.io`, which is what the new registry server validates against. Bumped the action's `default` from `v1.5.0` to `v1.7.6` (and added an explanatory comment so the next bump prompt has the rationale at hand). Re-running the failed `publish-registry` job on the existing v0.5.1 tag won't pick up this fix because `actions/checkout@v6` resolves to the tag's commit; the next release tag will exercise the fix end-to-end. v0.5.1 itself is on PyPI as expected and is the install path users actually hit; the missed registry entry is purely directory metadata. +- **Keyring exception handler narrows + logs root cause** (#80) — closes #38. `core/auth.py:147-148` previously caught a bare `except Exception:` with a flat `logger.debug("Keyring not available.")`, hiding every keyring failure mode behind one generic line: locked macOS keychain (operator-actionable: unlock the keychain), `keyring.errors.NoKeyringError` on a headless host (signals a config issue), `keyring.errors.InitError`, OS-level errors on the D-Bus reach path, and genuine library bugs. Operators running `mcp-synology check -v` saw "Keyring not available." with no clue whether the keychain was locked, the backend was missing, or the library blew up. Narrowed to two typed handlers: `except KeyringError as e` (the typed-error case — covers `KeyringLocked`, `NoKeyringError`, `InitError`, `PasswordSetError`, `PasswordDeleteError`, and any other `keyring.errors.*` class) and `except OSError as e` (D-Bus reach errors, permission failures on the OS keychain DB). Both log at DEBUG with `exc_info=True` so the actual exception type, message, and traceback land in the verbose-mode output. Genuine bugs are no longer caught — they propagate up so they can be triaged. The pre-keyring D-Bus socket pre-check (`auth.py:130-131`) was bumped from DEBUG to INFO and rewritten with three concrete remediations (`mcp-synology setup` from a real desktop session, `dbus-run-session` wrapper, or `SYNOLOGY_USERNAME` / `SYNOLOGY_PASSWORD` / `SYNOLOGY_DEVICE_ID` env vars to bypass keyring entirely) — this is the most common path on Linux services launched without a desktop session, and the previous DEBUG-only log meant operators running `mcp-synology check` (without `-v`) saw a generic "no credentials" error with no breadcrumb to the root cause. Updated `tests/core/test_auth.py::_no_keyring()` fixture to raise `keyring.errors.NoKeyringError` instead of bare `Exception` so existing tests exercise the production-shaped error path. New `TestKeyringErrorHandling` (3 cases): `KeyringError` logged with `exc_info` and message text at DEBUG, `OSError` logged separately, and a defense-in-depth case proving a keyring blow-up doesn't block credential resolution from config/env. Strengthened `TestDbusSocketMissing::test_dbus_not_set_when_socket_missing_on_linux` to assert the new INFO-level remediation hint contains the socket path AND all three remediation strings. 553 unit tests pass at 96.13% coverage. ## 0.5.1 (2026-05-01) diff --git a/src/mcp_synology/core/auth.py b/src/mcp_synology/core/auth.py index 04b3a6e..a74d8af 100644 --- a/src/mcp_synology/core/auth.py +++ b/src/mcp_synology/core/auth.py @@ -14,6 +14,7 @@ from typing import TYPE_CHECKING, Any import keyring as kr +from keyring.errors import KeyringError from mcp_synology.core.errors import AuthenticationError, SynologyError @@ -127,7 +128,17 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]: os.environ["DBUS_SESSION_BUS_ADDRESS"] = dbus_addr logger.debug("Set DBUS_SESSION_BUS_ADDRESS=%s for keyring access", dbus_addr) else: - logger.debug("D-Bus socket not found at %s; keyring may not work", socket_path) + # INFO not DEBUG — without D-Bus the keyring path is dead + # for this session; the operator needs to know how to + # recover. Closes #38 (the operator-actionable hint). + logger.info( + "D-Bus socket not found at %s; OS keyring is unavailable. " + "Run 'mcp-synology setup' from a real desktop session " + "(or under 'dbus-run-session'), or set credentials via the " + "SYNOLOGY_USERNAME / SYNOLOGY_PASSWORD / SYNOLOGY_DEVICE_ID " + "env vars to bypass the keyring entirely.", + socket_path, + ) try: service = f"mcp-synology/{self._config.instance_id or 'default'}" @@ -144,8 +155,25 @@ def _resolve_credentials(self) -> tuple[str, str, str | None]: if kr_device and not device_id: device_id = kr_device logger.debug("Device ID from keyring") - except Exception: - logger.debug("Keyring not available.") + except KeyringError as e: + # Typed keyring failure (NoKeyringError, InitError, + # KeyringLocked, PasswordSetError, etc.). Log at DEBUG with + # `exc_info` so `mcp-synology check -v` surfaces the actual + # cause (locked keychain on macOS is operator-actionable; + # NoKeyringError on a headless host signals a config issue). + # Closes #38 — previously caught by a bare `except Exception` + # with a flat "Keyring not available." line that hid all of + # these. Falls through to the no-credentials check below; + # the resolver chain still works without keyring. + logger.debug("Keyring access failed: %s", e, exc_info=True) + except OSError as e: + # D-Bus socket present but unreachable, permission errors on + # the OS keychain backend, etc. Same DEBUG-with-exc_info + # treatment as the typed-error case above. The pre-check at + # line 123-131 already emitted an INFO hint if the D-Bus + # socket was missing entirely; this branch covers the + # reach-but-fail variant. + logger.debug("Keyring OS-level error: %s", e, exc_info=True) if not username or not password: msg = ( diff --git a/tests/core/test_auth.py b/tests/core/test_auth.py index 9f30d3b..8ac9735 100644 --- a/tests/core/test_auth.py +++ b/tests/core/test_auth.py @@ -39,9 +39,19 @@ def _make_client() -> DsmClient: def _no_keyring() -> MagicMock: - """Return a mock keyring module where get_password raises.""" + """Return a mock keyring module where get_password raises a realistic + `keyring.errors.NoKeyringError`. + + Pre-#38 this used bare `Exception("No keyring backend")` and the + production code's bare `except Exception` swallowed it. After #38 the + handler narrowed to `KeyringError`/`OSError`, so the realistic typed + error is also what the tests should mock — keeps the production-shaped + error path exercised everywhere this fixture is used. + """ + from keyring.errors import NoKeyringError + mock = MagicMock() - mock.get_password.side_effect = Exception("No keyring backend") + mock.get_password.side_effect = NoKeyringError("No keyring backend") return mock @@ -609,8 +619,22 @@ def test_session_name_format(self) -> None: class TestDbusSocketMissing: - def test_dbus_not_set_when_socket_missing_on_linux(self) -> None: - """Linux + DBUS unset + socket missing → log debug, do not set env var.""" + def test_dbus_not_set_when_socket_missing_on_linux( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Linux + DBUS unset + socket missing → log INFO with remediation hint + and do not set env var. + + Closes #38 (the operator-actionable hint half). Pre-fix this branch + only logged at DEBUG ("D-Bus socket not found at %s; keyring may not + work"), so an operator running `mcp-synology check` (without -v) saw + a generic "no credentials" error with no clue that the missing D-Bus + socket was the root cause. INFO-level message names the socket path + AND points at three concrete remediations: run setup from a real + session, wrap with `dbus-run-session`, or use SYNOLOGY_* env vars. + """ + import logging + config = _make_config(auth={"username": "admin", "password": "secret"}) client = _make_client() auth = AuthManager(config, client) @@ -624,14 +648,136 @@ def test_dbus_not_set_when_socket_missing_on_linux(self) -> None: patch("sys.platform", "linux"), patch("pathlib.Path.exists", return_value=False), patch("os.getuid", return_value=1000), + caplog.at_level(logging.INFO, logger="mcp_synology.core.auth"), ): username, _, _ = auth._resolve_credentials() - # Falls back to config-file credentials; the missing-socket branch - # logs at debug and does NOT set the env var (assert inside the - # patch.dict block so the assertion sees the patched env, not the - # restored one) assert "DBUS_SESSION_BUS_ADDRESS" not in os.environ assert username == "admin" + # INFO record carries actionable remediation, not just a description. + info_messages = [r.getMessage() for r in caplog.records if r.levelname == "INFO"] + assert any( + "D-Bus socket not found" in msg + and "/run/user/1000/bus" in msg + and "mcp-synology setup" in msg + and "SYNOLOGY_USERNAME" in msg + for msg in info_messages + ), f"expected actionable INFO hint, got: {info_messages}" + + +class TestKeyringErrorHandling: + """Closes #38 — narrow keyring exception handler + log root cause. + + Pre-fix `core/auth.py:147-148` caught bare `except Exception` with a flat + `logger.debug("Keyring not available.")`, hiding the actual failure + (locked macOS keychain, NoKeyringError on a headless host, OSError on + D-Bus socket reach issues, library bugs). Now narrows to + `keyring.errors.KeyringError` + `OSError` and logs each at DEBUG with + `exc_info=True` so `mcp-synology check -v` surfaces the real cause. + """ + + def _make_keyring_that_raises(self, exc: Exception) -> MagicMock: + mock = MagicMock() + mock.get_password.side_effect = exc + return mock + + def test_keyring_error_logged_with_exc_info_at_debug( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Typed `KeyringError` (e.g. macOS locked keychain) is logged with + the exception message and traceback at DEBUG, NOT swallowed. + """ + import logging + + from keyring.errors import KeyringLocked + + config = _make_config(auth={"username": "admin", "password": "secret"}) + client = _make_client() + auth = AuthManager(config, client) + + with ( + patch.dict(os.environ, _clean_env(), clear=True), + patch( + "mcp_synology.core.auth.kr", + self._make_keyring_that_raises(KeyringLocked("Keychain is locked")), + ), + caplog.at_level(logging.DEBUG, logger="mcp_synology.core.auth"), + ): + auth._resolve_credentials() + + debug_records = [r for r in caplog.records if r.levelname == "DEBUG"] + # Failure log carries the exception text and exc_info traceback. + matching = [ + r + for r in debug_records + if "Keyring access failed" in r.getMessage() and "Keychain is locked" in r.getMessage() + ] + assert matching, ( + f"expected DEBUG log with KeyringError message, got: " + f"{[r.getMessage() for r in debug_records]}" + ) + assert matching[0].exc_info is not None, ( + "expected exc_info traceback on the keyring failure log" + ) + # The pre-fix flat "Keyring not available." string must NOT appear. + assert not any(r.getMessage() == "Keyring not available." for r in caplog.records), ( + "regression: legacy flat 'Keyring not available.' message reappeared" + ) + + def test_keyring_oserror_logged_with_exc_info_at_debug( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """OS-level keyring failure (D-Bus reach errors, permission denied + on macOS keychain DB, etc.) is logged with the exception message + and traceback at DEBUG, separate from the typed-error branch. + """ + import logging + + config = _make_config(auth={"username": "admin", "password": "secret"}) + client = _make_client() + auth = AuthManager(config, client) + + with ( + patch.dict(os.environ, _clean_env(), clear=True), + patch( + "mcp_synology.core.auth.kr", + self._make_keyring_that_raises(OSError("Connection refused: /run/user/1000/bus")), + ), + caplog.at_level(logging.DEBUG, logger="mcp_synology.core.auth"), + ): + auth._resolve_credentials() + + debug_records = [r for r in caplog.records if r.levelname == "DEBUG"] + matching = [ + r + for r in debug_records + if "Keyring OS-level error" in r.getMessage() and "Connection refused" in r.getMessage() + ] + assert matching, ( + f"expected DEBUG log with OSError message, got: " + f"{[r.getMessage() for r in debug_records]}" + ) + assert matching[0].exc_info is not None + + def test_keyring_failure_does_not_block_config_credentials(self) -> None: + """A keyring blow-up must not prevent the resolver from returning + credentials sourced from the config file (or env). Defense in depth. + """ + from keyring.errors import NoKeyringError + + config = _make_config(auth={"username": "admin", "password": "secret"}) + client = _make_client() + auth = AuthManager(config, client) + + with ( + patch.dict(os.environ, _clean_env(), clear=True), + patch( + "mcp_synology.core.auth.kr", + self._make_keyring_that_raises(NoKeyringError("No backend")), + ), + ): + username, password, _ = auth._resolve_credentials() + assert username == "admin" + assert password == "secret" class TestLoginErrorPaths: