fix(file-safety): read-deny external provider-CLI credential stores (Claude Code / Copilot / MiniMax) - #38852
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR strengthens read_file denial rules by blocking reads of external provider CLI credential stores that live outside HERMES_HOME, and adds tests to prevent regressions.
Changes:
- Add denylist checks for external credential store paths (Claude Code, GitHub Copilot, MiniMax), honoring
XDG_CONFIG_HOMEfor Copilot. - Expand documentation in
get_read_block_errorto describe the new blocked category and rationale. - Add unit tests to verify reads are denied for these external credential files, and explicitly allow
~/.codex/auth.json.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
agent/file_safety.py |
Extends the read-block logic to include external provider credential store locations (outside HERMES_HOME). |
tests/agent/test_file_safety.py |
Adds coverage validating the new deny behavior and the explicit ~/.codex/auth.json exclusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * External provider-CLI credential stores that Hermes imports OAuth | ||
| tokens from but which live OUTSIDE HERMES_HOME: | ||
| ``~/.claude/.credentials.json``, ``~/.claude.json``, | ||
| ``~/.config/github-copilot/{hosts,apps}.json`` (``XDG_CONFIG_HOME`` | ||
| honored), and ``~/.minimax/credentials.json``. ``~/.codex/auth.json`` | ||
| is intentionally excluded (#12360 made Hermes stop touching it). |
| home = Path(os.path.expanduser("~")) | ||
| xdg = os.environ.get("XDG_CONFIG_HOME", "").strip() | ||
| config_home = Path(xdg) if xdg else (home / ".config") |
| def test_xdg_config_home_honored_for_copilot(self, monkeypatch, tmp_path): | ||
| """Copilot store under a custom XDG_CONFIG_HOME is still read-denied.""" | ||
| monkeypatch.setenv("HOME", str(tmp_path / "home")) | ||
| monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes_home")) | ||
| xdg = tmp_path / "xdg" | ||
| monkeypatch.setenv("XDG_CONFIG_HOME", str(xdg)) | ||
| target = xdg / "github-copilot" / "hosts.json" | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| target.write_text("{}") | ||
|
|
||
| assert get_read_block_error(str(target)) is not None |
|
@copilot All three findings addressed in commit c4316c1aa:
|
alt-glitch
added
type/security
c4316c1 to
26099d4
Compare
26099d4 to
1c4f60c
Compare
…Claude Code / Copilot / MiniMax) NousResearch#17656 / NousResearch#30721 / NousResearch#30972 added read_file denial for credential stores under HERMES_HOME / the Hermes root (auth.json, .anthropic_oauth.json, .env, webhook_subscriptions.json, auth/google_oauth.json, cache/bws_cache.json, mcp-tokens/). Their resolve-loop only walks HERMES_HOME and hermes_root, so the EXTERNAL provider-CLI credential stores Hermes imports OAuth tokens from — which live in each provider tool's own home, outside HERMES_HOME — are never reached. These hold plaintext OAuth / API-key material and are live, reachable paths: the anthropic adapter reads ~/.claude/.credentials.json directly, and the model layer enumerates ~/.config/github-copilot/{hosts,apps}.json and ~/.minimax/credentials.json. read_file could therefore exfiltrate them under the same prompt-injection vector NousResearch#17656 hardened against. Extend get_read_block_error with the same exact-path read-deny for ~/.claude/.credentials.json, ~/.claude.json, ~/.config/github-copilot/{hosts,apps}.json (XDG_CONFIG_HOME honored), and ~/.minimax/credentials.json. ~/.codex/auth.json is intentionally excluded — NousResearch#12360 made Hermes stop touching it by design (single-use refresh-token race). Defense-in-depth, not a security boundary: the terminal tool can still bypass.
…ry count Address Copilot review on NousResearch#38852: - Normalize XDG_CONFIG_HOME via os.path.expanduser/expandvars before building config_home, so a common "~/.config" / "$HOME/.config" value still resolves to the real directory and the Copilot credential stores stay read-denied. - Update the get_read_block_error docstring count from "Three" to "Four" categories to match the external-credential-store bullet added in this branch. - Add a companion test asserting a tilde-containing XDG_CONFIG_HOME is expanded.
1c4f60c to
2de51ce
Compare
samherring99
left a comment
samherring99
left a comment
There was a problem hiding this comment.
PR Review — NousResearch/hermes-agent #38852
Branch: fix/deny-external-cred-reads (target: main)
Title: fix(file-safety): read-deny external provider-CLI credential stores
Reviewer: external (samherring99)
Verdict: REQUEST_CHANGES
Summary
The change extends get_read_block_error in agent/file_safety.py to deny reads
of external provider-CLI credential stores (Claude Code, GitHub Copilot,
MiniMax). The intent is correct and the test coverage is welcome. However, the
XDG_CONFIG_HOME handling is broken: the value is read from the environment and
passed directly to Path() without expanding ~ or environment variables. That
means a user with a common XDG_CONFIG_HOME=/.config (or $HOME/.config) will/.config/...").resolve()
not have the Copilot credential stores read-denied, because Path("
will be resolved relative to the process cwd instead of the real home directory.
This is a real correctness/safety gap that undermines the new guard. Please fix
before merging.
Inline comment (agent/file_safety.py)
Line (initial commit da92259):
config_home = Path(xdg) if xdg else (home / ".config")
Issue: XDG_CONFIG_HOME is not expanded. Expand it with
os.path.expanduser(os.path.expandvars(xdg)) before building paths, otherwise
values like "~/.config" or "$HOME/.config" bypass the Copilot denylist.
Suggested fix:
config_home = Path(os.path.expanduser(os.path.expandvars(xdg))) if xdg else (home / ".config")
Also add a test that pins this behavior, e.g. set XDG_CONFIG_HOME="~/xdgconfig"
and assert the github-copilot apps.json under the real home is denied.
Other notes
- Code style / coverage: good. The negative tests for ~/.codex/auth.json and
non-credential files under ~/.claude are appropriate. - Docstring says "XDG_CONFIG_HOME honored"; the implementation does not yet
honor tilde/variable values, so the docstring is currently misleading. - This is not a security boundary (acknowledged), but the intended defense-in-
depth guard should at least cover the documented cases.
samherring99
left a comment
samherring99
left a comment
There was a problem hiding this comment.
@review_verdict.txt
samherring99
left a comment
samherring99
left a comment
There was a problem hiding this comment.
PR Review — NousResearch/hermes-agent #38852
Branch: fix/deny-external-cred-reads (target: main)
Title: fix(file-safety): read-deny external provider-CLI credential stores
Reviewer: external (samherring99)
Verdict: REQUEST_CHANGES
Summary
The change extends get_read_block_error in agent/file_safety.py to deny reads
of external provider-CLI credential stores (Claude Code, GitHub Copilot,
MiniMax). The intent is correct and the test coverage is welcome. However, the
XDG_CONFIG_HOME handling is broken: the value is read from the environment and
passed directly to Path() without expanding ~ or environment variables. That
means a user with a common XDG_CONFIG_HOME=/.config (or $HOME/.config) will/.config/...").resolve()
not have the Copilot credential stores read-denied, because Path("
will be resolved relative to the process cwd instead of the real home directory.
This is a real correctness/safety gap that undermines the new guard. Please fix
before merging.
Inline comment (agent/file_safety.py)
Line (initial commit da92259):
config_home = Path(xdg) if xdg else (home / ".config")
Issue: XDG_CONFIG_HOME is not expanded. Expand it with
os.path.expanduser(os.path.expandvars(xdg)) before building paths, otherwise
values like "~/.config" or "$HOME/.config" bypass the Copilot denylist.
Suggested fix:
config_home = Path(os.path.expanduser(os.path.expandvars(xdg))) if xdg else (home / ".config")
Also add a test that pins this behavior, e.g. set XDG_CONFIG_HOME="~/xdgconfig"
and assert the github-copilot apps.json under the real home is denied.
Other notes
- Code style / coverage: good. The negative tests for ~/.codex/auth.json and
non-credential files under ~/.claude are appropriate. - Docstring says "XDG_CONFIG_HOME honored"; the implementation does not yet
honor tilde/variable values, so the docstring is currently misleading. - This is not a security boundary (acknowledged), but the intended defense-in-
depth guard should at least cover the documented cases.
This is a sibling follow-up to #17656 (commits 056e00a / 567ea61, landed via #30721)
auth.json,.anthropic_oauth.json,.env,webhook_subscriptions.json,auth/google_oauth.json,cache/bws_cache.json,mcp-tokens/).hermes_dirsresolve-loop inget_read_block_errornever reaches them.~/.claude/.credentials.json,~/.claude.json,~/.config/github-copilot/{hosts,apps}.json, and~/.minimax/credentials.json(XDG_CONFIG_HOMEhonored).~/.codex/auth.jsonis intentionally excluded — fix(codex): Hermes owns its own Codex auth; stop touching ~/.codex/auth.json #12360 made Hermes stop touching it by design.What does this PR do?
Extends
get_read_block_error(agent/file_safety.py) to denyread_fileon the external, well-known provider credential stores Hermes imports OAuth tokens from. These hold plaintext OAuth / API-key material and are reachable by the agent today because the existing denylist only resolves candidate paths againsthermes_dirs(HERMES_HOME + the Hermes root) — external provider homes are never walked. Same prompt-injection exfiltration vector as #17656.The stores are live paths, not theoretical:
~/.claude/.credentials.json/~/.claude.json— Claude Code OAuth, read by the anthropic adapter.~/.config/github-copilot/{hosts,apps}.json— GitHub Copilot OAuth, enumerated by the model layer.~/.minimax/credentials.json— MiniMax credentials, enumerated by the model layer.~/.codex/auth.jsonis deliberately left readable here: #12360 made Hermes stop owning/touching it (single-use refresh-token race), so it is external by design and out of scope for this guard. Calling that out so the exclusion reads as a conscious decision rather than an oversight.Defense-in-depth, not a security boundary — the terminal tool runs as the same OS user and can still
catthe file; the read-deny returns a clear error that models respecting tool denials honor, and leaves an audit trail.Related Issue
Sibling follow-up to #17656 — no separate issue.
Type of Change
Changes Made
agent/file_safety.py: inget_read_block_error, add an external provider-CLI credential-store exact-path block after themcp-tokens/check (Claude Code, GitHub Copilot, MiniMax;XDG_CONFIG_HOMEhonored;~/.codex/auth.jsondeliberately excluded per fix(codex): Hermes owns its own Codex auth; stop touching ~/.codex/auth.json #12360). Reuses the function's existingresolvedlocal; no new top-level imports (os/Pathalready imported). Docstring updated to list the new category.tests/agent/test_file_safety.py: newTestExternalCredentialStoreReadBlocking— parametrized deny tests for all five stores, anXDG_CONFIG_HOME-override case, a negative~/.codex/auth.json-stays-readable case, and a negative non-credential-file-under-~/.claude-stays-readable case. Tests pin bothHOMEandHERMES_HOMEso the HERMES_HOME loop cannot coincidentally match.How to Test
uv run --with pytest --with pytest-asyncio python3 -m pytest tests/agent/test_file_safety.py -v→ 26 passed (7 new).agent/file_safety.pyhunk → the 6 deny-assertions fail (get_read_block_errorreturnsNone); restore → all pass. The 2 negative tests pass both ways, so they are not tautological.Checklist
Code
fix(scope):)Documentation & Housekeeping
get_read_block_error) — or N/ApathlibandXDG_CONFIG_HOMEis honored; only tested on macOSRelated / Positioning
Disjoint from the open #35997 (generic OS/cloud user secrets:
~/.ssh,~/.aws,~/.gnupg,~/.kube,~/.docker,~/.azure,~/.config/gh,~/.config/gcloud,~/.netrc, etc.). This PR targets the narrower, Hermes-specific surface: the provider-CLI OAuth stores Hermes' own adapters read live and import tokens from. The two are complementary — neither covers the other's paths.Disjoint from the open write-deny work (#37336 / Dusk1e's #38490–#38493), which adds
write_file/patchdenial on HERMES_HOME credential stores. This is read-deny on external stores.