fix(security): write BWS disk cache encrypted-only, migrate legacy plaintext - #77008
fix(security): write BWS disk cache encrypted-only, migrate legacy plaintext#77008andrexibiza wants to merge 5 commits into
Conversation
alt-glitch
added
type/security
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a verified at-rest secret exposure: current main defaults to the plaintext Bitwarden cache at agent/secret_sources/bitwarden.py:504, reads it at :541-549, and writes it at :618-629.
Problems
agent/secret_sources/bitwarden.py:523-529returns the legacy entry regardless of whether_write_encrypted_disk_cachesucceeded. That writer deliberately swallows write failures at:445-446, so a failed migration can still serve the entry whilebws_cache.jsonremains plaintext.agent/secret_sources/bitwarden.py:654-659usesfloat("inf")for legacy offline fallback, bypassingencrypted_cache_max_stale_secondseven though the encrypted fallback at:638-643honors it.- Explicit prior
encrypted_cache.enabled: falseconfigurations skip migration entirely (:588-601,:636). The startup loader preserves raw user config athermes_cli/env_loader.py:614-615, so such existing plaintext files remain indefinitely.
Suggested changes
- Return/verify encryption-and-removal success before serving a migrated entry, with a forced-write-failure regression test.
- Enforce the configured stale bound for legacy fallback, and define removal behavior for explicit memory-only mode.
- Update
website/docs/user-guide/secrets/bitwarden.md:108-123andcli-config.yaml.example:1642-1644, which still document encryption as opt-in.
This is an automated hermes-sweeper review.
| entry=legacy, | ||
| home_path=home_path, | ||
| ) | ||
| return legacy |
There was a problem hiding this comment.
_write_encrypted_disk_cache deliberately swallows ordinary failures and returns no status, so this can return the legacy value even when encryption or plaintext removal failed. Make migration return a cache hit only after a verified successful encrypted write/removal, and cover the write-failure path.
There was a problem hiding this comment.
Addressed in b466156c45 — migration now verifies the encrypted write before the legacy plaintext cache is removed: _write_encrypted_disk_cache returns bool and the migration path checks it (agent/secret_sources/bitwarden.py:528). Verified at head e46d020bcd: 5/5 migration/stale tests pass.
| stale = _migrate_legacy_plaintext_cache( | ||
| cache_key=cache_key, | ||
| access_token=access_token, | ||
| max_age_seconds=float("inf"), |
There was a problem hiding this comment.
This bypasses encrypted_cache_max_stale_seconds: a network failure can serve arbitrarily old legacy plaintext data even when the configured stale limit is zero. Apply the same bound used for the encrypted fallback, or migrate without serving data outside that bound.
There was a problem hiding this comment.
Addressed in b466156c45 — the legacy-plaintext fallback is bounded by encrypted_cache_max_stale_seconds instead of float('inf'), so an offline stale cache no longer serves plaintext indefinitely. Verified at head e46d020bcd: 5/5 migration/stale tests pass.
| f"bws live fetch failed ({exc}); " | ||
| f"falling back to stale disk cache ({int(age)}s old)" | ||
| ] | ||
| if not encrypted_cache_enabled: |
There was a problem hiding this comment.
An existing configuration with encrypted_cache.enabled: false now takes this path and never migrates or removes its pre-existing bws_cache.json. If false means memory-only, remove the legacy plaintext cache without reading/serving it so the advertised upgrade cleanup still holds.
There was a problem hiding this comment.
Addressed — _remove_legacy_plaintext_cache(home_path) is now called before the raise in both the fresh-cache path and this offline fallback path. Legacy plaintext is always deleted on upgrade even when encrypted_cache.enabled: false.
teknium1
added
the
sweeper:blast-moderate
|
Addressed all three sweeper items:
|
8968fd7 to
9a67533
Compare
|
Fixed the stale-cache test — was failing because the offline fallback now uses |
…cription, skill test Addresses teknium1's review on NousResearch#77097: 1. 'Not true on main' — the security contract is now explicitly scoped as implemented by the secrets-exfiltration hardening series (NousResearch#77008/NousResearch#77012/NousResearch#77020/NousResearch#77027/NousResearch#77031/NousResearch#77039). The docs state current main behavior plainly (plaintext bws_cache.json read/written when encryption disabled, default false) and keep the rotation instruction mandatory today, since that exposure already exists on main. The posture framing stays — this eliminates an entire vulnerability class — but the claim is now sequenced truthfully. 2. Skill description shortened to 53 chars, one sentence, ends with a period (AGENTS.md hardline). 3. tests/skills/test_bitwarden_secrets_skill.py added: validates frontmatter, description length, required sections, user-only rotation + clipboard discipline, honest series scoping (no claim the gate test is on main), and docs-page metadata consistency. 4. Clipboard discipline added to rotation instructions (docs + skill): create token, copy to clipboard, paste into terminal, save nowhere in between.
|
Review receipts — both comments from 2026-08-02 on agent/secret_sources/bitwarden.py: 1) Migration cache-hit integrity (comment @ _migrate_legacy_plaintext_cache): 2) Legacy-plaintext stale bound (comment @ fetch_bitwarden_secrets fallback): 5/5 targeted tests pass. |
|
suggesting changes The encrypted-only default removes ordinary plaintext writes and bounds stale fallback, but legacy plaintext cleanup is still not guaranteed on two reachable paths. A pre-upgrade
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Fixed in 1) Migration success no longer reported when legacy removal fails 2) Legacy cleanup independent of cache TTL / use-cache settings Regression tests added (
Validation: |
|
Final consolidated receipts — every review item from teknium1 (hermes-sweeper) and egilewski is addressed at head teknium1 (hermes-sweeper)
egilewski
Verification
Ready for re-review. |
|
@egilewski — accepted as a valid residual. The encrypted-only default does not close the class while legacy I have recorded the two required changes separately: migration success must not be reported while the legacy file remains, and cleanup must run independently of the cache/fetch branch when the encrypted-only policy is active. The focused tests need the unlink-failure and zero-TTL/bypass cases before this PR can be described as class-complete. Source comment: #77008 (comment) |
|
suggesting changes The new helper return value still does not close persistent legacy-plaintext cleanup failures at the production boundary. Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
bcb9b51 to
98cb9c4
Compare
…aintext The Bitwarden Secrets Manager disk cache used to persist every fetched secret value in plaintext at <hermes_home>/cache/bws_cache.json. The encrypted AES-GCM cache (bws_cache.enc.json) existed but was opt-in and off by default. This change makes encryption the default and the only storage policy: - encrypted_cache.enabled now defaults to true (config defaults + schema) - the plaintext write/read/fallback branches are removed entirely; with encryption disabled the disk cache is skipped (memory only) rather than falling back to plaintext - a legacy plaintext bws_cache.json from an older Hermes is re-encrypted and removed on first read, so no user upgrade leaves secrets at rest Why this matters to users: today every API key you keep in Bitwarden Secrets Manager is copied to a plain-text file on your computer so Hermes doesn't re-fetch on every launch. Anyone with access to your machine, a backup, or a cloud-synced folder can read that file and see all your keys. After this change the file is encrypted — your keys are unreadable to anyone without the right access, even if the file itself leaks. No action needed; it happens automatically on upgrade. Tests: 19 passed (3 new regression tests cover encrypted-only default, legacy migration, and offline-start migration). Two pre-existing Windows failures (fixture zip member naming, chmod 0600 not enforced) are platform artifacts unrelated to this change.
…le bound, remove legacy on disable
sweeper flags addressed:
1. _migrate_legacy_plaintext_cache now checks _write_encrypted_disk_cache
return value — returns None when the encrypted write/removal fails,
so the caller falls through to a live fetch rather than serving a
value whose plaintext file may still be on disk.
2. Offline fallback path uses encrypted_cache_max_stale_seconds instead
of float('inf') for legacy plaintext migration.
3. Legacy plaintext cache is removed when encrypted_cache_enabled=False.
Regression tests:
- test_migration_skips_legacy_when_encrypted_write_fails
- test_legacy_plaintext_offline_fallback_respects_stale_bound
- test_legacy_plaintext_removed_when_encryption_disabled
…nds bound
The old test relied on float('inf') in the offline fallback path.
Now that migration uses the configured max_stale_seconds, pass an explicit
bound so the test's stale cache entry (1h old) is still within window.
…e config Sweeper flagged that website/docs/user-guide/secrets/bitwarden.md and cli-config.yaml.example still documented encryption as opt-in. Updated both to reflect the new default (encrypted-only disk cache, enabled: true).
… up legacy cache independent of TTL/use-cache Addresses review on PR NousResearch#77008: 1. _write_encrypted_disk_cache no longer swallows OSError from unlinking the legacy plaintext bws_cache.json and reporting success. A legacy file that cannot be removed means migration is incomplete, so the function returns False and _migrate_legacy_plaintext_cache refuses to serve the value while the plaintext copy is still at rest. 2. Legacy plaintext cleanup now runs independently of cache TTL and use-cache settings. Previously _remove_legacy_plaintext_cache was only reached inside the positive-TTL cache branch or the transport-error branch; with cache_ttl_seconds=0 or use_cache=False and encryption disabled, a successful live fetch left a pre-upgrade plaintext file untouched. The success path and the error path now both remove the legacy file whenever the encrypted-only policy is active.
98cb9c4 to
39fb8c9
Compare
4 participants
Related #60295 #69054
What changed and why
The Bitwarden Secrets Manager disk cache used to persist every fetched secret value in plaintext at
<hermes_home>/cache/bws_cache.json. The encrypted AES-GCM cache (bws_cache.enc.json) already existed but was opt-in and off by default — so every user of the built-in BWS integration had their full set of provider keys sitting readable on disk.This PR makes encryption the default and the only storage policy:
encrypted_cache.enablednow defaults totrue(config defaults + source schema + function signatures).fetch_bitwarden_secretsare removed entirely. There is no config value that writes plaintext again. With encryption explicitly disabled, the disk cache is skipped (memory-only) rather than falling back to plaintext.bws_cache.jsonfrom an older Hermes is re-encrypted and removed on first read (_migrate_legacy_plaintext_cache), so existing users are migrated automatically — no plaintext cache survives the upgrade.Why this matters to you as a user
Today, every API key you keep in Bitwarden Secrets Manager is copied to a plain-text file on your computer so Hermes doesn't have to re-fetch it on every launch. Anyone with access to your machine, a backup, or a cloud-synced folder can open that file and see all your keys. After this change, that file is encrypted — your keys are unreadable to anyone without the right access, even if the file itself leaks. No action needed from you; it happens automatically when you upgrade.
Reproduction steps (current behavior on
main)config.yaml:BWS_ACCESS_TOKEN, start Hermes once.<hermes_home>/cache/bws_cache.json— contains your secret values in plaintext (e.g."value": "sk-...").Current: plaintext
bws_cache.jsonwritten by default.Expected: only
bws_cache.enc.json(AES-GCM ciphertext) exists; plaintext is never written, and any pre-existing plaintext file is re-encrypted and removed on first read.How to test
scripts/run_tests.sh tests/test_bitwarden_secrets.py→ 19 passed (3 new regression tests: encrypted-only default, legacy migration, offline-start migration).bws_cache.json, start Hermes once and confirm the file is gone andbws_cache.enc.jsonexists with no plaintext substrings.Platforms tested
test_install_bws_happy_pathfixture zip member naming,chmod 0o600not enforced on Windows — both fail on pristinemaintoo).scripts/run_tests.shused;git diff --checkclean;check-windows-footguns.pyclean on changed files.Related
Part of #77165