fix(file-safety): extend is_write_denied to cover inactive profile credentials - #37625
fix(file-safety): extend is_write_denied to cover inactive profile credentials#37625AhmetArif0 wants to merge 1 commit into
Conversation
…edentials is_write_denied() built hermes_dirs from _hermes_home_path() (active profile) and _hermes_root_path() (root). Credentials under ~/.hermes/profiles/<inactive>/ were not in either set, so a prompt- injected write_file targeting another profile's auth.json, config.yaml, mcp-tokens/, or pairing/ was allowed without any block or warning. Enumerate the profiles/ directory and add every profile subdirectory to hermes_dirs so the existing control-file and directory checks apply across all profiles, not just the active one. The loop is wrapped in try/except so a missing or unreadable profiles/ directory cannot break the write guard. The companion soft-warning path (classify_cross_profile_target) is addressed separately by NousResearch#35902; this change adds the hard block that prevents writes regardless of approval state. Fixes NousResearch#37617.
|
Nice fix — the new branch in is_write_denied() closes the cross-profile write hole cleanly and the test coverage covers all three control file names plus the mcp-tokens/ subdir. One small thing worth considering: the new loop at the bottom of is_write_denied() walks profiles_dir.iterdir() and resolves realpath() per entry, but it lives inside a function that gets called on every file write the agent attempts. For an agent doing many file ops under one process, that's an extra stat() + realpath() per call per profile directory. Caching the resolved list for a short TTL (or invalidating on a known mutator) would keep this O(1) on the hot path. Probably worth a follow-up if you ever see this function in a flame graph, not a blocker for this PR. Also the underscore-prefixed locals (_entry, _real) read like linter-silenced unused vars — _real is consumed on the next line so it's fine, but _entry is purely a loop binding and could just be |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the inactive-profile path gap. The mcp-tokens/ and pairing/ portion is still relevant: current origin/main only checks the active home and root in agent/file_safety.py:111-132.
Problems
- The PR's retained
control_file_namesbehavior would again denyauth.json,config.yaml, andwebhook_subscriptions.json. Current main intentionally made those files writable in commit81e42335; its tests assert that contract intests/tools/test_file_operations.py:84-91and:129-140. - The new class says pairing is covered, but
tests/tools/test_write_deny.py:127-162tests auth, MCP tokens, and config only.
Suggested changes
- Rescope the enumeration to the existing hard-denied
mcp-tokens/andpairing/paths; preserve the current writable control-file policy. - Add inactive-profile regression cases for both directories in the current file-safety test surface.
Automated hermes-sweeper review.
| except Exception: | ||
| continue | ||
|
|
||
| # Also cover every inactive profile directory so a prompt-injected |
There was a problem hiding this comment.
Current main deliberately made auth.json, config.yaml, and webhook_subscriptions.json writable in 81e4233. Please salvage this enumeration only for the still-hard-denied mcp-tokens/ and pairing/ paths rather than restoring the removed control-file deny loop.
| """Inactive profile credential stores must be write-denied (#37617). | ||
|
|
||
| When an agent runs under one profile, prompt-injected write_file calls | ||
| must not be able to overwrite another profile's auth.json, mcp-tokens/, |
There was a problem hiding this comment.
The docstring says pairing is covered, but this class adds no inactive-profile pairing assertion. Add that regression case when rescoping this to the currently hard-denied directory paths.
|
suggesting changes P2: The patch replay extends inactive-profile coverage for protected state, session, token, and pairing subtrees, but its regression assertions also require denials for authentication, configuration, and webhook control files that the current write policy does not implement. Reconcile the production predicate and tests around one explicit control-file policy; preserve inactive token and pairing protection, and add runtime coverage for every credential target retained in scope. The review used a run-owned patch replay against current GitHub Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
Problem
is_write_denied()buildshermes_dirsfrom_hermes_home_path()(active profile) and_hermes_root_path()(root only). Credentials under~/.hermes/profiles/<inactive>/don't match either prefix, so a prompt-injectedwrite_filecall targeting another profile'sauth.json,config.yaml,mcp-tokens/, orpairing/is allowed with no block or warning.Attack scenario (from #37617)
Active profile:
default. Injected prompt:is_write_denied("~/.hermes/profiles/hermes-security/auth.json")→hermes_dirs = [~/.hermes]→False(allowed)Fix
After building
hermes_dirsfrom active home + root, enumerateprofiles/*/and add each profile subdirectory. The existing control-file checks (auth.json,config.yaml,webhook_subscriptions.json) and directory checks (mcp-tokens/,pairing/) then apply across all profiles, not just the active one.The companion soft-warning path (
classify_cross_profile_target) is addressed separately by #35902; this PR adds the hard block that prevents the write regardless of approval state.Test plan
test_inactive_profile_auth_json— denied ✓test_inactive_profile_mcp_tokens— denied ✓test_inactive_profile_config_yaml— denied ✓test_active_profile_still_denied— regression guard ✓Closes #37617.