Repository navigation
refactor(security): harden api_key_leak_scanner (redaction, streaming, CI exit codes) - #1
Conversation
krishsharma-code
left a comment
krishsharma-code
left a comment
There was a problem hiding this comment.
Review: api_key_leak_scanner hardening
Reviewed: python_scripts/api_key_leak_scanner.py at head c521a53 (plus the new tests/ suite).
Scope. The script end to end - path/input handling, the regex pattern set, file reading, output and the CLI contract - checked against the security boundary (does the tool leak what it finds?), memory use, failure handling, and docstring/comment clarity.
What was found
- Security - output re-leaked the secret.
print(f" Match: {match.group(0)[:30]}...")echoed the first 30 characters of the matched credential to stdout. Scanner output is routinely captured in CI logs and pasted into reports, so the tool re-exposed the very secret it exists to protect. - Memory - whole-file reads.
content = f.read()loaded every file fully into memory before matching, with no size bound. - Failure handling - no target validation, no exit code. A non-existent path walked to nothing and printed "no leaks" (a false negative), and the process always exited 0, so it could never gate CI.
- Standards - argument handling and docs. Raw
sys.argvinstead ofargparse/--help(CONTRIBUTING.md), and only a one-line docstring.
What changed and why
- Matched values are redacted through
mask_line/_secret_span, so the report keeps surrounding context but never the credential. Covered bytest_scan_file_redacts_the_secretandtest_mask_line_removes_only_the_secret. - Files are read line by line; binaries are skipped via a NUL-byte sniff and files over 5 MiB are skipped, bounding memory.
- The target is validated (exit 2) and the scanner now returns 0/1/2, making it a usable CI gate. Covered by the exit-code tests.
argparse+--help, module/function docstrings and named constants replace the ad-hoc handling.
Behaviour is preserved except for the three genuine defects above (leaked secret, silent false negative, unusable exit code).
Self-review note: GitHub blocks self-approval on one's own PR, so this is submitted as a COMMENT review rather than an approval.
| the credential itself is never part of the returned string. | ||
| """ | ||
| start, end = _secret_span(match) | ||
| redacted = f"{line[:start]}{REDACTION_MARKER}{line[end:]}" |
There was a problem hiding this comment.
Core fix. The previous code printed match.group(0)[:30] - the first 30 characters of the credential - which re-leaked the secret into stdout/CI logs. Replacing only the captured secret span with ***REDACTED*** keeps the line context a reviewer needs while never emitting the value. _secret_span deliberately walks groups in reverse so the generic pattern's secret group (group 2) is redacted rather than the keyword group.
| return findings | ||
|
|
||
| size = path.stat().st_size | ||
| if size > MAX_FILE_BYTES: |
There was a problem hiding this comment.
Bound on memory: anything above MAX_FILE_BYTES (5 MiB) is skipped with a visible note. This prevents a stray log/archive from being slurped into memory and is easy to tune in one place.
| print(f"[!] Skipping large file ({size} bytes): {path}") | ||
| return findings | ||
|
|
||
| if _looks_binary(path): |
There was a problem hiding this comment.
Content-based binary detection, independent of extension: a NUL byte in the first KiB means it is not text worth scanning. This catches mislabelled binaries that the old extension-only filter would have read with errors='ignore'.
| return findings | ||
|
|
||
| with path.open("r", encoding="utf-8", errors="ignore") as handle: | ||
| for line_no, line in enumerate(handle, start=1): |
There was a problem hiding this comment.
Streaming read replaces content = f.read(). Matching is line-oriented, which is correct here because every signature is single-line; memory stays flat regardless of file size.
| print("=== API Key Leak Scanner ===") | ||
| print(f"[*] Scanning directory: {target}") | ||
|
|
||
| if not target.is_dir(): |
There was a problem hiding this comment.
Fixes a false negative: previously a bad path produced an empty walk and printed "No hardcoded API keys or secrets detected", which reads as a clean result. Now a non-directory target is reported and surfaced as exit code 2.
|
|
||
| findings = scan_directory(target) | ||
| report(findings) | ||
| return 1 if findings else 0 |
There was a problem hiding this comment.
Exit-code contract (0 clean / 1 findings / 2 error). This is what makes the scanner usable as a CI gate - the old script always returned 0, so a build could never fail on a detected secret.
Summary
Hardens
python_scripts/api_key_leak_scanner.pyand adds the first pytest suite for the Python scripts. The scanner previously re-leaked the very secrets it detected; it now redacts matches, streams files instead of reading them whole, validates its target and returns a real exit code so it can gate CI.Linked Issue
N/A - raised directly by the recurring code-quality audit (no tracking issue opened).
Type of Change
What Changed
match.group(0)[:30]- the first 30 characters of the matched secret - into stdout, which is routinely captured in CI logs and pasted into reports. Output now shows a redacted context snippet (***REDACTED***) that preserves the surrounding line but never the credential.f.read()-ing the whole file; binary files (NUL-byte sniff) and files over 5 MiB are skipped.sys.argvhandling withargparse+--help; added module and function docstrings and comments; centralised patterns and skip lists as named constants.tests/folder with a 17-case pytest suite covering detection, redaction, skip rules and exit codes.New or Modified Scripts
python_scripts/api_key_leak_scanner.pytests/conftest.pypython_scripts/tosys.path)tests/test_api_key_leak_scanner.pySafety Review
--dry-run(n/a - read-only scan)Testing Checklist
python3 -m py_compile python_scripts/*.pypassesflake8 python_scripts --select=E9,F63,F7,F82,F821,F823 --max-line-length=120passesfor f in bash_scripts/*.sh; do bash -n "$f"; donepasses (no shell files changed)shellcheck bash_scripts/*.shreports no errors (no shell files changed)python3 -m pytest- 17 passed)--helpoutputDocumentation
README.md(index entry already exists; its description update is noted below)Code Quality Checklist
Additional Notes
py_compileandflake8only; it does not execute pytest. The new suite runs locally withpython3 -m pytest. Wiring it intoci.ymlis proposed as a follow-up so the tests do not become dead code.README.mdindex line for this script still carries the original one-liner; the description update was split out to keep this PR's diff focused.