From 714013c4930dd0c715d98a543c9b82e72a0e1bcc Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 13 Sep 2026 00:08:06 -0700 Subject: [PATCH] fix(utils): new non-secret atomic writes follow the process umask again Every hand-rolled writer this PR folded into utils._atomic_write created a NEW file with write_text()/open("w"), i.e. at 0o666 masked by the umask (0644 under 022). The canonical helper publishes through mkstemp, whose temp is 0600, and with no explicit mode and no existing target to copy bits from it left that 0600 in place - so debug, model_catalog, profiles, breadcrumbs, worktree_ops, web_result_cache, plugin_compat, write_approval, rich_sent_store, active_sessions and the google_meet state files were silently tightened to owner-only, the volume-mount hazard _restore_file_metadata's own docstring warns about. Undeclared in the PR. Fix at the canonical: when mode is None and the target does not exist, apply default_new_file_mode() (0o666 masked by the umask, read via the umask two-call trick with a transient 0o077 so a racing thread can only get a tighter file). The helper is hermes_cli/backup._default_new_file_mode moved into utils and reused. Secret writers (mode=0o600) are 0600 before, during and after as before; an existing target keeps its bits; on non-POSIX the helper returns None so nothing is chmod'd. --- hermes_cli/backup.py | 18 ++------------ tests/test_atomic_json_writers_unified.py | 26 ++++++++++++++++++++ utils.py | 30 ++++++++++++++++++++--- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index ee00de7605..a7cf772bd8 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -22,6 +22,7 @@ from hermes_constants import ( from hermes_state_dbfile import RETIRED_GENERATION_DIR_SUFFIX from utils import ( _preserve_file_mode, _preserve_file_owner, _restore_file_mode, _restore_file_owner, atomic_replace, + default_new_file_mode, ) from hermes_cli.sizefmt import format_bytes as _format_size @@ -759,21 +760,6 @@ def _detect_prefix(zf: zipfile.ZipFile) -> str: return "" -def _default_new_file_mode() -> Optional[int]: - """The mode ``open(path, "wb")`` gives a file it has to create. - - ``mkstemp`` always creates at 0600, so staging an import through a temp file would tighten - every *newly created* file to owner-only — the Docker/NAS volume-mount hazard - ``utils._restore_file_mode`` documents. - """ - try: - current = os.umask(0o077) - os.umask(current) - except OSError: - return None - return 0o666 & ~current - - def _extract_member_atomically( zf: zipfile.ZipFile, member: str, target: Path, new_file_mode: Optional[int] = None) -> None: """Restore one zip member onto *target* with no truncation window. @@ -912,7 +898,7 @@ def _import_members( db_shrunk: list[tuple[str, tuple[int, int], tuple[int, int]]] = [] restored = restored_external = 0 home_dir = Path.home().resolve() - new_file_mode = _default_new_file_mode() # once: every member is published via mkstemp (0600) + new_file_mode = default_new_file_mode() # once: every member is published via mkstemp (0600) for member in members: # ``_external/`` members restore to their home-relative location (~/.honcho/config.json), # NOT under HERMES_HOME; provider configs commonly hold credentials, so tighten to 0600. diff --git a/tests/test_atomic_json_writers_unified.py b/tests/test_atomic_json_writers_unified.py index d635e28365..9f522125bb 100644 --- a/tests/test_atomic_json_writers_unified.py +++ b/tests/test_atomic_json_writers_unified.py @@ -84,3 +84,29 @@ def test_surrogate_escaped_strings_round_trip_through_atomic_json_write(tmp_path assert json.loads(target.read_bytes()) == payload assert _leftovers(target.parent, target.name) == [] + +@pytest.mark.linux_only +def test_new_non_secret_file_follows_umask_while_secret_and_existing_modes_hold(tmp_path): + """The writers this helper replaced created files at process umask; only ``mode=`` tightens.""" + import os + import stat + + from utils import atomic_json_write + + old_umask = os.umask(0o022) + try: + fresh = tmp_path / "cache.json" + atomic_json_write(fresh, {"a": 1}) + assert stat.S_IMODE(fresh.stat().st_mode) == 0o644, "new non-secret file must not inherit mkstemp's 0600" + + secret = tmp_path / "creds.json" + atomic_json_write(secret, {"token": "x"}, mode=0o600) + assert stat.S_IMODE(secret.stat().st_mode) == 0o600 + + existing = tmp_path / "state.json" + existing.write_text("{}", encoding="utf-8") + os.chmod(existing, 0o640) + atomic_json_write(existing, {"b": 2}) + assert stat.S_IMODE(existing.stat().st_mode) == 0o640 + finally: + os.umask(old_umask) diff --git a/utils.py b/utils.py index 9b3fe3e7e7..dabf197d07 100644 --- a/utils.py +++ b/utils.py @@ -68,6 +68,25 @@ def _restore_file_metadata(path: Path, owner: "tuple[int, int] | None", mode: "i os.chmod(path, mode) +def default_new_file_mode() -> "int | None": + """The mode ``open(path, "w")`` gives a file it has to create (``0o666 & ~umask``); ``None`` + when the umask cannot be read or on non-POSIX hosts (Windows mode bits are synthesized). + + ``mkstemp`` always creates at 0600, so publishing a *new* non-secret file through a temp + file would tighten it to owner-only — the Docker/NAS volume-mount hazard + :func:`_restore_file_metadata` documents. The transient mask is 0o077: a thread that opens + a file in the read window gets a tighter file, never a looser one. + """ + if os.name != "posix": + return None + try: + current = os.umask(0o077) + os.umask(current) + except OSError: + return None + return 0o666 & ~current + + def _restore_file_owner(path: Path, owner: "tuple[int, int] | None") -> None: _restore_file_metadata(path, owner, None) @@ -202,11 +221,16 @@ def _atomic_write(path: Path, write, *, prefix: str, encoding: str = "utf-8", mo is created by ``mkstemp`` — ``O_CREAT|O_EXCL`` at 0600 regardless of umask — so a secret is never readable at process umask, not even between create and chmod. *mode* is fchmod'd onto the temp fd BEFORE the replace so the target never transits through mkstemp's 0600 (fchmod is - Unix-only; the post-replace chmod is the sole path on Windows). *fsync_dir* also fsyncs the - parent so the rename itself is durable. The temp file is removed on any failure — - ``BaseException`` on purpose, so KeyboardInterrupt / SystemExit still clean up. + Unix-only; the post-replace chmod is the sole path on Windows). With no *mode* a NEW target + gets what ``open(path, "w")`` would have given it (process umask) — the callers this replaced + wrote at umask, and silently tightening every fresh cache/state file to 0600 breaks shared + volume mounts; an existing target with no *mode* keeps mkstemp's bits, as before. *fsync_dir* + also fsyncs the parent so the rename itself is durable. The temp file is removed on any + failure — ``BaseException`` on purpose, so KeyboardInterrupt / SystemExit still clean up. """ path.parent.mkdir(parents=True, exist_ok=True) + if mode is None and not path.exists(): + mode = default_new_file_mode() original_owner = _preserve_file_owner(path) if preserve_owner else None fd, tmp_path = tempfile.mkstemp(dir=str(path.parent), prefix=prefix, suffix=".tmp") try: