Skip to content

fix(review): widen the gate's hard-blocking secret scan to match REES's richer ruleset - #2613

Merged
JSONbored merged 1 commit into
mainfrom
fix/gate-secret-scan-widen
Jul 2, 2026
Merged

fix(review): widen the gate's hard-blocking secret scan to match REES's richer ruleset#2613
JSONbored merged 1 commit into
mainfrom
fix/gate-secret-scan-widen

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • src/review/safety.ts's secretLeakFinding() — the ALWAYS-ON, unconditional secret_leak hard blocker that fires for every repo, every PR regardless of any opt-in — only recognized 5 concrete patterns via HARD_SECRET_KINDS: github_token, github_pat, private_key_block, aws_access_key, slack_token.
  • review-enrichment/src/analyzers/secret-scan.ts (feat(enrichment): gitleaks-grade secret scan analyzer with value redaction #1476) already has a richer, higher-recall rule set: Google API keys, JWTs, and a generic secret/password/token-assignment pattern. That richer scanner only fed the AI reviewer's advisory write-up (AI review off by default), never the deterministic hard blocker — so a leaked Google API key, JWT, or a plain password = "..." assignment sailed through the gate clean on every repo.

Scope

  • src/review/secrets-scan.ts — widened SECRET_PATTERNS to add google_api_key and jwt (format-precise, same near-zero false-positive risk as the existing 5) plus generic_secret_assignment — the one keyword-shaped pattern, so it needs real guardrails before becoming an unconditional blocker: isPlaceholderSecretValue rejects placeholder phrases (your-, changeme, redacted, example, <...>, etc.) and low-entropy filler ("xxxxxxxxxxxxxxxx") before a match counts, and the 16+ character floor already excludes short type/schema declarations like password: z.string() (no quoted literal to match at all).
  • src/review/safety.ts — added the 3 new kinds to HARD_SECRET_KINDS.
  • Kept this as an independent copy rather than a cross-package import: review-enrichment deploys standalone on Railway with its own tsconfig/build/test pipeline, so importing across that boundary would break its independence — the same reasoning secrets-scan.ts's own header already documents for staying self-contained relative to reviewbot.
  • Did not touch src/review/content-lane/security-scan.ts, a separate self-contained copy for the unrelated content-submission feature — out of scope for this issue.
  • Tests: all-new-kind positive tests plus explicit false-positive regression tests for every shape the issue calls out (schema/type declarations, placeholder/redacted values, repeated-character filler, short values under the 16-char floor).

Validation

  • npx vitest run test/unit/secrets-scan.test.ts test/unit/safety-wiring.test.ts test/unit/safety.test.ts — 48/48 passing, 100% line/branch coverage on the changed files
  • npm run typecheck — clean
  • npm run test:ci — full local gate green
  • npm audit --audit-level=moderate — 0 vulnerabilities

Safety

  • No secret value is ever echoed — only kind names surface in the blocker message (unchanged from before).
  • Additive-only pattern widening; the original 5 hard-blocking kinds and their behavior are byte-identical.

Closes #2553

@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 16:51:44 UTC

4 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly wires the new scanner kinds through `secretLeakFinding()` into the existing hard-block gate, and the added tests cover the main positive path plus several placeholder exclusions. The format-specific Google API key and JWT additions are straightforward. The generic assignment scanner is intentionally conservative, but its current value grammar leaves important real-world assignment forms uncovered.

Nits — 7 non-blocking
  • nit: `src/review/secrets-scan.ts:42` only matches quoted generic assignment values, so common `.env`-style lines like `TOKEN=sk_live_...` stay outside the widened hard-blocking path.
  • nit: `src/review/secrets-scan.ts:42` limits generic values to `[A-Za-z0-9+/=_-]`, which excludes common secret characters such as `.`, `:`, `@​`, and `$` in quoted password/token assignments.
  • nit: `src/review/secrets-scan.ts:47` treats any value containing placeholder words anywhere as non-secret, which is defensible but broad enough that it deserves an explicit test for a mixed value if that behavior is intentional.
  • In `src/review/secrets-scan.ts:42`, consider capturing the whole quoted value with a delimiter-safe pattern instead of a narrow character class, then let `isPlaceholderSecretValue()` decide whether to suppress it.
  • In `test/unit/secrets-scan.test.ts`, add explicit coverage for unquoted `.env` assignments and punctuation-bearing quoted values, either as supported cases or documented non-goals.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2553
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 529 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 529 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 529 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.03%. Comparing base (c4b797b) to head (dc1496c).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2613   +/-   ##
=======================================
  Coverage   96.02%   96.03%           
=======================================
  Files         233      233           
  Lines       26077    26096   +19     
  Branches     9474     9481    +7     
=======================================
+ Hits        25041    25060   +19     
  Misses        425      425           
  Partials      611      611           
Files with missing lines Coverage Δ
src/review/safety.ts 100.00% <ø> (ø)
src/review/secrets-scan.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

A string with no repeated characters (e.g. "abcdefghijklmnop123") has high
Shannon entropy by raw character-frequency counting, but is obviously not
a real secret since entropy alone measures frequency, not order. A
keyboard-sequential or alphabetical run slipped past the prior distinct-
character-count placeholder check. Detect the longest run of consecutive
ascending or descending character codes and treat a 6+ character run as a
human-constructed test value rather than a randomly generated credential.
@JSONbored
JSONbored force-pushed the fix/gate-secret-scan-widen branch from 1b1693a to dc1496c Compare July 2, 2026 16:48
@JSONbored
JSONbored merged commit 074b888 into main Jul 2, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix/gate-secret-scan-widen branch July 2, 2026 17:00
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): widen the gate's hard-blocking secret scan to match REES's richer ruleset

1 participant