fix(signals): cap globToRegExp wildcard count to prevent ReDoS - #2445
Conversation
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-02 01:32:30 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 5 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## claude/pr-b-content-lane-genericity #2445 +/- ##
====================================================================
Coverage 95.91% 95.92%
====================================================================
Files 225 225
Lines 25290 25302 +12
Branches 9201 9205 +4
====================================================================
+ Hits 24258 24270 +12
Misses 419 419
Partials 613 613
🚀 New features to boost your workflow:
|
hardGuardrailGlobs (src/review/guardrail-config.ts) and any future maintainer-supplied glob compiled via globToRegExp are vulnerable to catastrophic backtracking on chained `*` wildcards: 5 chained wildcards against a 300-char adversarial path took ~19 seconds. Cap wildcard count at compile time (MAX_GLOB_WILDCARDS = 6, well above any of the ~10 real guardrail globs today). An over-complex glob is treated as matching every path rather than failing open, since a guardrail's job is to force manual review on uncertainty — mirroring isGuardrailHit's existing "unknown ⇒ treat as a hit" fail-safe direction.
23e3fcd to
2ca08ee
Compare
7eb67a1 to
55e5bff
Compare
The wildcard-count guard previously lived only in matchesAny's wrapper, so any other direct caller of the exported globToRegExp (e.g. content-lane/spec-resolver.ts) could still compile and .test() a pathological glob and hit catastrophic backtracking — the exact gap this PR set out to close. globToRegExp now short-circuits an over-complex glob to a never-matching sentinel regex instead of compiling it, so every caller (present or future, direct or indirect) is protected automatically. "Never matches" (not "matches everything") is the correct default for the general-purpose compiler, since a false "matches everything" would misclassify unrelated files for a non-guardrail caller; matchesAny keeps its own override to the opposite fail direction for guardrail semantics specifically.
…DoS cap The flagged blocker was correct: MAX_GLOB_WILDCARDS=6 let the PR's own motivating example (5 chained wildcards, empirically catastrophic) through uncapped. Lowering the raw-character cap to 2 fixed that but broke a real consumer -- content-lane/spec-resolver.ts's artifactGlob "public/**/*.json" has 3 raw '*' characters (** counts as two) despite being empirically instant even against a 4,000-char adversarial path, because a `**` globstar compiles to a single .* group, not two independent wildcards. Re-benchmarked with the right unit (wildcard GROUPS, where a ** pair is one group): 2 groups stays sub-second even at 32,000 adversarial chars; 3 groups is already dangerous (over 2s at ~4,000 chars for a chained-* shape, over 100ms at ~1,600 chars for a chained-** shape); 4+ groups is catastrophic (35s at 1,614 chars for 4 chained ** groups). Caps MAX_GLOB_WILDCARD_GROUPS at 2 -- the highest value proven safe -- via a new countWildcardGroups that mirrors globToRegExp's own tokenization (consuming a ** pair, and its trailing /, as one group), so both the original flagged case and real 2-group globs like public/**/*.json are handled correctly.
47f71f1
into
claude/pr-b-content-lane-genericity
…he registry-review engine (#2443) * refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine gittensory is meant to be installed by any self-hosted repo maintainer, not just JSONbored/metagraphed. The RegistryLaneSpec abstraction was already generic, but nothing let a second maintainer actually reach it without editing gittensory's own TypeScript source and redeploying. - Rename runMetagraphedSurfaceGate to runRegistrySurfaceGate; stop re-exporting netuid-verification.ts's Bittensor-only helpers from the generic content-lane barrel (closes #2433). - Add assessAppendedEntry/assessProviderEntry callback fields to RegistryLaneSpec; the orchestrator calls the spec-supplied validators instead of hardcoded imports, so a different registry can supply its own domain validator without touching shared engine code (closes #2434). - Add a contentLane: block to .gittensory.yml (FocusManifestContentLaneConfig) and a resolveRegistryLaneSpec resolver mirroring resolveConvergedFeature's precedence (env kill-switch -> per-repo config -> allowlist default), so a second maintainer's registry repo can activate the deterministic surface lane purely from their own config, with today's zero-config behavior for metagraphed unchanged (closes #2435). - Glob fields (entryFileGlob/providerFileGlob/artifactGlob) are capped at parse time to a safe wildcard count: an adversarial-review pass on this change found the shared glob-to-RegExp compiler is exponential-time on chained wildcards (empirically ~19s at 5 chained wildcards), so the new config surface rejects an over-complex glob before it ever reaches RegExp compilation. * fix(content-lane): warn on an unregistered contentLane.validatorId buildRegistryLaneSpecFromConfig already degraded an unregistered validatorId to structural-only gating silently (a legitimate mode for a registry with no validator yet), which made an operator typo (e.g. "metagraph" instead of "metagraphed") indistinguishable from a deliberate choice — no signal reached the maintainer. Add unregisteredValidatorId()/registeredValidatorIds() to spec-resolver.ts and surface a non-blocking advisory finding from evaluateWithSurfaceLane so a bad validatorId shows up directly in the PR comment, naming the offending id and the known registered ids. Also switch the REGISTRY_VALIDATORS lookup to Object.hasOwn instead of bracket-truthiness, so a validatorId matching an inherited Object.prototype key (e.g. "toString") is correctly reported as unregistered rather than silently matching a prototype method. * fix(content-lane): hold the gate on an unreadable manifest, reject over-long globs Two gaps flagged in evaluateWithSurfaceLane and normalizeOptionalGlob: - A non-allowlisted repo's ONLY way to configure a registry content lane is its own .gittensory.yml. A manifest-load failure was caught as null and silently treated the same as "no contentLane configured" -- letting a registry-submission PR merge unevaluated on nothing more than a transient read blip. Now holds the gate neutral in that specific case (never overriding a real generic hard blocker, which is always preserved). - An over-long contentLane glob (entryFileGlob/providerFileGlob/ artifactGlob) was truncated to MAX_ITEM_LENGTH and still returned, silently compiling a DIFFERENT file-scope pattern than configured. Now rejected outright, matching the function's own doc comment and the established pattern used elsewhere in this file. * fix(signals): cap globToRegExp wildcard count to prevent ReDoS (#2445) * fix(signals): cap globToRegExp wildcard count to prevent ReDoS hardGuardrailGlobs (src/review/guardrail-config.ts) and any future maintainer-supplied glob compiled via globToRegExp are vulnerable to catastrophic backtracking on chained `*` wildcards: 5 chained wildcards against a 300-char adversarial path took ~19 seconds. Cap wildcard count at compile time (MAX_GLOB_WILDCARDS = 6, well above any of the ~10 real guardrail globs today). An over-complex glob is treated as matching every path rather than failing open, since a guardrail's job is to force manual review on uncertainty — mirroring isGuardrailHit's existing "unknown ⇒ treat as a hit" fail-safe direction. * fix(signals): bake the ReDoS wildcard cap into globToRegExp itself The wildcard-count guard previously lived only in matchesAny's wrapper, so any other direct caller of the exported globToRegExp (e.g. content-lane/spec-resolver.ts) could still compile and .test() a pathological glob and hit catastrophic backtracking — the exact gap this PR set out to close. globToRegExp now short-circuits an over-complex glob to a never-matching sentinel regex instead of compiling it, so every caller (present or future, direct or indirect) is protected automatically. "Never matches" (not "matches everything") is the correct default for the general-purpose compiler, since a false "matches everything" would misclassify unrelated files for a non-guardrail caller; matchesAny keeps its own override to the opposite fail direction for guardrail semantics specifically. * fix(signals): count wildcard GROUPS, not raw * characters, for the ReDoS cap The flagged blocker was correct: MAX_GLOB_WILDCARDS=6 let the PR's own motivating example (5 chained wildcards, empirically catastrophic) through uncapped. Lowering the raw-character cap to 2 fixed that but broke a real consumer -- content-lane/spec-resolver.ts's artifactGlob "public/**/*.json" has 3 raw '*' characters (** counts as two) despite being empirically instant even against a 4,000-char adversarial path, because a `**` globstar compiles to a single .* group, not two independent wildcards. Re-benchmarked with the right unit (wildcard GROUPS, where a ** pair is one group): 2 groups stays sub-second even at 32,000 adversarial chars; 3 groups is already dangerous (over 2s at ~4,000 chars for a chained-* shape, over 100ms at ~1,600 chars for a chained-** shape); 4+ groups is catastrophic (35s at 1,614 chars for 4 chained ** groups). Caps MAX_GLOB_WILDCARD_GROUPS at 2 -- the highest value proven safe -- via a new countWildcardGroups that mirrors globToRegExp's own tokenization (consuming a ** pair, and its trailing /, as one group), so both the original flagged case and real 2-group globs like public/**/*.json are handled correctly. * fix(content-lane): share the wildcard-safety predicate between glob parsing and compilation normalizeOptionalGlob (focus-manifest.ts) capped contentLane globs at 3 raw '*' characters, but globToRegExp (change-guardrail.ts) rejects any glob with more than 2 wildcard GROUPS (a '**' pair counts as one group, not two) by compiling it to NEVER_MATCHES. A glob like "a*b*c*.json" (3 groups, no '**' pairs) was accepted as configured but silently could never match any file once compiled -- a content lane that looks active but never fires. Export hasUnsafeWildcardCount from change-guardrail.ts and reuse it directly in normalizeOptionalGlob instead of an independently-counted threshold, so the parser and compiler can never drift apart again.
…oS (#2482) labelPatternToRegExp compiled a registry-supplied label_multipliers key into a RegExp with unbounded chained-wildcard translation and no length/complexity cap -- the exact class of bug #2445 fixed in change-guardrail.ts's globToRegExp, which caps chained wildcard groups at 2 after benchmarking confirmed catastrophic backtracking (over 2s at ~4,000 chars for 3 chained groups). This sibling was never patched. The pattern originates from a repo's registryConfig.labelMultipliers, sourced from the externally-fetched gittensor registry (registry/ sync.ts + registry/normalize.ts) with no validation -- not a value a repo's own maintainer directly controls via .gittensory.yml, despite an inaccurate in-code comment claiming the key set was "bounded, not attacker-supplied". Reachable via the public score-preview API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could hang scoring for every PR on that repo. Reuses change-guardrail.ts's exported hasUnsafeWildcardCount directly (same *-group counting, same empirically-safe threshold) rather than reimplementing it -- a `*` in this fnmatch-style pattern language has the identical "any run of chars" semantics as that glob compiler's `*`, so the same catastrophic-backtracking risk and safe boundary apply. An over-complex pattern now compiles to a safe never-match instead of a pathological RegExp. Fixes #2456
Summary
globToRegExp(src/signals/change-guardrail.ts) compiles a maintainer-facing path glob to aRegExpusing chained[^/]*groups for each single-segment*wildcard. The compiled pattern's.test()call is exponential-time on an adversarial near-miss input once multiple wildcards chain in one glob — empirically verified: 5 chained wildcards against a 300-char adversarial input took ~19 seconds; 3 stayed under 5ms at the same length.hardGuardrailGlobs(src/review/guardrail-config.ts) is 100% hardcoded engine constants today (no maintainer/contributor input reaches it), so there is no live exploit path — butglobToRegExpis exported and reused elsewhere in the review engine (content-lane/spec-resolver.ts, from refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine #2443), so this hardens the function itself rather than relying on every future caller to separately remember the risk.globToRegExpitself, not just inmatchesAny's wrapper. An earlier revision only guarded the wildcard count insidematchesAny, leaving the exportedglobToRegExpfree to compile and.test()a pathological glob if called directly — exactly whatcontent-lane/spec-resolver.tsdoes.globToRegExpnow short-circuits an over-complex glob (MAX_GLOB_WILDCARDS = 6) to a never-matching sentinel RegExp (/^(?!)$/) instead of compiling it, so every caller — present or future, direct or indirect — is protected automatically. "Never matches" is the correct general-purpose default (a false "matches everything" would misclassify unrelated files for a non-guardrail caller like content-lane's file-scope matching);matchesAnykeeps its ownhasUnsafeWildcardCountpre-check to override to the opposite fail direction ("matches everything", forcing a guardrail hold) for its specific safety-critical semantics.guardrail-config.tsexceed 2 wildcards, so this is a zero-behavior-change hardening for current production config.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateSafety
UI Evidencesection below with JPG/JPEG or PNG screenshots. (N/A — no UI changes.)Notes
claude/pr-b-content-lane-genericity(refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine #2443) rather thanmainbecause it hardensglobToRegExp, which refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine #2443'scontent-lane/spec-resolver.tsnewly reuses for config-supplied globs; rebasing ontomainonce refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine #2443 merges is trivial (no conflicts expected — this PR only touchessrc/signals/change-guardrail.tsand its test file, both untouched by refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine #2443).test/unit/change-guardrail.test.ts: a ReDoS regression test (7-star pathological glob resolves in <1s and fails toward guarding viamatchesAny), an at-cap test (exactly 6 stars still matches normally, proving the cap is inclusive), a mixed-globs test (one pathological glob among fine ones still forces a hold for any path), a wildcard-free literal-glob test using a real example fromCONFIG_AS_CODE_GUARDRAIL_GLOBS, and a dedicatedglobToRegExpblock proving the compiler is safe when called directly (bypassingmatchesAnyentirely) — the exact scenario the gate's own review flagged.