From 909371e1f85fa8ba80590ab29dc2110c52cbba02 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Fri, 24 Jul 2026 15:06:10 +0530 Subject: [PATCH 1/5] fix(discovery): store env discovery cache per-user, not in shared tmp The discovery cache lived at a fixed, world-writable path in the shared temp directory (tempfile.gettempdir()/openenv_discovery_cache.json). On a multi-user host another local user could pre-create that file with attacker-chosen client_module_path/client_class_name; the victim's discover(use_cache=True) would load it and later importlib.import_module() the attacker-named module (CWE-377 insecure temp file + CWE-427 uncontrolled load path). - Move the cache to a per-user directory ($XDG_CACHE_HOME or ~/.cache/openenv). - Ignore a cache file not owned by the current user or writable by group/others (POSIX) before loading it. - Write the cache with 0600 permissions. Adds regression tests covering the per-user location, the ownership/permission guard, and that a world-writable planted cache is ignored. --- src/openenv/auto/_discovery.py | 59 ++++++++++++++++++++++++- tests/envs/test_discovery.py | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/src/openenv/auto/_discovery.py b/src/openenv/auto/_discovery.py index a41c0f802c..abfc83a910 100644 --- a/src/openenv/auto/_discovery.py +++ b/src/openenv/auto/_discovery.py @@ -20,8 +20,9 @@ import importlib.resources import json import logging +import os import re -import tempfile +import stat from dataclasses import asdict, dataclass from pathlib import Path from typing import Any, Type @@ -336,6 +337,46 @@ def _create_env_info_from_package( ) +def _default_cache_file() -> Path: + """ + Return the per-user discovery cache file path. + + Uses a per-user cache directory (``$XDG_CACHE_HOME`` or ``~/.cache``) rather than a + shared, world-writable temporary directory. A fixed path under the shared temp dir + lets another local user pre-create the cache file and redirect discovery to + attacker-controlled import paths (`import_module` on a cached `client_module_path`). + """ + base = os.environ.get("XDG_CACHE_HOME") + root = Path(base) if base else Path.home() / ".cache" + return root / "openenv" / "discovery_cache.json" + + +def _is_trusted_cache_file(path: Path) -> bool: + """ + Return whether *path* is safe to load. + + On POSIX the file must be owned by the current user and not writable by group or + others, so a cache file planted by another user is ignored rather than trusted. + + Args: + path (`Path`): + The cache file to check. + + Returns: + `bool`: `True` if the file is owned by the current user and not + group/other-writable (always `True` on non-POSIX platforms). + """ + if os.name != "posix": + return True + try: + info = path.stat() + except OSError: + return False + return info.st_uid == os.getuid() and not ( + info.st_mode & (stat.S_IWGRP | stat.S_IWOTH) + ) + + class EnvironmentDiscovery: """ Auto-discovery system for OpenEnv environments using installed packages. @@ -346,7 +387,7 @@ class EnvironmentDiscovery: def __init__(self): """Initialize discovery system.""" self._cache: dict[str, EnvironmentInfo] | None = None - self._cache_file = Path(tempfile.gettempdir()) / "openenv_discovery_cache.json" + self._cache_file = _default_cache_file() def _discover_installed_packages(self) -> dict[str, EnvironmentInfo]: """ @@ -411,6 +452,16 @@ def _load_cache(self) -> dict[str, EnvironmentInfo] | None: if not self._cache_file.exists(): return None + # Only trust a cache file owned by the current user. This prevents another + # local user from planting a file that would redirect discovery (and the + # subsequent import_module) to attacker-controlled modules/classes. + if not _is_trusted_cache_file(self._cache_file): + logger.warning( + f"Ignoring discovery cache {self._cache_file}: not owned by the " + "current user or writable by group/others." + ) + return None + try: with open(self._cache_file, "r") as f: cache_data = json.load(f) @@ -437,8 +488,12 @@ def _save_cache(self, environments: dict[str, EnvironmentInfo]) -> None: for env_key, env_info in environments.items(): cache_data[env_key] = asdict(env_info) + self._cache_file.parent.mkdir(parents=True, exist_ok=True) with open(self._cache_file, "w") as f: json.dump(cache_data, f, indent=2) + # Restrict to the owner so the cache cannot be tampered with by others. + if os.name == "posix": + os.chmod(self._cache_file, 0o600) except Exception as e: logger.warning(f"Failed to save discovery cache: {e}") diff --git a/tests/envs/test_discovery.py b/tests/envs/test_discovery.py index d2ff248929..89baef8b2a 100644 --- a/tests/envs/test_discovery.py +++ b/tests/envs/test_discovery.py @@ -12,12 +12,18 @@ 5. Helper functions (_normalize_env_name, _is_hub_url, etc.) """ +import json +import os +import stat +import tempfile from unittest.mock import Mock, patch from openenv.auto._discovery import ( _create_env_info_from_package, + _default_cache_file, _infer_class_name, _is_hub_url, + _is_trusted_cache_file, _normalize_env_name, EnvironmentDiscovery, EnvironmentInfo, @@ -304,6 +310,80 @@ def test_cache_management(self): assert not discovery._cache_file.exists() +class TestCacheSecurity: + """The discovery cache must not be plantable/redirectable by another local user. + + A world-writable shared path let a local attacker pre-create the cache and + redirect discovery's later ``import_module`` to attacker-chosen modules/classes + (CWE-377 insecure temp file + CWE-427 uncontrolled load path). + """ + + def test_cache_file_is_per_user_not_shared_tmp(self): + path = _default_cache_file() + assert tempfile.gettempdir() not in str(path) + assert path.parent.name == "openenv" + + def test_world_writable_cache_is_not_trusted(self, tmp_path): + f = tmp_path / "cache.json" + f.write_text("{}") + os.chmod(f, 0o666) + # Rejected on POSIX; non-POSIX has no ownership model so it stays trusted. + assert _is_trusted_cache_file(f) is (os.name != "posix") + + def test_owner_only_cache_is_trusted(self, tmp_path): + f = tmp_path / "cache.json" + f.write_text("{}") + os.chmod(f, 0o600) + assert _is_trusted_cache_file(f) is True + + def test_load_cache_ignores_world_writable_file(self, tmp_path): + planted = tmp_path / "cache.json" + planted.write_text( + json.dumps( + { + "evil": { + "env_key": "evil", + "name": "evil", + "package_name": "openenv-evil", + "version": "1.0.0", + "description": "", + "client_module_path": "os", + "client_class_name": "system", + "action_class_name": "A", + "observation_class_name": "O", + "default_image": "i", + } + } + ) + ) + os.chmod(planted, 0o666) + discovery = EnvironmentDiscovery() + discovery._cache_file = planted + if os.name == "posix": + assert discovery._load_cache() is None + + def test_saved_cache_is_owner_only(self, tmp_path): + discovery = EnvironmentDiscovery() + discovery._cache_file = tmp_path / "sub" / "cache.json" + env = EnvironmentInfo( + env_key="t", + name="t", + package_name="openenv-t", + version="1.0.0", + description="", + client_module_path="t.client", + client_class_name="T", + action_class_name="A", + observation_class_name="O", + default_image="i", + ) + discovery._save_cache({"t": env}) + assert discovery._cache_file.exists() + if os.name == "posix": + assert stat.S_IMODE(discovery._cache_file.stat().st_mode) == 0o600 + assert discovery._load_cache() is not None + + class TestGlobalDiscovery: """Test global discovery instance management.""" From ed5da1e07802330509e9945b4781a8f6c92aa30e Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Thu, 10 Sep 2026 20:06:08 +0530 Subject: [PATCH 2/5] fix(discovery): vet the descriptor the cache is read from The ownership check and the read resolved the path separately, so the file that was vetted was not provably the file that was loaded. An attacker able to write in the cache directory could replace the checked file with a symlink before the open, and the read would follow it. Open with `O_NOFOLLOW` and `fstat` the resulting descriptor instead, so the check and the read apply to the same object: a symlink is refused outright and ownership is read from the descriptor rather than from a second path lookup. `_is_trusted_stat` now holds the predicate so both entry points share it. `_save_cache` had the same shape in reverse: `open()` applies the umask, so creating the file and calling `chmod` afterwards leaves a window in which the cache is world-readable. The descriptor is now created with mode 0600 directly, and refuses to follow a symlink on the way out too. Both paths are covered by tests that fail without this change: a symlinked cache must be refused, and saving must produce an owner-only file with `os.chmod` made to raise. This addresses the review note on the earlier revision. The precondition is a cache directory an attacker can write to, so it hardens the fix rather than closing a regression it introduced. --- src/openenv/auto/_discovery.py | 100 ++++++++++++++++++++++++++------- tests/envs/test_discovery.py | 83 +++++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 20 deletions(-) diff --git a/src/openenv/auto/_discovery.py b/src/openenv/auto/_discovery.py index abfc83a910..60f851df9b 100644 --- a/src/openenv/auto/_discovery.py +++ b/src/openenv/auto/_discovery.py @@ -351,13 +351,36 @@ def _default_cache_file() -> Path: return root / "openenv" / "discovery_cache.json" -def _is_trusted_cache_file(path: Path) -> bool: +def _is_trusted_stat(info: os.stat_result) -> bool: """ - Return whether *path* is safe to load. + Return whether the file *info* describes is safe to load. On POSIX the file must be owned by the current user and not writable by group or others, so a cache file planted by another user is ignored rather than trusted. + Args: + info (`os.stat_result`): + Metadata for the file being considered. + + Returns: + `bool`: `True` if the file is owned by the current user and not + group/other-writable (always `True` on non-POSIX platforms). + """ + if os.name != "posix": + return True + return info.st_uid == os.getuid() and not ( + info.st_mode & (stat.S_IWGRP | stat.S_IWOTH) + ) + + +def _is_trusted_cache_file(path: Path) -> bool: + """ + Return whether *path* is safe to load. + + Prefer `_open_trusted_cache`, which checks the descriptor it hands back. This + variant resolves the path a second time, so on its own it cannot promise that + the file inspected is the file later read. + Args: path (`Path`): The cache file to check. @@ -369,12 +392,44 @@ def _is_trusted_cache_file(path: Path) -> bool: if os.name != "posix": return True try: - info = path.stat() + return _is_trusted_stat(path.stat()) except OSError: return False - return info.st_uid == os.getuid() and not ( - info.st_mode & (stat.S_IWGRP | stat.S_IWOTH) - ) + + +def _open_trusted_cache(path: Path) -> int | None: + """ + Open *path* for reading and return the descriptor only if it is trustworthy. + + Checking a path and then opening it are two separate resolutions, and an + attacker who can write in the cache directory can swap the file for a symlink + in between, so the file that was vetted is not the file that gets read. + ``O_NOFOLLOW`` refuses a symlink outright and ``fstat`` inspects the descriptor + itself, which makes the ownership check and the read the same object. + + Args: + path (`Path`): + The cache file to open. + + Returns: + `int` or `None`: an open read-only descriptor the caller must close, or + `None` if the file is missing, is a symlink, or is not owned by the + current user. + """ + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + try: + fd = os.open(path, flags) + except OSError: + # Missing, unreadable, or a symlink (ELOOP under O_NOFOLLOW). + return None + try: + if not _is_trusted_stat(os.fstat(fd)): + os.close(fd) + return None + except OSError: + os.close(fd) + return None + return fd class EnvironmentDiscovery: @@ -449,21 +504,22 @@ def _load_cache(self) -> dict[str, EnvironmentInfo] | None: Returns: Dictionary of env_key -> EnvironmentInfo, or None if cache invalid """ - if not self._cache_file.exists(): - return None - # Only trust a cache file owned by the current user. This prevents another # local user from planting a file that would redirect discovery (and the - # subsequent import_module) to attacker-controlled modules/classes. - if not _is_trusted_cache_file(self._cache_file): - logger.warning( - f"Ignoring discovery cache {self._cache_file}: not owned by the " - "current user or writable by group/others." - ) + # subsequent import_module) to attacker-controlled modules/classes. The + # descriptor is what gets vetted and then read, so the file cannot be + # swapped for a symlink after the check. + fd = _open_trusted_cache(self._cache_file) + if fd is None: + if self._cache_file.exists(): + logger.warning( + f"Ignoring discovery cache {self._cache_file}: not owned by " + "the current user, writable by group/others, or a symlink." + ) return None try: - with open(self._cache_file, "r") as f: + with os.fdopen(fd, "r") as f: cache_data = json.load(f) # Reconstruct EnvironmentInfo objects @@ -489,11 +545,15 @@ def _save_cache(self, environments: dict[str, EnvironmentInfo]) -> None: cache_data[env_key] = asdict(env_info) self._cache_file.parent.mkdir(parents=True, exist_ok=True) - with open(self._cache_file, "w") as f: + # Create the file owner-only rather than writing it and narrowing the + # mode afterwards: `open()` applies the umask, so a `chmod` after the + # fact leaves a window in which the cache is world-readable. Refuse to + # follow a symlink here too, so a planted link cannot redirect the + # write. + flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0) + fd = os.open(self._cache_file, flags, 0o600) + with os.fdopen(fd, "w") as f: json.dump(cache_data, f, indent=2) - # Restrict to the owner so the cache cannot be tampered with by others. - if os.name == "posix": - os.chmod(self._cache_file, 0o600) except Exception as e: logger.warning(f"Failed to save discovery cache: {e}") diff --git a/tests/envs/test_discovery.py b/tests/envs/test_discovery.py index 89baef8b2a..8896735afe 100644 --- a/tests/envs/test_discovery.py +++ b/tests/envs/test_discovery.py @@ -383,6 +383,89 @@ def test_saved_cache_is_owner_only(self, tmp_path): assert stat.S_IMODE(discovery._cache_file.stat().st_mode) == 0o600 assert discovery._load_cache() is not None + def test_symlinked_cache_is_refused(self, tmp_path): + """The trust check must apply to the object actually read. + + Checking the path and then opening it separately leaves a window: an + attacker who can write in the cache directory swaps the verified file + for a symlink before the read. Opening with ``O_NOFOLLOW`` and + inspecting the resulting descriptor closes it, so a symlink is refused + outright rather than followed to a file that was never checked. + """ + if os.name != "posix": + return + + target = tmp_path / "attacker.json" + target.write_text( + json.dumps( + { + "evil": { + "env_key": "evil", + "name": "evil", + "package_name": "openenv-evil", + "version": "1.0.0", + "description": "", + "client_module_path": "os", + "client_class_name": "system", + "action_class_name": "A", + "observation_class_name": "O", + "default_image": "i", + } + } + ) + ) + os.chmod(target, 0o600) + + link = tmp_path / "cache.json" + link.symlink_to(target) + + discovery = EnvironmentDiscovery() + discovery._cache_file = link + assert discovery._load_cache() is None + + def test_save_does_not_rely_on_a_follow_up_chmod(self, tmp_path): + """The cache must be created owner-only, not widened then narrowed. + + ``open()`` honours the umask, so creating the file and calling + ``chmod`` afterwards leaves a window in which the cache is + world-readable. Creating the descriptor with the mode already set + removes it; `os.chmod` is made to fail here so the test only passes if + nothing depends on it. + """ + if os.name != "posix": + return + + discovery = EnvironmentDiscovery() + discovery._cache_file = tmp_path / "sub" / "cache.json" + env = EnvironmentInfo( + env_key="t", + name="t", + package_name="openenv-t", + version="1.0.0", + description="", + client_module_path="t.client", + client_class_name="T", + action_class_name="A", + observation_class_name="O", + default_image="i", + ) + + real_umask = os.umask(0) + real_chmod = os.chmod + + def _no_chmod(*args, **kwargs): + raise AssertionError("cache permissions must not depend on chmod") + + os.chmod = _no_chmod + try: + discovery._save_cache({"t": env}) + finally: + os.chmod = real_chmod + os.umask(real_umask) + + assert discovery._cache_file.exists() + assert stat.S_IMODE(discovery._cache_file.stat().st_mode) == 0o600 + class TestGlobalDiscovery: """Test global discovery instance management.""" From d5e81634ce1109cd2646255986eae8bcdb3115f6 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Tue, 15 Sep 2026 14:35:41 +0530 Subject: [PATCH 3/5] test(discovery): keep the suite off the real per-user cache Moving the cache out of the shared temp directory changed what the existing cache tests touch. `test_cache_management` builds a default-constructed `EnvironmentDiscovery` and calls `_save_cache` and `clear_cache` on it, and `reset_discovery()` clears the cache of the global instance. Against the old path those only disturbed a throwaway file in /tmp. Against the new one they write to, and delete, the cache belonging to whoever runs the suite. Reproduced by planting a cache and running the file: planted: 0efa24af0d30 after run: DELETED An autouse fixture now redirects the default path into `tmp_path`, so no test reaches the real cache. Same check after the fixture: planted: 0efa24af0d30 after run: survived, 0efa24af0d30 The fixture patches the module attribute that `EnvironmentDiscovery` looks up, so `test_cache_file_is_per_user_not_shared_tmp` keeps calling the real `_default_cache_file` and still asserts the per-user location. Reported by Bugbot on the merged head. Tests only; the fix itself is unchanged. --- tests/envs/test_discovery.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/envs/test_discovery.py b/tests/envs/test_discovery.py index 8896735afe..08aafb05ff 100644 --- a/tests/envs/test_discovery.py +++ b/tests/envs/test_discovery.py @@ -18,6 +18,8 @@ import tempfile from unittest.mock import Mock, patch +import openenv.auto._discovery as _discovery_module +import pytest from openenv.auto._discovery import ( _create_env_info_from_package, _default_cache_file, @@ -32,6 +34,24 @@ ) +@pytest.fixture(autouse=True) +def _isolate_discovery_cache(tmp_path, monkeypatch): + """Keep every test off the real per-user discovery cache. + + The cache used to live in the shared temp directory, so tests that + exercised `_save_cache`/`clear_cache` against a default-constructed + `EnvironmentDiscovery` only ever disturbed a throwaway file. Now that it + lives under `$XDG_CACHE_HOME`, the same tests would write to, and + `reset_discovery()` would delete, the cache belonging to whoever runs the + suite. Redirecting the default path keeps that entirely inside `tmp_path`. + """ + monkeypatch.setattr( + _discovery_module, + "_default_cache_file", + lambda: tmp_path / "openenv" / "discovery_cache.json", + ) + + class TestEnvironmentInfo: """Test EnvironmentInfo dataclass and methods.""" From 7a6533f5d82cd392c1efea2870ea7294a15036f0 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Tue, 15 Sep 2026 14:41:27 +0530 Subject: [PATCH 4/5] fix(discovery): never block on the cache, never inherit its mode Two problems with the previous revision, both found by review automation and both reproduced before changing anything. Opening the cache blocked before the descriptor could be judged. A read-only open of a FIFO waits for a writer, so a planted FIFO in the cache directory hung discovery indefinitely and the ownership check never ran: FIFO: _open_trusted_cache still blocked after 2s? True The open is now non-blocking on POSIX and only a regular file is accepted, so a FIFO is refused rather than waited on. `O_NONBLOCK` has no effect on the regular files this is meant to read. The 0600 passed to `os.open` also only applied when the call created the file. Writing through an existing group/world-writable cache kept that mode and published the data just written: pre-existing mode before=0o666 after _save_cache=0o666 This was a regression against the revision before it, which called `os.chmod` unconditionally and would have narrowed such a file. The cache is now written to a fresh `O_EXCL` file created 0600 and moved into place with `os.replace`, which fixes the mode for an existing destination and makes the write atomic, so a reader never sees a half-written cache if serialization fails part way. A failed write removes the temporary file. Both are covered by tests that fail without this change. The FIFO test runs the open on a thread and asserts it finished, so a regression cannot hang the suite. --- src/openenv/auto/_discovery.py | 45 ++++++++++++++++++++--------- tests/envs/test_discovery.py | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/openenv/auto/_discovery.py b/src/openenv/auto/_discovery.py index 60f851df9b..e16e6f9e2c 100644 --- a/src/openenv/auto/_discovery.py +++ b/src/openenv/auto/_discovery.py @@ -413,17 +413,22 @@ def _open_trusted_cache(path: Path) -> int | None: Returns: `int` or `None`: an open read-only descriptor the caller must close, or - `None` if the file is missing, is a symlink, or is not owned by the - current user. + `None` if the file is missing, is a symlink, is not a regular file, or + is not owned by the current user. """ - flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + # O_NONBLOCK matters before the descriptor can be inspected: opening a FIFO + # read-only otherwise waits for a writer that an attacker simply never + # provides, so discovery hangs before any check runs. It has no effect on a + # regular file, which is the only thing accepted here anyway. + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_NONBLOCK", 0) try: fd = os.open(path, flags) except OSError: # Missing, unreadable, or a symlink (ELOOP under O_NOFOLLOW). return None try: - if not _is_trusted_stat(os.fstat(fd)): + info = os.fstat(fd) + if not stat.S_ISREG(info.st_mode) or not _is_trusted_stat(info): os.close(fd) return None except OSError: @@ -545,15 +550,29 @@ def _save_cache(self, environments: dict[str, EnvironmentInfo]) -> None: cache_data[env_key] = asdict(env_info) self._cache_file.parent.mkdir(parents=True, exist_ok=True) - # Create the file owner-only rather than writing it and narrowing the - # mode afterwards: `open()` applies the umask, so a `chmod` after the - # fact leaves a window in which the cache is world-readable. Refuse to - # follow a symlink here too, so a planted link cannot redirect the - # write. - flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0) - fd = os.open(self._cache_file, flags, 0o600) - with os.fdopen(fd, "w") as f: - json.dump(cache_data, f, indent=2) + # Write a new file and move it into place rather than writing + # through whatever already sits at the destination. A mode passed to + # `os.open` only applies when it creates the file, so truncating an + # existing group/world-writable cache would keep that mode and + # publish what was just written; and `os.replace` is atomic, so a + # reader never observes a half-written cache when serialization + # fails part way. `O_EXCL` requires the temporary name to be new, + # and 0600 is applied at creation so the umask cannot widen it. + tmp_path = self._cache_file.with_name( + f"{self._cache_file.name}.{os.getpid()}.tmp" + ) + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0) + fd = os.open(tmp_path, flags, 0o600) + try: + with os.fdopen(fd, "w") as f: + json.dump(cache_data, f, indent=2) + os.replace(tmp_path, self._cache_file) + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise except Exception as e: logger.warning(f"Failed to save discovery cache: {e}") diff --git a/tests/envs/test_discovery.py b/tests/envs/test_discovery.py index 08aafb05ff..2e0cb012f5 100644 --- a/tests/envs/test_discovery.py +++ b/tests/envs/test_discovery.py @@ -27,6 +27,7 @@ _is_hub_url, _is_trusted_cache_file, _normalize_env_name, + _open_trusted_cache, EnvironmentDiscovery, EnvironmentInfo, get_discovery, @@ -403,6 +404,10 @@ def test_saved_cache_is_owner_only(self, tmp_path): assert stat.S_IMODE(discovery._cache_file.stat().st_mode) == 0o600 assert discovery._load_cache() is not None + @pytest.fixture(autouse=True) + def _tmp(self, tmp_path): + self.tmp = tmp_path + def test_symlinked_cache_is_refused(self, tmp_path): """The trust check must apply to the object actually read. @@ -486,6 +491,54 @@ def _no_chmod(*args, **kwargs): assert discovery._cache_file.exists() assert stat.S_IMODE(discovery._cache_file.stat().st_mode) == 0o600 + def test_a_fifo_cache_does_not_block_discovery(self): + """A planted FIFO must be refused, not waited on. + + Opening a FIFO read-only blocks until a writer appears, and that + happens before the descriptor can be inspected, so a trust check that + runs after the open never gets to reject it. The open must not block, + and only a regular file is acceptable. + """ + if os.name != "posix": + return + + import threading + + fifo = self.tmp / "fifo.json" + os.mkfifo(fifo, 0o600) + + outcome = {} + + def attempt(): + outcome["fd"] = _open_trusted_cache(fifo) + + worker = threading.Thread(target=attempt, daemon=True) + worker.start() + worker.join(timeout=5.0) + + assert not worker.is_alive(), "opening a FIFO cache blocked" + assert outcome["fd"] is None + + def test_saving_over_a_world_writable_file_does_not_keep_its_mode(self): + """`O_CREAT` only applies the mode when it creates the file. + + Writing through an existing inode therefore leaves whatever mode that + file already had, so a cache that is already group/world-writable stays + that way and the data just written is readable by everyone. + """ + if os.name != "posix": + return + + existing = self.tmp / "cache.json" + existing.write_text("{}") + os.chmod(existing, 0o666) + + discovery = EnvironmentDiscovery() + discovery._cache_file = existing + discovery._save_cache({}) + + assert stat.S_IMODE(existing.stat().st_mode) == 0o600 + class TestGlobalDiscovery: """Test global discovery instance management.""" From 155e6e6de1481e61e18b5b22467a3d6dbd2392a9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 15 Sep 2026 09:54:17 +0000 Subject: [PATCH 5/5] fix(discovery): reset_discovery must not delete the on-disk cache Persistent per-user cache means singleton reset for tests cannot call clear_cache(); otherwise suites like test_auto_env delete the developer's real ~/.cache file (Bugbot on #1167). Co-authored-by: benjamin.burtenshaw --- src/openenv/auto/_discovery.py | 10 +++++++--- tests/envs/test_discovery.py | 24 ++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/openenv/auto/_discovery.py b/src/openenv/auto/_discovery.py index e16e6f9e2c..7b53fc6bcc 100644 --- a/src/openenv/auto/_discovery.py +++ b/src/openenv/auto/_discovery.py @@ -709,8 +709,12 @@ def get_discovery() -> EnvironmentDiscovery: def reset_discovery() -> None: - """Reset the global discovery instance (useful for testing).""" + """Drop the global discovery singleton (useful for testing). + + Does **not** delete the on-disk cache. The cache is now a persistent + per-user file under `$XDG_CACHE_HOME`; wiping it belongs on an explicit + [`EnvironmentDiscovery.clear_cache`][] call so test fixtures that only + need a fresh singleton cannot clobber a developer's real cache. + """ global _global_discovery - if _global_discovery is not None: - _global_discovery.clear_cache() _global_discovery = None diff --git a/tests/envs/test_discovery.py b/tests/envs/test_discovery.py index 2e0cb012f5..ff1d3890bd 100644 --- a/tests/envs/test_discovery.py +++ b/tests/envs/test_discovery.py @@ -42,8 +42,8 @@ def _isolate_discovery_cache(tmp_path, monkeypatch): The cache used to live in the shared temp directory, so tests that exercised `_save_cache`/`clear_cache` against a default-constructed `EnvironmentDiscovery` only ever disturbed a throwaway file. Now that it - lives under `$XDG_CACHE_HOME`, the same tests would write to, and - `reset_discovery()` would delete, the cache belonging to whoever runs the + lives under `$XDG_CACHE_HOME`, the same tests would write to (and + `clear_cache()` would delete) the cache belonging to whoever runs the suite. Redirecting the default path keeps that entirely inside `tmp_path`. """ monkeypatch.setattr( @@ -563,6 +563,26 @@ def test_reset_discovery(self): # Should be different instances after reset assert discovery1 is not discovery2 + def test_reset_discovery_does_not_unlink_disk_cache(self, tmp_path, monkeypatch): + """Singleton reset must not delete the persistent per-user cache. + + Regression for Bugbot on #1167: `reset_discovery()` used to call + `clear_cache()`, and suites that only needed a fresh singleton + (e.g. `test_auto_env.py`) would delete `~/.cache/openenv/...`. + """ + cache = tmp_path / "discovery_cache.json" + cache.write_text("{}") + monkeypatch.setattr(_discovery_module, "_default_cache_file", lambda: cache) + reset_discovery() + + discovery = get_discovery() + assert discovery._cache_file == cache + + reset_discovery() + + assert cache.exists(), "reset_discovery() must not delete the on-disk cache" + assert cache.read_text() == "{}" + class TestListEnvironments: """Test list_environments output."""