fix(honcho): refuse to rewrite a honcho.json that exists but does not parse
a truncated or hand-edited honcho.json reads as {} on the tolerant read
path. the next write then replaced the file with only the current host
block: an oauth refresh, a login, or any 'hermes honcho' command that
saves a setting wiped every other host and the root keys.
_read_config_strict now raises on a parse error the same way it raises
on a read error, and logs one sentence naming the file. _rotate_and_persist
treats that as a refresh failure and enters the cooldown without spending
the refresh token. install_grant raises into the setup flow. in cli.py
every write goes through _write_config, which runs the same check first
and raises ConfigWriteRefused; the honcho router, the setup wizard and the
profile sync print the sentence instead of a traceback. the wizard checks
before asking its questions. read paths keep the {} fallback.
follows #95860, which added the strict reader for unreadable files and
kept a .corrupt copy on a parse error. leaving the original file in place
keeps the same bytes without a second copy of the tokens on disk.
This commit is contained in:
@@ -70,8 +70,24 @@ def _read_config() -> dict:
|
||||
return read_json_or_empty(_config_path())
|
||||
|
||||
|
||||
class ConfigWriteRefused(Exception):
|
||||
"""honcho.json exists on disk but does not parse, so no command may overwrite it."""
|
||||
|
||||
|
||||
def _refuse_unparseable(path: Path) -> None:
|
||||
"""Raise ``ConfigWriteRefused`` when ``path`` exists but cannot be read or parsed. ``_read_config``
|
||||
returns ``{}`` for such a file, and writing that back would drop every other host and root key."""
|
||||
from plugins.memory.honcho.oauth import _read_config_strict
|
||||
try:
|
||||
_read_config_strict(path)
|
||||
except (OSError, ValueError) as e:
|
||||
raise ConfigWriteRefused(f"{path} exists but could not be read as JSON ({e}). Nothing was written. "
|
||||
"Fix or move the file, then re-run.") from e
|
||||
|
||||
|
||||
def _write_config(cfg: dict, path: Path | None = None) -> None:
|
||||
path = path or _local_config_path()
|
||||
_refuse_unparseable(path)
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
from utils import atomic_json_write
|
||||
atomic_json_write(path, cfg, mode=0o600)
|
||||
@@ -240,7 +256,11 @@ def _sync_profiles(verbose: bool) -> int:
|
||||
|
||||
created = skipped = 0
|
||||
for p in (p for p in profiles if p.name != "default"):
|
||||
if clone_honcho_for_profile(p.name):
|
||||
try:
|
||||
cloned = clone_honcho_for_profile(p.name)
|
||||
except ConfigWriteRefused as e:
|
||||
return say(f" {e}\n") or created
|
||||
if cloned:
|
||||
say(f" + {p.name} -> {profile_host_key(p.name)}")
|
||||
created += 1
|
||||
else:
|
||||
@@ -702,8 +722,16 @@ def _setup_tuning(cfg: dict, hermes_host: dict) -> None:
|
||||
|
||||
def cmd_setup(args) -> None:
|
||||
"""Interactive Honcho setup wizard."""
|
||||
try:
|
||||
_setup_wizard(args)
|
||||
except ConfigWriteRefused as e:
|
||||
print(f" {e}\n")
|
||||
|
||||
|
||||
def _setup_wizard(args) -> None:
|
||||
cfg = _read_config()
|
||||
write_path, read_path = _local_config_path(), _config_path()
|
||||
_refuse_unparseable(write_path) # before the questions, not after them
|
||||
print(f"\nHoncho memory setup\n{RULE}\n Honcho gives Hermes persistent cross-session memory.\n Config: {write_path}")
|
||||
if read_path != write_path and read_path.exists():
|
||||
print(f" (seeding from existing config at {read_path})")
|
||||
@@ -1341,7 +1369,10 @@ def honcho_command(args) -> None:
|
||||
if handler is None:
|
||||
return print(f" Unknown honcho command: {sub}\n"
|
||||
" Available: status, sessions, map, peer, mode, strategy, tokens, identity, migrate, enable, disable, sync\n")
|
||||
handler(args)
|
||||
try:
|
||||
handler(args)
|
||||
except ConfigWriteRefused as e:
|
||||
print(f" {e}\n")
|
||||
|
||||
|
||||
def register_cli(subparser) -> None:
|
||||
|
||||
@@ -108,34 +108,16 @@ def _mark_grant_dead(key: tuple[str, str], cred: OAuthCredential) -> None:
|
||||
_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.
|
||||
"""
|
||||
"""Reader for the WRITE paths. A missing file reads as ``{}``. A file that exists but cannot be read
|
||||
or parsed raises so the caller leaves it alone: ``atomic_json_write`` replaces the whole file, and a
|
||||
write seeded from ``{}`` would erase every other host's credentials and the root keys."""
|
||||
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)
|
||||
except (OSError, ValueError) as exc:
|
||||
logger.warning("Honcho config at %s exists but could not be read (%s); refusing to overwrite it.", path, exc)
|
||||
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``)."""
|
||||
@@ -285,7 +267,7 @@ def _rotate_and_persist(
|
||||
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:
|
||||
except (OSError, ValueError):
|
||||
_refresh_failure_at[key] = time.monotonic()
|
||||
return None
|
||||
try:
|
||||
|
||||
@@ -3,6 +3,8 @@
|
||||
from types import SimpleNamespace
|
||||
import json
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
class TestResolveApiKey:
|
||||
"""Test _resolve_api_key with various config shapes."""
|
||||
@@ -744,3 +746,50 @@ class TestCmdSetupDeviceFlow:
|
||||
assert len(calls) == 1
|
||||
assert "apiKey" not in cfg.get("hosts", {}).get("hermes", {})
|
||||
|
||||
|
||||
|
||||
class TestWriteRefusesUnparseableStore:
|
||||
"""A honcho.json that exists but does not parse reads as {} on the tolerant path. Writing that
|
||||
back would replace every host and root key with the current command's block, so writes refuse."""
|
||||
|
||||
def _point_at(self, monkeypatch, cfg_path):
|
||||
import plugins.memory.honcho.cli as honcho_cli
|
||||
monkeypatch.setattr(honcho_cli, "_config_path", lambda: cfg_path)
|
||||
monkeypatch.setattr(honcho_cli, "_local_config_path", lambda: cfg_path)
|
||||
monkeypatch.setattr(honcho_cli, "_host_key", lambda: "hermes_coder")
|
||||
monkeypatch.setattr(honcho_cli, "_ensure_peer_exists", lambda host_key=None: False)
|
||||
return honcho_cli
|
||||
|
||||
def test_write_config_refuses_and_leaves_file_alone(self, monkeypatch, tmp_path):
|
||||
cfg_path = tmp_path / "honcho.json"
|
||||
cfg_path.write_text('{"hosts": {"hermes": {"apiKey": "k"', encoding="utf-8")
|
||||
honcho_cli = self._point_at(monkeypatch, cfg_path)
|
||||
with pytest.raises(honcho_cli.ConfigWriteRefused):
|
||||
honcho_cli._write_config({"hosts": {"hermes_coder": {"enabled": True}}})
|
||||
assert cfg_path.read_text(encoding="utf-8") == '{"hosts": {"hermes": {"apiKey": "k"'
|
||||
|
||||
def test_write_config_still_bootstraps_a_missing_file(self, monkeypatch, tmp_path):
|
||||
cfg_path = tmp_path / "honcho.json"
|
||||
honcho_cli = self._point_at(monkeypatch, cfg_path)
|
||||
honcho_cli._write_config({"apiKey": "k"})
|
||||
assert json.loads(cfg_path.read_text(encoding="utf-8")) == {"apiKey": "k"}
|
||||
|
||||
def test_enable_prints_one_sentence_and_writes_nothing(self, monkeypatch, tmp_path, capsys):
|
||||
cfg_path = tmp_path / "honcho.json"
|
||||
cfg_path.write_text("{not json", encoding="utf-8")
|
||||
honcho_cli = self._point_at(monkeypatch, cfg_path)
|
||||
monkeypatch.setenv("HONCHO_API_KEY", "env-key")
|
||||
honcho_cli.honcho_command(SimpleNamespace(honcho_command="enable", target_profile=None))
|
||||
out = capsys.readouterr().out
|
||||
assert "could not be read as JSON" in out
|
||||
assert "Nothing was written" in out
|
||||
assert cfg_path.read_text(encoding="utf-8") == "{not json"
|
||||
|
||||
def test_setup_refuses_before_asking_anything(self, monkeypatch, tmp_path, capsys):
|
||||
cfg_path = tmp_path / "honcho.json"
|
||||
cfg_path.write_text("{not json", encoding="utf-8")
|
||||
honcho_cli = self._point_at(monkeypatch, cfg_path)
|
||||
monkeypatch.setattr(honcho_cli, "_prompt", lambda *a, **k: pytest.fail("wizard asked a question"))
|
||||
honcho_cli.cmd_setup(SimpleNamespace())
|
||||
assert "Nothing was written" in capsys.readouterr().out
|
||||
assert cfg_path.read_text(encoding="utf-8") == "{not json"
|
||||
|
||||
@@ -272,17 +272,42 @@ class TestPersistReadFailure:
|
||||
oauth._refresh_failure_at.pop(key, None)
|
||||
assert path.read_bytes() == before
|
||||
|
||||
def test_corrupt_store_degrades_but_preserves_a_copy(self, tmp_path):
|
||||
def test_corrupt_store_refuses_to_persist(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")
|
||||
cred = OAuthCredential.from_host_block(_host_block(refresh="hch-rt-new"))
|
||||
|
||||
assert oauth._read_config_strict(path) == {}
|
||||
corrupt = path.with_name(path.name + ".corrupt")
|
||||
assert corrupt.exists()
|
||||
assert corrupt.read_text(encoding="utf-8") == truncated
|
||||
with pytest.raises(ValueError):
|
||||
oauth._read_config_strict(path)
|
||||
with pytest.raises(ValueError):
|
||||
oauth._persist_credential(path, "keep.example", cred)
|
||||
with pytest.raises(ValueError):
|
||||
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_text(encoding="utf-8") == truncated
|
||||
assert not path.with_name(path.name + ".corrupt").exists()
|
||||
|
||||
def test_corrupt_store_fails_the_refresh_before_the_exchange(self, tmp_path, monkeypatch):
|
||||
path = tmp_path / "honcho.json"
|
||||
path.write_text("{not json", encoding="utf-8")
|
||||
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 a corrupt store")
|
||||
|
||||
monkeypatch.setattr(oauth, "_exchange_with_retry", no_exchange)
|
||||
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_text(encoding="utf-8") == "{not json"
|
||||
|
||||
def test_missing_store_still_bootstraps_empty(self, tmp_path):
|
||||
assert oauth._read_config_strict(tmp_path / "honcho.json") == {}
|
||||
@@ -295,7 +320,6 @@ class TestPersistReadFailure:
|
||||
)
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user