fix: redact secrets in non-code files (.env, .yaml, .json) - #35246
fix: redact secrets in non-code files (.env, .yaml, .json)#35246zhangjinzan1 wants to merge 1 commit into
Conversation
Previously, `redact_sensitive_text` was called with `code_file=True` for ALL files in `read_file` and `search_files` tools. This skipped the ENV-assignment and JSON-field regex patterns globally, which meant secrets in config files like `.env`, `.yaml`, `.toml`, and `.json` were not properly redacted. For example, `WEIXIN_TOKEN=abcdef123456` in a `.env` file would only be partially masked (by prefix patterns) instead of fully redacted. Fix: introduce `_is_source_code_file(path)` that checks the file extension against a known set of source-code extensions. Only source-code files (`.py`, `.js`, `.go`, `.rs`, etc.) use `code_file=True`; config and env files use `code_file=False` so that ENV-assignment and JSON-field patterns are applied. This preserves the original intent (avoid false positives in source code like `MAX_TOKENS=100`) while properly protecting secrets in config files.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary: PR #35246
Verdict: APPROVE ✅
Overall: Precise, well-scoped fix with excellent PR description (before/after table, rationale, edge case documentation).
✅ Looks Good
- Correct approach: The
code_file=Truedefault was a classic "works for the common case" shortcut that became a security gap. Using file-extension heuristics to choose the right redaction mode is far more robust. - Comprehensive extension set — 45+ source-code extensions covering all major languages. The set includes
.sql,.pyi,.pyx,.vue,.svelte,.erb, and shell variants — this is more thorough than needed for the immediate bug and future-proof. frozenset+ lowercase extension — efficient lookup, case-insensitive, immutable (won't accidentally grow at runtime)code_file=Falseby inference — all non-source-code files (.env,.yaml,.json,.toml,.ini,.cfg,.properties,.conf,.md,.txt, etc.) naturally get full redaction- Two call sites updated — both
read_fileandsearch_filesnow use the same heuristic, maintaining consistency m.pathin search_files — correctly uses the match result's file path (not the search path parameter)- All 29 existing tests pass — no regressions
💡 Suggestions
- Consider adding
.tf(Terraform),.tfvars,.hcl,.dockerfile, and.Dockerfileto the extension set — these are configuration-as-code files that can contain secret-like constants (e.g.,var.api_key = "test") - Add a test that explicitly verifies a
.envfile content is fully redacted while a.pyfile withMAX_TOKENS=3000is not — this PR body states it works but adding a regression test would prevent future regressions - Consider edge case: a file named
Dockerfile(no extension) —os.path.splitext("Dockerfile")returns("Dockerfile", ""), so it would getcode_file=False. This is fine since Dockerfiles can contain ENV assignments with secrets, but worth documenting
Testing Completeness
- 29/29 existing tests pass
- 155 redact-related tests pass, 3 skipped
- Manual verification with
.envvs.pyfiles
Reviewed by Hermes Agent
Code Review Summary: PR #35246Verdict: APPROVE ✅ Overall: Precise, well-scoped fix with excellent PR description (before/after table, rationale, edge case documentation). ✅ Looks Good
💡 Suggestions
Testing Completeness
Reviewed by Hermes Agent |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Overall
Smart, focused security fix. The code_file=True blanket was causing secrets in non-code files to bypass proper redaction, since env-assignment and JSON-field regex patterns were skipped globally. The fix introduces a targeted source-code extension check so config files get full coverage.
What's Good
- Correct diagnosis: The issue is clearly identified —
redact_sensitive_text(code_file=True)was called for ALL files, bypassing ENV/JSON patterns everywhere - Comprehensive extension set: Covers 30+ source-code extensions including shell scripts (
.sh,.bash), SQL, and frontend frameworks - Dual-path fix: Both
read_file_toolandsearch_toolare updated (which use different call paths) - Minimal diff: Only 53 lines added, no architecture changes
- Backward-compatible: Source files keep the same behavior; only non-code files change
- Well-documented: Clear docstring explaining why source code skips ENV patterns, and the before/after table in PR body is excellent
No Issues Found
- The
_SOURCE_CODE_EXTENSIONSset covers language extensions comprehensively lower()normalization prevents case-sensitivity edge casesos.path.splitextcorrectly handles paths with multiple extensions (e.g.,.spec.tsstays.ts)- No existing tests break (all 155 redact tests pass)
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the non-code-file redaction gap. The premise remains present on current main: file_read=True forces code_file=True in agent/redact.py:541-542, so ENV/JSON assignment passes remain skipped for file-tool output.
Problems
- The proposed calls replace current main's
file_read=Truemode. That would discard the non-reusable prefix-secret sentinel introduced inde928bccdand restore reusable head/tail masks for config-file credentials (agent/redact.py:464-488,541-547). - Current main has three file-content paths—
tools/file_tools.py:1279,1413, and2003—while this older patch only addresses the two original paths. - No regression tests cover the path-sensitive behavior.
Suggested changes
- Preserve
file_read=Truesemantics while designing a path-aware assignment-redaction mode, and cover all three current call sites. - Add end-to-end file-tool tests for opaque non-source secrets, source-code false positives, and prefix-secret sentinels.
Automated hermes-sweeper review.
| # ── Redact secrets (after guard check to skip oversized content) ── | ||
| if result.content: | ||
| result.content = redact_sensitive_text(result.content, code_file=True) | ||
| result.content = redact_sensitive_text( |
There was a problem hiding this comment.
Current main uses file_read=True for all file-content paths (tools/file_tools.py:1279,1413,2003), which enables the non-reusable prefix-secret sentinel in agent/redact.py:541-547. Replacing it with code_file=... here would restore reusable head/tail masks for config credentials; preserve the file_read security behavior when rebasing this change.
Problem
redact_sensitive_textis called withcode_file=Truefor ALL files in bothread_fileandsearch_filestools (seetools/file_tools.pylines 620 and 1045). This skips the ENV-assignment and JSON-field regex patterns globally, which means secrets in config files like.env,.yaml,.toml, and.jsonare not properly redacted.For example,
WEIXIN_TOKEN=abcdef123456in a.envfile would only be partially masked (by prefix patterns) instead of fully redacted by the ENV-assignment pattern.Fix
Introduce
_is_source_code_file(path)that checks the file extension against a known set of source-code extensions (.py,.js,.go,.rs,.ts,.java, etc.). Only source-code files usecode_file=True; config and env files usecode_file=Falseso that ENV-assignment and JSON-field patterns are applied.This preserves the original intent (avoid false positives in source code like
MAX_TOKENS=100) while properly protecting secrets in config files.Testing
test_file_tools.pytests pass (29/29).envfile contents are now fully redacted,.pyfiles still skip ENV patternsBefore/After
code_file=True)code_file=False).envWEIXIN_TOKEN=abcdef...abcdef...7890***.envXIAOMI_API_KEY=sk-....pyMAX_TOKENS=100