fix: avoid racy umask probe for config writes

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

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: