Skip to content

fix(scripts): drift-check forbidden-content.ts's secret patterns against secret-patterns.ts (#8674) - #8729

Closed
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-forbidden-content-parity-8674
Closed

fix(scripts): drift-check forbidden-content.ts's secret patterns against secret-patterns.ts (#8674)#8729
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-forbidden-content-parity-8674

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Problem

Closes #8674.

scripts/forbidden-content.ts's FORBIDDEN_CONTENT is a third hand-copy (#7433) of the HARD_SECRET_KINDS regex bodies in src/review/secret-patterns.ts — consumed by the four package-manifest checkers (check-mcp/miner/engine/ui-kit-package.ts) to reject a packed tarball embedding a provider secret. Unlike the REES copy (guarded by SECRET_DETECTION_TWIN_PAIR), it was never registered in check-engine-parity.ts's NAMED_TWIN_PAIRS, so a tightened or added HARD_SECRET_KINDS pattern would silently leave packaged tarballs scanning with a stale body — no CI signal.

Fix

  • Register scripts/forbidden-content.ts as a new NamedTwinPair (FORBIDDEN_CONTENT_TWIN_PAIR) against src/review/secret-patterns.ts in NAMED_TWIN_PAIRS.
  • Define FORBIDDEN_CONTENT_MARKERS: the distinctive backslash-free core of each HARD_SECRET_KINDS regex body forbidden-content copied exactly (13 kinds, aws_access_keyjwt). Anchoring on the inner body (not the \b/\. escapes, which necessarily differ between regex literals and string bodies) means a reformatted boundary never false-fails while a real change to a character class / length / watermark does.

github_token / github_pat / private_key_block are deliberately excluded — forbidden-content keeps its own looser pre-#7433 bodies for those three, a pre-existing intentional divergence that would false-fail, exactly as SECRET_DETECTION_MARKERS excludes its own naming-divergent kinds.

Tests

  • A drift-fail case proving a dropped shared body fails presence, plus assertions of the pair's identity and the three deliberate exclusions.
  • The existing "all named pairs pass against the real repo" regression guard now also iterates this pair — all 13 markers verified present in both live files, so no false positive.

git diff --check clean. (Rebased onto latest main after #8719 landed a gate-decision enhancement in the same files.)

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 26, 2026 01:10
@superagent-security

Copy link
Copy Markdown
Contributor

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

…atterns against secret-patterns.ts

scripts/forbidden-content.ts's FORBIDDEN_CONTENT is a THIRD hand-copy (JSONbored#7433) of secret-patterns.ts's
HARD_SECRET_KINDS regex bodies -- consumed by the four package-manifest checkers (check-mcp/miner/engine/
ui-kit-package.ts) to reject a packed tarball embedding a provider secret. Unlike the REES copy (already
guarded by SECRET_DETECTION_TWIN_PAIR), it was never registered in check-engine-parity.ts's NAMED_TWIN_PAIRS,
so a tightened or added HARD_SECRET_KINDS pattern would silently leave packaged tarballs scanning with a
stale body, with no CI signal.

Register scripts/forbidden-content.ts as a new named twin pair against src/review/secret-patterns.ts, with a
marker set covering the distinctive backslash-free core of each HARD_SECRET_KINDS regex body forbidden-content
hand-copied exactly (13 kinds, aws through jwt). github_token/github_pat/private_key_block are deliberately
excluded -- forbidden-content keeps its own looser pre-JSONbored#7433 bodies for those three, a pre-existing intentional
divergence that would false-fail, exactly as SECRET_DETECTION_MARKERS excludes its own naming-divergent kinds.

Tests: a drift-fail case proving a dropped shared body fails presence, plus assertions of the pair's identity
and the three deliberate exclusions. The existing 'all named pairs pass against the real repo' regression test
now also covers this pair (all 13 markers verified present in both live files -- no false positive).
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.79%. Comparing base (abf88ab) to head (2a565f2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8729   +/-   ##
=======================================
  Coverage   93.79%   93.79%           
=======================================
  Files         797      797           
  Lines       79478    79478           
  Branches    24079    24079           
=======================================
  Hits        74543    74543           
  Misses       3563     3563           
  Partials     1372     1372           
Flag Coverage Δ
backend 95.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-26 01:27:58 UTC

2 files · 1 AI reviewer · no blockers · CI green · dirty

⏸️ Suggested Action - Manual Review

Review summary
This PR closes #8674 by registering scripts/forbidden-content.ts as a new NamedTwinPair (FORBIDDEN_CONTENT_TWIN_PAIR) against src/review/secret-patterns.ts, adding 13 backslash-free regex-body markers that mirror HARD_SECRET_KINDS while deliberately excluding github_token/github_pat/private_key_block for their pre-existing looser bodies. The drift-detection test is real (not fabricated): it builds a full marker set, drops one marker to simulate the exact HARD_SECRET_KINDS-tightened-but-forbidden-content-stale scenario the pair exists to catch, and asserts checkGateDecisionTwinPresence surfaces exactly that failure — this is the same pattern already used for SECRET_DETECTION_TWIN_PAIR and DIFF_FILE_PRIORITY_TWIN_PAIR, so it's a proven, non-vacuous guard. The regression test 'passes marker presence for all six named pairs against the real repo' confirms all 13 markers are genuinely present in both live files today, so this doesn't introduce a false-positive CI failure on merge.

Nits — 3 non-blocking
  • I can't see the full content of scripts/forbidden-content.ts in this diff, so I can't independently verify the 13 markers are byte-for-byte substrings of its actual FORBIDDEN_CONTENT bodies — the real-repo regression test (test/unit/check-engine-parity-script.test.ts) is the actual proof here, and it's green against the real files per the PR description.
  • Several markers (e.g. sendgrid_key's bare 43-char class, jwt's single-segment prefix) are intentionally partial substrings of the full regex body rather than the complete body — worth a one-line callout in the doc comment for jwt similar to the one already given for sendgrid_key, so a future reader doesn't assume every marker is the full pattern.
  • Consider whether scripts/forbidden-content.ts's own header comment should now point at FORBIDDEN_CONTENT_TWIN_PAIR/FORBIDDEN_CONTENT_MARKERS so someone editing FORBIDDEN_CONTENT directly discovers the drift guard without first reading check-engine-parity.ts.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8674
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 329 registered-repo PR(s), 137 merged, 37 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 329 PR(s), 37 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The PR registers FORBIDDEN_CONTENT_TWIN_PAIR in NAMED_TWIN_PAIRS with hostRelative/engineRelative pointing at secret-patterns.ts and forbidden-content.ts, defines FORBIDDEN_CONTENT_MARKERS mirroring the HARD_SECRET_KINDS bodies, and adds tests covering the drift-fail path, the deliberate exclusions, and the real-repo regression pass — matching all four listed deliverables.

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Ruby, TypeScript, Svelte, Cuda, JavaScript, Markdown, MDX
  • Official Gittensor activity: 329 PR(s), 37 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (conflicts with the base branch — resolve and open a fresh PR). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 26, 2026
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(scripts): forbidden-content.ts's hand-copied secret-pattern regex has zero drift protection against its canonical source

1 participant