Skip to content

Commit ee48b7e

Browse files
committed
fix(engine): count fnmatch label-pattern wildcard groups per raw star, not by path-glob rules
labelPatternToRegExp reused change-guardrail's path-glob wildcard-group counter to guard against catastrophic backtracking, but that counter treats a ** pair as ONE group (the path compiler collapses ** into a single .*). The fnmatch compiler here has no ** concept and emits one .* per *, so the count and the compiled regex disagreed for any ** pattern: *a**b counted 2 but compiled 3 .* groups and was wrongly accepted, admitting a pattern this compiler builds into a catastrophic- backtracking RegExp on an adversarial near-miss label. Count one group per raw * (no ** pairing; ? and [..] are not counted) and compare against the shared MAX_GLOB_WILDCARD_GROUPS, now exported from change-guardrail rather than redeclared. An over-complex registry key degrades to the existing LABEL_PATTERN_NEVER_MATCHES and is still cached. The path-glob counter and every path consumer keep their **-is-one-group semantics unchanged. Closes #9994
1 parent 9f673b8 commit ee48b7e

5 files changed

Lines changed: 92 additions & 11 deletions

File tree

25.3 KB
Binary file not shown.

packages/loopover-engine/src/scoring/label-match.ts

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { hasUnsafeWildcardCount } from "../signals/change-guardrail.js";
1+
import { MAX_GLOB_WILDCARD_GROUPS } from "../signals/change-guardrail.js";
22

33
export function labelMatchesPattern(label: string, pattern: string): boolean {
44
return labelPatternToRegExp(pattern.toLowerCase()).test(label.toLowerCase());
@@ -42,6 +42,19 @@ const LABEL_PATTERN_NEVER_MATCHES = /^(?!)$/;
4242
// change-guardrail.ts (there `*` stops at `/` and `?` is literal): labels are flat strings, so `*` matches any
4343
// run, `?` any single character, and `[seq]`/`[!seq]` a character class. Literal keys are unaffected — for a
4444
// pattern with no glob metacharacter the RegExp is an exact match, so existing configs score identically.
45+
46+
/** Count the backtracking-capable wildcard GROUPS this fnmatch compiler will emit: one per raw `*` (each
47+
* compiles to a `.*` below), with NO `**`-is-one-group rule — unlike change-guardrail's path-glob counter,
48+
* this compiler has no globstar concept, so `**` is two `.*` groups, not one (#9994). `?` is not counted (it
49+
* compiles to a single `.`, which cannot backtrack ambiguously), and neither are `[…]` classes. */
50+
function fnmatchWildcardGroups(pattern: string): number {
51+
let count = 0;
52+
for (let i = 0; i < pattern.length; i += 1) {
53+
if (pattern.charAt(i) === "*") count += 1;
54+
}
55+
return count;
56+
}
57+
4558
function labelPatternToRegExp(pattern: string): RegExp {
4659
const cached = labelPatternRegExpCache.get(pattern);
4760
if (cached !== undefined) {
@@ -51,13 +64,15 @@ function labelPatternToRegExp(pattern: string): RegExp {
5164
labelPatternRegExpCache.set(pattern, cached);
5265
return cached;
5366
}
54-
// Reuses change-guardrail.ts's wildcard-GROUP counting (a `*` here matches the same "any run of chars"
55-
// semantics as that glob compiler's `*`, so the same catastrophic-backtracking risk and the same empirically-
56-
// safe threshold apply) — an over-complex registry-sourced label_multipliers key degrades to a safe never-match
57-
// instead of hanging RegExp.test() on an adversarial near-miss label (#2456). Reachable via the public
58-
// score-preview API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could otherwise
59-
// hang scoring for every PR on that repo.
60-
if (hasUnsafeWildcardCount(pattern)) {
67+
// Reject an over-complex registry-sourced label_multipliers key so it degrades to a safe never-match instead
68+
// of hanging RegExp.test() on an adversarial near-miss label (#2456). Reachable via the public score-preview
69+
// API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could otherwise hang
70+
// scoring for every PR on that repo. Counting is fnmatch-specific, NOT change-guardrail's path-glob count:
71+
// this compiler emits one `.*` per `*` with no `**` pairing (see below), so `**` is TWO backtracking groups
72+
// here, not the single `.*` the path compiler collapses it into (#9994) — counting via that predicate would
73+
// undercount `**` and admit a glob this compiler builds into a catastrophic-backtracking RegExp. The threshold
74+
// itself is the shared MAX_GLOB_WILDCARD_GROUPS, so the two surfaces stay on one empirically-safe boundary.
75+
if (fnmatchWildcardGroups(pattern) > MAX_GLOB_WILDCARD_GROUPS) {
6176
setLabelPatternRegExpCacheEntry(pattern, LABEL_PATTERN_NEVER_MATCHES);
6277
return LABEL_PATTERN_NEVER_MATCHES;
6378
}

packages/loopover-engine/src/signals/change-guardrail.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@ export function canonicalize(value: string): string {
3434
// protected automatically rather than needing to separately remember the risk. The boundary is set at the
3535
// highest GROUP count proven safe by the benchmark above (2) — a boundary that itself sits inside the
3636
// empirically dangerous range would defeat the point of a cap.
37-
const MAX_GLOB_WILDCARD_GROUPS = 2;
37+
// Exported (#9994) so label-match.ts's fnmatch compiler applies the SAME empirically-safe threshold rather
38+
// than redeclaring its own literal — a second, independently-chosen cap is exactly the drift the
39+
// hasUnsafeWildcardCount export below warns about.
40+
export const MAX_GLOB_WILDCARD_GROUPS = 2;
3841

3942
/** Count `*` GROUPS in `glob` — a `**` pair is ONE group (it compiles to a single `.*`, see globToRegExp), not
4043
* two. Mirrors globToRegExp's own tokenization exactly (including consuming a `**`'s trailing `/`) so the count
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { test } from "node:test";
2+
import assert from "node:assert/strict";
3+
4+
import {
5+
clearLabelPatternRegExpCacheForTest,
6+
labelMatchesPattern,
7+
labelPatternRegExpCacheKeysForTest,
8+
} from "../dist/scoring/label-match.js";
9+
10+
// #9994: the fnmatch compiler emits one `.*` per `*` and has no `**` concept, so `*a**b` compiles to THREE
11+
// `.*` groups. The old guard reused change-guardrail's path-glob counter, which scores a `**` pair as ONE
12+
// group, undercounting `**` and admitting a pattern this compiler builds into a catastrophic-backtracking
13+
// RegExp. Counting is now fnmatch-specific (one per raw `*`), so `**`-containing patterns over the cap are
14+
// rejected — they fail SAFE toward no-multiplier (never match).
15+
test("#9994: a pattern whose COMPILED groups exceed the cap via `**` is rejected (never matches)", () => {
16+
clearLabelPatternRegExpCacheForTest();
17+
assert.equal(labelMatchesPattern("anything", "*a**b"), false); // 3 stars → 3 groups → rejected
18+
assert.equal(labelMatchesPattern("x/y", "**/**"), false); // 4 stars → 4 groups → rejected
19+
});
20+
21+
test("#9994: the preserved 2-group and non-`*` cases still match exactly", () => {
22+
assert.equal(labelMatchesPattern("type:bug-fix", "type:*"), true);
23+
assert.equal(labelMatchesPattern("priority:1", "priority:?"), true); // `?` is not a counted group
24+
assert.equal(labelMatchesPattern("a-b-c", "a*b*c"), true); // 2 stars, at the cap
25+
assert.equal(labelMatchesPattern("kind:bug", "kind:[bc]ug"), true); // classes are not counted groups
26+
});
27+
28+
test("#9994: a rejected over-complex pattern is still cached (repeated read served from cache)", () => {
29+
clearLabelPatternRegExpCacheForTest();
30+
assert.equal(labelMatchesPattern("anything", "*a**b"), false);
31+
assert.ok(labelPatternRegExpCacheKeysForTest().includes("*a**b"));
32+
assert.equal(labelMatchesPattern("something-else", "*a**b"), false); // cache-hit arm, still false
33+
assert.equal(labelPatternRegExpCacheKeysForTest().filter((k) => k === "*a**b").length, 1);
34+
});

test/unit/scoring.test.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -960,9 +960,12 @@ NOVELTY_BONUS_SCALAR = 3
960960
// Regex metacharacters in a literal key stay literal: `.` matches only a dot, not any char.
961961
expect(labelMultiplierFor({ "v1.0": 1.1 }, ["v1.0"])).toBe(1.1);
962962
expect(labelMultiplierFor({ "v1.0": 1.1 }, ["v1x0"])).toBe(1);
963-
// `**/` counts as one wildcard group and matches across path-like label segments.
963+
// #9994: this fnmatch compiler counts one wildcard group per raw `*` (it has no `**`-is-one-group rule —
964+
// that is the PATH compiler's semantics), so `**/bug` and `**bug` are 2 groups (at the cap → still
965+
// compile and match), but `public/**/*.json` is THREE (`**` + `*`) and is now rejected as over-complex,
966+
// failing SAFE toward no multiplier — where the old path-glob count wrongly scored it as 2 and matched it.
964967
expect(labelMultiplierFor({ "**/bug": 1.45 }, ["feature/bug"])).toBe(1.45);
965-
expect(labelMultiplierFor({ "public/**/*.json": 1.2 }, ["public/release/config.json"])).toBe(1.2);
968+
expect(labelMultiplierFor({ "public/**/*.json": 1.2 }, ["public/release/config.json"])).toBe(1);
966969
expect(labelMultiplierFor({ "**bug": 1.35 }, ["feature-bug"])).toBe(1.35);
967970
// When several patterns match, the highest multiplier wins (mirrors upstream `max(...)`).
968971
expect(labelMultiplierFor({ "kind/*": 1.1, "*/bug": 1.6 }, ["kind/bug"])).toBe(1.6);
@@ -2150,4 +2153,30 @@ describe("label pattern matcher memoization (#2106)", () => {
21502153
expect(labelMatchesPattern("type-bug-fix", "type-*-*")).toBe(true);
21512154
expect(labelMatchesPattern("type-bug", "type-*-*")).toBe(false);
21522155
});
2156+
2157+
it("#9994: a `**`-containing pattern is counted by its COMPILED groups (one per raw *), not the path-glob `**`-is-one rule, so it is rejected", () => {
2158+
// The fnmatch compiler emits one `.*` per `*` and has no `**` concept, so `*a**b` compiles to THREE `.*`
2159+
// groups. change-guardrail's path counter scored it as 2 (a `**` pair = one group) and wrongly ACCEPTED it,
2160+
// admitting a pattern this compiler builds into a catastrophic-backtracking RegExp. All three fail SAFE
2161+
// toward no-multiplier (never matches).
2162+
clearLabelPatternRegExpCacheForTest();
2163+
expect(labelMatchesPattern("anything", "*a**b")).toBe(false); // 3 stars → 3 compiled groups → rejected
2164+
expect(labelMatchesPattern("x/y", "**/**")).toBe(false); // 4 stars → 4 compiled groups → rejected
2165+
expect(labelMatchesPattern("abc", "a**b**c")).toBe(false); // 4 stars → rejected
2166+
2167+
// The preserved 2-group cases and non-`*` metacharacters still compile and match exactly as before.
2168+
expect(labelMatchesPattern("type:bug-fix", "type:*")).toBe(true);
2169+
expect(labelMatchesPattern("priority:1", "priority:?")).toBe(true); // `?` is not a counted group
2170+
expect(labelMatchesPattern("a-b-c", "a*b*c")).toBe(true);
2171+
expect(labelMatchesPattern("kind:bug", "kind:[bc]ug")).toBe(true); // classes are not counted groups
2172+
});
2173+
2174+
it("#9994: a rejected over-complex pattern is still cached, so a repeated read is served from the cache", () => {
2175+
clearLabelPatternRegExpCacheForTest();
2176+
expect(labelMatchesPattern("anything", "*a**b")).toBe(false);
2177+
expect(labelPatternRegExpCacheKeysForTest()).toContain("*a**b");
2178+
// Second read of the same over-complex pattern is served from the cache (cache-hit arm), still false.
2179+
expect(labelMatchesPattern("something-else", "*a**b")).toBe(false);
2180+
expect(labelPatternRegExpCacheKeysForTest().filter((k) => k === "*a**b")).toHaveLength(1);
2181+
});
21532182
});

0 commit comments

Comments
 (0)