diff --git a/api/config.py b/api/config.py index a5e30d7e..999d30a6 100644 --- a/api/config.py +++ b/api/config.py @@ -780,7 +780,8 @@ def _save_yaml_config_file(config_path: Path, config_data: dict) -> None: raise RuntimeError("PyYAML is required to write Hermes config.yaml") from exc config_path.parent.mkdir(parents=True, exist_ok=True) - config_path.write_text( + _paths._atomic_write_text( + config_path, _yaml.safe_dump(_config_for_yaml_save(config_data), sort_keys=False, allow_unicode=True), encoding="utf-8", ) diff --git a/api/onboarding.py b/api/onboarding.py index 7af2910a..cfff9ea2 100644 --- a/api/onboarding.py +++ b/api/onboarding.py @@ -28,6 +28,7 @@ from api.config import ( save_settings, verify_hermes_imports, ) +from api.paths import _atomic_write_text from api.providers import _write_env_file # shared impl with _ENV_LOCK (#1164) from api.workspace import get_last_workspace, load_workspaces @@ -251,7 +252,8 @@ def _save_yaml_config(config_path: Path, config: dict) -> None: raise RuntimeError("PyYAML is required to write Hermes config.yaml") from exc config_path.parent.mkdir(parents=True, exist_ok=True) - config_path.write_text( + _atomic_write_text( + config_path, _yaml.safe_dump(config, sort_keys=False, allow_unicode=True), encoding="utf-8", ) diff --git a/api/paths.py b/api/paths.py index ea00f848..358352d9 100644 --- a/api/paths.py +++ b/api/paths.py @@ -5,12 +5,258 @@ that need the default Hermes home can import them without triggering config's larger startup side effects. """ +import errno import os +import secrets +import stat +import tempfile +import threading from pathlib import Path HOME = Path.home() +def _create_atomic_temp_file(write_path: Path, *, existing: bool) -> tuple[int, str]: + """Create a same-directory temp file without reading the process umask. + + ``tempfile.mkstemp`` deliberately creates files as ``0600``. For a brand-new + config, use ``os.open(..., 0666)`` instead so the kernel applies the active + process umask as it would for ``Path.write_text``. Reading umask by calling + ``os.umask`` is unsafe because that syscall mutates process-wide state, and + imports may happen after the threaded server has started. + """ + if existing: + return tempfile.mkstemp( + dir=str(write_path.parent), prefix=f".{write_path.name}_", suffix=".tmp" + ) + + flags = os.O_RDWR | os.O_CREAT | os.O_EXCL + for _ in range(100): + tmp = write_path.parent / f".{write_path.name}_{secrets.token_hex(16)}.tmp" + try: + return os.open(tmp, flags, 0o666), str(tmp) + except FileExistsError: + continue + raise FileExistsError("unable to allocate unique config temporary file") + +# In-place compatibility writes cannot use rename isolation. Serialize every +# such write in this process so concurrent WebUI saves cannot interleave their +# truncate/write/fsync sequences. A single lock avoids a permanent per-path +# lock cache for arbitrary profile/config locations. +_IN_PLACE_WRITE_LOCK = threading.RLock() + + +def _fsync_directory(directory: Path) -> None: + """Persist a completed rename on filesystems that support directory fsync.""" + if os.name == "nt": + return + flags = os.O_RDONLY | getattr(os, "O_DIRECTORY", 0) + try: + fd = os.open(directory, flags) + except PermissionError: + return + try: + try: + os.fsync(fd) + except OSError as exc: + if exc.errno not in {errno.EINVAL, errno.ENOTSUP}: + raise + finally: + os.close(fd) + + +def _has_extended_attributes(path: Path) -> bool: + """Return whether *path* has xattrs that an inode replacement would discard.""" + listxattr = getattr(os, "listxattr", None) + if listxattr is None: + return False + try: + return bool(listxattr(path)) + except OSError as exc: + unsupported_errnos = { + errno.ENOSYS, + errno.ENOTSUP, + getattr(errno, "EOPNOTSUPP", errno.ENOTSUP), + } + if exc.errno in unsupported_errnos: + return False + raise + + +def _require_writable_target(write_path: Path) -> os.stat_result | None: + """Reject writes to a read-only target before any replacement work. + + ``os.replace`` happily swaps a fresh inode over a ``0444`` file when the + directory is writable, which would silently defeat a deliberately locked + config. The old in-place ``Path.write_text`` raised ``PermissionError`` + there; preserve that contract with a non-truncating ``O_WRONLY`` probe, + letting the failure propagate. Returns the ``fstat`` of the inode the + probe actually opened (so the caller's metadata describes the verified + file even if a concurrent writer replaced it since its own ``stat``), or + ``None`` if the target vanished concurrently. + """ + try: + probe_fd = os.open(write_path, os.O_WRONLY) + except FileNotFoundError: + return None + try: + return os.fstat(probe_fd) + finally: + os.close(probe_fd) + + +def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> None: + """Atomically replace *path* with *text*. + + Writes to a temp file in the same directory, flushes + ``os.fsync``, then + ``os.replace``s it into place, so a crash (or exception) mid-write can never + truncate an existing file — the old contents stay intact until the rename + commits the new ones in one step. Mirrors the tempfile+fsync+os.replace + pattern already used for ``.env`` and cost snapshots in ``api.providers``; + extracted here so ``config.yaml`` / profile ``config.yaml`` writers can share + it without pulling in env-file-specific logic. + + Permissions and ownership are preserved: ``os.replace`` carries the temp + file's metadata onto the target, and ``tempfile.mkstemp`` starts with the + writer's owner/group and mode ``0600``. Without correcting those values, + every rewrite could silently reset a shared config's group and tighten a + group/other-readable ``config.yaml`` (commonly ``0644``/``0664``) down to + owner-only. Existing files copy uid/gid and mode onto the temp descriptor; + new files use the active process umask, exactly as a normal + ``open(..., "w")`` would. + (Unlike ``.env``, ``config.yaml`` holds no secrets and is not meant to be + forced to ``0600``.) + + Symlinks and hard links keep the same follow-through semantics as + ``Path.write_text``: writing ``config.yaml`` through a symlink updates the + referent, while writing an inode with multiple hard links updates every + alias rather than severing the selected path. A symlink target change + observed before commit is rejected rather than updating a stale referent. + + The temp+rename dance needs WRITE PERMISSION ON THE DIRECTORY, which the + plain ``Path.write_text`` it replaced did not: hardened deployments + (``HERMES_CONFIG_PATH`` pointing into a root-owned, read-only dir with a + writable ``config.yaml`` inside) would otherwise lose the ability to save + settings at all. When temp creation is denied, or the original uid/gid + cannot be transferred to the temp inode, we fall back to a guarded + descriptor write only if the target is still the regular-file inode + inspected above. Hard-linked files and files with extended attributes use + that same path because replacing their directory entry would respectively + break ``Path.write_text`` compatibility or discard metadata such as POSIX + ACLs. These in-place writes are serialized across process-local WebUI + writers. They give up crash-atomicity (and raw concurrent readers may + observe the write in progress) only where atomic replace with preserved + semantics is impossible. Other ``PermissionError``s still propagate. + Successful renames fsync the parent directory where supported. + + The caller is responsible for ensuring ``path.parent`` exists. + """ + path = Path(path) + symlink_target = path.resolve(strict=False) if path.is_symlink() else None + write_path = symlink_target or path + try: + existing_stat = os.stat(write_path) + except FileNotFoundError: + existing_stat = None + mode = stat.S_IMODE(existing_stat.st_mode) if existing_stat else None + + def _verify_symlink_target() -> None: + if symlink_target is not None and path.resolve(strict=False) != symlink_target: + raise RuntimeError("config symlink target changed during atomic write") + + def _write_in_place() -> None: + with _IN_PLACE_WRITE_LOCK: + if existing_stat is None or not stat.S_ISREG(existing_stat.st_mode): + raise PermissionError(f"Cannot replace config path: {write_path}") + _verify_symlink_target() + fallback_fd = os.open(write_path, os.O_WRONLY) + owns_fallback_fd = True + try: + opened_stat = os.fstat(fallback_fd) + if (opened_stat.st_dev, opened_stat.st_ino) != ( + existing_stat.st_dev, + existing_stat.st_ino, + ): + raise PermissionError("config target changed before fallback write") + _verify_symlink_target() + os.ftruncate(fallback_fd, 0) + fallback_file = os.fdopen(fallback_fd, "w", encoding=encoding) + owns_fallback_fd = False + with fallback_file: + fallback_file.write(text) + fallback_file.flush() + os.fsync(fallback_file.fileno()) + finally: + if owns_fallback_fd: + os.close(fallback_fd) + + if existing_stat is not None and stat.S_ISREG(existing_stat.st_mode): + probed_stat = _require_writable_target(write_path) + if probed_stat is not None and stat.S_ISREG(probed_stat.st_mode): + existing_stat = probed_stat + mode = stat.S_IMODE(existing_stat.st_mode) + else: + existing_stat = probed_stat + mode = None + if existing_stat is not None and ( + existing_stat.st_nlink > 1 or _has_extended_attributes(write_path) + ): + _write_in_place() + return + + try: + fd, tmp = _create_atomic_temp_file(write_path, existing=existing_stat is not None) + except PermissionError: + _write_in_place() + return + owns_fd = True + try: + if existing_stat is not None and hasattr(os, "fchown"): + try: + os.fchown(fd, existing_stat.st_uid, existing_stat.st_gid) + except (OSError, NotImplementedError) as exc: + unsupported_ownership_errnos = { + errno.EINVAL, + errno.ENOSYS, + errno.ENOTSUP, + } + if ( + isinstance(exc, OSError) + and not isinstance(exc, PermissionError) + and exc.errno not in unsupported_ownership_errnos + ): + raise + os.close(fd) + owns_fd = False + os.unlink(tmp) + _write_in_place() + return + if mode is not None and hasattr(os, "fchmod"): + os.fchmod(fd, mode) + elif mode is not None: + os.chmod(tmp, mode) + f = os.fdopen(fd, "w", encoding=encoding) + owns_fd = False + with f: + f.write(text) + f.flush() + os.fsync(f.fileno()) + _verify_symlink_target() + os.replace(tmp, write_path) + _fsync_directory(write_path.parent) + except BaseException: + if owns_fd: + try: + os.close(fd) + except OSError: + pass + try: + os.unlink(tmp) + except OSError: + pass + raise + + def _hermes_home_has_webui_state(base: Path) -> bool: """Return True when *base* holds real WebUI state under its ``webui/`` dir. diff --git a/api/profiles.py b/api/profiles.py index 1bc80047..22668048 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -21,6 +21,7 @@ from typing import Optional import yaml +from api.paths import _atomic_write_text from api.session_events import publish_session_list_changed logger = logging.getLogger(__name__) @@ -2374,7 +2375,7 @@ def _write_endpoint_to_config(profile_dir: Path, base_url: str = None, api_key: if base_url: model_section['base_url'] = base_url cfg['model'] = model_section - config_path.write_text(_yaml.dump(cfg, default_flow_style=False, allow_unicode=True), encoding='utf-8') + _atomic_write_text(config_path, _yaml.dump(cfg, default_flow_style=False, allow_unicode=True), encoding='utf-8') def _clean_profile_config_value(value: Optional[str], field: str) -> Optional[str]: @@ -2513,7 +2514,7 @@ def _write_model_defaults_to_config( if model_provider: model_section['provider'] = model_provider cfg['model'] = model_section - config_path.write_text(_yaml.dump(cfg, default_flow_style=False, allow_unicode=True), encoding='utf-8') + _atomic_write_text(config_path, _yaml.dump(cfg, default_flow_style=False, allow_unicode=True), encoding='utf-8') def create_profile_api(name: str, clone_from: str = None, diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py new file mode 100644 index 00000000..179320ac --- /dev/null +++ b/tests/test_5774b_atomic_config_writes.py @@ -0,0 +1,823 @@ +"""Regression tests for atomic config.yaml / profile config.yaml writes. + +Pre-fix behaviour: ``_save_yaml_config_file`` (api.config) and the profile +model-config writers (api.profiles) persisted YAML via a plain +``Path.write_text``. A crash — or any exception — after ``open(..., "w")`` +truncated the target file but before the full payload was flushed left the +live ``config.yaml`` truncated / corrupt, so the next agent or WebUI start +would fail to parse it (an availability regression, not a disclosure one). + +Fix: a shared ``api.paths._atomic_write_text`` helper writes to a temp file in +the same directory, ``fsync``s, then ``os.replace``s it into place. Because +``os.replace`` is atomic, a failure at any point before the rename commits +leaves the ORIGINAL file byte-for-byte intact, and a success swaps in the new +contents in a single step. + +These tests pin the helper and all four config.yaml callers across success, +fault, metadata, symlink, permission, and concurrency paths so a future +refactor cannot silently reintroduce the truncating plain-write. +""" + +import errno +import os +import shutil +import stat +import subprocess +import sys +import threading +import time +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +import pytest + +from api.paths import _atomic_write_text + + +def test_atomic_write_replaces_contents(tmp_path: Path) -> None: + target = tmp_path / "config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + + _atomic_write_text(target, "model:\n default: new\n") + + assert target.read_text(encoding="utf-8") == "model:\n default: new\n" + # No temp files left lying around after a clean write. + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +def test_atomic_write_creates_new_file(tmp_path: Path) -> None: + target = tmp_path / "config.yaml" + assert not target.exists() + + _atomic_write_text(target, "created: true\n") + + assert target.read_text(encoding="utf-8") == "created: true\n" + + +def test_atomic_write_preserves_existing_permissions(tmp_path: Path) -> None: + """Rewriting a 0644/0664 config.yaml must not tighten it to 0600. + + ``tempfile.mkstemp`` hard-codes 0600 and ``os.replace`` carries the temp + file's mode onto the target, so without an explicit chmod every save would + silently strip group/other read from a world-readable ``config.yaml`` (the + live homelab install ships 0644, profiles 0664). config.yaml holds no + secrets, so that tightening is a real regression, not a hardening. + """ + target = tmp_path / "config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + os.chmod(target, 0o644) + + _atomic_write_text(target, "model:\n default: new\n") + + assert target.read_text(encoding="utf-8") == "model:\n default: new\n" + assert (os.stat(target).st_mode & 0o777) == 0o644 + + # A 0664 (group-writable profile config) survives its mode too. + os.chmod(target, 0o664) + _atomic_write_text(target, "model:\n default: newer\n") + assert (os.stat(target).st_mode & 0o777) == 0o664 + + # Preserve special permission bits too; replacing the inode must not + # silently discard an administrator's setgid policy on a shared config. + os.chmod(target, 0o2664) + _atomic_write_text(target, "model:\n default: newest\n") + assert stat.S_IMODE(os.stat(target).st_mode) == 0o2664 + + +@pytest.mark.skipif( + not all(hasattr(os, name) for name in ("getxattr", "listxattr", "setxattr")), + reason="extended attributes are unavailable on this platform", +) +def test_atomic_write_preserves_existing_user_xattr(tmp_path: Path) -> None: + """Replacing config contents must not discard administrator metadata.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + attribute = "user.hermes_review" + value = b"preserve-me" + try: + os.setxattr(target, attribute, value) + except OSError as exc: + if exc.errno in {errno.ENOSYS, errno.ENOTSUP, errno.EOPNOTSUPP}: + pytest.skip("test filesystem does not support user extended attributes") + raise + + _atomic_write_text(target, "new: true\n") + + assert attribute in os.listxattr(target) + assert os.getxattr(target, attribute) == value + + +@pytest.mark.skipif(not hasattr(os, "listxattr"), reason="xattr probe unavailable") +def test_unsupported_xattr_probe_keeps_atomic_replace( + tmp_path: Path, monkeypatch +) -> None: + """Filesystems without xattr support retain the normal atomic path.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + replace_calls = 0 + real_replace = os.replace + + def _unsupported_xattrs(_path) -> list[str]: + raise OSError(errno.ENOTSUP, "xattrs unsupported") + + def _recording_replace(src, dst) -> None: + nonlocal replace_calls + replace_calls += 1 + real_replace(src, dst) + + monkeypatch.setattr(os, "listxattr", _unsupported_xattrs) + monkeypatch.setattr(os, "replace", _recording_replace) + + _atomic_write_text(target, "new: true\n") + + assert target.read_text(encoding="utf-8") == "new: true\n" + assert replace_calls == 1 + + +@pytest.mark.skipif(not hasattr(os, "listxattr"), reason="xattr probe unavailable") +def test_xattr_probe_failure_does_not_silently_drop_metadata( + tmp_path: Path, monkeypatch +) -> None: + """Unexpected metadata-read failures abort before touching the old file.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + + def _denied_xattrs(_path) -> list[str]: + raise PermissionError("xattr access denied") + + monkeypatch.setattr(os, "listxattr", _denied_xattrs) + + with pytest.raises(PermissionError, match="xattr access denied"): + _atomic_write_text(target, "new: true\n") + + assert target.read_text(encoding="utf-8") == "old: true\n" + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +@pytest.mark.skipif( + not shutil.which("getfacl") or not shutil.which("setfacl"), + reason="getfacl/setfacl are unavailable", +) +def test_atomic_write_preserves_existing_posix_acl(tmp_path: Path) -> None: + """POSIX access ACLs, commonly stored as xattrs, survive a rewrite.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + subprocess.run( + ["setfacl", "-m", "u:12345:r--", str(target)], + check=True, + capture_output=True, + text=True, + ) + expected_acl = subprocess.run( + ["getfacl", "-cp", str(target)], + check=True, + capture_output=True, + text=True, + ).stdout + + _atomic_write_text(target, "new: true\n") + + actual_acl = subprocess.run( + ["getfacl", "-cp", str(target)], + check=True, + capture_output=True, + text=True, + ).stdout + assert actual_acl == expected_acl + + +@pytest.mark.skipif(not hasattr(os, "chown"), reason="POSIX ownership semantics") +def test_atomic_write_preserves_existing_group(tmp_path: Path) -> None: + """Replacing the inode must not silently reset an existing config's group.""" + supplementary_groups = [gid for gid in os.getgroups() if gid != os.getegid()] + if not supplementary_groups: + pytest.skip("requires a supplementary group distinct from the effective gid") + + target = tmp_path / "config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + expected_gid = supplementary_groups[0] + os.chown(target, -1, expected_gid) + + _atomic_write_text(target, "model:\n default: new\n") + + assert os.stat(target).st_gid == expected_gid + + +@pytest.mark.skipif(not hasattr(os, "fchown"), reason="POSIX ownership semantics") +@pytest.mark.parametrize( + "failure", + [ + PermissionError("simulated ownership denial"), + OSError(errno.EINVAL, "simulated unsupported ownership transfer"), + ], +) +def test_fchown_denial_falls_back_without_temp_debris( + tmp_path: Path, monkeypatch, failure: OSError +) -> None: + """A writer unable to transfer ownership must keep the old inode contract.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + + def _deny_fchown(*_args) -> None: + raise failure + + monkeypatch.setattr(os, "fchown", _deny_fchown) + + _atomic_write_text(target, "new: true\n") + + assert target.read_text(encoding="utf-8") == "new: true\n" + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +def test_atomic_write_without_posix_fd_metadata_helpers( + tmp_path: Path, monkeypatch +) -> None: + """Windows-like platforms still use same-directory atomic replacement.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + replace_calls: list[tuple[Path, Path]] = [] + real_replace = os.replace + + monkeypatch.delattr(os, "fchown", raising=False) + monkeypatch.delattr(os, "fchmod", raising=False) + + def _recording_replace(src, dst) -> None: + replace_calls.append((Path(src), Path(dst))) + real_replace(src, dst) + + monkeypatch.setattr(os, "replace", _recording_replace) + + _atomic_write_text(target, "new: true\n") + + assert target.read_text(encoding="utf-8") == "new: true\n" + assert len(replace_calls) == 1 + assert replace_calls[0][0].parent == target.parent + assert replace_calls[0][1] == target + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +def test_atomic_write_fsyncs_file_and_parent_directory( + tmp_path: Path, monkeypatch +) -> None: + """Durability requires syncing both payload bytes and the committed rename.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + synced_types: list[str] = [] + real_fsync = os.fsync + + def _recording_fsync(fd: int) -> None: + mode = os.fstat(fd).st_mode + synced_types.append("directory" if stat.S_ISDIR(mode) else "file") + real_fsync(fd) + + monkeypatch.setattr(os, "fsync", _recording_fsync) + + _atomic_write_text(target, "new: true\n") + + assert synced_types == ["file", "directory"] + + +def test_fdopen_failure_closes_temp_descriptor(tmp_path: Path, monkeypatch) -> None: + """A wrapper-construction failure must not leak the mkstemp descriptor.""" + target = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + captured_fd: list[int] = [] + real_mkstemp = __import__("tempfile").mkstemp + + def _capturing_mkstemp(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + captured_fd.append(fd) + return fd, name + + def _failing_fdopen(*_args, **_kwargs): + raise OSError("simulated fdopen failure") + + from api import paths + + monkeypatch.setattr(paths.tempfile, "mkstemp", _capturing_mkstemp) + monkeypatch.setattr(os, "fdopen", _failing_fdopen) + + with pytest.raises(OSError, match="simulated fdopen failure"): + _atomic_write_text(target, "new: true\n") + + assert len(captured_fd) == 1 + with pytest.raises(OSError) as exc_info: + os.fstat(captured_fd[0]) + assert exc_info.value.errno == errno.EBADF + assert target.read_text(encoding="utf-8") == "old: true\n" + + +def test_concurrent_writers_expose_only_complete_versions(tmp_path: Path) -> None: + """Concurrent saves may be last-writer-wins, but never partial or mixed.""" + target = tmp_path / "config.yaml" + original = b"model:\n default: original\n" + payloads = [ + f"model:\n default: writer-{index}\n padding: {'x' * 200_000}\n".encode() + for index in range(8) + ] + target.write_bytes(original) + valid_versions = {original, *payloads} + invalid_versions: list[bytes] = [] + reading = threading.Event() + reading.set() + + def _observe() -> None: + while reading.is_set(): + observed = target.read_bytes() + if observed not in valid_versions: + invalid_versions.append(observed) + return + time.sleep(0.001) + + observer = threading.Thread(target=_observe) + observer.start() + try: + with ThreadPoolExecutor(max_workers=len(payloads)) as pool: + list(pool.map(lambda payload: _atomic_write_text(target, payload.decode()), payloads)) + finally: + reading.clear() + observer.join() + + assert invalid_versions == [] + assert target.read_bytes() in payloads + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +def test_forced_fallback_serializes_writers_and_syncs_only_complete_versions( + tmp_path: Path, monkeypatch +) -> None: + """The non-atomic compatibility path must not interleave WebUI writers.""" + target = tmp_path / "config.yaml" + payloads = [ + f"writer: {index}\npadding: {'x' * 20_000}\n".encode() + for index in range(6) + ] + valid_versions = set(payloads) + observed_at_sync: list[bytes] = [] + active_writers = 0 + max_active_writers = 0 + state_lock = threading.Lock() + real_fdopen = os.fdopen + real_fsync = os.fsync + + from api import paths + + def _force_fallback(*_args, **_kwargs): + raise PermissionError("simulated non-writable parent") + + class _SlowWriter: + def __init__(self, wrapped): + self._wrapped = wrapped + + def __enter__(self): + nonlocal active_writers, max_active_writers + self._wrapped.__enter__() + with state_lock: + active_writers += 1 + max_active_writers = max(max_active_writers, active_writers) + return self + + def __exit__(self, *args): + nonlocal active_writers + try: + return self._wrapped.__exit__(*args) + finally: + with state_lock: + active_writers -= 1 + + def write(self, value: str) -> None: + midpoint = len(value) // 2 + self._wrapped.write(value[:midpoint]) + self._wrapped.flush() + time.sleep(0.01) + self._wrapped.write(value[midpoint:]) + + def flush(self) -> None: + self._wrapped.flush() + + def fileno(self) -> int: + return self._wrapped.fileno() + + def _slow_fdopen(fd: int, *args, **kwargs): + return _SlowWriter(real_fdopen(fd, *args, **kwargs)) + + def _observing_fsync(fd: int) -> None: + if stat.S_ISREG(os.fstat(fd).st_mode): + observed_at_sync.append(target.read_bytes()) + real_fsync(fd) + + target.write_bytes(payloads[0]) + monkeypatch.setattr(paths.tempfile, "mkstemp", _force_fallback) + monkeypatch.setattr(os, "fdopen", _slow_fdopen) + monkeypatch.setattr(os, "fsync", _observing_fsync) + + with ThreadPoolExecutor(max_workers=len(payloads)) as pool: + list(pool.map(lambda payload: _atomic_write_text(target, payload.decode()), payloads)) + + assert max_active_writers == 1 + assert len(observed_at_sync) == len(payloads) + assert all(observed in valid_versions for observed in observed_at_sync) + assert target.read_bytes() in valid_versions + + +def test_new_files_follow_the_current_umask_without_a_global_probe(tmp_path: Path) -> None: + """New config files use the kernel's current umask without mutating it. + + Importing this helper can occur on a request thread, so caching a mode by + calling ``os.umask`` at import time is still racy. A subprocess isolates the + process-global umask and proves two writes after import follow two different + active masks. + """ + script = """ +import os +import stat +import sys +from pathlib import Path +from api.paths import _atomic_write_text + +base = Path(sys.argv[1]) +os.umask(0o077) +_atomic_write_text(base / 'private.yaml', 'private: true\\n') +os.umask(0o022) +_atomic_write_text(base / 'shared.yaml', 'shared: true\\n') +print(oct(stat.S_IMODE(os.stat(base / 'private.yaml').st_mode))) +print(oct(stat.S_IMODE(os.stat(base / 'shared.yaml').st_mode))) +""" + + result = subprocess.run( + [sys.executable, "-c", script, str(tmp_path)], + check=True, + capture_output=True, + text=True, + ) + + assert result.stdout.splitlines() == ["0o600", "0o644"] + + +def test_atomic_write_follows_config_symlink(tmp_path: Path) -> None: + """Writing through a config.yaml symlink updates the target, not the link. + + ``Path.write_text`` follows symlinks. The atomic rewrite must preserve that + contract because ``HERMES_CONFIG_PATH`` and profile config paths may point at + a shared config via symlink; replacing the symlink itself would silently + sever the user's chosen config location. + """ + target_dir = tmp_path / "target" + link_dir = tmp_path / "link" + target_dir.mkdir() + link_dir.mkdir() + target = target_dir / "config.yaml" + link = link_dir / "config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + os.chmod(target, 0o644) + link.symlink_to(target) + + _atomic_write_text(link, "model:\n default: new\n") + + assert link.is_symlink() + assert os.readlink(link) == str(target) + assert target.read_text(encoding="utf-8") == "model:\n default: new\n" + assert link.read_text(encoding="utf-8") == "model:\n default: new\n" + assert (os.stat(target).st_mode & 0o777) == 0o644 + assert [p.name for p in link_dir.iterdir()] == ["config.yaml"] + + +def test_atomic_write_preserves_hard_link_aliases(tmp_path: Path) -> None: + """Path.write_text updates a shared inode instead of severing hard links.""" + target = tmp_path / "config.yaml" + alias = tmp_path / "shared-config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + os.link(target, alias) + inode = os.stat(target).st_ino + + _atomic_write_text(target, "model:\n default: new\n") + + assert target.read_text(encoding="utf-8") == "model:\n default: new\n" + assert alias.read_text(encoding="utf-8") == "model:\n default: new\n" + assert os.stat(target).st_ino == inode + assert os.stat(alias).st_ino == inode + assert os.stat(target).st_nlink == 2 + + +def test_symlink_retarget_during_write_aborts_without_touching_either_target( + tmp_path: Path, monkeypatch +) -> None: + """A concurrent symlink retarget must not commit to the stale referent.""" + old_target = tmp_path / "old.yaml" + new_target = tmp_path / "new.yaml" + link = tmp_path / "config.yaml" + old_target.write_text("old-target: true\n", encoding="utf-8") + new_target.write_text("new-target: true\n", encoding="utf-8") + link.symlink_to(old_target) + real_fsync = os.fsync + retargeted = False + + def _retarget_after_payload_sync(fd: int) -> None: + nonlocal retargeted + real_fsync(fd) + if not retargeted and stat.S_ISREG(os.fstat(fd).st_mode): + link.unlink() + link.symlink_to(new_target) + retargeted = True + + monkeypatch.setattr(os, "fsync", _retarget_after_payload_sync) + + with pytest.raises(RuntimeError, match="symlink target changed"): + _atomic_write_text(link, "replacement: true\n") + + assert link.resolve() == new_target + assert old_target.read_text(encoding="utf-8") == "old-target: true\n" + assert new_target.read_text(encoding="utf-8") == "new-target: true\n" + assert sorted(p.name for p in tmp_path.iterdir()) == [ + "config.yaml", + "new.yaml", + "old.yaml", + ] + + +def test_partial_temp_write_failure_leaves_old_file_intact( + tmp_path: Path, monkeypatch +) -> None: + """A failure after writing some temp bytes must preserve valid old YAML.""" + target = tmp_path / "config.yaml" + original = "model:\n default: keep-me\n" + target.write_text(original, encoding="utf-8") + + class _PartialWriter: + def __init__(self, fd: int): + self.fd = fd + + def __enter__(self): + return self + + def __exit__(self, *_args): + os.close(self.fd) + + def write(self, value: str) -> None: + os.write(self.fd, value[:8].encode("utf-8")) + raise OSError("simulated partial temp write") + + def _partial_fdopen(fd: int, *_args, **_kwargs): + return _PartialWriter(fd) + + monkeypatch.setattr(os, "fdopen", _partial_fdopen) + + with pytest.raises(OSError, match="simulated partial temp write"): + _atomic_write_text(target, "model:\n default: replacement\n") + + assert target.read_text(encoding="utf-8") == original + assert [p.name for p in tmp_path.iterdir()] == ["config.yaml"] + + +def test_encoding_and_newline_bytes_match_path_write_text(tmp_path: Path) -> None: + """The atomic helper preserves the prior writer's encoding/newline contract.""" + target = tmp_path / "config.yaml" + expected = tmp_path / "expected.yaml" + text = "greeting: Grüß dich\r\nnext: line\n" + expected.write_text(text, encoding="utf-16") + + _atomic_write_text(target, text, encoding="utf-16") + + assert target.read_bytes() == expected.read_bytes() + + +def test_symlink_chain_updates_final_referent(tmp_path: Path) -> None: + target = tmp_path / "real.yaml" + middle = tmp_path / "middle.yaml" + outer = tmp_path / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + middle.symlink_to(target) + outer.symlink_to(middle) + + _atomic_write_text(outer, "new: true\n") + + assert outer.is_symlink() + assert middle.is_symlink() + assert target.read_text(encoding="utf-8") == "new: true\n" + + +def test_failed_write_leaves_old_file_intact(tmp_path: Path, monkeypatch) -> None: + """A crash at the os.replace step must not touch the original file.""" + target = tmp_path / "config.yaml" + original = "model:\n default: keep-me\n" + target.write_text(original, encoding="utf-8") + + boom = RuntimeError("simulated crash mid-write") + + def _failing_replace(src, dst): + raise boom + + monkeypatch.setattr(os, "replace", _failing_replace) + + with pytest.raises(RuntimeError, match="simulated crash mid-write"): + _atomic_write_text(target, "model:\n default: half-written\n") + + # Original config survives untouched — the whole point of the fix. + assert target.read_text(encoding="utf-8") == original + # And the temp file was cleaned up rather than left as debris. + leftovers = [p.name for p in tmp_path.iterdir() if p.name != "config.yaml"] + assert leftovers == [] + + +def test_failed_write_through_symlink_leaves_link_and_target_intact( + tmp_path: Path, monkeypatch +) -> None: + """A failed symlink write must not replace the symlink or truncate target.""" + target_dir = tmp_path / "target" + link_dir = tmp_path / "link" + target_dir.mkdir() + link_dir.mkdir() + target = target_dir / "config.yaml" + link = link_dir / "config.yaml" + original = "model:\n default: keep-me\n" + target.write_text(original, encoding="utf-8") + link.symlink_to(target) + + boom = RuntimeError("simulated crash mid-write") + + def _failing_replace(src, dst): + raise boom + + monkeypatch.setattr(os, "replace", _failing_replace) + + with pytest.raises(RuntimeError, match="simulated crash mid-write"): + _atomic_write_text(link, "model:\n default: half-written\n") + + assert link.is_symlink() + assert os.readlink(link) == str(target) + assert target.read_text(encoding="utf-8") == original + assert [p.name for p in link_dir.iterdir()] == ["config.yaml"] + assert [p.name for p in target_dir.iterdir()] == ["config.yaml"] + + +@pytest.mark.parametrize( + "writer", ["main", "onboarding", "profile_endpoint", "profile_defaults"] +) +def test_each_config_writer_preserves_old_bytes_when_replace_fails( + tmp_path: Path, monkeypatch, writer: str +) -> None: + """Pin every config.yaml writer to the shared atomic failure contract.""" + original = b"model:\n default: keep-me\n" + target = tmp_path / "config.yaml" + target.write_bytes(original) + + def _failing_replace(_src, _dst): + raise OSError("simulated replace failure") + + monkeypatch.setattr(os, "replace", _failing_replace) + + with pytest.raises(OSError, match="simulated replace failure"): + if writer == "main": + from api import config + + config._save_yaml_config_file(target, {"model": {"default": "new"}}) + elif writer == "onboarding": + from api import onboarding + + onboarding._save_yaml_config( + target, {"model": {"default": "replacement-model"}} + ) + else: + from api import profiles + + if writer == "profile_endpoint": + profiles._write_endpoint_to_config(tmp_path, base_url="https://example.invalid") + else: + profiles._write_model_defaults_to_config( + tmp_path, default_model="replacement-model" + ) + + assert target.read_bytes() == original + + +_ROOT_SKIP = pytest.mark.skipif( + getattr(os, "geteuid", lambda: -1)() == 0, + reason="root bypasses directory permission bits", +) + + +@_ROOT_SKIP +def test_readonly_parent_with_writable_file_falls_back_to_write_through( + tmp_path: Path, +) -> None: + """A locked-down config dir with a writable config.yaml must still save. + + Hardened deployments (documented ``HERMES_CONFIG_PATH`` layouts) keep the + containing directory read-only while leaving ``config.yaml`` itself + writable. The old ``Path.write_text`` only needed file write permission; + ``mkstemp(dir=parent)`` needs DIRECTORY write permission, so without the + fallback every settings/onboarding/profile save would start failing there. + """ + cfg_dir = tmp_path / "locked" + cfg_dir.mkdir() + target = cfg_dir / "config.yaml" + target.write_text("model:\n default: old\n", encoding="utf-8") + os.chmod(target, 0o644) + os.chmod(cfg_dir, 0o555) + try: + _atomic_write_text(target, "model:\n default: new\n") + + assert target.read_text(encoding="utf-8") == "model:\n default: new\n" + # In-place write-through keeps the file's mode and leaves no debris. + assert (os.stat(target).st_mode & 0o777) == 0o644 + assert [p.name for p in cfg_dir.iterdir()] == ["config.yaml"] + finally: + os.chmod(cfg_dir, 0o755) + + +@_ROOT_SKIP +def test_writable_unreadable_parent_still_commits_atomically(tmp_path: Path) -> None: + """A best-effort directory fsync must not reject a writable 0333 parent.""" + cfg_dir = tmp_path / "cfg" + cfg_dir.mkdir() + target = cfg_dir / "config.yaml" + target.write_text("old: true\n", encoding="utf-8") + os.chmod(cfg_dir, 0o333) + try: + _atomic_write_text(target, "new: true\n") + assert target.read_text(encoding="utf-8") == "new: true\n" + finally: + os.chmod(cfg_dir, 0o755) + + +def test_fallback_rejects_target_swap_before_open(tmp_path: Path, monkeypatch) -> None: + """The non-atomic fallback must not overwrite a swapped-in unrelated inode.""" + target = tmp_path / "config.yaml" + victim = tmp_path / "victim.yaml" + target.write_text("original: true\n", encoding="utf-8") + victim.write_text("victim: keep\n", encoding="utf-8") + + from api import paths + + def _swap_then_deny(*_args, **_kwargs): + target.unlink() + target.symlink_to(victim) + raise PermissionError("simulated non-writable parent") + + monkeypatch.setattr(paths.tempfile, "mkstemp", _swap_then_deny) + + with pytest.raises(PermissionError, match="changed before fallback"): + _atomic_write_text(target, "victim: overwritten\n") + + assert target.is_symlink() + assert victim.read_text(encoding="utf-8") == "victim: keep\n" + + +@_ROOT_SKIP +def test_readonly_parent_without_writable_target_still_raises( + tmp_path: Path, +) -> None: + """The fallback is scoped to existing writable files, not a broad catch. + + With no target file to write through (or an unwritable one), swallowing + the ``PermissionError`` would turn a genuinely misconfigured deployment + into a silent no-op save — the error must keep propagating. + """ + cfg_dir = tmp_path / "locked" + cfg_dir.mkdir() + missing = cfg_dir / "config.yaml" + os.chmod(cfg_dir, 0o555) + try: + with pytest.raises(PermissionError): + _atomic_write_text(missing, "model:\n default: new\n") + assert not missing.exists() + finally: + os.chmod(cfg_dir, 0o755) + + +@_ROOT_SKIP +def test_writable_parent_with_readonly_file_raises_and_keeps_bytes(tmp_path: Path) -> None: + """A deliberately locked config (0444) in a writable directory must keep + rejecting writes: without the target-writability probe, atomic replace + creates a fresh temp inode and renames over the read-only file.""" + cfg_dir = tmp_path / "open-dir" + cfg_dir.mkdir() + target = cfg_dir / "config.yaml" + original = "model:\n default: keep-me\n" + target.write_text(original, encoding="utf-8") + os.chmod(target, 0o444) + os.chmod(cfg_dir, 0o755) + try: + with pytest.raises(PermissionError): + _atomic_write_text(target, "model:\n default: new\n") + assert target.read_text(encoding="utf-8") == original + assert stat.S_IMODE(os.stat(target).st_mode) == 0o444 + finally: + os.chmod(target, 0o644) + + +def test_readonly_parent_with_unwritable_file_still_raises(tmp_path: Path) -> None: + cfg_dir = tmp_path / "locked" + cfg_dir.mkdir() + target = cfg_dir / "config.yaml" + original = "model:\n default: keep-me\n" + target.write_text(original, encoding="utf-8") + os.chmod(target, 0o444) + os.chmod(cfg_dir, 0o555) + try: + with pytest.raises(PermissionError): + _atomic_write_text(target, "model:\n default: new\n") + assert target.read_text(encoding="utf-8") == original + finally: + os.chmod(cfg_dir, 0o755) + os.chmod(target, 0o644)