From 6549d631bc906eda2214f9747348e025968e7024 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 22:43:12 -0700 Subject: [PATCH] fix(scoring): cap labelPatternToRegExp wildcard groups to prevent ReDoS 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 --- src/scoring/preview.ts | 28 ++++++++++++++++++++++++---- test/unit/scoring.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/scoring/preview.ts b/src/scoring/preview.ts index 0337e416f0..15cbd58d8e 100644 --- a/src/scoring/preview.ts +++ b/src/scoring/preview.ts @@ -1,6 +1,7 @@ import type { ContributorEvidenceRecord, JsonValue, RepositoryRecord, RepoTimeDecayOverrides, ScoringModelSnapshotRecord, ScorePreviewRecord } from "../types"; import { DEFAULT_SCORING_CONSTANTS } from "./model"; import { nowIso } from "../utils/json"; +import { hasUnsafeWildcardCount } from "../signals/change-guardrail"; export type ScorePreviewInput = { repoFullName: string; @@ -927,12 +928,21 @@ export function labelMatchesPattern(label: string, pattern: string): boolean { // Compiled fnmatch→RegExp matchers are memoized by pattern. The same small, // config-derived set of label keys is matched on every scored PR/issue, so the // per-call recompile inside the nested label loops in engine.ts is pure waste. -// Keys are configured label patterns (bounded, not attacker-supplied), so the -// cache needs no eviction bound. The compiled RegExp carries only the "i" flag -// (no global/sticky `lastIndex` state), so sharing one instance across calls is -// safe and byte-identical to recompiling on every call. +// Keys come from a repo's registryConfig.labelMultipliers, sourced from the externally-fetched gittensor +// registry (registry/sync.ts + registry/normalize.ts, not a value this repo's own maintainer directly controls +// via .gittensory.yml) — so the pattern SET is small per repo, but individual pattern CONTENT is untrusted, not +// literally attacker-supplied-per-request the way GitHub PR content is. The cache still needs no eviction bound +// (the key set is bounded by real registry entries, not attacker-loop-controlled at request time), but the +// wildcard-count cap below (#2456) matters regardless of that distinction. The compiled RegExp carries only the +// "i" flag (no global/sticky `lastIndex` state), so sharing one instance across calls is safe and byte-identical +// to recompiling on every call. const labelPatternRegExpCache = new Map(); +// A RegExp that never matches any input — mirrors change-guardrail.ts's identical NEVER_MATCHES fallback for an +// over-complex pattern, so a pathological registry entry degrades to "this label multiplier never applies" +// instead of hanging the scoring path that evaluates it. +const LABEL_PATTERN_NEVER_MATCHES = /^(?!)$/; + // Upstream resolves label multipliers by matching each configured key as a Python `fnmatch` GLOB, not a // literal string: `fnmatch(label.lower(), pattern.lower())` in // gittensor/validator/oss_contributions/label_resolution.py, so a repo can configure `type:*`, `kind/*`, or @@ -946,6 +956,16 @@ const labelPatternRegExpCache = new Map(); function labelPatternToRegExp(pattern: string): RegExp { const cached = labelPatternRegExpCache.get(pattern); if (cached !== undefined) return cached; + // Reuses change-guardrail.ts's wildcard-GROUP counting (a `*` here matches the same "any run of chars" + // semantics as that glob compiler's `*`, so the same catastrophic-backtracking risk and the same empirically- + // safe threshold apply) — an over-complex registry-sourced label_multipliers key degrades to a safe never-match + // instead of hanging RegExp.test() on an adversarial near-miss label (#2456). Reachable via the public + // score-preview API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could otherwise + // hang scoring for every PR on that repo. + if (hasUnsafeWildcardCount(pattern)) { + labelPatternRegExpCache.set(pattern, LABEL_PATTERN_NEVER_MATCHES); + return LABEL_PATTERN_NEVER_MATCHES; + } let regex = ""; let i = 0; while (i < pattern.length) { diff --git a/test/unit/scoring.test.ts b/test/unit/scoring.test.ts index 086746af6e..9778d31f07 100644 --- a/test/unit/scoring.test.ts +++ b/test/unit/scoring.test.ts @@ -1913,4 +1913,27 @@ describe("label pattern matcher memoization (#2106)", () => { expect(labelMatchesPattern("bug", "bug")).toBe(true); expect(labelMatchesPattern("bugfix", "bug")).toBe(false); }); + + it("SECURITY (ReDoS, #2456): a label pattern with too many chained wildcards no longer risks catastrophic backtracking — it fails SAFE TOWARD NO MULTIPLIER (never matches) instead of ever compiling the pathological pattern", () => { + // 3 chained wildcards is already empirically dangerous for the identical `.*`-chaining shape this reuses + // from change-guardrail.ts's globToRegExp (see MAX_GLOB_WILDCARD_GROUPS's rationale: over 2 seconds at a + // ~4,000-char adversarial input) — one over the cap, proving the boundary itself is safe, not just an + // extreme over-the-top example. Must resolve INSTANTLY even against that adversarial length. Unlike the + // guardrail glob (which fails toward MATCHING, the safe direction for a security hold), a label multiplier + // pattern fails toward NEVER matching — the safe direction here is "no multiplier applies", not "every label + // gets a multiplier". + const pathological = "*-*-*-final"; + const adversarialLabel = "a-".repeat(2000) + "X"; // ~4,000 chars — the empirically dangerous length for 3 wildcards + const start = Date.now(); + expect(labelMatchesPattern(adversarialLabel, pathological)).toBe(false); + expect(labelMatchesPattern("completely-unrelated-label", pathological)).toBe(false); + expect(labelMatchesPattern("", pathological)).toBe(false); + expect(labelMatchesPattern("a-b-c-final", pathological)).toBe(false); // even a "near miss" that would otherwise match + expect(Date.now() - start).toBeLessThan(1000); + }); + + it("a label pattern AT the safe cap (2 wildcards) still compiles and matches NORMALLY, not the fail-safe path — proves the cap is inclusive, not exclusive", () => { + expect(labelMatchesPattern("type-bug-fix", "type-*-*")).toBe(true); + expect(labelMatchesPattern("type-bug", "type-*-*")).toBe(false); + }); });