Fix credential redaction and bidi sanitization gaps - #8
Merged
Conversation
redact(): the Authorization scrubber only handled the Bearer/Token schemes, so "Authorization: Basic <base64>" leaked the credential and Digest-style multi-token values leaked their parameters. Redact everything after the header name to the end of the line. sanitize(): the bidi-control blocklist missed the Cf format controls U+061C (Arabic Letter Mark) and U+08E2 (Arabic Disputed End of Ayah), both of which alter bidi processing when a hostile bio reaches the terminal. Add them to the zero-width/bidi set. Adds targeted regression tests for both; full suite 310 passed.
📝 WalkthroughWalkthroughChangesThe PR expands Text safety
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@githubfetch/api.py`:
- Around line 67-68: Update the Authorization-redaction regex in re.sub to use
horizontal whitespace only around the header delimiter, replacing
newline-permitting spacing with a space/tab character class so redaction cannot
consume subsequent physical lines.
In `@tests/test_api.py`:
- Around line 77-90: Strengthen test_redact_scrubs_arbitrary_auth_scheme and
test_redact_preserves_text_before_header_and_other_lines to verify complete
authorization-value removal: assert realm=example is absent from the Digest
output and trailing text such as rejected is absent from the single-line output,
or assert the exact expected redacted lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23171179-d110-434d-a71d-48a77264af56
📒 Files selected for processing (4)
githubfetch/api.pygithubfetch/sanitize.pytests/test_api.pytests/test_sanitize.py
Comment on lines
67
to
+68
| out = re.sub( | ||
| r"(?i)(authorization[\"']?\s*[:=]\s*[\"']?)(bearer|token)?\s*\S+", | ||
| r"(?i)(authorization[\"']?\s*[:=]\s*[\"']?)[^\n\r]*", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python - <<'PY'
import re
sample = "Authorization: \nNext: keep"
replacement = r"\1***REDACTED***"
current = re.compile(
r"(?i)(authorization[\"']?\s*[:=]\s*[\"']?)[^\n\r]*"
)
fixed = re.compile(
r"(?i)(authorization[\"']?[ \t]*[:=][ \t]*[\"']?)[^\n\r]*"
)
assert current.sub(replacement, sample) == "Authorization: \n***REDACTED***"
assert fixed.sub(replacement, sample) == (
"Authorization: ***REDACTED***\nNext: keep"
)
PYRepository: alptekinege/githubfetch
Length of output: 161
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## File existence and line count"
if [ -f githubfetch/api.py ]; then
wc -l githubfetch/api.py
else
fd -a 'api.py$' . | sed 's#^\./##'
fi
echo
echo "## githubfetch/api.py outline"
ast-grep outline githubfetch/api.py || true
echo
echo "## Relevant lines 40-90"
sed -n '40,90p' githubfetch/api.py | cat -n
echo
echo "## Search for redact usages and tests"
rg -n "def redact|redact\(|Authorization|authorization|authorization" githubfetch tests . 2>/dev/null | head -200Repository: alptekinege/githubfetch
Length of output: 7643
Keep Authorization header redaction on the same physical line.
The regex allows \s* before and after :/=, so Authorization: \nNext: keep can consume Next: keep as part of the header value and redact unrelated output. Restrict delimiter spacing to horizontal whitespace, e.g. [ \t]*.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@githubfetch/api.py` around lines 67 - 68, Update the Authorization-redaction
regex in re.sub to use horizontal whitespace only around the header delimiter,
replacing newline-permitting spacing with a space/tab character class so
redaction cannot consume subsequent physical lines.
Comment on lines
+77
to
+90
| def test_redact_scrubs_arbitrary_auth_scheme(): | ||
| out = redact("authorization = \"Digest realm=example, nonce=deadbeef\"") | ||
| assert "nonce=deadbeef" not in out | ||
| assert "Digest" not in out | ||
|
|
||
|
|
||
| def test_redact_preserves_text_before_header_and_other_lines(): | ||
| out = redact("Error 401: Authorization: Bearer abc123 rejected") | ||
| assert "abc123" not in out | ||
| assert "Error 401:" in out | ||
| multiline = redact("GET /users HTTP/1.1\nAuthorization: Bearer abc123\nAccept: */*") | ||
| assert "abc123" not in multiline | ||
| assert "GET /users HTTP/1.1" in multiline | ||
| assert "Accept: */*" in multiline |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert removal of the complete authorization value.
The Digest test does not assert that realm=example is removed. The single-line test does not assert that trailing text such as rejected is removed. Add these assertions, or assert the exact redacted line, so a partial redaction cannot pass.
Suggested test additions
out = redact("authorization = \"Digest realm=example, nonce=deadbeef\"")
assert "nonce=deadbeef" not in out
+ assert "realm=example" not in out
assert "Digest" not in out
...
out = redact("Error 401: Authorization: Bearer abc123 rejected")
assert "abc123" not in out
+ assert "rejected" not in out📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_redact_scrubs_arbitrary_auth_scheme(): | |
| out = redact("authorization = \"Digest realm=example, nonce=deadbeef\"") | |
| assert "nonce=deadbeef" not in out | |
| assert "Digest" not in out | |
| def test_redact_preserves_text_before_header_and_other_lines(): | |
| out = redact("Error 401: Authorization: Bearer abc123 rejected") | |
| assert "abc123" not in out | |
| assert "Error 401:" in out | |
| multiline = redact("GET /users HTTP/1.1\nAuthorization: Bearer abc123\nAccept: */*") | |
| assert "abc123" not in multiline | |
| assert "GET /users HTTP/1.1" in multiline | |
| assert "Accept: */*" in multiline | |
| def test_redact_scrubs_arbitrary_auth_scheme(): | |
| out = redact("authorization = \"Digest realm=example, nonce=deadbeef\"") | |
| assert "nonce=deadbeef" not in out | |
| assert "realm=example" not in out | |
| assert "Digest" not in out | |
| def test_redact_preserves_text_before_header_and_other_lines(): | |
| out = redact("Error 401: Authorization: Bearer abc123 rejected") | |
| assert "abc123" not in out | |
| assert "rejected" not in out | |
| assert "Error 401:" in out | |
| multiline = redact("GET /users HTTP/1.1\nAuthorization: Bearer abc123\nAccept: */*") | |
| assert "abc123" not in multiline | |
| assert "GET /users HTTP/1.1" in multiline | |
| assert "Accept: */*" in multiline |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_api.py` around lines 77 - 90, Strengthen
test_redact_scrubs_arbitrary_auth_scheme and
test_redact_preserves_text_before_header_and_other_lines to verify complete
authorization-value removal: assert realm=example is absent from the Digest
output and trailing text such as rejected is absent from the single-line output,
or assert the exact expected redacted lines.
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
1 participant
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.
Summary
Review of the recent security-hardening work found two gaps, both now fixed:
1.
redact()leaked non-Bearer/Token authorization credentialsgithubfetch/api.py— theAuthorizationscrubber only redacted the value when preceded by theBearer/Tokenscheme:Authorization: Basic dXNlcjpwYXNz→Authorization: ***REDACTED*** dXNlcjpwYXNz(base64 creds leaked to the terminal)Authorization: Digest realm=x, nonce=deadbeef→ parameters leakedFix: redact everything after
Authorization:through the end of the line — scheme and credential together, regardless of scheme, with no multi-token gaps. Other lines and text before the header are preserved.2.
sanitize_text()missed two bidi format controlsgithubfetch/sanitize.py— the zero-width/bidi blocklist missed theCfformat controls U+061C (Arabic Letter Mark) and U+08E2 (Arabic Disputed End of Ayah, which disables bidi for the rest of the line), both of which passed through hostile profile content to the terminal.Fix: add both to
_ZERO_WIDTH_AND_BIDI(targeted — legitimateCfchars like emoji variation selectors are untouched).Validation
test_api.py,test_sanitize.py)ruff check .andmypycleanSummary by CodeRabbit