diff --git a/.github/skills/experimental/mural/scripts/mural/_backends.py b/.github/skills/experimental/mural/scripts/mural/_backends.py index 7a0e0a333..787163eba 100644 --- a/.github/skills/experimental/mural/scripts/mural/_backends.py +++ b/.github/skills/experimental/mural/scripts/mural/_backends.py @@ -34,7 +34,11 @@ _state, ) from ._constants import _LINE_RE, ENV_NONINTERACTIVE -from ._credentials import _resolve_credential_file +from ._credentials import ( + _backend_has_credentials, + _resolve_credential_file, + _service_name_for, +) from ._exceptions import MuralError from ._protocols import CredentialBackend @@ -212,7 +216,9 @@ def resolve_backend(profile: str = "default") -> CredentialBackend: ``MURAL_CREDENTIAL_BACKEND`` selects the backend (``auto`` default, ``keyring``, ``file``, ``env-only``). On ``auto``, KeyringBackend is tried first and falls back to FileBackend when ``_KeyringUnavailable`` - is raised; a one-shot WARN per profile records the fallback. After + is raised. Auto mode also falls back when keyring is available but has + no usable credentials and the file backend is populated. A one-shot + WARN per profile records whichever fallback path is taken. After backend selection (skipped for env-only), a probe checks whether the other persistent backend also holds non-empty values and emits a second one-shot WARN per ``(profile, selected_backend)`` pair when so. @@ -230,6 +236,26 @@ def resolve_backend(profile: str = "default") -> CredentialBackend: elif selector == "auto": try: selected = KeyringBackend() + service = _service_name_for(profile) + keyring_populated = _backend_has_credentials(selected, service) + if not keyring_populated: + file_backend = FileBackend(file_path) + file_populated = _backend_has_credentials( + file_backend, + service, + enforce_file_perms=True, + file_env=os.environ, + ) + if file_populated: + if profile not in _state.seen_fallback_warn(): + _state.seen_fallback_warn().add(profile) + msg = ( + f"keyring backend available but empty for " + f"profile {profile!r}; " + f"using file backend at {file_path}" + ) + _emit(msg, level=logging.WARNING) + selected = file_backend except _KeyringUnavailable as exc: if profile not in _state.seen_fallback_warn(): _state.seen_fallback_warn().add(profile) @@ -239,6 +265,12 @@ def resolve_backend(profile: str = "default") -> CredentialBackend: level=logging.WARNING, ) selected = FileBackend(file_path) + _backend_has_credentials( + selected, + _service_name_for(profile), + enforce_file_perms=True, + file_env=os.environ, + ) else: raise MuralError( f"MURAL_CREDENTIAL_BACKEND={selector!r} is not one of " diff --git a/.github/skills/experimental/mural/scripts/mural/_credentials.py b/.github/skills/experimental/mural/scripts/mural/_credentials.py index fe088c721..0b3b202ff 100644 --- a/.github/skills/experimental/mural/scripts/mural/_credentials.py +++ b/.github/skills/experimental/mural/scripts/mural/_credentials.py @@ -239,6 +239,35 @@ def _probe_keyring_availability() -> tuple[bool, str | None, str | None]: return _keyring_probe_cache +def _backend_has_credentials( + backend: CredentialBackend, + service: str, + *, + enforce_file_perms: bool = False, + file_env: Mapping[str, str] | None = None, +) -> bool: + """Return True when ``backend`` holds any non-empty credential value. + + FileBackend probes can optionally enforce the credential file permission + contract before reading so callers may choose between strict resolve-time + validation and best-effort warning-only inspection. + """ + if isinstance(backend, _pkg().FileBackend): + if not backend._path.exists(): + return False + if enforce_file_perms: + _pkg()._check_credential_file_perms( + backend._path, + file_env if file_env is not None else os.environ, + ) + entries = backend._read_all() + return any(entries.get(key) for key in _KNOWN_CREDENTIAL_KEYS) + for key in _KNOWN_CREDENTIAL_KEYS: + if backend.get(service, key): + return True + return False + + def _maybe_warn_concurrent_state( profile: str, selected: CredentialBackend, @@ -253,24 +282,19 @@ def _maybe_warn_concurrent_state( dedup_key = (profile, selected.name) if dedup_key in _pkg()._state.seen_concurrent_warn(): return - keyring_populated = False - file_populated = False service = _service_name_for(profile) try: probe_keyring = _pkg().KeyringBackend() - for key in _KNOWN_CREDENTIAL_KEYS: - value = probe_keyring.get(service, key) - if value: - keyring_populated = True - break + keyring_populated = _backend_has_credentials(probe_keyring, service) except _KeyringUnavailable: keyring_populated = False except Exception: # noqa: BLE001 - probe must never raise keyring_populated = False try: - if file_path.exists(): - entries = _pkg().FileBackend(file_path)._read_all() - file_populated = any(entries.get(k) for k in _KNOWN_CREDENTIAL_KEYS) + file_populated = _backend_has_credentials( + _pkg().FileBackend(file_path), + service, + ) except Exception: # noqa: BLE001 - probe must never raise file_populated = False if keyring_populated and file_populated: diff --git a/.github/skills/experimental/mural/tests/test_credential_storage.py b/.github/skills/experimental/mural/tests/test_credential_storage.py index 8ed81aa7f..021bc88dc 100644 --- a/.github/skills/experimental/mural/tests/test_credential_storage.py +++ b/.github/skills/experimental/mural/tests/test_credential_storage.py @@ -379,6 +379,37 @@ def test_auto_returns_keyring_when_available( backend = mural_module.resolve_backend("default") assert isinstance(backend, mural_module.KeyringBackend) + def test_auto_falls_back_to_file_when_keyring_is_empty( + self, + mural_module: Any, + monkeypatch: pytest.MonkeyPatch, + tmp_path: pathlib.Path, + caplog: pytest.LogCaptureFixture, + ) -> None: + _isolate_credential_env(monkeypatch, tmp_path) + monkeypatch.setenv( + "MURAL_KEYRING_BACKEND", "keyrings.alt.file.PlaintextKeyring" + ) + + service = mural_module._service_name_for("default") + file_path = mural_module._resolve_credential_file("default", os.environ) + file_backend = mural_module.FileBackend(file_path) + file_backend.set(service, "MURAL_CLIENT_ID", "from-file") + + caplog.set_level(logging.WARNING, logger="mural") + for _ in range(3): + backend = mural_module.resolve_backend("default") + assert isinstance(backend, mural_module.FileBackend) + + warns = [ + r + for r in caplog.records + if r.levelno == logging.WARNING + and "keyring backend available but empty for profile 'default'" in r.message + and "using file backend" in r.message + ] + assert len(warns) == 1 + def test_auto_falls_back_to_file_on_keyring_unavailable( self, mural_module: Any, @@ -404,6 +435,43 @@ def _raise_unavailable(self: Any) -> None: ] assert len(warns) == 1 + @pytest.mark.skipif( + os.name == "nt", reason="POSIX-only credential file permission semantics" + ) + def test_auto_keyring_unavailable_enforces_file_permissions( + self, + mural_module: Any, + monkeypatch: pytest.MonkeyPatch, + tmp_path: pathlib.Path, + ) -> None: + _isolate_credential_env(monkeypatch, tmp_path) + + def _raise_unavailable(self: Any) -> None: + raise mural_module._KeyringUnavailable("test-induced") + + monkeypatch.setattr(mural_module.KeyringBackend, "__init__", _raise_unavailable) + + checked_paths: list[pathlib.Path] = [] + + def _raise_bad_perms(path: pathlib.Path, environ: dict[str, str]) -> None: + checked_paths.append(path) + raise mural_module.MuralError("simulated bad permissions") + + monkeypatch.setattr( + mural_module, + "_check_credential_file_perms", + _raise_bad_perms, + ) + + file_path = mural_module._resolve_credential_file("default", os.environ) + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text("MURAL_CLIENT_ID=from-file\n", encoding="utf-8") + + with pytest.raises(mural_module.MuralError, match="simulated bad permissions"): + mural_module.resolve_backend("default") + + assert checked_paths == [file_path] + def test_auto_fallback_warn_dedupes_per_profile_per_process( self, mural_module: Any,