fix(subdirectory_hints): prevent loading AGENTS.md outside workspace#32103
fix(subdirectory_hints): prevent loading AGENTS.md outside workspace#32103ffr31mr wants to merge 1 commit into
Conversation
SubdirectoryHintTracker was scanning directories outside the active working directory, allowing files like ~/.codex/AGENTS.md or ~/.claude/CLAUDE.md to be loaded and injected into the agent context. This causes cross-agent context contamination and instruction mixup. Add _is_ancestor_or_same() helper and a path boundary check in _is_valid_subdir(): only directories within the working directory tree (i.e. path.is_relative_to(working_dir)) are allowed. Also add exist_ok=True to mkdir() calls in new tests to prevent pytest-xdist race conditions when workers share the same tmp_path parent. Tests added: - test_outside_working_dir_rejected: verifies sibling dirs are blocked - test_outside_working_dir_absolute_path_rejected: verifies ~/.codex paths blocked - test_inside_workspace_subdir_allowed: verifies normal subdir access unaffected - test_sibling_repo_not_loaded_via_ancestor_walk: ancestor walk stays within workspace
hclsys
left a comment
There was a problem hiding this comment.
The fix is correct and the boundary check works — I traced both candidate sources and they're safe:
working_diris real-path resolved at construction (agent/subdirectory_hints.py:62).- Every path that reaches
_is_valid_subdiris also resolved first:_add_path_candidatedoesp = p.resolve()(:122) before walking ancestors into the guard, and_extract_paths_from_commandresolves at:124._load_hints_for_directoryonly sees dirs from that already-resolved candidate set.
So a symlink inside the workspace pointing outside (./link → ~/.codex) resolves outside working_dir and correctly fails the new is_relative_to check — the contamination path the PR targets is closed. Good. The _is_ancestor_or_same fallback for the pre-3.9 is_relative_to absence is also right (not redundant — it's the except branch).
One correctness-of-comment nit worth fixing (not a logic bug): the inline comment
# path.resolve() may differ from working_dir.resolve() due to symlinks,
# but path.is_relative_to(working_dir) handles both absolute and
# symlinked paths correctly on Python 3.9+.
is misleading — Path.is_relative_to is purely lexical; it does no symlink resolution. The symlink safety here comes entirely from callers resolving the path before calling this method (:122/:124), not from is_relative_to. As written, the comment could lead a future caller to add an unresolved path and silently lose the guard. Suggest rewording to something like "callers must pass a resolved path; is_relative_to is a lexical containment check." Optionally assert/resolve at the top of _is_valid_subdir to make the method self-defending.
Otherwise LGTM.
|
Salvaged in PR #32342 → merged to main. Your workspace-boundary fix is in; ancestor walk in Thanks for catching the cross-agent contamination vector — this is a real indirect-prompt-injection sink and the fix is the right shape. |
Pre-salvage prep for the must-have security cluster (NousResearch#32103, NousResearch#32155). NousResearch#32103 author commit uses dearmayo@localhost; PR opener is ffr31mr — same pattern as the existing holynn-q localhost mapping.
Pre-salvage prep for the must-have security cluster (NousResearch#32103, NousResearch#32155). NousResearch#32103 author commit uses dearmayo@localhost; PR opener is ffr31mr — same pattern as the existing holynn-q localhost mapping.
Pre-salvage prep for the must-have security cluster (NousResearch#32103, NousResearch#32155). NousResearch#32103 author commit uses dearmayo@localhost; PR opener is ffr31mr — same pattern as the existing holynn-q localhost mapping. #AI commit#
Pre-salvage prep for the must-have security cluster (NousResearch#32103, NousResearch#32155). NousResearch#32103 author commit uses dearmayo@localhost; PR opener is ffr31mr — same pattern as the existing holynn-q localhost mapping.
Pre-salvage prep for the must-have security cluster (NousResearch#32103, NousResearch#32155). NousResearch#32103 author commit uses dearmayo@localhost; PR opener is ffr31mr — same pattern as the existing holynn-q localhost mapping.
Summary
SubdirectoryHintTrackerwas scanning directories outside the active working directory. This allowed files like~/.codex/AGENTS.mdor~/.claude/CLAUDE.mdto be loaded and injected into the agent context, causing cross-agent context contamination and instruction mixup.Changes
_is_ancestor_or_same()helper for boundary checking_is_valid_subdir(): only directories within the working directory tree (path.is_relative_to(working_dir)) are allowed to be scannedexist_ok=Truetomkdir()calls in new tests to preventpytest-xdistrace conditions when parallel workers share the sametmp_pathparentTests added
test_outside_working_dir_rejected: sibling directories are blockedtest_outside_working_dir_absolute_path_rejected: paths like~/.codex/are blockedtest_inside_workspace_subdir_allowed: normal subdirectory access is unaffectedtest_sibling_repo_not_loaded_via_ancestor_walk: ancestor walk stays within workspaceTest plan
pytest tests/agent/test_subdirectory_hints.pypassespytest -n auto tests/agent/test_subdirectory_hints.pypasses (xdist race check)