Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary — PR #35798
Verdict: Comment (Draft PR — preliminary review)
Author: zapabob | Type: security/hardening | Files: tools/environments/local.py, tests/tools/test_local_env_blocklist.py, tests/test_subprocess_home_isolation.py
Review Findings
✅ Looks Good
- All added env vars correctly belong in the subprocess blocklist (DingTalk, Feishu, WeCom, Weixin, Yuanbao secrets, matrix password, Twilio auth, QQ STT API key, terminal SSH key, Langfuse secret key).
test_secret_extra_env_keys_are_in_blocklistis an excellent invariant test that prevents future_EXTRA_ENV_KEYSentries from leaking out of the blocklist. It scans for keys containing sensitive markers (TOKEN, SECRET, PASSWORD, API_KEY, etc.).test_gateway_credentials_are_stripped_from_all_local_env_pathstests both_make_run_envand_sanitize_subprocess_envcode paths — good coverage.- Windows-native test fix (path parts instead of
endswith) is sensible and improves portability. - The
pytest.mark.skipif(sys.platform == "win32")on the Homebrew test prevents a test failure on native Windows.
ℹ️ Note: This PR is a draft. If #35769 lands first, this branch can be rebased down. The invariant test and Windows fix are the most valuable standalone pieces.
Reviewed by Hermes Agent
5078002 to
ff5c81d
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (Draft PR — note: this PR is currently a draft)
✅ What's Good
- Comprehensive subprocess env scrubbing: Adds ~25 new credential env vars to
_HERMES_PROVIDER_ENV_BLOCKLIST, covering DingTalk, Feishu, WeCom, Weixin, Yuanbao, Matrix, Twilio, QQ STT, Terminal SSH, and Langfuse. This closes real subprocess credential leakage vectors. - Invariant test:
test_secret_extra_env_keys_are_in_blocklistdetects future drift between_EXTRA_ENV_KEYSand the blocklist — excellent engineering discipline. - Dual-path coverage: The new
test_gateway_credentials_are_stripped_from_all_local_env_pathstests BOTH_make_run_envand_sanitize_subprocess_env— thorough. - Windows portability: Good use of
Path.partsandsys.platform == "win32"skip to support native Windows test runs.
💡 Suggestions (non-blocking)
- Consider extracting the credential names into a shared data structure rather than maintaining two parallel lists (blocklist + tests). The invariant test helps, but a single source of truth would be ideal for future PRs.
Summary
This is a well-structured security hardening PR. The invariant test is especially valuable for preventing future drift. I'll leave this as COMMENT since it's still a draft and depends on #35769.
Reviewed by Hermes Agent
|
Thanks for the focused subprocess-hardening work. The terminal/background premise still holds on current main for residual Problems
Suggested changes
Automated hermes-sweeper review. |
|
Bound to the child-process credential-inheritance class under #83565 (#83565) — same bug class, different surface. gateway secrets from subprocess env; Wave B — shares tools/environments/local.py with the anchor fix. The EPIC carries the live class table, dedup adjudication, and the dependency-driven merge order. |
Summary
This red-team hardening extends subprocess environment scrubbing for gateway/runtime secrets.
_EXTRA_ENV_KEYSthat were still missing from_HERMES_PROVIDER_ENV_BLOCKLIST:LANGFUSE_SECRET_KEY,QQ_STT_API_KEY, andTERMINAL_SSH_KEY._EXTRA_ENV_KEYSdo not drift out of the subprocess blocklist.Relationship to #35769
This is intentionally still a draft while #35769 is open. If #35769 lands first, I will rebase this branch down to the non-duplicate residual slice only: the
_EXTRA_ENV_KEYSsecret coverage, the invariant test, and the Windows-native test portability fixes.Complementary context:
Verification
Fresh checks after rebasing onto current
upstream/main(1fc7bdc5e):uv run --extra dev ruff check tools/environments/local.py tests/tools/test_local_env_blocklist.py tests/test_subprocess_home_isolation.py->All checks passed!uv run --extra dev python -X utf8 -m pytest tests/tools/test_local_env_blocklist.py tests/test_subprocess_home_isolation.py -q --timeout-method=thread->39 passed, 1 skippeduv run --extra dev ruff check .->All checks passed!git diff --check upstream/main..HEAD-> no whitespace errors