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: