fix(security): deny writes to read-blocked HERMES_HOME credential stores - #38490
fix(security): deny writes to read-blocked HERMES_HOME credential stores#38490Dusk1e wants to merge 1 commit into
Conversation
|
Closely overlaps #37336, which pairs write-deny for the same read-protected credential stores (google_oauth.json, bws_cache.json, auth.lock). Maintainers may want to consolidate these two PRs. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Changes
Merges the credential-file list into a single CREDENTIAL_FILE_NAMES constant shared between read block (get_read_block_error) and write deny (is_write_denied), and adds config.yaml to the write-deny list (already readable, but not writable) while introducing auth.lock to the shared credential list.
Security Observations
- Single source of truth — eliminates the duplication that caused the prior gap where
auth.lockwas blocklisted inget_read_block_errorbut missing fromis_write_denied. - Correctly distinguishes read-blocked files (credentials that must not be read OR written) from config, which is readable but must not be written (to prevent profile overrides).
config.yamlwrite-block rationale is well documented in the code comment.
Testing
Comprehensive coverage in TestCredentialStoreWriteParity:
- Per-profile and root-path write denies for
auth.lock,google_oauth.json,bws_cache.json. - Invariant test that every
CREDENTIAL_FILE_NAMESentry is write-denied.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real remaining gap: current agent/file_safety.py:241-251 read-blocks auth.lock, Google OAuth tokens, and the Bitwarden cache, while is_write_denied at agent/file_safety.py:109-144 does not cover them.
Problems
- The new universal parity at
agent/file_safety.py:139also re-deniesauth.json,webhook_subscriptions.json, andconfig.yaml. That conflicts with merged maintainer policy81e42335(#45947), which intentionally made Hermes control files writable; currenttests/tools/test_file_operations.py:84-91enforces that contract. tests/tools/test_write_deny.py:146-159freezes that broader parity, so it would prevent the intentional read/write distinction from being retained.
Suggested changes
- Re-scope the write-deny entries and regression tests to the three remaining targets, preserving the current writable-control-file behavior.
- Cover active-profile and root paths for each retained write-denied target.
Automated hermes-sweeper review.
| # be silently overwritten. ``config.yaml`` is the one extra: write-denied to | ||
| # protect the profile, but intentionally still readable (see | ||
| # get_read_block_error / test_config_yaml_not_blocked). | ||
| denied_file_names = ("config.yaml",) + CREDENTIAL_FILE_NAMES |
There was a problem hiding this comment.
This couples all read-blocked files to write denial, but current main deliberately keeps auth.json, config.yaml, and webhook_subscriptions.json writable (81e42335, with coverage in tests/tools/test_file_operations.py:84-91). Please use a narrower write-denied set for the three intended stores instead of universal read/write parity.
|
suggesting changes The PR identifies a real security gap: on current main, Please rebase/replay this change onto current main and narrow the write-side additions to those three stores. The current file-safety policy intentionally preserves write access for Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
What & why
get_read_block_errorhides several HERMES_HOME credential stores from theagent, but
is_write_denieddid not block writing three of them — so aprompt-injected
write_filecould still overwrite real secrets:auth/google_oauth.json(Gemini OAuth tokens)cache/bws_cache.json(Bitwarden plaintext secrets)auth.lock(auth.json advisory lock)This closes the read/write asymmetry by enforcing the invariant "every
credential file hidden from reads is also protected from overwrites." The read
and write guards now share a single
CREDENTIAL_FILE_NAMESlist, so any futurecredential added to the read block is automatically write-protected too — the
lists can't drift apart again.
config.yamlstays write-denied butintentionally readable. Internal writers (
auth.py,google_oauth.py,bitwarden.py) write directly and don't pass through this guard, so no runtimeflow changes.
Changes
agent/file_safety.py— liftCREDENTIAL_FILE_NAMESto a shared moduleconstant;
is_write_deniednow denies("config.yaml",) + CREDENTIAL_FILE_NAMES.tests/tools/test_write_deny.py—TestCredentialStoreWriteParity: per-fileregression tests, root-widening-under-profile test, and a contract test
asserting
CREDENTIAL_FILE_NAMES ⊆ write-denied.Testing
scripts/run_tests.sh tests/tools/test_write_deny.py tests/agent/test_file_safety_credentials.py -q→ 44 passed, 1 skipped (Windows symlink test). Broader file-safety + write-path
suites: no regressions.
scripts/check-windows-footguns.py: clean.