Skip to content

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

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)#8722
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 MCP/miner/engine/ui-kit tarballs scanning with a stale body — no CI signal at all.

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 (e.g. gh[pousr]_[A-Za-z0-9_]+ vs gh[pousr]_[A-Za-z0-9]{20,}), 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 (mirrors the existing secret-detection drift test), 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 against the current tree.

git diff --check clean.

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 26, 2026 00:57
@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
@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:07:17 UTC

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

⏸️ Suggested Action - Manual Review

Review summary
This PR mechanically extends the existing named-twin-pair pattern (mirroring SECRET_DETECTION_TWIN_PAIR) to register scripts/forbidden-content.ts against src/review/secret-patterns.ts, closing a real drift-detection gap for the fourth hand-copy of HARD_SECRET_KINDS regex bodies. The FORBIDDEN_CONTENT_MARKERS correctly anchor on backslash-free regex cores (verified against the actual SECRET_PATTERNS bodies in secret-patterns.ts shown in context) and deliberately exclude the three kinds (github_token/github_pat/private_key_block) known to diverge, which is consistent with forbidden-content.ts's stated pre-#7433 looser bodies. Tests cover pair registration, the deliberate exclusions, and a drift-fail scenario, and the existing 'all named pairs' regression guard is extended to include this pair.

Nits — 3 non-blocking
  • The sendgrid_key marker `[A-Za-z0-9_-]{43}(?![A-Za-z0-9_-])` (scripts/check-engine-parity.ts:150) is only the distinctive secret-half substring rather than the full body (unlike every other entry) — the comment explains why, but it's worth double-checking this substring can't spuriously match an unrelated 43-char run elsewhere in forbidden-content.ts's string bodies and produce a false pass.
  • The long block comment above FORBIDDEN_CONTENT_TWIN_PAIR (scripts/check-engine-parity.ts:136-142) duplicates most of the PR description almost verbatim — fine for now, but consider trimming to just the marker-anchoring rationale to avoid the comment rotting out of sync with the description over time.
  • Consider asserting the full 13-marker set length (e.g. `expect(FORBIDDEN_CONTENT_MARKERS).toHaveLength(13)`) in a test so an accidental deletion of a marker line is caught even if the deleted marker's substring happens to still appear elsewhere in both files.

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 scripts/forbidden-content.ts as a new NamedTwinPair (FORBIDDEN_CONTENT_TWIN_PAIR) against secret-patterns.ts, defines FORBIDDEN_CONTENT_MARKERS mirroring SECRET_DETECTION_MARKERS' shape, and adds tests covering pair identity, deliberate exclusions, a drift-fail case, and inclusion in the real-repo regression guard, matching all four stated 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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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
@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 (ca9afd9) to head (04768e6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8722   +/-   ##
=======================================
  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.

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