Repository navigation
fix(discovery): per-user cache with trusted open/atomic save (from #1011) - #1167
Conversation
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.
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.
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.
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.
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
burtenshaw
added
bug
Cursor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| with os.fdopen(fd, "w") as f: | ||
| json.dump(cache_data, f, indent=2) | ||
| os.replace(tmp_path, self._cache_file) | ||
| except BaseException: |
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: |
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alignment Review Report
Scope: the trigger's 3-dot base f941d82a is exactly this branch's merge-base with main, so the diff equals GitHub's "Files changed": 2 files — src/openenv/auto/_discovery.py (+146) and tests/envs/test_discovery.py (+236). The change hardens the local EnvironmentDiscovery cache against a shared-/tmp planting attack (CWE-377 insecure temp file → CWE-427 uncontrolled load path, since _load_cache feeds a cached client_module_path into import_module).
Automated Checks
- Lint: PASS (for this PR).
ruff format --check,ruff check src/ tests/, andusort checkall pass on both changed files.lint.shexits 1, but only on ~26 pre-existingenvs/**files (agent_world_model, chat_env, coding_tools_env, jupyter_env, opencode_env, pi_env, repl_env, terminus_env, textarena_env) that CI never lints — none are touched here. - Debug code: CLEAN.
check-debug.shflags only pre-existingprint(...)inlist_environmentsand>>>docstring examples; nothing on this PR's added lines, and no new prints/TODOs/breakpoints. - Tests: 31/31 pass, including the 9 new
TestCacheSecuritycases (world-writable rejected, owner-only trusted, symlink refused viaO_NOFOLLOW, FIFO non-blocking viaO_NONBLOCK, atomic owner-only save, no reliance on a follow-upchmod, no mode inheritance when replacing a world-writable file).
Open RFCs Context
- RFC 011 — ARD-backed catalog discovery (Draft, @thegovind) is the discovery-area RFC. It explicitly does not change
AutoEnv, listsEnvironmentDiscoveryas a separate existing surface, and its Security section requires that "credentials and caches are scoped to the caller and configured origin." This per-user cache move supports that direction — no conflict. - RFC 008 — auto-validation (In Review): not relevant to the cache-security surface.
Tier 1: Fixes Required
None. The change is mechanically clean (passes all CI-scope checks + tests). The security design is sound: descriptor-based trust (os.open(..., O_NOFOLLOW|O_NONBLOCK) + fstat + S_ISREG + owner/perm check) closes the check-vs-read TOCTOU, and the save path is atomic (O_CREAT|O_EXCL, mode 0600, os.replace) so it never inherits a planted file's mode nor publishes a half-written cache.
Tier 2: Alignment Discussion
Principle Conflicts
None identified. Pure security hardening; aligns with PRINCIPLES.md "Container isolation for reproducibility and security" (security intent) and touches none of the Gym API / rewards-in-env / client-server / MCP-WS / agent-isolation surfaces.
RFC Conflicts
None identified. Complementary to RFC 011 (see context above); it reinforces that RFC's caller-scoped-cache security note rather than pre-empting any decision in it.
Non-blocking nits (optional polish; see inline)
_default_cache_file: emptyXDG_CACHE_HOMEis handled fine (falsy →~/.cache), but a relative value isn't ignored as the XDG spec requires. Low impact — the descriptor trust +O_EXCL/0600save protect the file wherever it lands._save_cache: the temp name keys onos.getpid(); a stale same-PID*.tmp(after a crash) or two threads in one process collide underO_EXCLand silently skip that save (best-effort + logged).tempfile.mkstemp(...)gives the sameO_EXCL/0600guarantees with guaranteed uniqueness._is_trusted_cache_fileis now referenced only by tests — the load path uses_open_trusted_cache(as its own docstring recommends). Consider dropping it or marking it test-only so the TOCTOU-prone shape isn't reached for later.
Awareness (behavioral, pre-existing)
Moving the cache from ephemeral /tmp to persistent ~/.cache lengthens the staleness window: discover(use_cache=True) returns the file cache without re-checking installed packages, so a newly-installed env won't appear until clear_cache() / reset_discovery() / use_cache=False. Pre-existing behavior, just more persistent now — a TTL or package-set signature might be a worthwhile follow-up (likely out of scope here).
Freshness FYI
The branch's merge-base is f941d82a; origin/main has since advanced to e4cbca68 (docs + tests/test_core/test_generic_client.py), none of which touch _discovery.py/test_discovery.py. git merge-tree reports no conflict, so it's cleanly mergeable — a rebase/merge before landing is just hygiene.
Summary
- 0 mechanical issues to fix (3 optional non-blocking nits + 1 awareness note)
- 0 alignment points for human review
- 0 RFC conflicts to discuss
Suggested reviewers: @Darktex (Security-invariants author + top _discovery.py committer), cc @thegovind (RFC 011 discovery area) and @burtenshaw (top _discovery.py committer).
Sent by Cursor Automation: Pre-review
| base = os.environ.get("XDG_CACHE_HOME") | ||
| root = Path(base) if base else Path.home() / ".cache" |
There was a problem hiding this comment.
Minor (non-blocking, XDG spec): empty XDG_CACHE_HOME is handled correctly here (falsy → ~/.cache), but the XDG Base Directory spec also says a relative value must be ignored (paths must be absolute). As written, XDG_CACHE_HOME=cache yields a CWD-relative cache path. Low impact since _open_trusted_cache + the O_EXCL/0600 save still protect the file wherever it lands, but you could guard it:
base = os.environ.get("XDG_CACHE_HOME")
root = Path(base) if base and os.path.isabs(base) else Path.home() / ".cache"| 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) |
There was a problem hiding this comment.
Minor (non-blocking): the temp name keys on os.getpid(), which is unique across processes but not across two threads in one process, and a stale *.<pid>.tmp left by a crashed same-PID process would make O_EXCL fail and silently skip this save. It's best-effort + logged, so not a correctness bug — but tempfile.mkstemp(dir=self._cache_file.parent, prefix=self._cache_file.name + ".", suffix=".tmp") gives a guaranteed-unique descriptor with the same O_EXCL/0600 guarantees you rely on here.
| ) | ||
|
|
||
|
|
||
| def _is_trusted_cache_file(path: Path) -> bool: |
There was a problem hiding this comment.
Minor (non-blocking): _is_trusted_cache_file is now referenced only by the tests — _load_cache uses _open_trusted_cache (as this docstring recommends). Since the stat-then-open variant is exactly the TOCTOU-prone shape this PR set out to replace, consider dropping it (and its two unit tests) or clearly marking it test-only, so nobody reaches for it later.
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cf26cc3. Configure here.
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 <benjamin.burtenshaw@huggingface.co>
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 <benjamin.burtenshaw@huggingface.co>
b0b5241 to
155e6e6
Compare
Summary
Replacement for #1011 (fork workflows stuck at
action_required; update-branch also 403). Same change: per-user discovery cache, trusted descriptor checks (nonblocking / regular-file only), atomic0600save viaO_EXCL+os.replace, and suite isolation from the real user cache.Burtenshaw already APPROVED #1011; release automation re-APPROVED after the FIFO/mode/test fixes. Please close #1011 once this lands.
Type of Change
Alignment Checklist
mainso required CI can run on-repoRFC Status
Test Plan
tests/envs/test_discovery.pyClaude Code Review
N/A — release automation replacement so Ben's watch/merge ask on #1011 is not blocked on fork workflow approval.
Note
Medium Risk
Changes where cached
client_module_pathvalues are read and later imported; mistakes in trust or atomic write logic could break discovery or reintroduce local cache poisoning.Overview
Hardens environment auto-discovery so a local attacker cannot plant or redirect the on-disk discovery cache to steer later
import_modulecalls toward arbitrary modules.The cache path moves from a fixed file under the shared temp directory to a per-user file at
$XDG_CACHE_HOME/openenv/discovery_cache.json(or~/.cache/openenv/...). Loads go through_open_trusted_cache:O_NOFOLLOW+O_NONBLOCK,fstaton the descriptor, owner-only and non–group/other-writable checks on POSIX, and refusal of non-regular files (symlinks, FIFOs). Saves use a pid-suffixed temp file created withO_EXCLand mode 0600, thenos.replace, so umask/truncation cannot leave a world-writable cache.reset_discovery()now only clears the in-process singleton and no longer deletes the persistent cache (explicitclear_cache()is required). Tests add an autouse redirect of_default_cache_fileintotmp_pathplusTestCacheSecuritycoverage for the trust and save semantics.Reviewed by Cursor Bugbot for commit 155e6e6. Bugbot is set up for automated code reviews on this repo. Configure here.