diff --git a/plugins/memory/honcho/oauth.py b/plugins/memory/honcho/oauth.py index 7c612bc8e2..94e87429c9 100644 --- a/plugins/memory/honcho/oauth.py +++ b/plugins/memory/honcho/oauth.py @@ -107,6 +107,36 @@ def _mark_grant_dead(key: tuple[str, str], cred: OAuthCredential) -> None: _dead_grants[key] = hashlib.sha256(cred.refresh_token.encode("utf-8")).hexdigest() _reauth_check_cache.pop(key, None) # verdict changed without a config rewrite +def _read_config_strict(path: Path) -> dict[str, Any]: + """Reader for the WRITE paths: never lets a failed read become an empty store. + + A missing file is the bootstrap case and reads as ``{}``. A file that EXISTS but cannot be read + (EACCES after a root-owned write, EIO, a stalled mount) raises instead: ``atomic_json_write`` replaces + the whole file and ``os.replace`` needs only a writable parent, so degrading here would let the next + persist erase every other host's credentials. Genuine corruption still degrades, but only after + preserving a ``.corrupt`` copy: a truncated store usually holds the other hosts' tokens verbatim. + """ + try: + return json.loads(path.read_text(encoding="utf-8-sig")) + except FileNotFoundError: + return {} + except OSError: + logger.warning("Honcho config at %s could not be read; refusing to treat it as empty " + "(a persist would erase every other host's credentials)", path, exc_info=True) + raise + except json.JSONDecodeError as exc: + corrupt = path.with_name(path.name + ".corrupt") + preserved = False + try: + import shutil + shutil.copy2(path, corrupt) + preserved = True + except Exception: + logger.debug("could not preserve a copy at %s", corrupt, exc_info=True) + logger.warning("Honcho config at %s is corrupt (%s); starting from an empty store. %s", path, exc, + f"Original preserved at {corrupt}" if preserved else f"A copy could NOT be preserved at {corrupt}") + return {} + def _load_cred(path: Path, host: str, raw: dict[str, Any] | None = None) -> OAuthCredential | None: """Credential from ``host``'s block in ``raw`` (or the file at ``path``).""" source = raw if raw is not None else read_json_or_empty(path) @@ -252,6 +282,12 @@ def _rotate_and_persist( ) -> OAuthCredential | None: """Exchange ``cred`` and persist the rotation; ``None`` on failure (logged). A permanent OAuth error marks the grant dead so later calls skip the endpoint until a new login rotates the refresh token.""" + try: + # Read before spending the single-use refresh token: an exchange that cannot be persisted loses the grant. + raw = _read_config_strict(path) + except OSError: + _refresh_failure_at[key] = time.monotonic() + return None try: rotated = _exchange_with_retry(cred, now=now) except Exception as exc: @@ -272,7 +308,7 @@ def _rotate_and_persist( _refresh_failure_at[key] = time.monotonic() logger.warning("Honcho OAuth %s failed for host %s: %s", op_label, host, redact_sensitive_text(str(exc), force=True)) return None - _persist_credential(path, host, rotated) + _persist_credential(path, host, rotated, raw=raw) return rotated def _deep_merge(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any]: @@ -284,11 +320,12 @@ def _deep_merge(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any] return base def _persist_credential(path: Path, host: str, cred: OAuthCredential, raw: dict[str, Any] | None = None) -> None: - """Write ``cred`` into ``host``'s block (apiKey + oauth) of ``raw`` (default: - the file's current content), leaving the rest intact; marks the grant live.""" + """Write ``cred`` into ``host``'s block (apiKey + oauth) of ``raw`` (default: a strict read of the + file), leaving the rest intact; marks the grant live. "Leaving the rest intact" is only true when the + existing store was actually read, so an unreadable file raises instead of becoming a single-host file.""" from utils import atomic_json_write - raw = read_json_or_empty(path) if raw is None else raw + raw = _read_config_strict(path) if raw is None else raw block = raw.setdefault("hosts", {}).setdefault(host, {}) block["apiKey"], block["oauth"] = cred.access_token, cred.oauth_block() atomic_json_write(path, raw, mode=0o600) @@ -374,7 +411,8 @@ def install_grant( ``apiKey`` and ``oauth`` block. ``apply_config=False`` stores tokens only.""" now = time.time() if now is None else now cred = OAuthCredential.from_token_response(grant, now=now, client_id=client_id, token_endpoint=token_endpoint) - raw = read_json_or_empty(path) + # Strict: a fresh login must not seed its root-merge from a store that exists but could not be read. + raw = _read_config_strict(path) granted_config = grant.get("config") if isinstance(granted_config, dict): cred.consent_peer_name = granted_config.get("peerName") diff --git a/tests/honcho_plugin/test_oauth.py b/tests/honcho_plugin/test_oauth.py index bdaebe3ab0..a0cbb55bf5 100644 --- a/tests/honcho_plugin/test_oauth.py +++ b/tests/honcho_plugin/test_oauth.py @@ -199,3 +199,119 @@ class TestApplyTokenToClient: def test_returns_false_when_shape_unknown(self): assert oauth.apply_token_to_client(object(), "hch-at-new") is False + + +class TestPersistReadFailure: + """One unreadable honcho.json must never become a single-host store. + + ``_persist_credential`` promises "leaving all else intact", but it seeded + the write from a reader that returned ``{}`` on ANY read failure, and + ``_atomic_write_config`` replaces the whole file via ``os.replace`` — + which needs only a writable parent, so a present-but-unreadable store did + not stop the overwrite. Same class as the auth-store fix in #75206. + """ + + @staticmethod + def _seed_two_hosts(path: Path) -> bytes: + _write(path, { + "hosts": { + "keep.example": _host_block(refresh="hch-rt-keep"), + "rotate.example": _host_block(refresh="hch-rt-rot"), + }, + "defaults": {"workspace": "w1"}, + }) + return path.read_bytes() + + def _unreadable(self, monkeypatch, target: Path): + real = Path.read_text + + def boom(self, *a, **kw): + if self.name == target.name: + raise PermissionError(13, "Permission denied") + return real(self, *a, **kw) + + monkeypatch.setattr(Path, "read_text", boom) + + def test_persist_raises_and_preserves_store_when_unreadable( + self, tmp_path, monkeypatch + ): + path = tmp_path / "honcho.json" + before = self._seed_two_hosts(path) + cred = OAuthCredential.from_host_block(_host_block(refresh="hch-rt-new")) + + with monkeypatch.context() as m: + self._unreadable(m, path) + with pytest.raises(OSError): + oauth._persist_credential(path, "rotate.example", cred) + + assert path.read_bytes() == before + + def test_rotate_refuses_before_spending_the_refresh_token( + self, tmp_path, monkeypatch + ): + # Rotation is single-use: an exchange whose result cannot be persisted + # loses the grant, so an unreadable store must fail the refresh BEFORE + # the exchange, not after. + path = tmp_path / "honcho.json" + before = self._seed_two_hosts(path) + cred = OAuthCredential.from_host_block(_host_block(refresh="hch-rt-rot")) + key = (str(path), "rotate.example") + oauth._refresh_failure_at.pop(key, None) + + def no_exchange(*a, **kw): # pragma: no cover - must not run + raise AssertionError("exchange ran against an unreadable store") + + with monkeypatch.context() as m: + m.setattr(oauth, "_exchange_with_retry", no_exchange) + self._unreadable(m, path) + assert oauth._rotate_and_persist( + path, "rotate.example", key, cred, now=1.0 + ) is None + + assert key in oauth._refresh_failure_at + oauth._refresh_failure_at.pop(key, None) + assert path.read_bytes() == before + + def test_corrupt_store_degrades_but_preserves_a_copy(self, tmp_path): + path = tmp_path / "honcho.json" + truncated = json.dumps( + {"hosts": {"keep.example": _host_block(refresh="hch-rt-keep")}} + )[:-5] + path.write_text(truncated, encoding="utf-8") + + assert oauth._read_config_strict(path) == {} + corrupt = path.with_name(path.name + ".corrupt") + assert corrupt.exists() + assert corrupt.read_text(encoding="utf-8") == truncated + + def test_missing_store_still_bootstraps_empty(self, tmp_path): + assert oauth._read_config_strict(tmp_path / "honcho.json") == {} + + def test_bom_prefixed_store_is_not_corruption(self, tmp_path): + path = tmp_path / "honcho.json" + path.write_text( + json.dumps({"hosts": {"keep.example": _host_block()}}), + encoding="utf-8-sig", + ) + assert "keep.example" in oauth._read_config(path).get("hosts", {}) + assert "keep.example" in oauth._read_config_strict(path).get("hosts", {}) + assert not path.with_name(path.name + ".corrupt").exists() + + def test_install_grant_raises_and_preserves_store_when_unreadable( + self, tmp_path, monkeypatch + ): + path = tmp_path / "honcho.json" + before = self._seed_two_hosts(path) + + with monkeypatch.context() as m: + self._unreadable(m, path) + with pytest.raises(OSError): + oauth.install_grant( + path, "new.example", + {"access_token": "hch-at-new", "refresh_token": "hch-rt-new", + "expires_in": 3600}, + client_id="hermes-desktop", + token_endpoint="http://localhost:8000/oauth/token", + ) + + assert path.read_bytes() == before