fix(review): stop generic_secret_assignment false-flagging self-naming fixture/enum values - #4587
Merged
Merged
Conversation
…g fixture/enum values
Confirmed live false positives across all three repos this rule covers:
metagraphed/gittensory#4524 ("token = default-session-token" / "beta-session-token",
both test fixtures), awesome-claude#4758 ("embedded_secret: unsafe_install_or_secret",
an enum/category label) -- none had a real secret present, yet all closed the PR
outright via the hard-blocking generic_secret_assignment kind.
Root cause: isPlaceholderSecretValue's existing fixture carve-out only recognized an
EXACT two-segment lowercase-hyphenated value assigned to a literal `token` key
(`^[a-z]+-[a-z]+$`) -- missing 3+-segment fixtures and any key name other than `token`
entirely. Replaces it with a narrower, evidence-driven check: a value whose OWN last
segment self-names as a secret kind (ends in -token/-secret/-key/-password/-passwd)
reads as a NAME for a concept, not an opaque credential -- deliberately narrower than
"any multi-segment lowercase phrase" so a real Diceware-style passphrase like
"alpha-bravo-charlie-delta" (an existing, deliberate test case) still correctly flags.
Applied identically across all three independent copies of this heuristic (each
self-contained by design, no cross-package imports): the Worker's src/review/secrets-scan.ts
(feeds the deterministic hard-block gate), src/review/content-lane/security-scan.ts
(awesome-claude content-lane submissions), and review-enrichment/src/analyzers/secret-scan.ts
(REES's own standalone-deployed advisory analyzer) -- REES's rule-application loop
didn't do ANY placeholder filtering for this kind before, so its advisory briefs were
even noisier than the two hard-blocking copies.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
2 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4587 +/- ##
=======================================
Coverage 94.12% 94.12%
=======================================
Files 430 430
Lines 38167 38171 +4
Branches 13917 13918 +1
=======================================
+ Hits 35924 35928 +4
Misses 1585 1585
Partials 658 658
🚀 New features to boost your workflow:
|
JSONbored
added a commit
that referenced
this pull request
Jul 10, 2026
secrets-scan.ts (PR-diff hard-block) and content-lane/security-scan.ts (content-lane hard-block) hand-duplicated the same format-specific patterns and placeholder-value heuristics with no automated pairing between them, despite living under src/ in the same build and deploy. That already caused two independent, live drifts (#4587, #4604) even after a same-day commit edited both files for one change. Extract the shared primitives (SECRET_PATTERNS, GENERIC_SECRET_ASSIGNMENT_PATTERN, hasLongSequentialRun, isPlaceholderSecretValue, hasGenericSecretAssignment, HARD_SECRET_KINDS) into src/review/secret-patterns.ts, imported by secrets-scan.ts, content-lane/security-scan.ts, and safety.ts (which carried its own third copy of HARD_SECRET_KINDS). Pure extraction, no behavior change: the only functional edit is canonicalizing GENERIC_SECRET_ASSIGNMENT_PATTERN to one capture-group shape used consistently by both callers. review-enrichment/src/analyzers/secret-scan.ts (REES) stays untouched and genuinely separate (standalone Railway deploy, deliberately wider rule set). Add a named twin-pair entry to scripts/check-engine-parity.ts (mirroring the #4605 SAFE_URL_TWIN_PAIR/DIFF_FILE_PRIORITY_TWIN_PAIR precedent) that mechanically drift-checks only the subset REES shares with the new module: the isPlaceholderSecretValue algorithm and the hard-kind names that are exact string matches on both sides today (excluding private_key_block/aws_access_key, which REES already names differently, so the check doesn't false-fail on introduction).
7 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 12, 2026
) PR #5346 (a resubmission of #5341) was auto-closed over two inert test-fixture strings that matched the generic_secret_assignment keyword-plus-quoted-value SHAPE but weren't real credentials -- the same heuristic has now caused at least eight prior false-positive incidents (#2613, #3178, #3673, #3866, #4587, #4733, plus several fixture-rewording commits), each patched by narrowing an allowlist rather than fixing the underlying design. REES's own copy of this rule already rates it "medium confidence" ("catches real keys but also the occasional long opaque non-secret"), and content-lane/security-scan.ts's own header states the design principle this violated: a gate that auto-closes with no human queue may only hard-close on a signal unambiguous enough that a false positive is essentially impossible. Split generic_secret_assignment out of HARD_SECRET_KINDS into a new ADVISORY_ONLY_SECRET_KINDS: it still surfaces (a warning-severity possible_secret_assignment finding / a "manual" content-lane verdict), but never auto-blocks or auto-closes on its own. Concrete credential formats (github_token, aws_access_key, private_key_block, ...) are unaffected and remain unconditional hard blockers. Also add a structural placeholder heuristic (looksLikeDescriptive PlaceholderPhrase, mirrored in REES): a value with 5+ lowercase-only hyphen/underscore segments containing an English function word reads as written prose describing the value, not a credential or a chosen passphrase -- this independently resolves both PR #5346 literals without weakening detection of a genuine human-chosen passphrase like "correct-horse-battery-secret" (no function words, by design).
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
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
Confirmed live false positives across all three repos this rule covers — none had a real secret present, yet all closed the PR outright (or, for REES, produced a noisy advisory finding):
token = "default-session-token"/"beta-session-token"(test fixtures)embedded_secret: "unsafe_install_or_secret"(an enum/category label)Root cause:
isPlaceholderSecretValue's existing fixture carve-out only recognized an exact two-segment lowercase-hyphenated value assigned to a literaltokenkey (^[a-z]+-[a-z]+$) — missing 3+-segment fixtures and any key name other thantokenentirely.Fix: a narrower, evidence-driven check — a value whose own last segment self-names as a secret kind (ends in
-token/-secret/-key/-password/-passwd) reads as a NAME for a concept, not an opaque credential. Deliberately narrower than "any multi-segment lowercase phrase": a real Diceware-style passphrase like"alpha-bravo-charlie-delta"(an existing, deliberate test case) still correctly flags.Applied identically across all three independent copies of this heuristic (each self-contained by design, no cross-package imports):
src/review/secrets-scan.ts— feeds the deterministic hard-block gate (safety.ts)src/review/content-lane/security-scan.ts— awesome-claude content-lane submissionsreview-enrichment/src/analyzers/secret-scan.ts— REES's own standalone-deployed (Railway) advisory analyzer, which previously did no placeholder filtering at all for this kind, so its briefs were even noisier than the two hard-blocking copiesTest plan
npm run typecheck, fullnpm run test:coverage,npm run test:ci(all green)review-enrichment's ownnpm test(1220 tests, separate Node test runner + sourcemap/metadata checks) — all green