Skip to content

fix(review): tighten generic secret fixture allowlist - #4733

Merged
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-secret-scan-vulnerability
Jul 11, 2026
Merged

fix(review): tighten generic secret fixture allowlist#4733
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-secret-scan-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Recent change introduced a broad suffix-based placeholder exemption that suppressed plausible real credentials (lowercase hyphen/underscore segmented values ending in -token/-secret/-key/-password/-passwd) and caused false-negative secret-scan behavior across the Worker, content-lane, and review-enrichment scanners.
  • The goal is to preserve the confirmed false-positive carve-outs for a small set of known test fixtures/enum literals while preventing a security-relevant bypass of generic_secret_assignment for real credentials.

Description

  • Replace the broad self-naming suffix exemption with a closed allowlist called KNOWN_FIXTURE_SECRET_VALUES in the three scanner copies at src/review/secrets-scan.ts, src/review/content-lane/security-scan.ts, and review-enrichment/src/analyzers/secret-scan.ts so only explicit, reviewed fixture literals are exempted.
  • Remove the all-lowercase-segments + secret-suffix heuristic and instead check membership in the small known-literals Set inside isPlaceholderSecretValue to avoid suppressing real credentials that happen to end with secret-kind words.
  • Add and update regression/unit tests to preserve the intended fixture exclusions and to assert that lowercase segmented credentials such as client_secret = "correct-horse-battery-secret", password = "legacy-system-passwd", and api_key = "internal-service-key" still produce generic_secret_assignment findings (tests in test/unit/secrets-scan.test.ts, test/unit/content-lane-security-scan.test.ts, and review-enrichment/test/secret-scan.test.ts).

Testing

  • Ran unit tests with npx vitest run test/unit/secrets-scan.test.ts test/unit/content-lane-security-scan.test.ts and all tests passed (110 tests across the two suites succeeded).
  • Built and executed the review-enrichment scanner tests via npm --prefix review-enrichment run build && cd review-enrichment && node --test --experimental-strip-types test/secret-scan.test.ts and the targeted scanner tests passed.
  • Typecheck with npm run typecheck succeeded and git diff --check produced no problems.
  • npm audit --audit-level=moderate could not complete in this environment (registry audit endpoint returned 403 Forbidden).

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 10, 2026
…patterns module

secrets-scan.ts and content-lane/security-scan.ts now import
isPlaceholderSecretValue from secret-patterns.ts (#4608/#4678), so the
fixture-allowlist fix needs to land there too, plus the engine-parity
drift-check marker that mirrors this logic into review-enrichment.
@JSONbored
JSONbored force-pushed the codex/propose-fix-for-secret-scan-vulnerability branch from 07216e8 to 92afa8c Compare July 11, 2026 00:07
@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.22%. Comparing base (3ae7ece) to head (92afa8c).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4733      +/-   ##
==========================================
- Coverage   94.22%   94.22%   -0.01%     
==========================================
  Files         441      441              
  Lines       38741    38740       -1     
  Branches    14117    14117              
==========================================
- Hits        36504    36503       -1     
  Misses       1577     1577              
  Partials      660      660              
Files with missing lines Coverage Δ
src/review/secret-patterns.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.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-11 06:33:05 UTC

8 files · 1 AI reviewer · 3 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Review summary
Replaces a broad heuristic (any all-lowercase, hyphen/underscore-segmented value ending in -token/-secret/-key/-password/-passwd is exempted) with a closed 4-literal allowlist (KNOWN_FIXTURE_SECRET_VALUES) in the shared src/review/secret-patterns.ts (also covers content-lane/security-scan.ts, which imports from it) and the standalone review-enrichment copy, plus a matching parity-marker update in scripts/check-engine-parity.ts. New regression tests correctly assert that plausible real credentials like `client_secret = "correct-horse-battery-secret"`, `password = "legacy-system-passwd"`, and `api_key = "internal-service-key"` now trigger generic_secret_assignment again, closing the false-negative bypass described in the motivation. The change is narrow, the three scanner copies stay in sync (parity script updated), and the fix is applied at the correct shared source (secret-patterns.ts) rather than patched in each caller.

Nits — 5 non-blocking
  • The PR description says the file is `src/review/secrets-scan.ts`, but the actual edit is in `src/review/secret-patterns.ts` (secrets-scan.ts just imports isPlaceholderSecretValue from it) — worth correcting so future readers grep the right file.
  • `installation-token` is added to KNOWN_FIXTURE_SECRET_VALUES in both copies but, unlike the other three entries, isn't tied to a cited confirmed-false-positive issue (feat(miner-hands): CLI-subprocess CodingAgentDriver #4524/feat(rees): add real before/after complexity-delta analyzer #4758) in the comment or PR description — worth a one-line justification so a future auditor can tell it's a reviewed fixture and not an ad hoc addition.
  • The external brief's 'potential leaked secrets' hits are all inside test fixtures/regex literals used to exercise the scanner itself (e.g. `correct-horse-battery-secret` in secrets-scan.test.ts), not real credentials — no action needed, but worth noting for anyone skimming the secret-scan alerts on this PR.
  • Fix the file path in the PR description (secret-patterns.ts, not secrets-scan.ts) for future traceability.
  • Add a short citation/comment for why `installation-token` is a confirmed fixture literal, matching the sourcing style used for the other three entries.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.
Signal Result Evidence
Code review ❌ 3 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 423 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 423 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 423 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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 the manual-review Gittensor contributor context label Jul 11, 2026
@JSONbored
JSONbored merged commit 5eab3e5 into main Jul 11, 2026
11 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-secret-scan-vulnerability branch July 11, 2026 06:33
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant