fix(cli): stop profile rename widening Honcho credential mode and detaching a symlinked config - #81296
Conversation
…aching a symlinked config `_migrate_honcho_profile_host` is the last writer of the Honcho config files that does not go through the shared atomic helper. It rewrote them with a bare `tmp.write_text(...)` followed by `tmp.replace(path)`, which carries three defects into files that hold the Honcho `apiKey` — under a browser OAuth grant that key is the auto-refreshing access token, and one of the three candidate paths is the global `~/.honcho/config.json` shared with every other Honcho-enabled app: 1. The temp file is created at the process umask (typically 0644) and `Path.replace` carries that mode onto the target, so renaming a profile silently widened a 0600 credential file to group/world-readable. 2. `Path.replace` swaps the symlink itself, detaching a `honcho.json` that a managed deployment symlinks out to a dotfiles or profile package. 3. There is no fsync, so a crash after the rename can leave a truncated credential config. Routing the write through `utils.atomic_json_write(path, raw, mode=0o600)` closes all three: the helper fchmods the descriptor before the replace (no chmod-after-write TOCTOU), flushes and fsyncs, and replaces via `atomic_replace`, which resolves a symlinked target and swaps in place on the real file. The fail-soft `except OSError: continue` is preserved so the loop still advances to the next candidate path, and the helper's own cleanup removes the temp file that the old inline handler unlinked by hand. `mode=0o600` matches the convention already established for these exact files by every other writer: `plugins/memory/honcho/__init__.py` and `plugins/memory/honcho/cli.py` both call `atomic_json_write(..., mode=0o600)`, `plugins/memory/honcho/oauth.py` opens with 0600, and `cli.py` chmods 0600 after writing. This is not new policy, it is the one site that missed it.
There was a problem hiding this comment.
Pull request overview
This PR hardens hermes profile rename by fixing _migrate_honcho_profile_host to rewrite Honcho credential/config JSON via the shared atomic JSON writer, preventing permission widening, symlink detachment, and truncated writes for files that may contain Honcho OAuth access tokens (apiKey).
Changes:
- Switch Honcho host-block migration writes to
utils.atomic_json_write(..., mode=0o600)to preserve owner-only permissions and symlink semantics while also adding fsync durability. - Remove the bespoke temp-file write/replace logic from
_migrate_honcho_profile_host, keeping the existing fail-soft “try next candidate” behavior onOSError. - Add focused regression tests covering mode preservation, symlink preservation, and advancing to later candidates after an
OSError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hermes_cli/profiles.py |
Routes Honcho config rewrites through atomic_json_write(mode=0o600) to preserve secret-file permissions, symlinks, and durability during profile renames. |
tests/hermes_cli/test_profiles.py |
Adds regression tests ensuring the migration rewrite preserves 0600, preserves symlinks, and keeps fail-soft candidate advancement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alt-glitch
added
type/security
|
CI audit — the single test failure on this PR is a known intermittent on
This PR cannot reach that test. It changes exactly two files — Verification run against this branch, where The suites this PR actually affects are green: Happy to rebase once #75955 lands if that is easier than re-running the slice. |
|
suggesting changes The change correctly routes Honcho profile renames through the 0600, fsyncing, symlink-preserving JSON helper, and the PR-head regression coverage passes. One residual integrity gap remains: the helper's EXDEV/EBUSY fallback copies directly into the resolved credential file, so an interrupted or failed copy can truncate the existing OAuth-token configuration. Make credential writes use a same-filesystem rename or fail closed on a non-atomic fallback before merge.
Security evidence:
Signed: GPT-5.6-luna-max in Codex |
|
On the The truncation is in This PR's diff is It's also still a strict improvement over main for the file it does touch. Before this PR, I've filed #81384 to close the helper-level gap at the source. It stages the new content into the resolved target's own directory and renames from there, so the On CI: the single failure is |
|
suggesting changes The follow-up scope argument does not remove the blocker: this PR newly routes the symlinked credential path through PR #81384 addresses the helper-level fallback. Please either land and rebase onto that fix before this change, integrate the equivalent fail-safe behavior here, or make this credential migration fail closed without an in-place copy. A regression should force the fallback copy to fail after a partial write and assert that the real symlink target remains byte-for-byte unchanged. Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
…tory _migrate_honcho_profile_host already resolves each candidate to dedup the loop, then hands the unresolved candidate to atomic_json_write. The helper stages its temp with tempfile.mkstemp(dir=path.parent), so a symlinked config is staged on the link's filesystem and renamed onto the real file's. ~/.honcho/config.json is routinely linked into a dotfiles repo, an encrypted volume or a mounted secrets share. Where those are separate filesystems the rename fails EXDEV, and the fallback copies straight onto the resolved target with the destination opened "wb" -- emptying the file that holds the Honcho apiKey (the auto-refreshing OAuth access token under a browser grant) before a single replacement byte exists. A copy that then fails part-way (ENOSPC, I/O error) leaves it truncated, and this loop's `except OSError: continue` swallows the error, so the profile rename finishes without ever reporting that the credentials were destroyed. Passing the resolved target makes the staging directory the real file's own directory, so the rename is same-filesystem by construction and EXDEV is unreachable from this call site. The symlink survives because it is never written through, and the user-facing output still names the candidate path.
…ailed replace
Models the filesystem boundary the way the kernel does: os.replace raises
EXDEV exactly when the staged temp is not in the destination's own
directory, so the staging directory alone decides whether the fallback is
reachable. With the rewrite staged beside the symlink it fires, and an
ENOSPC injected mid-copy leaves the credential file holding 8 bytes.
Two invariants, deliberately separate:
- the outcome -- the apiKey survives, the host migration applied, the mode
is still 0600, the symlink still resolves to the real file, and no staged
temp leaks into either directory;
- the construction -- every rename this rewrite issues stays inside one
directory. That is what pins the fix itself, and it stays meaningful
regardless of how the cross-device fallback is implemented.
Red on the parent commit with the target truncated to '{\n "hos'.
|
Self-audit: this diff moved one exposure while fixing three. Fix pushed. Swapping the hand-rolled
The file on the receiving end holds the Honcho Relative to Fix — head
|
|
suggesting changes Please make the Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
The rewrite below is only safe when it is handed the resolved target. As its own comment states, ``atomic_json_write`` stages its temp file in the parent of whatever path it is given, so passing a symlink stages on the link's filesystem while ``atomic_replace`` renames onto the real file's. When those differ — a config linked into a dotfiles repo, an encrypted volume, a mounted secrets share — the rename fails EXDEV and the copy fallback opens the real file "wb", emptying the user's Honcho ``apiKey`` before a replacement byte exists. The candidate loop broke that invariant on its own error path: when ``Path.resolve`` raised — ELOOP on a symlink cycle, ENAMETOOLONG, EACCES on a parent directory — it fell back to the unresolved candidate and handed that straight to the writer. So the exact input the invariant forbids was reachable, and the ``except OSError: continue`` on the write then swallowed the resulting failure, leaving the credential file truncated with nothing reported to the user. Fail closed instead: a candidate whose target cannot be determined is skipped. This also repairs the dedup set, which degraded on the same path — an unresolved entry can never match the resolved entry for the same file, so a config reachable under two candidate paths could be rewritten twice.
…hrough Drives ``Path.resolve`` to ELOOP on the symlinked ``~/.honcho/config.json`` candidate — the case a symlink cycle, an over-long path or an EACCES parent produces — with the cross-device rename and the "wb"-then-ENOSPC copy fallback in place, i.e. the real deployment shape where the link points into a dotfiles repo or a mounted secrets share. Asserts the two things that matter: the pre-existing credential file is byte-identical afterwards, and the writer was never handed the symlink. Both fail without the skip — the copy fallback leaves the apiKey file holding eight bytes — and the pre-existing tests cannot detect it, because none of them make resolution itself fail.
|
The resolution-failure path now fails closed. Pushed. The write in The candidate loop broke that invariant on its own error path: try:
resolved = path.resolve()
except OSError:
resolved = path # ← the unresolved candidate, possibly the symlink
...
atomic_json_write(resolved, raw, mode=0o600)
Fix: skip the candidate instead of falling back to it. Two consequences worth naming:
Commits: Regression, which fails without the skip: It drives |
|
suggesting changes The profile rename still has one credential-integrity failure. The new code correctly resolves symlink targets and makes cross-device staging same-directory, so the mode, symlink, resolution-failure, duplicate, and malformed-candidate cases now behave as intended. However, the shared writer still handles a busy target by copying the staged JSON directly over the resolved Honcho file. A partial copy can truncate the file containing Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
Scope note: the residual
|
What does this PR do?
hermes profile renamecalls_migrate_honcho_profile_hostto move the profile's Honcho host block to its new key. That function is the last writer of the Honcho config files that does not go through the shared atomic helper — it rewrote them with a baretmp.write_text(...)followed bytmp.replace(path).That matters because of what those files hold. Its three candidate paths are:
Each host block carries
apiKey, whichplugins/memory/honcho/README.mddocuments as the auto-refreshing access token under a browser OAuth grant, and the third candidate is the global~/.honcho/config.jsonthatplugins/memory/honcho/client.pydocuments as "global, shared across all Honcho-enabled apps". So a profile rename rewrites a live credential file, including one shared outside Hermes.The old write carried three defects:
0644— andPath.replacecarries that mode onto the target. Ahoncho.jsonthe user (orplugins/memory/honcho/oauth.py) had left at0600came back group- and world-readable after a rename, with no output saying so.Path.replacereplaces the symlink with a regular file, so a deployment that symlinkshoncho.jsonout to a dotfiles repo or a profile package silently stopped tracking it — the same failure modeatomic_replacewas introduced to fix in [Bug]: atomic writes to HERMES_HOME files replace symlinked targets (config.yaml/SOUL.md) #16743.The fix is one substitution:
utils.atomic_json_writefchmods the descriptor before the replace (so there is no chmod-after-write TOCTOU window on a secret-bearing file), flushes andfsyncs, and replaces throughatomic_replace, whose comment reads "Preserve symlinks — swap in-place on the real file (GitHub #16743)". One call closes all three defects. The fail-softexcept OSError: continueis kept so the loop still advances to the remaining candidate paths, and the helper's own cleanup replaces the temp-fileunlinkthe old inline handler did by hand.mode=0o600is this file's own established convention, not new policy. Every other writer of these exact files already enforces it:plugins/memory/honcho/__init__.pyatomic_json_write(config_path, existing, mode=0o600)plugins/memory/honcho/cli.pyatomic_json_write(path, cfg, mode=0o600)plugins/memory/honcho/oauth.pyos.open(tmp, ..., 0o600)cli.pyos.chmod(config_path, 0o600)hermes_cli/backup.pyos.chmod(target, 0o600)when restoring external provider configshermes_cli/profiles.pyOne intentional byte-level difference: the helper does not append a trailing newline, where the old inline write did. This makes the rename path byte-consistent with the two Honcho plugin writers that already use
atomic_json_write, and nothing reads these files other thanjson.loads.Coverage
I swept every writer of the Honcho config files rather than fixing only the reported one.
hermes_cli/memory_oauth.pyonly resolves and reads them;hermes_cli/backup.pyalready chmods0600on restore; the three plugin writers are listed above.hermes_cli/profiles.py::_migrate_honcho_profile_hostwas the only site missing the enforcement, so this is the complete set for this root cause. This PR deliberately does not widen into a mechanical repo-widechmod-after-write sweep — those are different files with different owners and sensitivities, and several are already correct or already claimed.Precedent
main:649ce1f8113"fix(cli): make profile.yaml and skin writes atomic to stop silent field loss" routed this same file'sprofile.yamlwrite throughatomic_yaml_write. This change follows it exactly, including the lazyfrom utils import ...idiom the module uses to stay import-light.profile_distribution.py,uninstall.py,web_routers/profiles.py,xai_retirement.py, andutils.py.hermes_cli/profiles.pywas not in that list, so this is a remaining site of a class that has already been accepted.utils.atomic_write_text's own docstring states the invariant this restores: "every destructive file rewrite in the codebase shares one implementation."Related Issue
No filed issue. #16743 is referenced as context for the symlink-detachment failure mode only; it is already closed and this PR does not reopen or claim it.
Type of Change
Changes Made
hermes_cli/profiles.py_migrate_honcho_profile_hostnow writes throughutils.atomic_json_write(path, raw, mode=0o600). The import is function-local at the top of the function, matching the module's existing lazy-import idiom (import yaml,from utils import atomic_yaml_write) so the module stays import-light; it is hoisted above the candidate loop rather than sitting inside it.except OSError: continuefail-soft is unchanged, so an unwritable candidate still lets the remaining candidates migrate.indentis not passed: the helper already defaults to2, matching the previous output.ensure_ascii=Falseis not passed either — the helper hardcodes it internally, and forwarding it through**dump_kwargswould raiseTypeError: got multiple values for keyword argument 'ensure_ascii'.tests/hermes_cli/test_profiles.pyTestMigrateHonchoProfileHostWriteclass, placed directly after the existingTestRenameProfilecluster. Nothing else in the file is reordered or reformatted.test_rewrite_keeps_credential_config_owner_only— seeds a0600config holding anapiKey, pinsumaskto0o022so the pre-fix behaviour is deterministic rather than dependent on the runner's umask, and asserts the mode is still0600afterwards.test_rewrite_preserves_a_symlinked_config— points~/.hermes/honcho.jsonat a file in another directory and asserts the path is still a symlink, still resolves to the same real file, that the real file received the migrated block, and that its mode survived.test_unwritable_candidate_still_advances_to_the_next— forcesOSErroron the first candidate and asserts the second is still migrated and no temp file is left behind. This is the guard on the unchanged fail-soft behaviour.skipif-gated onsys.platform == "win32". No new imports were added to the file.How to Test
Reproduce the mode widening on a POSIX box:
Before this change:
644, and the symlink is gone. After:600, and the symlink still points at the real file.Automated, with the before/after both verified:
With the production hunk reverted and the tests kept, all three fail on exactly the defects described:
With the fix restored:
Adjacent helper and consumer suites, unchanged:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passskipif-gated off Windows.Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A