fix(#6165): tighten auth_header redactor pattern to prevent JSON corruption - #6166
fix(#6165): tighten auth_header redactor pattern to prevent JSON corruption#6166fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 6:57 AM UTC · Completed 7:13 AM UTC Commit: |
ReviewFindingsLow
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsLow
Labels: PR modifies internal security scanner/redactor code and fixes a documented bug |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
/fs-fix fix CI issue and review comments |
|
🤖 Finished Fix · ✅ Success · Started 8:41 AM UTC · Completed 8:53 AM UTC Commit: |
🔧 Fix agent — iteration 1 (human-triggered)Fixed CI failure caused by a test expectation that encoded the old buggy auth_header behavior (double-masking and quote-eating). The test now expects the correct output where the prefix pattern redacts the token and structural delimiters are preserved. Acknowledged the reviewer's low-severity finding about the 8-to-16 minimum as an intentional tradeoff per #6165. Fixed (1):
Disagreed (1):
Tests: passedNext steps:
|
The auth_header pattern change in the previous commit (8→16 char minimum, excluding structural delimiters) correctly stops the regex from double-masking already-redacted prefix output and eating the closing quote. Update TestExtractSafeContext to match the new (correct) behavior: the ghp_ prefix pattern redacts the token to "ghp_..." and the closing quote is preserved. Addresses review feedback on #6166
|
🤖 Review · ❌ Terminated · Started 8:54 AM UTC · Ended 9:06 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 8:54 AM UTC · Completed 9:06 AM UTC Commit: |
…uption
The auth_header structural pattern used \S{8,} as its capture group,
which matches any non-whitespace character including JSON structural
delimiters (", }, ], etc.). When the matched text reached the end of
a JSON string value, the capture group consumed the closing quote,
and mask() replaced it, destroying JSON validity.
Two changes to the regex:
1. Replace \S{8,} with [^\s"'}\]),;]{16,} — excludes JSON/YAML
structural delimiters from the capture group, preventing the
capture from eating past string boundaries.
2. Raise minimum match length from 8 to 16 — short technical
references like "token_value" in conceptual discussion of
auth headers are not real secrets. Real auth tokens are
almost always 20+ characters.
Added four test cases covering: false positive rejection on short
technical references, JSON structure preservation after redaction,
real secret detection, and the specific regression where \S ate
past JSON closing quotes.
Closes #6165
The auth_header pattern change in the previous commit (8→16 char minimum, excluding structural delimiters) correctly stops the regex from double-masking already-redacted prefix output and eating the closing quote. Update TestExtractSafeContext to match the new (correct) behavior: the ghp_ prefix pattern redacts the token to "ghp_..." and the closing quote is preserved. Addresses review feedback on #6166
a1d49d9 to
eadbe1f
Compare
|
🤖 Finished Review · ✅ Success · Started 8:18 AM UTC · Completed 8:34 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 10:03 AM UTC · Completed 10:09 AM UTC Commit: |
Narrows the redaction coverage gap for 12-15 character credentials per reviewer feedback, while still preventing the documented false positive on short technical references like 'token_value' (11 chars). Addresses review feedback on #6166
🔧 Fix agent — iteration 2 (human-triggered)Addressed 1 of 2 review findings. Lowered auth_header minimum match length from 16 to 12 to narrow the redaction gap while still preventing false positives. Disagreed with 1 finding (follow-up issue for env_assignment is out of scope for this PR). Fixed (1):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:11 AM UTC · Completed 10:25 AM UTC Commit: |
| {"env_assignment", `(?i)(?:^|\s)(?:export\s+)?((?:[A-Za-z0-9]+_)*(?:KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|PASSWD|AUTH|API_KEY)(?:_[A-Za-z0-9]+)*)\s*=\s*['"]?([^\s'"]{8,})['"]?`}, | ||
| {"json_field", `(?:"[^"]*(?i:key|token|secret|password|credential|auth)[^"]*"|'[^']*(?i:key|token|secret|password|credential|auth)[^']*')\s*:\s*(?:"([^"]{8,})"|'([^']{8,})')`}, | ||
| {"auth_header", `(?i)(?:Authorization|X-Api-Key|X-Auth-Token)\s*:\s*(?:Bearer\s+)?(\S{8,})`}, | ||
| {"auth_header", `(?i)(?:Authorization|X-Api-Key|X-Auth-Token)\s*:\s*(?:Bearer\s+)?([^\s"'}\]),;]{12,})`}, |
There was a problem hiding this comment.
[low] scope-divergence
The PR body states the minimum match length was raised to 16, but the code implements {12,}. The linked issue (#6165) also suggested auditing env_assignment for the same \S boundary bug, which is deferred. Consider updating the PR description and filing a follow-up for the env_assignment audit.
Suggested fix: Update PR description to reflect the actual minimum (12). Consider filing a follow-up issue for auditing other structural patterns for the same \S boundary bug.
| {"env_assignment", `(?i)(?:^|\s)(?:export\s+)?((?:[A-Za-z0-9]+_)*(?:KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|PASSWD|AUTH|API_KEY)(?:_[A-Za-z0-9]+)*)\s*=\s*['"]?([^\s'"]{8,})['"]?`}, | ||
| {"json_field", `(?:"[^"]*(?i:key|token|secret|password|credential|auth)[^"]*"|'[^']*(?i:key|token|secret|password|credential|auth)[^']*')\s*:\s*(?:"([^"]{8,})"|'([^']{8,})')`}, | ||
| {"auth_header", `(?i)(?:Authorization|X-Api-Key|X-Auth-Token)\s*:\s*(?:Bearer\s+)?(\S{8,})`}, | ||
| {"auth_header", `(?i)(?:Authorization|X-Api-Key|X-Auth-Token)\s*:\s*(?:Bearer\s+)?([^\s"'}\]),;]{12,})`}, |
There was a problem hiding this comment.
[low] defense-in-depth-gap
Raising the minimum from 8 to 12 means secrets of 8-11 characters in auth headers will no longer be caught by this structural pattern. Practical impact is minimal since real auth tokens are typically 20+ characters and known token formats are independently caught by prefix patterns.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 11:56 AM UTC · Completed 12:03 PM UTC Commit: |
🔧 Fix agent — iteration 3 (human-triggered)No code changes needed. Both outstanding review findings are informational/low severity and the review approved the PR. The scope-divergence finding asks to update the PR body to reflect the actual minimum (12 instead of 16) — attempted but blocked by sandbox API restrictions. The defense-in-depth-gap finding is informational with no action suggested. All tests and CI checks pass. Disagreed (2):
Tests: passedNext steps:
|
1 participant
Summary
Tightens the
auth_headerstructural pattern ininternal/security/redactor.goto fix two compounding bugs: (1) the\S{8,}capture group consumed JSON structural characters (",},]), corrupting JSON output aftermask()replaced the match, and (2) the 8-character minimum triggered false positives on short technical references in triage agent output discussing auth headers.Changes
(\S{8,})with([^\s"'}\]),;]{16,})in theauth_headerregex — excludes JSON/YAML structural delimiters from the capture group and raises the minimum match length from 8 to 16 characters\Seating past JSON closing quotes regressionTesting
internal/security/passgo vetpassesdefaultStructuralPatternsCloses #6165
Post-script verification
agent/6165-fix-auth-header-pattern)3c7f5d034d3f9ace8efd3608a6322903da907809..HEAD)