fix(backup): restore import members atomically so a failed import can't erase config - #80572
fix(backup): restore import members atomically so a failed import can't erase config#80572briandevans wants to merge 5 commits into
Conversation
…'t erase config `hermes import` wrote every zip member with `open(target, "wb")` followed by `dst.write(src.read())`, at both restore sites in `run_import`. Opening for write truncates the user's existing file to zero *before* any replacement bytes exist, so a Ctrl-C, an ENOSPC, a corrupt zip member, or a crash leaves `config.yaml`, `.env`, or an external provider config (e.g. `~/.honcho/config.json`) empty with nothing behind it — during the disaster-recovery path the user is running precisely because they already lost something. The `_external/` branch writes outside HERMES_HOME, into third-party configs under the user's home, so the blast radius is not confined to Hermes state. Both sites now stage the member into the target's own directory, fsync it, and publish with `utils.atomic_replace`, so the target only ever moves from its old contents to the complete new contents. `atomic_replace` rather than a bare `os.replace`: it resolves a symlinked target first, so deployments that link `config.yaml` into a dotfiles repo keep the link instead of having it silently swapped for a regular file (NousResearch#16743), and it falls back to copy/fsync/unlink on EXDEV/EBUSY for cross-device and bind-mount installs. Members stream through `shutil.copyfileobj` instead of being read whole into memory. The temp file is removed on any failure so a partial import leaves no residue, and permission bits are carried across the replace so mkstemp's 0600 does not silently tighten restored files. This extends the module's own established idiom — `backup.py` already publishes atomically via `os.replace` in `_atomic_output_path` and in the snapshot writer — into the one path that still overwrote user files in place.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes hermes import restore so zip members are written atomically (no truncate-then-write window), preventing failed imports from zeroing existing config/state files—including _external/ targets outside HERMES_HOME.
Changes:
- Add
_extract_member_atomically()to stage each member in-target-dir,fsync, then publish viautils.atomic_replace. - Add
_default_new_file_mode()to preserve pre-existing and newly-created file permissions despitemkstemp(0600). - Add
TestImportAtomicWritesto pin invariants for partial failures, symlink preservation, and mode preservation across both restore branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hermes_cli/backup.py |
Reworks import extraction to stage + atomically publish, and adds logic to preserve file modes. |
tests/hermes_cli/test_backup.py |
Adds regression tests ensuring failed restores don’t truncate files and symlink/mode behavior is preserved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| current = os.umask(0o077) | ||
| os.umask(current) | ||
| except OSError: | ||
| return None |
There was a problem hiding this comment.
The thread-safety point is real, but I don't think either alternative is better here, so I've kept the probe. Reasoning, since it deserves more than "won't fix":
There is no read-only umask in CPython. os.umask is the only way to observe the process umask; there is no getumask. Linux exposes it via /proc/self/status (Umask:), but that is Linux-only and absent on macOS and BSD, so it cannot replace the probe — only add a platform-specific fast path.
The window is two syscalls, and it is deliberately fail-safe in the unsafe direction. The probe installs 0o077, not 0, precisely because of the race you're describing: a file another thread creates inside the window comes out owner-only, i.e. tighter than intended. The failure mode is a file that is too private, never one that is world-readable or world-writable. A 0 probe would have the security bug; this one has a cosmetic one.
It runs once per import, not once per member. _default_new_file_mode() is called a single time in run_import (hermes_cli/backup.py:1022) and the result is threaded through to every member, so the exposure is one two-syscall window per hermes import invocation — and hermes import is a single-threaded CLI command, not a service. There is no concurrent Hermes writer inside that window in practice.
The throwaway-file alternative trades the race for a worse one. Creating a file with open(..., 'xb'), stating it and unlinking it does I/O in the user's HERMES_HOME (or, on the _external/ branch, in a third-party config directory) on a disaster-recovery path where the directory may be read-only, full, or on a filesystem that ignores mode bits entirely — in which case the probe reports the wrong answer rather than failing. It also has its own race (another process observing or colliding with the throwaway) without removing the process-global umask mutation, since the umask is still what determines the resulting mode.
A module-level lock doesn't help either, because the races that matter are with code outside this module — any other thread in the process calling open() — and a lock in hermes_cli.backup cannot serialize those.
The alternative I would take if this window ever became a real concern is dropping the probe entirely and having _default_new_file_mode() return None, which the caller already handles by leaving mkstemp's mode alone. That is strictly safe but reintroduces the 0600 tightening on every newly created file that this PR exists to prevent, so it is a regression, not a fix.
Current head is dfda12ea3.
| fd, tmp_name = tempfile.mkstemp( | ||
| dir=str(target.parent), prefix=f".{target.name[:80]}.", suffix=".partial" | ||
| ) | ||
| try: | ||
| with os.fdopen(fd, "wb") as dst: |
There was a problem hiding this comment.
Not a reachable mismatch here, so I've left the diff alone.
utils.atomic_replace is annotated for exactly this call shape and normalizes both arguments itself:
def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str:
...
target_str = str(target)
real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str
tmp_str = str(tmp_path)(utils.py:91, 111-113). Everything downstream of those three lines is str, and the function's declared return is str — the resolved real path, not the symlink — which is the value that matters, since a symlinked config.yaml must have its mode and owner re-applied to the link target, not the link.
tmp_name is also handed to tempfile.mkstemp's own output and to os.unlink in the cleanup path, both of which are str-native, so converting it would add a Path() round-trip without removing one.
Where the mismatch would actually bite is on the receiving side, and dfda12ea3 (current head) does convert there: the replace result is now wrapped once as real_path = Path(atomic_replace(tmp_name, target)) (hermes_cli/backup.py:949) because _restore_file_owner / _restore_file_mode are annotated path: Path. That is the one boundary in this helper where the type was load-bearing.
| real_path = atomic_replace(tmp_name, target) | ||
| if mode is not None and not hasattr(os, "fchmod"): | ||
| try: | ||
| os.chmod(real_path, mode) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
Agreed, and fixed in dfda12ea3 (current head).
The docstring on this function already claimed the mode is applied "before the replace so the target never transits through mkstemp's 0600" — but that was only true on the os.fchmod branch. You're right that the fallback contradicted it, and there is a second consequence beyond the transit window: atomic_replace's EXDEV/EBUSY fallback does shutil.copyfile + shutil.copystat(tmp, real) (utils.py:125-127), so on a cross-device or bind-mount install the temp file's 0600 was being copied straight onto the target, and only the swallowed post-replace chmod undid it.
_extract_member_atomically now applies the mode to the temp file on both branches — os.fchmod(dst.fileno(), mode) where it exists, os.chmod(tmp_name, mode) where it does not (hermes_cli/backup.py:933-942) — and keeps the post-replace restore as the belt-and-braces path. That last call is now utils._restore_file_mode rather than a local swallowing try, which also picks up the ordering constraint: _restore_file_owner runs first because chown clears setuid/setgid, so the mode restore is what puts those bits back. Same shape atomic_yaml_write took in 43fc86562 ("close the yaml 0600 transit window") and atomic_write_text in 3556728a5, rather than a third variant.
On surfacing chmod failures in errors: I kept them best-effort deliberately. The member has already been restored correctly at that point, and the failure mode we care about — the file being published at 0600 — is now closed at the temp-file step, so the post-replace call is redundant on POSIX and only load-bearing on Windows, where os.chmod is a near-no-op. Turning a cosmetic permission miss into a reported import error would make hermes import look partially failed when the restore actually succeeded. This also matches every other atomic writer in utils.py, which all swallow there.
Test: tests/hermes_cli/test_backup.py::TestImportAtomicWrites::test_mode_is_applied_before_the_replace_without_fchmod (test_backup.py:789). It uses monkeypatch.delattr(os, "fchmod") — the convention 43fc86562 established for this branch — and spies the temp file's mode at atomic_replace time rather than after, so it reads 0o600 without the fix and 0o644 with it. Mutation-checked: deleting only the os.chmod(tmp_name, mode) line reds it (assert [384] == [420]).
alt-glitch
added
type/bug
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
…0 transit window Follow-up on the atomic-import restore, delegating both metadata concerns to the shared helpers instead of half-handling them locally. Owner preservation was missing entirely. `tempfile.mkstemp` + `atomic_replace` publishes a temp file owned by the *writing* user, so `sudo hermes import` re-owned every restored file to root — on the disaster-recovery path, and on exactly the Docker/NAS volume installs `utils._restore_file_owner` was added for. `_extract_member_atomically` now captures `_preserve_file_owner(target)` before staging and calls `_restore_file_owner` after the replace, before the mode restore (chown clears setuid/setgid, so the mode has to go back last). Mode handling was also only half applied before the replace: the `os.fchmod` branch applied it to the temp fd, but the platforms without `fchmod` fell through to a best-effort post-replace chmod, leaving the published file at mkstemp's 0600 until that chmod landed — permanently if the process died in between — and making `atomic_replace`'s EXDEV/EBUSY `shutil.copystat` fallback copy 0600 onto the target. The mode is now applied to the temp file on both branches, with the post-replace `_restore_file_mode` kept as the belt-and- braces path. This is the same shape `atomic_write_text` and `atomic_yaml_write` already carry after 3556728 and 43fc865; capture and restore now reuse `utils._preserve_file_mode` / `_preserve_file_owner` / `_restore_file_mode` / `_restore_file_owner` rather than re-deriving them, which also drops the local `import stat`. Tests (tests/hermes_cli/test_backup.py, class TestImportAtomicWrites): - test_restore_preserves_existing_file_owner — forces a uid/gid so it does not need root; asserts chown fires once, with the captured owner, on the pre-existing file only (a newly created member has no prior owner). Mutation-checked: dropping only the `_restore_file_owner` call reds it. - test_mode_is_applied_before_the_replace_without_fchmod — `monkeypatch.delattr` on `os.fchmod`, spies the temp file's mode at replace time. Reads 0o600 without the fix, 0o644 with it. Mutation-checked the same way.
…truncates The atomicity claim in _extract_member_atomically's docstring holds on the os.replace path but not on atomic_replace's EXDEV/EBUSY fallback, which uses shutil.copyfile and so opens the destination 'wb'. That is pre-existing behaviour shared by every atomic writer in the repo, and it is reachable here for a symlinked target whose real file lives on another filesystem. Scope the docstring to what the helper actually guarantees instead of overstating it; the fallback itself is a utils.atomic_replace change.
|
This one is correct, and I've scoped the docstring to match in To be precise about where the gap lives: The reachability point is fair, though: staging in the symlink's parent directory means a symlinked I have not fixed it here on purpose. The fix belongs in Head is now |
|
suggesting changes
Please preserve only ordinary permission bits for restored content (or explicitly clear setuid/setgid before publication) and add a regression covering a target with special mode bits. The existing Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
…files ``_extract_member_atomically`` carries the replaced file's permissions across the publish so that routing through mkstemp does not change what the caller would otherwise have produced. But ``_preserve_file_mode`` returns ``stat.S_IMODE``, which is all twelve bits, and this restore is deliberate on both sides of the replace: the mode is fchmod'd onto the temp before ``atomic_replace`` and re-applied afterwards because chown clears the elevated bits. So a target sitting at 0o4755 comes out of ``hermes import`` still at 0o4755 — with contents supplied by the zip. That is a regression introduced by the atomic rewrite rather than a pre-existing one. The overwrite it replaced was an in-place ``open(target, "wb")``, and an in-place write by a process without CAP_FSETID has the elevated bits stripped by the kernel, so the old path left 0o4755 as 0o755. The blast radius is not limited to Hermes' own state: the ``_external/`` branch of ``run_import`` publishes members anywhere under ``$HOME``, and this is the path that documents ``sudo`` use so ownership survives a restore. An archive that happens to contain a member matching some existing privileged file would take over the identity that file runs as. Mask the two bits off the preserved mode. The masking happens once, before the temp file is chmod'd, so there is no transient elevation either. The sticky bit is kept — it is inert on a regular file. The ordinary permission bits are unaffected, so the Docker/NAS installs the preservation exists for still get their broader modes back. This is the one write path in the repo where the bytes are untrusted; the ``utils`` writers that preserve the full mode re-serialize content the process itself produced, and are correct as they stand.
… bits Pre-creates a 0o6755 target, imports a member over it, and asserts the published file is 0o755 with both elevated bits gone — plus that the staged temp file never carried them either, so there is no window where archive content sits behind an elevated mode. The existing coverage in this class cannot see the failure: every mode assertion masks with ``& 0o777``, which discards exactly the bits at issue, and the fixtures chmod their targets to ordinary modes that never had them set. Without the mask on the preserved mode this test reports the published file still holding S_ISUID. Skipped where the platform or filesystem refuses setuid on a user-owned file, so the assertion never depends on running as root.
|
setuid/setgid are now masked off the preserved mode. Pushed. This one is a regression this PR introduces, not a pre-existing one, and it is worth being exact about why.
Main does not behave that way. The overwrite this PR replaces is an in-place The reach is not limited to Hermes' own state: the Fix: mask the two bits off the preserved mode once, before the temp file is chmod'd, so there is no transient elevation either. Deliberately not widened to Commits: Regression: It pre-creates a |
|
Merged via PR #86669. All five of your commits were cherry-picked onto current main with your authorship preserved in git log — thank you for the thorough work here, including the review-driven setuid/setgid masking and the sudo owner preservation. |
What does this PR do?
hermes importcould destroy the file it was restoring.Both restore sites in
run_importwrote each zip member like this:open(target, "wb")truncates the user's existing file to zero before any replacement bytes exist. There is noos.replace, no fsync, and no backup. A Ctrl-C, an ENOSPC, a corrupt or truncated zip member, or a crash between the truncate and the write leavesconfig.yaml,.env, or an external provider config empty, with nothing behind it — duringhermes import, which is the documented disaster-recovery path. The user is running it because they already lost something.Two things make this worse than a normal non-atomic write:
_external/branch restores memory-provider state to its original home-relative location (e.g.~/.honcho/config.json), so a failed import can zero a third-party config the user never associated with Hermes.Both sites now stage the member into the target's own directory, fsync, and publish with
utils.atomic_replace. The target only ever moves from its old contents to the complete new contents.This extends the module's own established idiom rather than introducing one.
hermes_cli/backup.pyalready publishes atomically twice —os.replace(partial_path, final_path)in_atomic_output_path(:211) andos.replace(staging_dir, snap_dir)in the snapshot writer (:1337, from the recently landedaad8f741"serialize and atomically publish snapshots").run_importwas the one path left overwriting user files in place.utils.atomic_write_textstates the invariant directly in its docstring: it exists so that "every destructive file rewrite in the codebase shares one implementation."Why
utils.atomic_replaceand not a bareos.replace— this is load-bearing.open(target, "wb")writes through a symlink, so a user who links~/.hermes/config.yamlinto a dotfiles repo keeps that link today. A naivemkstemp+os.replacewould replace the symlink with a regular file and silently detach the deployment — that is #16743, a bug this repo has already been bitten by.atomic_replaceresolves the link first, and falls back to copy/fsync/unlink onEXDEV/EBUSYfor cross-device and bind-mount installs. Two of the added tests pin this; both go red if the helper is swapped for a rawos.replace.Related Issue
No filed issue — found by inspection of the restore path. Refs #16743 (the symlink-detach hazard the helper is chosen to avoid).
Type of Change
Changes Made
hermes_cli/backup.py_extract_member_atomically():mkstempin the target's own directory (never/tmp, so theEXDEVfallback stays the exception) → stream the member →fsync→atomic_replace. The temp file is unlinked on any failure, includingKeyboardInterrupt, so a partial import leaves no residue.shutil.copyfileobjinstead ofsrc.read(), which also stops a multi-gigabytestate.dbmember being held in memory in one piece._default_new_file_mode():mkstempcreates at 0600, so without this every newly created file would be tightened to owner-only, breaking Docker/NAS installs that rely on broader permissions — the same hazardutils._restore_file_modedocuments. Existing files keep their own mode (captured and applied to the temp fd before the replace, so the target never transits 0600). Resolved once per import, and the probe installs a restrictive mask rather than 0 so nothing created by another thread in that two-syscall window is world-writable.run_importwrite sites call the helper. Theos.chmod(...)calls at both sites are untouched — permission behaviour is byte-for-byte what it was.tests/hermes_cli/test_backup.pyTestImportAtomicWrites, 5 tests, covering both restore branches:test_failed_member_leaves_existing_file_intact— a member whose stream dies mid-restore must not destroy the file it was replacing, and must leave no.config.yaml.*temp behind.test_failed_external_member_leaves_existing_file_intact— same invariant on the_external/branch.test_symlinked_target_keeps_its_symlink/test_symlinked_external_target_keeps_its_symlink— the target keeps its symlink and the real file receives the content.test_restore_preserves_existing_file_mode— staging throughmkstempdoes not tighten a 0644 file to 0600.Acceptance criteria, all covered: (1) no window in which the target exists truncated or partially written; (2) a symlinked target survives; (3) a mid-write failure leaves the original intact with no stray temp; (4) existing permission behaviour unchanged.
How to Test
Reproduce the bug on
main:printf 'model: keep-me\n' > $HERMES_HOME/config.yamlconfig.yamlmember is truncated/corrupt (or^Cduringhermes importon a large archive).hermes import backup.zip --force→config.yamlis now 0 bytes. The original is gone.With this PR, the same run reports the member in the warnings block and
config.yamlstill holdsmodel: keep-me.Automated, with the injected-failure direction verified in both directions:
Regression guard, run explicitly:
hermes_cli/backup.pytomainand re-running: the two data-loss tests fail (the file is 0 bytes — the truncate landed, the write did not), the 3 guards pass. Restoring the fix: 5 passed.atomic_replacefor a bareos.replace: the two symlink tests fail. The guards are not vacuous.Adjacent suites touching the same primitive, all green:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — not the full suite. Rantests/hermes_cli/test_backup.py(45 passed) plus the five adjacent atomic-write/utils-import suites listed above (12 passed). Exact commands and counts are in "How to Test".Documentation & Housekeeping
docs/, docstrings) — behaviour is documented in the new helpers' docstrings; no user-facing docs change, the CLI contract is unchangedos.fchmodis Unix-only, so where it is absent the mode is applied to the temp file with a path-basedos.chmodbefore the replace, withutils._restore_file_modekept afterwards as the belt-and-braces path (mirrorsutils.atomic_yaml_writeafter43fc86562);_preserve_file_owneris a no-op off POSIX;atomic_replacealready handles the Windows/bind-mountEBUSYpath; the symlink, mode, and owner tests areskipif(os.name != "posix")and the no-fchmodtest usesmonkeypatch.delattrso the Windows branch is covered on CIRelated / Positioning
I checked the open queue by symbol and by changed path before opening this. Several PRs are adjacent; I want to be precise about which are disjoint and which is not, rather than claim a clean field.
Complementary, content-disjoint — no action needed:
guard os.chmod on Windows where it is a no-op) and fix: use Windows ACLs for credential file permissions in backup import (#56923) #56949 (use Windows ACLs for credential file permissions) both edit only theos.chmod(...)lines at these two sites, leaving the write statements as unchanged context. I deliberately did not touch those lines, so both remain applicable on top of this._detect_prefix), fix(cli): honor active profile in backup and import #9839 (profile resolution), and fix(cron): hold _jobs_lock() in snapshot-restore paths that overwrite cron/jobs.json #46765 (restore_quick_snapshot/restore_cron_jobs_if_emptied) are elsewhere in the file.Genuinely overlapping, and it does not fix this bug:
fix(security): harden backup import and OOXML extraction #61881 (
harden backup import and OOXML extraction, currentlyCONFLICTING) replaces these same two statements with a_copy_zip_member()helper. That helper is:It is still truncate-then-write — no temp file, no
os.replace, no fsync, no symlink handling. It fixes the whole-member-in-memory problem (which this PR also fixes) but the data-loss window survives it unchanged. Its real value is elsewhere in its diff: zip-bomb limits, member-count/size/ratio caps, and an external-target allowlist, none of which this PR touches.So the two are not duplicates and neither is a subset of the other in intent — but they do collide textually on these two lines, and whichever lands second will need a trivial rebase. On the write statements themselves this PR is the superset: it carries fix(security): harden backup import and OOXML extraction #61881's streaming change and adds the atomicity, symlink preservation, and temp cleanup that it lacks. Happy to rebase onto it, or to fold the atomic helper into
_copy_zip_memberif you would rather take that one first — just say which.I did not extend this to
restore_quick_snapshot, which has the same class of non-atomicshutil.copy2overwrite: it is a different entry point (/snapshotrestore, nothermes import) and it is the function #46765 is actively editing. Happy to file it separately.Follow-up commit
dfda12ea3addresses the three inline findings onhermes_cli/backup.py; each thread has a reply with the reasoning. Two of them changed code.dc051c497(current head) then scopes the helper's docstring to what it actually guarantees — see the triage-comment reply below onatomic_replace's cross-device fallback.Owner preservation was missing, and that is the substantive one.
tempfile.mkstemp+atomic_replacepublishes a temp file owned by the writing user, sosudo hermes importwas re-owning every restored file to root — on the disaster-recovery path, and on exactly the Docker/NAS volume-mounted installsutils._restore_file_ownerwas added for (551e5af50, "preserve owner on atomic writes (#56644)")._extract_member_atomicallynow captures_preserve_file_owner(target)before staging and calls_restore_file_ownerafter the replace, ahead of the mode restore —chownclears setuid/setgid, so the mode has to go back last. This is the orderingatomic_yaml_writeandatomic_json_writealready use.The mode was only applied before the replace on the
os.fchmodbranch. Platforms withoutfchmodfell through to a best-effort post-replacechmod, which left the published file atmkstemp's 0600 until that call landed — permanently if the process died in between — and madeatomic_replace'sEXDEV/EBUSYshutil.copystatfallback copy 0600 onto the target. The mode is now applied to the temp file on both branches, with the post-replace restore kept as belt-and-braces.Both concerns now delegate to the shared
utilshelpers rather than being re-derived here —_preserve_file_mode/_preserve_file_owner/_restore_file_mode/_restore_file_owner, the same setatomic_write_textandatomic_yaml_writeuse after3556728a5("move mode+owner preservation into atomic_write_text") and43fc86562("close the yaml 0600 transit window"). They are underscore-private, and I took importing them over re-implementing deliberately: same repo,backup.pyalready importsatomic_replacefromutils, and a second copy of chown-then-chmod ordering is exactly the divergence those commits consolidated. If you would rather they were public, say the word and I will add a thin public wrapper inutils.pyin this PR. The localimport statis gone as a result.The third finding —
os.umaskmutating process-global state — I declined, with reasoning in the thread: CPython has no portable read-only umask query, the probe deliberately installs the restrictive0o077so anything a racing thread creates is owner-only rather than widened, the window is two syscalls, and it is resolved once per import rather than per member. The suggested alternative (create a throwaway file andstatit) adds I/O in the user's config directory on a recovery path and has its own race without removing the process-global one.Two tests added to
TestImportAtomicWrites, both mutation-checked in the final file location:test_restore_preserves_existing_file_ownerchown_calls == [][(config.yaml, 123, 456)]test_mode_is_applied_before_the_replace_without_fchmod0o6000o644The owner test forces a uid/gid so it does not need root, and asserts the chown fires once — on the pre-existing file, not on the newly created member, which pins that the owner is genuinely captured rather than invented. The mode test uses
monkeypatch.delattr(os, "fchmod")(the convention43fc86562established for this branch) and spies the temp file's mode atatomic_replacetime rather than after, so a post-replacechmodcannot make it pass. Deleting only the_restore_file_ownercall reds the first; deleting only theos.chmod(tmp_name, mode)line reds the second.