fix(detector): calibrate WEAK_CIPHER (kill .includes false positives) + unique IDs + downgrade test findings#17
Merged
Conversation
…e findings
User feedback: the internal detector "shouted fire because it saw the word match
in the dictionary." On a real project it flagged args.includes('--no-update-check')
and ids.includes('postgres') as CRITICAL Weak Cipher, because the WEAK_CIPHER regex
matched the "des" substring in inclu-des / exclu-des / mo-des (the bare
(?:DES|RC4|Blowfish|ECB)\b alternative: case-insensitive, no leading boundary).
150 of 192 detector findings on csreview's own code were this FP, cratering the
score to 0/100.
- WEAK_CIPHER is now context-bound: only real crypto APIs (createCipheriv,
Cipher.getInstance, CryptoJS.*, <Algo>.new(), MODE_ECB, algorithm/cipher config)
with proper word boundaries. Never a bare cipher-name substring.
- Deterministic, globally-unique finding IDs (the old ${id}_${index} scheme
collided across files, e.g. WEAK_CIPHER_0 in every file).
- Findings in non-source paths (tests/fixtures/examples/docs) are downgraded to
LOW + confidence LOW so intentional test vectors and example keys do not crater
the score; they stay visible rather than hidden.
Effect on csreview's own tree: 192 -> 51 detector findings, 0 duplicate IDs, the
.includes() false positives gone. Recall preserved (real weak-cipher usage still
flagged). Remaining src CRITICAL/HIGH are csreview matching its own rule
definitions in detector.js (only when scanning csreview itself).
Tests: +4 calibration + 3 negative-corpus guards. 175/175 - lint clean - typecheck 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Calibrate the internal detector (kill the WEAK_CIPHER false positives)
Driven by real user feedback (thanks @thiago): "the internal detector is shouting fire because it saw the word match in the dictionary." On a real project it flagged
args.includes('--no-update-check')andids.includes('postgres')as CRITICAL Weak Cipher — dropping the score to 0/100 and making the report untrustworthy.Root cause
The
WEAK_CIPHERregex had a bare(?:DES|RC4|Blowfish|ECB)\balternative — case-insensitive, no leading word boundary — so it matched thedessubstring insideincludes,excludes,modes, etc. 150 of 192 detector findings on csreview's own code were this single FP.Fixes
WEAK_CIPHERis now context-bound — it only matches real crypto APIs (createCipheriv,Cipher.getInstance,CryptoJS.*,<Algo>.new(,MODE_ECB,algorithm:/cipher:config) with proper word boundaries. Never a bare substring. (addresses suggestion chore(deps): bump actions/checkout from 5 to 6 #2)${id}_${index}scheme reset per file, soWEAK_CIPHER_0appeared in every file. (addresses [codex] Expose canonical SKILL.md on README #5)tests/,fixtures/,examples/,docs/,*.test.*,*.spec.*are downgraded to LOW + confidence LOW, so intentional test vectors / example keys don't crater the score (they stay visible, not hidden). (addresses [codex] Clarify CSReview scope and add GitHub security validation #1)Score (#4) recovers naturally once the bogus CRITICALs are gone; the severity cap is a deliberate design choice. Origin-grouping in the report (#3) — findings already carry
source+confidence; a grouped view can follow up.Effect (measured on csreview's own tree)
192 -> 51detector findings ·0duplicate IDs · the.includes()FPs gone · recall preserved (real weak-cipher usage still flagged). The residualsrc/CRITICAL/HIGH are csreview matching its own rule definitions indetector.js(only when auditing csreview itself — not a user project).Tests: +4 calibration + 3 negative-corpus guards. 175/175 · lint clean · typecheck 0.
🤖 Generated with Claude Code