diff --git a/CHANGELOG.md b/CHANGELOG.md index ebaa741..96adf28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,9 @@ and versions are tracked in the repo-root `VERSION` file. ## [Unreleased] -No unreleased changes yet. +- Keep private runtime files and directories owner-only on POSIX, use inherited + user-profile ACLs on Windows, and make history appends binary-safe across + locking backends. ## [0.2.0] - 2026-08-01 diff --git a/README.md b/README.md index 9730f08..3f088fd 100644 --- a/README.md +++ b/README.md @@ -454,9 +454,11 @@ Windows uses `%LOCALAPPDATA%` (falling back to `~/AppData/Local`). Set `BASE_CLI_CACHE_DIR` to override the default on any platform. The generic profile does not prescribe a product-wide cache name or cleanup command. -Each invocation is a run bundle containing private (`0600`) `run.json`, -`logs/`, and `tmp/`, while persistent component caches live in the -bundle's cache directory. +Each invocation is a run bundle containing a private `run.json`, `logs/`, and +`tmp/`, while persistent component caches live in the bundle's cache directory. +On POSIX, base-cli enforces owner-only `0600`/`0700` modes. On Windows, the +default user-local cache root relies on inherited user-profile ACLs; consumers +using a custom cache root must provide the appropriate ACL themselves. Use `ctx.on_cleanup()` for cleanup work that should happen even when helper code does not own the main command wrapper: diff --git a/docs/cache-ownership-and-layout.md b/docs/cache-ownership-and-layout.md index 9a8e682..b9c1ebb 100644 --- a/docs/cache-ownership-and-layout.md +++ b/docs/cache-ownership-and-layout.md @@ -26,5 +26,9 @@ Each invocation has a private run bundle containing: - `tmp/` for temporary command data. Persistent component caches live under the owner's `cache/components/` path. -Runtime directories are owner-only (`0700`), and runtime files are owner-only -(`0600`). +On POSIX systems, runtime directories are owner-only (`0700`) and runtime files +are owner-only (`0600`). On Windows, the default `%LOCALAPPDATA%` root relies +on the user-profile ACL inherited by its children; POSIX mode bits cannot +provide the same guarantee there. If `BASE_CLI_CACHE_DIR` points outside the +user profile on Windows, the consumer is responsible for supplying an +appropriately private ACL. diff --git a/lib/python/base_cli/_private_files.py b/lib/python/base_cli/_private_files.py index f7bbacb..44133d5 100644 --- a/lib/python/base_cli/_private_files.py +++ b/lib/python/base_cli/_private_files.py @@ -10,12 +10,25 @@ PRIVATE_FILE_MODE = 0o600 +PRIVATE_DIRECTORY_MODE = 0o700 def restrict_file(path: Path) -> None: - """Ensure an existing runtime file is readable and writable only by its owner.""" + """Apply owner-only POSIX permissions where mode bits are meaningful. - path.chmod(PRIVATE_FILE_MODE) + Windows inherits ACLs from the containing directory instead; the generic + package deliberately does not pretend that ``chmod`` can rewrite them. + """ + + if os.name != "nt": + path.chmod(PRIVATE_FILE_MODE) + + +def restrict_directory(path: Path) -> None: + """Apply owner-only POSIX directory permissions when supported.""" + + if os.name != "nt": + path.chmod(PRIVATE_DIRECTORY_MODE) def write_private_json(path: Path, value: Mapping[str, Any]) -> None: @@ -25,7 +38,7 @@ def write_private_json(path: Path, value: Mapping[str, Any]) -> None: fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, PRIVATE_FILE_MODE) try: fchmod = getattr(os, "fchmod", None) - if fchmod is not None: + if os.name != "nt" and fchmod is not None: fchmod(fd, PRIVATE_FILE_MODE) stream = os.fdopen(fd, "w", encoding="utf-8") fd = -1 diff --git a/lib/python/base_cli/_runtime.py b/lib/python/base_cli/_runtime.py index e1117f7..52f46a4 100644 --- a/lib/python/base_cli/_runtime.py +++ b/lib/python/base_cli/_runtime.py @@ -2,10 +2,11 @@ import json import logging +import os from dataclasses import dataclass from pathlib import Path -from ._private_files import write_private_json +from ._private_files import restrict_directory, write_private_json from .paths import runtime_run_directory_name, runtime_slug @@ -57,9 +58,9 @@ def create_runtime_directory(path: Path, cache_root: Path) -> None: restrict_permissions = _is_within(path, cache_root) try: path.mkdir(parents=True, exist_ok=True) - if restrict_permissions: + if restrict_permissions and os.name != "nt": for directory in [path, *missing]: - directory.chmod(0o700) + restrict_directory(directory) except OSError as exc: raise RuntimeError(_runtime_directory_error(path, cache_root, exc)) from exc diff --git a/lib/python/base_cli/history.py b/lib/python/base_cli/history.py index e5f502c..ac20615 100644 --- a/lib/python/base_cli/history.py +++ b/lib/python/base_cli/history.py @@ -188,13 +188,14 @@ def update_run_metadata(run_root: Path, record: dict[str, Any]) -> None: def append_history_line(path: Path, line: str) -> None: - fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_APPEND, 0o600) + binary_flag = getattr(os, "O_BINARY", 0) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_APPEND | binary_flag, 0o600) lock_fd = fd sidecar_fd: int | None = None try: if _fcntl is None and _msvcrt is not None: sidecar_path = path.with_name(f".{path.name}.lock") - sidecar_fd = os.open(sidecar_path, os.O_RDWR | os.O_CREAT, 0o600) + sidecar_fd = os.open(sidecar_path, os.O_RDWR | os.O_CREAT | binary_flag, 0o600) if os.fstat(sidecar_fd).st_size == 0: os.write(sidecar_fd, b"0") restrict_file(sidecar_path) diff --git a/lib/python/base_cli/logging.py b/lib/python/base_cli/logging.py index ece8bdb..89f5617 100644 --- a/lib/python/base_cli/logging.py +++ b/lib/python/base_cli/logging.py @@ -8,6 +8,7 @@ from pathlib import Path from typing import TextIO +from ._private_files import restrict_file from .context import get_current_context from .paths import current_working_dir from .redaction import redact_argv @@ -77,7 +78,7 @@ def _use_color(stream: TextIO) -> bool: def secure_log_file_permissions(log_file: Path) -> None: - log_file.chmod(0o600) + restrict_file(log_file) class SecureLogFileHandler(logging.FileHandler): @@ -85,7 +86,7 @@ def _open(self) -> TextIO: fd = os.open(self.baseFilename, _secure_log_file_open_flags(self.mode), 0o600) try: fchmod = getattr(os, "fchmod", None) - if fchmod is not None: + if os.name != "nt" and fchmod is not None: fchmod(fd, 0o600) return open(fd, self.mode, encoding=self.encoding, errors=self.errors, closefd=True) except BaseException: diff --git a/tests/test_history.py b/tests/test_history.py new file mode 100644 index 0000000..349af3e --- /dev/null +++ b/tests/test_history.py @@ -0,0 +1,49 @@ +from __future__ import annotations + +import json +import tempfile +import unittest +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from unittest import mock + +from base_cli import history + + +class _FakeMsvcrt: + LK_LOCK = 1 + LK_UNLCK = 2 + + def __init__(self) -> None: + self.calls: list[tuple[int, int]] = [] + + def locking(self, _fd: int, mode: int, size: int) -> None: + self.calls.append((mode, size)) + + +class HistoryAppendTests(unittest.TestCase): + def test_concurrent_appends_produce_complete_records(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + path = Path(tmpdir) / "history.jsonl" + lines = [json.dumps({"run": index}) + "\n" for index in range(24)] + + with ThreadPoolExecutor(max_workers=8) as executor: + list(executor.map(lambda line: history.append_history_line(path, line), lines)) + + records = [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines()] + + self.assertEqual(sorted(record["run"] for record in records), list(range(24))) + + def test_msvcrt_backend_uses_a_private_sidecar_lock(self) -> None: + fake_msvcrt = _FakeMsvcrt() + with tempfile.TemporaryDirectory() as tmpdir: + path = Path(tmpdir) / "history.jsonl" + with mock.patch.object(history, "_fcntl", None), mock.patch.object( + history, "_msvcrt", fake_msvcrt + ): + history.append_history_line(path, '{"run": 1}\n') + + self.assertEqual(path.read_text(encoding="utf-8"), '{"run": 1}\n') + self.assertTrue(path.with_name(".history.jsonl.lock").is_file()) + + self.assertEqual(fake_msvcrt.calls, [(_FakeMsvcrt.LK_LOCK, 1), (_FakeMsvcrt.LK_UNLCK, 1)])