fix(tools): reduce false positives in exfil_curl/exfil_wget patterns - #63994
Closed
liuhao1024 wants to merge 1 commit into
Closed
fix(tools): reduce false positives in exfil_curl/exfil_wget patterns#63994liuhao1024 wants to merge 1 commit into
liuhao1024 wants to merge 1 commit into
Conversation
Anchor env var name matches with \b to avoid matching legitimate env vars that contain KEY/TOKEN/API as substrings (e.g., $TRILLIUM_ETAPI_URL). The patterns now require KEY/TOKEN/SECRET/PASSWORD to appear at the END of the env var name, reducing false positives on common API-usage documentation in SOUL.md while still catching actual exfiltration attempts. Fixes NousResearch#63977
alt-glitch
added
type/bug
teknium1
reviewed
Jul 16, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for isolating the boundary false positive. The underlying problem is still present on current main: tools/threat_patterns.py:120-121 uses the unbounded keyword match, and agent/prompt_builder.py:61-64 replaces an entire context file after a context-scope finding.
Problems
- The changed expressions remain
scope="all". All-scope entries are added tocontext_patternsintools/threat_patterns.py:185-188, so a normal$CLOUDFLARE_API_TOKENAPI recipe still triggers the whole-file block described in #63977. Related PR #64053 identifies scope narrowing as the competing root-cause approach. - The new alternative set drops
CREDENTIALandAPI, which are present in the current expressions attools/threat_patterns.py:120-121; this weakens direct$CREDENTIAL/$APIdetection without coverage. - The new “legitimate” tests do not contain
$TRILLIUM_ETAPI_URLor another environment variable, so they pass on current main and do not exercise the reported regression.
Suggested changes
- Decide the context-vs-strict scope policy, then add direct
$TRILLIUM_ETAPI_URLcurl/wget cases and tests for the intended$CLOUDFLARE_API_TOKENcontext behavior. - Preserve or deliberately replace the removed
CREDENTIAL/APIsuffix coverage with positive tests.
Automated hermes-sweeper review.
| (r'wget\s+[^\n]{0,2048}\$\{?\w*(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|API)', "exfil_wget", "all"), | ||
| # Anchor env var name end with \b to avoid false positives on legitimate | ||
| # env vars like $TRILLIUM_ETAPI_URL that contain KEY/TOKEN/API as substrings. | ||
| (r'curl\s+[^\n]{0,2048}\$\{?\w*(?:KEY|TOKEN|SECRET|PASSWORD)S?\b', "exfil_curl", "all"), |
Contributor
There was a problem hiding this comment.
This drops CREDENTIAL and API from the current matcher, so direct $CREDENTIAL and $API exfiltration commands stop matching. Please retain suffix-aware coverage for every existing secret category, with positive regression cases.
| # Also, simple curl commands without a secret env var should not match. | ||
| assert "exfil_curl" not in scan_for_threats( | ||
| 'curl -s -H "Authorization: Bearer *** https://api.cloudflare.com/client/v4/zones', | ||
| scope="all" |
Contributor
There was a problem hiding this comment.
This input has no environment-variable interpolation, so it already passes the current expression. Add the reported $TRILLIUM_ETAPI_URL case here (and a wget equivalent) to exercise the boundary change.
teknium1
added
sweeper:risk-security-boundary
|
Superseded by #64724 (same fix). |
Contributor
SummaryTwenty-two PRs address or reference this scanner complex across five separable areas: emoji-aware U+200D handling, user-visible context-block warnings, Mythic/C2 false positives, curl/wget false positives, and adjacent scanner hardening. The diffs range from narrow blocklist or regex edits to shared cross-scanner validation, while notification, C2-policy, exfiltration-policy, and unrelated hardening changes should remain separate. Related pull requests
DuplicatesZWJ cluster: #24339 duplicates #12673; #59668, #59701, #59710, and #59925 duplicate the #59503 mechanism, while #76857 is the broader corrected cross-scanner variant. Notification cluster: #59625 and closed #59708 duplicate #59622, while closed #59918 duplicates the combined #59652 variant. Mythic cluster: closed #44665 duplicates #44638. Exfiltration cluster: #63994 and closed #64053 compete on #63977, but only #64053’s diff addresses both scope and suffix causes. Suggested consolidationKeep #63994 open with a salvage path, consistent with its contributor review: incorporate the root-cause scope decision evidenced by the recorded-best-fix #64053, preserve CREDENTIAL/API coverage or justify narrower replacements, and add direct $TRILLIUM_ETAPI_URL, bearer-token, malicious-context, and prompt-builder regressions; separately verify the reported supersession by #64724 before closing either PR as a duplicate. Consolidate the other independent lanes around #76857 for cross-scanner ZWJ handling, #59622 for user-visible block warnings, and #44638 for Mythic qualification, addressing each visible keep_open review rather than reopening or merging closed alternatives. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I63977(["issue #63977 (open)"])
P63994["PR #63994 (open)"]
P63994 -.->|partial| I63977
class I63977 open
class P63994 open
class P63994 target
click I63977 "https://github.com/NousResearch/hermes-agent/issues/63977"
click P63994 "https://github.com/NousResearch/hermes-agent/pull/63994"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 22 pull requests and 5 issues in this complex. Each diff was read against this issue; Assessment working set: 151 kB of PR diffs, 71 kB of issue/PR text, 38 kB of discussion (60 comments), 52 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Contributor
|
Merged via PR #98322 (rebase merge). Your commit was cherry-picked onto current main with your authorship preserved in git log, and we widened the same fix to the sibling env_exfil_* patterns in the skills-hub install scanner (tools/skills_guard.py), which had the identical unanchored-substring bug. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Reduces false positives in the
exfil_curlandexfil_wgetthreat patterns. The original patterns used\w*which matched any word characters after the curl/wget command, causing legitimate env vars like$TRILLIUM_ETAPI_URL(where "API" is a substring in the middle of the var name) to trigger the pattern. This resulted in legitimate API-usage documentation in SOUL.md files being completely blocked and replaced with[BLOCKED: ...]placeholders.The fix adds
\bword boundary anchors to require KEY/TOKEN/SECRET/PASSWORD to appear at the END of the env var name. This preserves detection of actual exfiltration attempts (where the env var name typically ends with these keywords) while avoiding false positives on common documentation patterns.Related Issue
Fixes #63977
Type of Change
Changes Made
tools/threat_patterns.py: Updatedexfil_curlandexfil_wgetpatterns to use\bword boundary anchorstests/tools/test_threat_patterns.py: Added 4 regression tests:test_exfil_curl_legitimate_api_usage_no_match: Confirms legitimate curl API usage doesn't matchtest_exfil_wget_legitimate_api_usage_no_match: Confirms legitimate wget API usage doesn't matchtest_exfil_curl_key_at_end_matches: Confirms real exfil patterns still matchtest_exfil_wget_key_at_end_matches: Confirms real exfil patterns still matchHow to Test
Run the new tests:
pytest tests/tools/test_threat_patterns.py::TestClassicInjection::test_exfil_curl_legitimate_api_usage_no_match tests/tools/test_threat_patterns.py::TestClassicInjection::test_exfil_wget_legitimate_api_usage_no_match tests/tools/test_threat_patterns.py::TestClassicInjection::test_exfil_curl_key_at_end_matches tests/tools/test_threat_patterns.py::TestClassicInjection::test_exfil_wget_key_at_end_matches -xvsObserved result: All 4 new tests pass
Verify all existing threat pattern tests still pass:
pytest tests/tools/test_threat_patterns.py -qObserved result: All 42 tests pass
Test manually with a SOUL.md file containing legitimate API usage:
curl -s -H "Authorization: Bearer *** https://api.cloudflare.com/client/v4/zones' > /tmp/test_soul.md
Verify it's not blocked
python3 -c "from tools.threat_patterns import scan_for_threats; import sys; content = open('/tmp/test_soul.md').read(); findings = scan_for_threats(content, 'all'); sys.exit(1 if any('exfil' in f for f in findings) else 0); print('NOT blocked' if sys.exitcode == 0 else 'BLOCKED')"
Observed result: "MATCHED: exfil_curl" printed (the pattern still detects actual exfiltration attempts)
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A\w*and\bwhich work consistently across platformsFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs