From 828b244031a5552ad13ada0c835957b9b54c90a6 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Wed, 8 Jul 2026 23:35:04 +0200 Subject: [PATCH 1/7] Atomic config.yaml writes to survive mid-write crashes config.yaml and profile config.yaml were persisted with a plain Path.write_text(), which truncates the target before writing. A crash (or exception) after the truncate but before the full payload was flushed left the live config truncated/corrupt, so the next agent/WebUI start failed to parse it (availability regression). Extract a shared api.paths._atomic_write_text() helper (tempfile in the same dir -> write -> flush + os.fsync -> os.replace; unlink tmp on error), mirroring the existing .env / cost-snapshot atomic pattern in api.providers, and apply it to _save_yaml_config_file (api.config) and the two profile model-config writers (api.profiles). On any mid-write failure os.replace never runs, so the original file stays byte-for-byte intact. Preserve the target's permissions: 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 tighten a group/other- readable config.yaml (the homelab install ships 0644, profiles 0664) down to owner-only. Copy the existing file's mode before the replace, falling back to the umask-adjusted 0666 for a new file. config.yaml holds no secrets, so that tightening would be a regression, not hardening (unlike .env, which stays 0600 in api.providers). settings.json is intentionally left untouched here. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/config.py | 3 +- api/paths.py | 165 +++++++ api/profiles.py | 5 +- tests/test_5774b_atomic_config_writes.py | 537 +++++++++++++++++++++++ 4 files changed, 707 insertions(+), 3 deletions(-) create mode 100644 tests/test_5774b_atomic_config_writes.py diff --git a/api/config.py b/api/config.py index 79a47994..c2e19bf8 100644 --- a/api/config.py +++ b/api/config.py @@ -761,7 +761,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/paths.py b/api/paths.py index ea00f848..2fad3194 100644 --- a/api/paths.py +++ b/api/paths.py @@ -5,12 +5,177 @@ that need the default Hermes home can import them without triggering config's larger startup side effects. """ +import errno import os +import stat +import tempfile from pathlib import Path HOME = Path.home() +def _probe_umask() -> int: + """Return the current process umask. + + ``os.umask`` has no read-only form: it always sets and returns the previous + value, so we set-then-restore. This is a process-wide syscall, so the + two-call dance is unsafe once request threads are running (another thread + creating a file in the tiny window would see umask 0). We therefore call this + exactly once, at import, while the module is still single-threaded, and cache + the derived default below. + """ + umask = os.umask(0) + os.umask(umask) + return umask + + +# umask-adjusted 0666 — the mode a plain ``open(..., "w")`` would produce for a +# brand-new file. Computed once at import (single-threaded) to avoid probing the +# process-wide umask on every new-file write; see ``_probe_umask``. +_NEW_FILE_MODE = 0o666 & ~_probe_umask() + + +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 _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 ``_NEW_FILE_MODE`` (the umask-adjusted ``0666`` a normal + ``open(..., "w")`` would have produced). + (Unlike ``.env``, ``config.yaml`` holds no secrets and is not meant to be + forced to ``0600``.) + + Symlinks keep the same follow-through semantics as ``Path.write_text``: + writing ``config.yaml`` through a symlink updates the referent instead of + replacing the link itself with a regular file. A 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. This gives up crash-atomicity only where atomic replace + with preserved metadata 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 _NEW_FILE_MODE + + 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: + 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) + + try: + fd, tmp = tempfile.mkstemp( + dir=str(write_path.parent), prefix=f".{write_path.name}_", suffix=".tmp" + ) + 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 (PermissionError, NotImplementedError): + os.close(fd) + owns_fd = False + os.unlink(tmp) + _write_in_place() + return + if hasattr(os, "fchmod"): + os.fchmod(fd, mode) + else: + 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 7f047673..fcc79b0c 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__) @@ -2262,7 +2263,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]: @@ -2401,7 +2402,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..30755700 --- /dev/null +++ b/tests/test_5774b_atomic_config_writes.py @@ -0,0 +1,537 @@ +"""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 three 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 stat +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 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 + + +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_new_file_uses_cached_umask_mode(tmp_path: Path, monkeypatch) -> None: + """A freshly created file uses _NEW_FILE_MODE (umask-adjusted 0666), not 0600. + + The mode is cached at import (the process umask is read once while the module + is single-threaded), so this pins the cached value rather than probing umask + per call. 0o644 = 0o666 & ~0o022, the common server umask. + """ + from api import paths + + monkeypatch.setattr(paths, "_NEW_FILE_MODE", 0o644) + target = tmp_path / "config.yaml" + assert not target.exists() + + _atomic_write_text(target, "created: true\n") + + assert target.exists() + assert (os.stat(target).st_mode & 0o777) == 0o644 + + +def test_probe_umask_is_read_once_at_import() -> None: + """_NEW_FILE_MODE is a plausible umask-derived file mode, cached at import.""" + from api import paths + + # A file mode: no execute/setuid bits beyond rw for the three classes, and + # never more permissive than 0o666 (umask only ever clears bits). + assert 0 <= paths._NEW_FILE_MODE <= 0o666 + assert paths._NEW_FILE_MODE & 0o111 == 0 # no execute bits on a data file + + +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_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", "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"}}) + 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( + os.geteuid() == 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_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) From d5904283ab38cad03a6166c24120d62f5eaf4e9d Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 10 Jul 2026 06:24:19 +0200 Subject: [PATCH 2/7] Cover onboarding with atomic config writes --- api/onboarding.py | 4 +- tests/test_5774b_atomic_config_writes.py | 59 +++++++++++++++++++++++- 2 files changed, 60 insertions(+), 3 deletions(-) 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/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index 30755700..a4200a3a 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -98,6 +98,52 @@ def test_atomic_write_preserves_existing_group(tmp_path: Path) -> None: assert os.stat(target).st_gid == expected_gid +@pytest.mark.skipif(not hasattr(os, "fchown"), reason="POSIX ownership semantics") +def test_fchown_denial_falls_back_without_temp_debris( + tmp_path: Path, monkeypatch +) -> 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 PermissionError("simulated ownership denial") + + 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: @@ -393,7 +439,9 @@ def test_failed_write_through_symlink_leaves_link_and_target_intact( assert [p.name for p in target_dir.iterdir()] == ["config.yaml"] -@pytest.mark.parametrize("writer", ["main", "profile_endpoint", "profile_defaults"]) +@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: @@ -412,6 +460,12 @@ def test_each_config_writer_preserves_old_bytes_when_replace_fails( 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 @@ -426,7 +480,8 @@ def test_each_config_writer_preserves_old_bytes_when_replace_fails( _ROOT_SKIP = pytest.mark.skipif( - os.geteuid() == 0, reason="root bypasses directory permission bits" + getattr(os, "geteuid", lambda: -1)() == 0, + reason="root bypasses directory permission bits", ) From c7ce0db4fa83c5981d4d74e6c906465ccf3d2279 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 10 Jul 2026 06:26:30 +0200 Subject: [PATCH 3/7] Handle unsupported ownership transfer in atomic writes --- api/paths.py | 13 ++++++++++++- tests/test_5774b_atomic_config_writes.py | 11 +++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/api/paths.py b/api/paths.py index 2fad3194..7254db26 100644 --- a/api/paths.py +++ b/api/paths.py @@ -144,7 +144,18 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non if existing_stat is not None and hasattr(os, "fchown"): try: os.fchown(fd, existing_stat.st_uid, existing_stat.st_gid) - except (PermissionError, NotImplementedError): + 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) diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index a4200a3a..bb0f06d5 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -99,15 +99,22 @@ def test_atomic_write_preserves_existing_group(tmp_path: Path) -> None: @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 + 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 PermissionError("simulated ownership denial") + raise failure monkeypatch.setattr(os, "fchown", _deny_fchown) From 297535bfba36ad6dae042c39060f12ae11de8607 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 10 Jul 2026 06:42:03 +0200 Subject: [PATCH 4/7] Preserve hard-linked configs during atomic writes --- api/paths.py | 75 +++++++++++------- tests/test_5774b_atomic_config_writes.py | 96 +++++++++++++++++++++++- 2 files changed, 141 insertions(+), 30 deletions(-) diff --git a/api/paths.py b/api/paths.py index 7254db26..e553a6de 100644 --- a/api/paths.py +++ b/api/paths.py @@ -9,6 +9,7 @@ import errno import os import stat import tempfile +import threading from pathlib import Path HOME = Path.home() @@ -34,6 +35,12 @@ def _probe_umask() -> int: # process-wide umask on every new-file write; see ``_probe_umask``. _NEW_FILE_MODE = 0o666 & ~_probe_umask() +# 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.""" @@ -76,10 +83,11 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non (Unlike ``.env``, ``config.yaml`` holds no secrets and is not meant to be forced to ``0600``.) - Symlinks keep the same follow-through semantics as ``Path.write_text``: - writing ``config.yaml`` through a symlink updates the referent instead of - replacing the link itself with a regular file. A target change observed - before commit is rejected rather than updating a stale referent. + 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 @@ -88,9 +96,13 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non 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. This gives up crash-atomicity only where atomic replace - with preserved metadata is impossible. Other ``PermissionError``s still - propagate. Successful renames fsync the parent directory where supported. + inspected above. Hard-linked files use that same path because replacing + their directory entry would break ``Path.write_text`` compatibility. 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. """ @@ -108,29 +120,34 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non raise RuntimeError("config symlink target changed during atomic write") def _write_in_place() -> None: - 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") + 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() - 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) + 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 existing_stat.st_nlink > 1: + _write_in_place() + return try: fd, tmp = tempfile.mkstemp( diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index bb0f06d5..996014d6 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -13,7 +13,7 @@ the same directory, ``fsync``s, then ``os.replace``s it into place. Because 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 three config.yaml callers across success, +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. """ @@ -238,6 +238,83 @@ def test_concurrent_writers_expose_only_complete_versions(tmp_path: Path) -> Non 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_file_uses_cached_umask_mode(tmp_path: Path, monkeypatch) -> None: """A freshly created file uses _NEW_FILE_MODE (umask-adjusted 0666), not 0600. @@ -295,6 +372,23 @@ def test_atomic_write_follows_config_symlink(tmp_path: Path) -> None: 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: From 27064338027022b0e5161c5b525c321595540b7b Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:48:32 +0200 Subject: [PATCH 5/7] fix: preserve config extended attributes --- api/paths.py | 39 +++++++-- tests/test_5774b_atomic_config_writes.py | 104 +++++++++++++++++++++++ 2 files changed, 135 insertions(+), 8 deletions(-) diff --git a/api/paths.py b/api/paths.py index e553a6de..4ec733d9 100644 --- a/api/paths.py +++ b/api/paths.py @@ -61,6 +61,24 @@ def _fsync_directory(directory: Path) -> None: 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 _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> None: """Atomically replace *path* with *text*. @@ -96,13 +114,14 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non 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 use that same path because replacing - their directory entry would break ``Path.write_text`` compatibility. 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. + 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. """ @@ -145,7 +164,11 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non if owns_fallback_fd: os.close(fallback_fd) - if existing_stat is not None and existing_stat.st_nlink > 1: + if ( + existing_stat is not None + and stat.S_ISREG(existing_stat.st_mode) + and (existing_stat.st_nlink > 1 or _has_extended_attributes(write_path)) + ): _write_in_place() return diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index 996014d6..d47d36b0 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -20,7 +20,9 @@ refactor cannot silently reintroduce the truncating plain-write. import errno import os +import shutil import stat +import subprocess import threading import time from concurrent.futures import ThreadPoolExecutor @@ -81,6 +83,108 @@ def test_atomic_write_preserves_existing_permissions(tmp_path: Path) -> None: 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.""" From 83bd7125bd8428febcdcc6b08ab108bca7da89e4 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 11:18:06 +0200 Subject: [PATCH 6/7] fix: avoid racy umask probe for config writes --- api/paths.py | 50 ++++++++++++------------ tests/test_5774b_atomic_config_writes.py | 50 +++++++++++++----------- 2 files changed, 54 insertions(+), 46 deletions(-) diff --git a/api/paths.py b/api/paths.py index 4ec733d9..7d167b10 100644 --- a/api/paths.py +++ b/api/paths.py @@ -7,6 +7,7 @@ larger startup side effects. import errno import os +import secrets import stat import tempfile import threading @@ -15,25 +16,28 @@ from pathlib import Path HOME = Path.home() -def _probe_umask() -> int: - """Return the current process umask. +def _create_atomic_temp_file(write_path: Path, *, existing: bool) -> tuple[int, str]: + """Create a same-directory temp file without reading the process umask. - ``os.umask`` has no read-only form: it always sets and returns the previous - value, so we set-then-restore. This is a process-wide syscall, so the - two-call dance is unsafe once request threads are running (another thread - creating a file in the tiny window would see umask 0). We therefore call this - exactly once, at import, while the module is still single-threaded, and cache - the derived default below. + ``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. """ - umask = os.umask(0) - os.umask(umask) - return umask + if existing: + return tempfile.mkstemp( + dir=str(write_path.parent), prefix=f".{write_path.name}_", suffix=".tmp" + ) - -# umask-adjusted 0666 — the mode a plain ``open(..., "w")`` would produce for a -# brand-new file. Computed once at import (single-threaded) to avoid probing the -# process-wide umask on every new-file write; see ``_probe_umask``. -_NEW_FILE_MODE = 0o666 & ~_probe_umask() + 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 @@ -96,8 +100,8 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non 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 ``_NEW_FILE_MODE`` (the umask-adjusted ``0666`` a normal - ``open(..., "w")`` would have produced). + 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``.) @@ -132,7 +136,7 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non existing_stat = os.stat(write_path) except FileNotFoundError: existing_stat = None - mode = stat.S_IMODE(existing_stat.st_mode) if existing_stat else _NEW_FILE_MODE + 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: @@ -173,9 +177,7 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non return try: - fd, tmp = tempfile.mkstemp( - dir=str(write_path.parent), prefix=f".{write_path.name}_", suffix=".tmp" - ) + fd, tmp = _create_atomic_temp_file(write_path, existing=existing_stat is not None) except PermissionError: _write_in_place() return @@ -201,9 +203,9 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non os.unlink(tmp) _write_in_place() return - if hasattr(os, "fchmod"): + if mode is not None and hasattr(os, "fchmod"): os.fchmod(fd, mode) - else: + elif mode is not None: os.chmod(tmp, mode) f = os.fdopen(fd, "w", encoding=encoding) owns_fd = False diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index d47d36b0..bfe2d48b 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -23,6 +23,7 @@ import os import shutil import stat import subprocess +import sys import threading import time from concurrent.futures import ThreadPoolExecutor @@ -419,33 +420,38 @@ def test_forced_fallback_serializes_writers_and_syncs_only_complete_versions( assert target.read_bytes() in valid_versions -def test_new_file_uses_cached_umask_mode(tmp_path: Path, monkeypatch) -> None: - """A freshly created file uses _NEW_FILE_MODE (umask-adjusted 0666), not 0600. +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. - The mode is cached at import (the process umask is read once while the module - is single-threaded), so this pins the cached value rather than probing umask - per call. 0o644 = 0o666 & ~0o022, the common server umask. + 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. """ - from api import paths + script = """ +import os +import stat +import sys +from pathlib import Path +from api.paths import _atomic_write_text - monkeypatch.setattr(paths, "_NEW_FILE_MODE", 0o644) - target = tmp_path / "config.yaml" - assert not target.exists() +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))) +""" - _atomic_write_text(target, "created: true\n") + result = subprocess.run( + [sys.executable, "-c", script, str(tmp_path)], + check=True, + capture_output=True, + text=True, + ) - assert target.exists() - assert (os.stat(target).st_mode & 0o777) == 0o644 - - -def test_probe_umask_is_read_once_at_import() -> None: - """_NEW_FILE_MODE is a plausible umask-derived file mode, cached at import.""" - from api import paths - - # A file mode: no execute/setuid bits beyond rw for the three classes, and - # never more permissive than 0o666 (umask only ever clears bits). - assert 0 <= paths._NEW_FILE_MODE <= 0o666 - assert paths._NEW_FILE_MODE & 0o111 == 0 # no execute bits on a data file + assert result.stdout.splitlines() == ["0o600", "0o644"] def test_atomic_write_follows_config_symlink(tmp_path: Path) -> None: From 0dc16dcce8abdc251ee4d5e9d7a223043b1582f1 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Sat, 18 Jul 2026 20:32:56 +0200 Subject: [PATCH 7/7] fix(config): restore read-only-target protection for atomic writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding 2 (10.07 gate): a deliberately locked config (0444) in a writable directory was silently overwritten — atomic replace creates a fresh temp inode and renames over the read-only file, defeating the old in-place Path.write_text PermissionError contract. Probe the existing target with a non-truncating O_WRONLY open before any replacement work and let the PermissionError propagate. The probe fstat()s the fd it actually opened and hands that stat to the rest of the write, so a concurrent writer replacing the inode between stat and probe refreshes the metadata instead of failing (keeps concurrent-writer semantics). Regression: writable parent + 0444 target now raises and keeps the original bytes and mode. Co-Authored-By: Claude Fable 5 --- api/paths.py | 42 ++++++++++++++++++++---- tests/test_5774b_atomic_config_writes.py | 20 +++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/api/paths.py b/api/paths.py index 7d167b10..358352d9 100644 --- a/api/paths.py +++ b/api/paths.py @@ -83,6 +83,28 @@ def _has_extended_attributes(path: Path) -> bool: 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*. @@ -168,13 +190,19 @@ def _atomic_write_text(path: Path, text: str, *, encoding: str = "utf-8") -> Non if owns_fallback_fd: os.close(fallback_fd) - if ( - existing_stat is not None - and stat.S_ISREG(existing_stat.st_mode) - and (existing_stat.st_nlink > 1 or _has_extended_attributes(write_path)) - ): - _write_in_place() - return + 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) diff --git a/tests/test_5774b_atomic_config_writes.py b/tests/test_5774b_atomic_config_writes.py index bfe2d48b..179320ac 100644 --- a/tests/test_5774b_atomic_config_writes.py +++ b/tests/test_5774b_atomic_config_writes.py @@ -786,6 +786,26 @@ def test_readonly_parent_without_writable_target_still_raises( @_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()