Skip to content

fix(signals): align focus-manifest.ts's glob cap with group-based counting - #2448

Closed
JSONbored wants to merge 1 commit into
claude/pr-c-guardrail-redos-hardeningfrom
claude/pr-d-focus-manifest-glob-cap
Closed

fix(signals): align focus-manifest.ts's glob cap with group-based counting#2448
JSONbored wants to merge 1 commit into
claude/pr-c-guardrail-redos-hardeningfrom
claude/pr-d-focus-manifest-glob-cap

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • focus-manifest.ts's normalizeOptionalGlob (parses contentLane.entryFileGlob/providerFileGlob/artifactGlob from .gittensory.yml) capped raw * character count at 3 — the same flawed model change-guardrail.ts's globToRegExp/matchesAny used before fix(signals): cap globToRegExp wildcard count to prevent ReDoS #2445's fix. A ** pair compiles to a single .* group, not two independent wildcards, so raw character counting is wrong in both directions: it wrongly rejects a legitimate 2-group glob like "public/**/*.json" (3 raw star characters) in some cases, and would wrongly admit a genuinely dangerous 3-single-star-group glob (e.g. "a-*-*-*-b") at the same raw count — exactly the class of bug fix(signals): cap globToRegExp wildcard count to prevent ReDoS #2445 fixed.
  • Not a live security hole. globToRegExp (from fix(signals): cap globToRegExp wildcard count to prevent ReDoS #2445) already has its own internal group-based cap (MAX_GLOB_WILDCARD_GROUPS = 2) that safely short-circuits any over-complex glob to a never-matching sentinel regex regardless of what this parse-time check lets through — full defense-in-depth already exists. This closes a consistency/UX gap: today some over-complex contentLane.*Glob values get an explicit "too many wildcards" parse-time warning while others silently pass validation and then silently compile to a never-matching pattern at match-time with zero warning.
  • Fix: export change-guardrail.ts's countWildcardGroups/MAX_GLOB_WILDCARD_GROUPS (both already benchmarked and battle-tested in fix(signals): cap globToRegExp wildcard count to prevent ReDoS #2445) and reuse them directly in focus-manifest.ts instead of re-deriving a second, separately-maintained threshold — the two glob-safety checks in this codebase now share one source of truth instead of risking future drift. change-guardrail.ts has zero imports of its own (confirmed dependency-free), so this is a safe, non-circular sibling import within src/signals/.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused (glob-cap consistency only) and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed. (No issue — a small, self-contained follow-up flagged during fix(signals): cap globToRegExp wildcard count to prevent ReDoS #2445's review, same class of fix.)

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — the one pre-existing sub-100% line in focus-manifest.ts (MAX_FOCUS_MANIFEST_BYTES over-length content path) is untouched by this diff and unrelated; every line/branch this PR changes is covered
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — added a REGRESSION test proving "public/**/*.json" (2 groups, 3 raw characters) is now accepted, and updated the existing "too many wildcards" test's "at cap" example to the new 2-group boundary

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no such changes.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — internal config-parsing helper only.)
  • UI changes use live API data or real empty/error/loading states. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section. (N/A — no UI changes.)
  • Public docs/changelogs are updated where needed. (N/A — no public-facing behavior change; this is an internal consistency fix.)

Notes

…nting

normalizeOptionalGlob (contentLane.*Glob parsing) capped raw `*`
character count at 3, the same flawed model just fixed in
change-guardrail.ts's globToRegExp: a `**` pair compiles to ONE
backtracking-capable group, not two, so raw counting both rejected a
legitimate 2-group glob like "public/**/*.json" (3 characters) in some
cases and would admit some genuinely dangerous 3-single-star-group
globs at the same raw count.

Export change-guardrail.ts's countWildcardGroups and
MAX_GLOB_WILDCARD_GROUPS and reuse them here instead of re-deriving a
separate threshold, so the two glob-safety checks in this codebase
share one source of truth instead of drifting.

Not a live security fix — globToRegExp's own internal cap already
defuses anything that slips past this parse-time check — this closes
a consistency gap where some over-complex globs got an explicit
parse-time warning while others silently degraded to a never-matching
pattern with no warning at all.
@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 01:45:41 UTC

3 files · 1 AI reviewer · no blockers · readiness 86/100 · CI pending · unstable

⏸️ Suggested Action - Manual Review

Review summary
The change correctly moves focus-manifest glob validation from raw `*` character counting to the same wildcard-group accounting used by `globToRegExp`, so `**/*.json` no longer gets rejected while over-complex grouped globs are rejected before matching. Exporting the guardrail constant and counter from `src/signals/change-guardrail.ts` is coherent here because that module remains dependency-free, and the added regression test covers the previously rejected globstar-plus-star shape. The visible diff is focused and preserves the existing fail-closed parse-time behavior for unsafe content-lane globs.

Nits — 4 non-blocking
  • nit: `test/unit/focus-manifest.test.ts:1096` still only proves rejection far above the cap with five groups; add a boundary regression for exactly three groups, e.g. `registry/*-*-*-final.json`, since that is the specific raw-count-equals-old-cap case this PR calls out.
  • nit: `src/signals/focus-manifest.ts:623` has an unusually long explanatory comment with escaped example text; trimming it to the invariant and pointing to `change-guardrail.ts` would reduce future drift in prose while keeping the code-level source of truth intact.
  • Add a focused test in `test/unit/focus-manifest.test.ts` that `contentLane.entryFileGlob: "registry/*-*-*-final.json"` is rejected with the warning, so the new group cap is locked at the dangerous boundary rather than only far beyond it.
  • Shorten the `normalizeOptionalGlob` comment in `src/signals/focus-manifest.ts` to state that it reuses `countWildcardGroups`/`MAX_GLOB_WILDCARD_GROUPS`, leaving the detailed ReDoS rationale in `src/signals/change-guardrail.ts` where the implementation lives.
Signal Result Evidence
Code review ✅ No 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 (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 563 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 563 issue(s).
Gate result ✅ Passing No configured blocker found.
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), 563 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.
  • Triage stale or unlinked PRs.
  • No action.
  • 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 gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Superseded by 536e46a ("fix(content-lane): share the wildcard-safety predicate between glob parsing and compilation"), already merged into claude/pr-b-content-lane-genericity — same fix, done concurrently in another session. Closing to avoid duplicate/conflicting work.

@JSONbored JSONbored closed this Jul 2, 2026
@JSONbored
JSONbored deleted the claude/pr-d-focus-manifest-glob-cap branch July 2, 2026 01:51
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap 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.
⚠️ Please upload report for BASE (claude/pr-c-guardrail-redos-hardening@8ef2e04). Learn more about missing BASE report.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@                           Coverage Diff                            @@
##             claude/pr-c-guardrail-redos-hardening    #2448   +/-   ##
========================================================================
  Coverage                                         ?   95.92%           
========================================================================
  Files                                            ?      225           
  Lines                                            ?    25301           
  Branches                                         ?     9204           
========================================================================
  Hits                                             ?    24269           
  Misses                                           ?      419           
  Partials                                         ?      613           
Files with missing lines Coverage Δ
src/signals/change-guardrail.ts 100.00% <100.00%> (ø)
src/signals/focus-manifest.ts 99.24% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant