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: