From 551e5af50dc6597069e57af047213f61e40246d6 Mon Sep 17 00:00:00 2001 From: Gille <4317663+helix4u@users.noreply.github.com> Date: Thu, 2 Jul 2026 22:27:34 -0600 Subject: [PATCH] fix(config): preserve owner on atomic writes (#56644) --- tests/test_atomic_replace_symlinks.py | 80 ++++++++++++++++++++++++++- utils.py | 45 +++++++++++++-- 2 files changed, 120 insertions(+), 5 deletions(-) diff --git a/tests/test_atomic_replace_symlinks.py b/tests/test_atomic_replace_symlinks.py index 594401d05654..3aab77cf1c77 100644 --- a/tests/test_atomic_replace_symlinks.py +++ b/tests/test_atomic_replace_symlinks.py @@ -26,7 +26,12 @@ _REPO_ROOT = Path(__file__).resolve().parent.parent if str(_REPO_ROOT) not in sys.path: sys.path.insert(0, str(_REPO_ROOT)) -from utils import atomic_json_write, atomic_replace, atomic_yaml_write +from utils import ( + atomic_json_write, + atomic_replace, + atomic_roundtrip_yaml_update, + atomic_yaml_write, +) # ─── Direct helper ──────────────────────────────────────────────────────────── @@ -139,6 +144,79 @@ def test_atomic_json_write_preserves_symlink_permissions(tmp_path: Path) -> None assert mode == 0o644, f"permissions drifted after symlinked write: {oct(mode)}" +def test_atomic_yaml_write_restores_owner_on_real_symlink_target( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Config writes through symlinks must restore the real file's owner. + + Docker support hit this when a root-run setup wizard rewrote a + hermes-owned /opt/data/config.yaml via atomic replace, leaving the new file + root-owned. The test forces a preserved uid/gid so it does not need root. + """ + if os.name != "posix": + pytest.skip("POSIX-only") + + real = tmp_path / "config.yaml" + link = tmp_path / "link.yaml" + real.write_text("old: true\n", encoding="utf-8") + link.symlink_to(real) + + chown_calls: list[tuple[Path, int, int]] = [] + monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (123, 456)) + monkeypatch.setattr( + "utils.os.chown", + lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)), + ) + + atomic_yaml_write(link, {"new": True}) + + assert chown_calls == [(real, 123, 456)] + + +def test_atomic_json_write_restores_owner_with_explicit_mode( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + if os.name != "posix": + pytest.skip("POSIX-only") + + target = tmp_path / "state.json" + target.write_text("{}", encoding="utf-8") + + chown_calls: list[tuple[Path, int, int]] = [] + monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (234, 567)) + monkeypatch.setattr( + "utils.os.chown", + lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)), + ) + + atomic_json_write(target, {"api_key": "secret"}, mode=0o600) + + assert chown_calls == [(target, 234, 567)] + assert target.stat().st_mode & 0o777 == 0o600 + + +def test_atomic_roundtrip_yaml_update_restores_owner( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + if os.name != "posix": + pytest.skip("POSIX-only") + + target = tmp_path / "config.yaml" + target.write_text("model:\n provider: openrouter\n", encoding="utf-8") + + chown_calls: list[tuple[Path, int, int]] = [] + monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (345, 678)) + monkeypatch.setattr( + "utils.os.chown", + lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)), + ) + + atomic_roundtrip_yaml_update(target, "model.provider", "nvidia") + + assert chown_calls == [(target, 345, 678)] + assert yaml.safe_load(target.read_text(encoding="utf-8"))["model"]["provider"] == "nvidia" + + # ─── Broken-symlink edge case ───────────────────────────────────────────── diff --git a/utils.py b/utils.py index 7d4ee3810ac7..713a9f5731bd 100644 --- a/utils.py +++ b/utils.py @@ -43,6 +43,34 @@ def _preserve_file_mode(path: Path) -> "int | None": return None +def _preserve_file_owner(path: Path) -> "tuple[int, int] | None": + """Capture the owning uid/gid of *path* if the platform supports it.""" + if os.name != "posix": + return None + try: + st = path.stat() + except OSError: + return None + return st.st_uid, st.st_gid + + +def _restore_file_owner(path: Path, owner: "tuple[int, int] | None") -> None: + """Re-apply uid/gid after an atomic replace when permitted. + + Docker and NAS-backed installs often run some commands as root while the + persistent volume is owned by the runtime user. ``os.replace`` swaps in the + temp file's owner, so a root-run config write can leave ``config.yaml`` owned + by root. Best-effort chown preserves the existing owner for privileged + callers and is harmless for unprivileged callers that cannot chown. + """ + if owner is None or not hasattr(os, "chown"): + return + try: + os.chown(path, owner[0], owner[1]) + except OSError: + pass + + def _restore_file_mode(path: Path, mode: "int | None") -> None: """Re-apply *mode* to *path* after an atomic replace. @@ -136,6 +164,7 @@ def atomic_json_write( path.parent.mkdir(parents=True, exist_ok=True) original_mode = None if mode is not None else _preserve_file_mode(path) + original_owner = _preserve_file_owner(path) fd, tmp_path = tempfile.mkstemp( dir=str(path.parent), @@ -160,13 +189,15 @@ def atomic_json_write( os.fsync(f.fileno()) # Preserve symlinks — swap in-place on the real file (GitHub #16743). real_path = atomic_replace(tmp_path, path) + real_path_obj = Path(real_path) + _restore_file_owner(real_path_obj, original_owner) if mode is not None: try: - os.chmod(real_path, mode) + os.chmod(real_path_obj, mode) except OSError: pass else: - _restore_file_mode(Path(real_path), original_mode) + _restore_file_mode(real_path_obj, original_mode) except BaseException: # Intentionally catch BaseException so temp-file cleanup still runs for # KeyboardInterrupt/SystemExit before re-raising the original signal. @@ -219,6 +250,7 @@ def atomic_yaml_write( path.parent.mkdir(parents=True, exist_ok=True) original_mode = _preserve_file_mode(path) + original_owner = _preserve_file_owner(path) fd, tmp_path = tempfile.mkstemp( dir=str(path.parent), @@ -248,7 +280,9 @@ def atomic_yaml_write( os.fsync(f.fileno()) # Preserve symlinks — swap in-place on the real file (GitHub #16743). real_path = atomic_replace(tmp_path, path) - _restore_file_mode(real_path, original_mode) + real_path_obj = Path(real_path) + _restore_file_owner(real_path_obj, original_owner) + _restore_file_mode(real_path_obj, original_mode) except BaseException: # Match atomic_json_write: cleanup must also happen for process-level # interruptions before we re-raise them. @@ -303,6 +337,7 @@ def atomic_roundtrip_yaml_update( current[keys[-1]] = value original_mode = _preserve_file_mode(path) + original_owner = _preserve_file_owner(path) fd, tmp_path = tempfile.mkstemp( dir=str(path.parent), prefix=f".{path.stem}_", @@ -314,7 +349,9 @@ def atomic_roundtrip_yaml_update( f.flush() os.fsync(f.fileno()) real_path = atomic_replace(tmp_path, path) - _restore_file_mode(real_path, original_mode) + real_path_obj = Path(real_path) + _restore_file_owner(real_path_obj, original_owner) + _restore_file_mode(real_path_obj, original_mode) except BaseException: try: os.unlink(tmp_path)