fix(security): strip BWS token and *_PASSWORD from child-process envs - #77027
fix(security): strip BWS token and *_PASSWORD from child-process envs#77027andrexibiza wants to merge 7 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the child-process environment boundary; current main does allow unlisted *_PASSWORD values through both factories (tools/environments/local.py:469-495, tools/environments/local.py:606-625).
Problems
tools/environments/local.py:401treats every*_ACCESS_TOKENas Hermes-internal. The terminal path checks that predicate before passthrough (tools/environments/local.py:497), and passthrough registration rejects it (tools/env_passthrough.py:88). This blocks legitimate third-party access tokens despite the documented passthrough contract (tools/env_passthrough.py:61-63).- The suffix check does not implement arbitrary Bitwarden remaps:
BitwardenSource.fetch()reads the exact configuredaccess_token_envname (agent/secret_sources/bitwarden.py:916-917), including names not ending in_ACCESS_TOKEN.
Suggested changes
- Protect the exact configured Bitwarden token-variable name rather than the global
_ACCESS_TOKENsuffix. - Add tests for an explicitly passed-through
DB_PASSWORD,inherit_credentials=Trueonhermes_subprocess_env, and a non-suffix Bitwarden token-variable remap.
Automated hermes-sweeper review.
| @@ -391,9 +398,27 @@ def _is_hermes_internal_secret(key: str) -> bool: | |||
| upper.endswith("_SECRET") or upper.endswith("_KEY") or upper.endswith("_TOKEN") | |||
| ): | |||
| return True | |||
| if upper.endswith("_ACCESS_TOKEN"): | |||
There was a problem hiding this comment.
This makes every third-party *_ACCESS_TOKEN non-passthroughable: _sanitize_subprocess_env() checks _is_hermes_internal_secret() before env_passthrough, and tools/env_passthrough.py rejects internal secrets during registration. Please narrow this to the exact configured Bitwarden access_token_env; the current suffix check also misses custom names such as MY_BWS_TOKEN.
There was a problem hiding this comment.
Fixed in 67dc4e8cb5. _is_hermes_internal_secret no longer matches the *_ACCESS_TOKEN suffix — it matches only the exact configured secrets.bitwarden.access_token_env name (per-Hermes-home cached), so third-party tokens like STRIPE_ACCESS_TOKEN are env_passthrough-registerable again, while non-suffix Bitwarden remaps like MY_BWS_TOKEN are stripped exactly. Regression tests: test_bws_token_env_remap_non_suffix_stripped plus the passthrough-survival cases; the per-profile cache sequence (A → B → A, no global resets) is covered by test_bws_token_env_cache_is_scoped_per_profile. Verified at head b7dfe883e8: 38/38 touched tests pass.
| monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.abc123.def456:xyz789") | ||
| monkeypatch.setenv("DB_PASSWORD", "db-pass-9f2c1a") | ||
|
|
||
| env = hermes_subprocess_env() # inherit_credentials=False (default) |
There was a problem hiding this comment.
This only tests the default inherit_credentials=False path. Add coverage for inherit_credentials=True, since the implementation and PR description require *_PASSWORD values to be stripped unconditionally on this factory.
There was a problem hiding this comment.
Fixed in 67dc4e8cb5. test_hermes_subprocess_env_strips_password_with_inherit_credentials covers the non-terminal factory stripping *_PASSWORD unconditionally even when the caller opts into inherit_credentials=True. Verified at head b7dfe883e8: 38/38 touched tests pass (tests/tools/test_build_subprocess_env.py 15 + tests/tools/test_env_passthrough.py 23).
|
Fixed in Both sweeper problems addressed:
Tests added (
Validation: |
|
Both suggested changes are in, fixed in 67dc4e8cb5. 1. Exact configured Bitwarden token-variable name, not the
2. Coverage for the three cases. Added to
Verification: |
…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: *1) Over-broad _ACCESS_TOKEN match (tools/environments/local.py): 2) inherit_credentials=True coverage (tests/tools/test_build_subprocess_env.py): 11/11 test_build_subprocess_env.py tests pass. |
|
suggesting changes The patch does not close the claimed child-process credential boundary. The LocalEnvironment terminal spawn path still propagates arbitrary variables ending in _PASSWORD, and the new Bitwarden token-name cache can reuse one profile's configuration while serving another. Filter password variables in the actual terminal spawn factory and scope Bitwarden token-name resolution to the active Hermes home.
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Both P1s addressed — fixed in P1 #1 — P1 #2 — process-global BWS cache (local.py:413):
Validation: 38/38 tests pass across |
|
Follow-up in |
b7dfe88 to
51a3fc5
Compare
51a3fc5 to
06a7837
Compare
|
suggesting changes The post-review commits close the two earlier findings: the local terminal and non-terminal factories now strip the tested password class, explicit terminal passthrough still works, and the Bitwarden token-name cache follows A→B→A profile switches. The same child-process invariant is still bypassed by the Singularity/Apptainer terminal backend, though. Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
Audit of the two subprocess-env factories (build_subprocess_env for the terminal surface, hermes_subprocess_env for the non-terminal surface) found two credential classes that leaked into children by default: 1. BWS_ACCESS_TOKEN — Hermes's own Bitwarden Secrets Manager bootstrap token. The static provider blocklist doesn't know it (it's not an LLM provider key), so it flowed to every spawned child. The one child that legitimately needs it — the bws CLI — receives it explicitly via build_subprocess_env(scrub_secrets=False) in bitwarden.py, never by inheritance. 2. *_PASSWORD values (DB_PASSWORD, POSTGRES_PASSWORD, REDIS_PASSWORD). Only exact names like EMAIL_PASSWORD were blocklisted; the general credential shape fell through. Fix: _is_hermes_internal_secret now matches *_ACCESS_TOKEN (the BWS token and any access_token_env remap); a new _is_credential_shaped_password predicate strips *_PASSWORD — passthrough-aware on the terminal path (a skill-registered command that legitimately needs the value still gets it via env_passthrough) and unconditional on the non-terminal surface. Why this matters to users: a spawned child process (browser worker, ACP executor, computer-use driver, a shell command) could previously read your Bitwarden vault token and database/service passwords from its environment. Those values no longer cross the process boundary unless a command explicitly needs them. Tests: 8/8 in test_build_subprocess_env.py (2 new E2E regression tests spawn a real child and assert the BWS token + *_PASSWORD are absent); 23/23 env_passthrough tests pass, confirming passthrough semantics are preserved. Pre-existing Windows HOME-semantics failures in test_base_environment.py / test_subprocess_home_isolation.py are unrelated and fail identically on main.
…CESS_TOKEN suffix The over-broad suffix match treated every third-party *_ACCESS_TOKEN as Hermes-internal, blocking legitimate passthrough registration via tools/env_passthrough.py (GHSA-adjacent regression: STRIPE_ACCESS_TOKEN etc. became non-passthroughable). It also missed non-suffix Bitwarden remaps (e.g. access_token_env: MY_BWS_TOKEN), which BitwardenSource.fetch reads by exact configured name. Now _is_hermes_internal_secret matches only the exact configured secrets.bitwarden.access_token_env name (default BWS_ACCESS_TOKEN), cached per process via _get_configured_bws_token_env(). Third-party *_ACCESS_TOKEN vars stay registerable and pass through unchanged. Tests: - non-suffix Bitwarden remap (MY_BWS_TOKEN) is stripped exactly while STRIPE_ACCESS_TOKEN survives and is passthrough-registerable - DB_PASSWORD passthrough on the terminal path is honored - *_PASSWORD stripped unconditionally on hermes_subprocess_env even with inherit_credentials=True
…rminal spawn Addresses egilewski review P1s on NousResearch#77027: 1. _make_run_env (the LocalEnvironment terminal spawn factory) now applies the passthrough-aware _is_credential_shaped_password filter before values land in run_env, closing the live sink where a DB_PASSWORD / REDIS_PASSWORD stayed visible to ordinary terminal commands even though build_subprocess_env and hermes_subprocess_env already stripped them. 2. The Bitwarden access_token_env name cache is now keyed by the active Hermes home instead of a process-global single-entry flag. A gateway serving multiple profiles by switching the context-local home per turn no longer reuses the first profile's token-variable name, so a later profile's differently-named vault bootstrap token is matched and stripped instead of passing into terminal children. Adds unit + E2E LocalEnvironment tests (default denial, explicit passthrough, per-profile remap isolation).
The per-home cache (be5bd41878) correctly makes each profile's configured access_token_env authoritative, but the rule must hold in both directions: the process-global os.environ carries the default profile's BWS_ACCESS_TOKEN across turns (multiplex cron loads each profile's .env into the shared namespace), so a remapped profile's terminal, Docker-passthrough and non-terminal children must still strip the default name. Third-party *_ACCESS_TOKEN vars stay registerable everywhere. Verified by live probes: under profile B (remap MY_BWS_TOKEN), the stale-cache probe now resolves B's own name (STALE=False), MY_BWS_TOKEN is denied in _sanitize_subprocess_env/_make_run_env/hermes_subprocess_env and excluded from the Docker passthrough filter, and BWS_ACCESS_TOKEN is stripped in B's scope as well. 38/38 touched tests pass (15 build_subprocess_env + 23 env_passthrough). Signed-off-by: Andrex Ibiza, MBA <84248988+andrexibiza@users.noreply.github.com>
…lass _is_credential_shaped_password only matched the *_PASSWORD suffix, leaving PGPASSWORD, MYSQL_PWD and bare PASSWORD visible to terminal children even though the execute_code sandbox already scrubs the same class by substring (code_execution_tool.py). The terminal path must be at least as protective as the sandbox for the same secret class. New rule: PASSWORD substring OR *_PWD suffix — never PWD itself (the shell's working-directory variable, which children still receive). Passthrough semantics unchanged (registration is checked before the predicate on the terminal path). Tests extended in test_make_run_env_strips_password_by_default: PGPASSWORD, MYSQL_PWD, PASSWORD denied; PWD and control vars survive. 38/38 touched tests pass; diff-check and Windows footgun lint clean. Signed-off-by: Andrex Ibiza, MBA <84248988+andrexibiza@users.noreply.github.com>
06a7837 to
de0e88d
Compare
|
Confirmed — this is a real remaining sibling bypass. The production-path evidence in this review is sufficient. I am treating the Singularity start/exec environment as part of the same child-process credential boundary. This PR is not ready to claim closure until that path uses the same passthrough-aware sanitizer and carries regression coverage for the listed Bitwarden/password variables plus required non-secret controls. |
…NousResearch#77027) Closes the child-process credential-inheritance class where trusted Hermes credentials and other sensitive parent-environment values reached untrusted or model-authored subprocesses. Confirmed production chain (pinned head de0e88d): SingularityEnvironment._run_bash -> _popen_bash with no sanitized env -> subprocess.Popen inherited the trusted Hermes process environment. Docker, SSH, and future backends converge on the same shared boundary. Changes: - tools/environments/base.py: _popen_bash now builds a sanitized child env by default (build_subprocess_env(base=...)) and applies the same policy to caller-supplied env maps, so omitting env can never re-open ambient inheritance. - tools/environments/local.py: case-insensitive provider/security matching (_is_blocked_provider_env) for Windows env semantics; effective destination keys behind APPTAINERENV_/SINGULARITYENV_ wrappers are checked (_credential_target_env_name); BWS_ACCESS_TOKEN promoted to Tier-1 _ALWAYS_STRIP_KEYS so inherit_credentials=True paths cannot export the vault bootstrap token. - tools/environments/singularity.py: preflight, instance start, image build, and cleanup subprocesses all use the sanitized builder; image construction re-adds ONLY the six explicit Apptainer/Singularity Docker registry-auth variables instead of the full parent environment. - tools/environments/docker.py: explicit docker_forward_env entries may not export Hermes-internal secrets (AUXILIARY_*/GATEWAY_RELAY_*/BWS) — the forwarding list is a capability boundary, not an unconditional bypass. - tools/env_passthrough.py: uses the case-insensitive blocklist predicate. - tests/tools/test_backend_subprocess_env_boundary.py: 12-test regression matrix covering Docker/SSH/Singularity exec, caller-supplied and empty base envs, mixed-case credentials, nested children, Singularity lifecycle, narrow image-build auth, and Docker explicit forwarding. Evidence: - Focused matrix: 12/12 passed (was 2/12 RED on pinned head). - Adjacent suites: 118 passed / 3 failed / 4 skipped; backend suites: 93 passed / 1 failed / 5 skipped — all 4 failures reproduced identically on a pristine pinned-head worktree (pre-existing Windows/MSYS host failures, zero regressions). - Synthetic merge on fresh main (8edcdd1): cumulative diff applied cleanly, focused matrix 12/12 passed. - git diff --check clean; ruff clean on all changed files. Co-authored-by: Axl Ibiza, MBA <andrexibiza@gmail.com>
Closing the child-process credential-inheritance classThis PR closes the bug class where trusted Hermes credentials and other sensitive parent-environment values reach untrusted or model-authored subprocesses. It is the terminal-backend half of a class that spans the whole repository; the sibling PRs below are the other halves, and this comment is the map that ties them together. The confirmed production chain
Why this is the right fix
Evidence
The sibling map — the rest of the classThis PR is one half of a class that spans the repository. The other halves are open PRs and issues that fix the same inheritance pattern on their own surfaces. They are not duplicates of this PR — they are the same bug class on different boundaries — and they should be linked, reviewed, and merged as a coordinated class closure:
Directly interlocked with this PR: #77164, #77193, #77463, #77528, #78033, #78330. Closest siblings (same files, same leak class): #57639 (singularity.py), #59840 (base.py), #73051 / #83007 (local.py + env_passthrough.py). Why this is the best solution
The class is not closed until the siblings land. This PR is the anchor — the boundary fix that makes the terminal backends safe by default, with the evidence and the map to finish the rest. |
|
suggesting changes The PR substantially closes ambient credential inheritance for local, Docker, SSH, and Singularity terminal children and its focused regression suite passes. Two gaps remain: names containing PASSWD still reach model-authored terminal children, and the per-home Bitwarden token-name cache does not refresh after a same-home credential/config rotation.
Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
What changed and why
Closes the child-process credential-inheritance bug class where trusted Hermes credentials and other sensitive parent-environment values reached untrusted or model-authored subprocesses.
Confirmed production chain (pinned head
de0e88d8):SingularityEnvironment._run_bash→_popen_bashwith no sanitizedenv→subprocess.Popeninherited the trusted Hermes process environment. Docker, SSH, and future backends converge on the same shared boundary, so safety cannot depend on every caller remembering to passenv.Changes
tools/environments/base.py—_popen_bashnow builds a sanitized child env by default (build_subprocess_env(base=...)) and applies the same policy to caller-suppliedenvmaps, so omittingenvcan never re-open ambient inheritance.tools/environments/local.py— case-insensitive provider/security matching (_is_blocked_provider_env) for Windows environment semantics; effective destination keys behindAPPTAINERENV_/SINGULARITYENV_wrappers are checked (_credential_target_env_name);BWS_ACCESS_TOKENpromoted to Tier-1_ALWAYS_STRIP_KEYSsoinherit_credentials=Truepaths cannot export the vault bootstrap token.tools/environments/singularity.py— preflight, instance start, image build, and cleanup subprocesses all use the sanitized builder; image construction re-adds ONLY the six explicit Apptainer/Singularity Docker registry-auth variables instead of the full parent environment.tools/environments/docker.py— explicitdocker_forward_enventries may not export Hermes-internal secrets (AUXILIARY_*/GATEWAY_RELAY_*/BWS_ACCESS_TOKEN) — the forwarding list is a capability boundary, not an unconditional bypass.tools/env_passthrough.py— uses the case-insensitive blocklist predicate.tests/tools/test_backend_subprocess_env_boundary.py— 12-test regression matrix covering Docker/SSH/Singularity exec, caller-supplied and empty base envs, mixed-case credentials, nested children, Singularity lifecycle, narrow image-build auth, and Docker explicit forwarding.Why this matters to you as a user
A spawned child process (terminal command, Docker/SSH/Singularity backend, browser worker, ACP executor, model-driving CLI) could previously read your provider API keys, Bitwarden vault token, GitHub token, gateway secrets, and database/service passwords straight from its environment. After this change, those values don't cross the process boundary unless a command explicitly needs them — and on the terminal path, a command that legitimately needs a value (registered via
env_passthrough) still gets it.Reproduction steps (current behavior on
main)export BWS_ACCESS_TOKEN=0.abc123.def456:xyz789andexport DB_PASSWORD=db-pass-9f2c1a.hermes_subprocess_env().Current: both keys present in the child env.
Expected: both absent (BWS token unconditionally;
*_PASSWORDunless explicitly registered for passthrough).How to test
scripts/run_tests.sh tests/tools/test_backend_subprocess_env_boundary.py→ 12 passed (Docker/SSH/Singularity exec boundary, caller-supplied and empty base envs, mixed-case credentials, nested children, Singularity lifecycle, narrow image-build auth, Docker explicit forwarding).scripts/run_tests.sh tests/tools/test_build_subprocess_env.py→ 8 passed (2 new E2E tests spawn a real child viasys.executableand assert the BWS token andDB_PASSWORDare absent; 1 new unit test assertshermes_subprocess_envstrips both).scripts/run_tests.sh tests/tools/test_env_passthrough.py→ 23 passed — confirms a passthrough-registered command still receives its value.Evidence
main: cumulative diff applied cleanly, focused matrix 12/12 passed.git diff --checkclean; ruff clean on all changed files.Part of #83565 — the boundary fix: sanitize-by-default at the shared
_popen_bash(Docker/SSH/Singularity + every future backend). Focused matrix 12/12 GREEN, CI green, mergeable clean.