diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 437f3f2c66..51fff896c4 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -12,11 +12,74 @@ function canonicalize(value: string): string { return value.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\/+/, "").toLowerCase(); } +// globToRegExp's COMPILATION is linear-time, but the COMPILED pattern's .test() can be polynomial-to-exponential +// time on an adversarial near-miss input when MULTIPLE wildcard GROUPS chain in one glob (a "group" is one `*` +// OR one `**` — a `**` pair compiles to a SINGLE `.*`, not two independent wildcards, so it must be counted as +// ONE group, not two characters; see countWildcardGroups below). Both group TYPES contribute to the same danger +// once chained — `[^/]*` groups separated by a literal that class doesn't exclude (e.g. "-", not "/") back- +// track ambiguously, and `.*` groups back-track ambiguously EVEN when "/"-separated, since `.*` crosses `/` +// freely. Re-benchmarked against `path` lengths GitHub can plausibly deliver via a deeply nested file path in a +// malicious PR (both `path` and, via `.gittensory.yml`'s contentLane.*Glob fields, the glob itself can be +// attacker-influenced in the same PR): +// 2 wildcard groups (any mix of `*`/`**`, any arrangement): sub-second even at a wildly implausible 32,000- +// char adversarial path (worst case observed: ~400ms) — quadratic, bounded, never a +// realistic hang. +// 3 wildcard groups: OVER 2 SECONDS at just ~4,000 chars for one chained-`*` shape, over 100ms at ~1,600 +// chars for a chained-`**` shape — already dangerous well within a plausible path length. +// 4+ wildcard groups: confirmed catastrophic — 35 SECONDS at just 1,614 chars for 4 chained `**` groups. +// hardGuardrailGlobs today are 100% hardcoded engine constants (see review/guardrail-config.ts) — no +// maintainer/contributor input reaches globToRegExp via that path today, and none of those real globs exceed 1 +// wildcard group — but it is also exported for reuse by other maintainer-config-driven consumers +// (content-lane/spec-resolver.ts, whose real globs like "public/**/*.json" are exactly 2 groups: this cap must +// stay inclusive of that legitimate shape, not just "safer than before"), so the cap lives INSIDE globToRegExp +// itself (not just in a wrapper like matchesAny below) — every caller, present or future, direct or indirect, is +// protected automatically rather than needing to separately remember the risk. The boundary is set at the +// highest GROUP count proven safe by the benchmark above (2) — a boundary that itself sits inside the +// empirically dangerous range would defeat the point of a cap. +const MAX_GLOB_WILDCARD_GROUPS = 2; + +/** Count `*` GROUPS in `glob` — a `**` pair is ONE group (it compiles to a single `.*`, see globToRegExp), not + * two. Mirrors globToRegExp's own tokenization exactly (including consuming a `**`'s trailing `/`) so the count + * reflects the actual number of backtracking-capable groups the compiled RegExp will contain, not raw `*` + * character count (which would double-count every globstar and reject legitimate globs like + * "public/**\/*.json" — 2 real groups — as if they were 3-groups-dangerous). */ +function countWildcardGroups(glob: string): number { + let count = 0; + for (let i = 0; i < glob.length; i += 1) { + if (glob.charAt(i) !== "*") continue; + count += 1; + if (glob.charAt(i + 1) === "*") { + i += 1; // consume the second star of the "**" pair — one group, not two + if (glob.charAt(i + 1) === "/") i += 1; // `**/` also matches zero segments, mirroring globToRegExp + } + } + return count; +} + +/** True if `glob` has more wildcard GROUPS than can be safely compiled to a RegExp without risking catastrophic + * backtracking (see the MAX_GLOB_WILDCARD_GROUPS rationale above). */ +function hasUnsafeWildcardCount(glob: string): boolean { + return countWildcardGroups(glob) > MAX_GLOB_WILDCARD_GROUPS; +} + +// A RegExp that never matches any input, at any position — the safe, conservative compiled form of an +// over-complex glob. "Never matches" (not "matches everything") is the correct default HERE because +// globToRegExp has no context on caller intent, and a false "matches everything" would be actively wrong for a +// non-guardrail caller (e.g. content-lane file-scope matching, where "matches everything" would misclassify +// every changed file as a registry submission). A caller whose OWN semantics want the opposite fail direction +// (a security guardrail, where under-protection is worse than an unnecessary hold) checks hasUnsafeWildcardCount +// itself and overrides — see matchesAny below. +const NEVER_MATCHES = /^(?!)$/; + /** Convert a path glob (`*` matches within a segment, `**` matches across `/`) to an anchored RegExp. The * glob is canonicalized first, so matching is case-insensitive against a canonicalized path. Exported for * reuse anywhere a maintainer-supplied path pattern needs compiling — never compile a raw regex string from - * config (ReDoS risk); this linear-time glob compiler is the one safe path pattern this codebase uses. */ + * config (ReDoS risk); this linear-time glob compiler is the one safe path pattern this codebase uses. + * + * An over-complex glob (see MAX_GLOB_WILDCARD_GROUPS) short-circuits to NEVER_MATCHES instead of being compiled — + * this function never returns a RegExp that risks catastrophic backtracking on .test(), for any input. */ export function globToRegExp(glob: string): RegExp { + if (hasUnsafeWildcardCount(glob)) return NEVER_MATCHES; const canonical = canonicalize(glob); let re = ""; for (let i = 0; i < canonical.length; i += 1) { @@ -38,10 +101,18 @@ export function globToRegExp(glob: string): RegExp { return new RegExp(`^${re}$`); } -/** True if `path` matches any of the globs (`*` within a segment, `**` across `/`), case-insensitively. */ +/** + * True if `path` matches any of the globs (`*` within a segment, `**` across `/`), case-insensitively. A glob + * with more wildcards than can be safely compiled (see hasUnsafeWildcardCount) is treated as matching EVERY + * path — fail SAFE TOWARD GUARDING, mirroring isGuardrailHit's own "unknown ⇒ treat as a hit" philosophy (an + * over-complex guardrail glob still forces manual review) rather than the NEVER_MATCHES default globToRegExp + * itself falls back to, which would silently disable the maintainer's intended protection — the worse failure + * mode for a safety guardrail specifically (see globToRegExp's own docstring for why NEVER_MATCHES is still the + * right default for globToRegExp as a general-purpose compiler). + */ export function matchesAny(path: string, globs: string[]): boolean { const canonicalPath = canonicalize(path); - return globs.some((g) => globToRegExp(g).test(canonicalPath)); + return globs.some((g) => hasUnsafeWildcardCount(g) || globToRegExp(g).test(canonicalPath)); } /** diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts index 7246724c2f..65ab3a1bb0 100644 --- a/test/unit/change-guardrail.test.ts +++ b/test/unit/change-guardrail.test.ts @@ -1,5 +1,45 @@ import { describe, expect, it } from "vitest"; -import { changedPathsHittingGuardrail, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; +import { changedPathsHittingGuardrail, globToRegExp, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; + +describe("globToRegExp (the exported compiler itself — must be safe for ANY direct caller, not just matchesAny)", () => { + it("compiles an ordinary glob to a working anchored RegExp", () => { + expect(globToRegExp("scripts/**").test("scripts/build.mjs")).toBe(true); + expect(globToRegExp("src/*.ts").test("src/auth.ts")).toBe(true); + expect(globToRegExp("src/*.ts").test("src/auth/session.ts")).toBe(false); + }); + + it("SECURITY (ReDoS): called DIRECTLY (bypassing matchesAny entirely) on a pathological glob, resolves instantly against a genuinely adversarial multi-KB path and matches nothing — the cap lives inside the compiler itself, not just in matchesAny's wrapper", () => { + // 3 chained single-segment wildcards is already empirically dangerous (over 2 seconds at ~4,000 chars — see + // MAX_GLOB_WILDCARD_GROUPS's rationale), so this glob alone proves the cap rejects the FIRST unsafe value, not + // just an extreme one. + const pathological = "src/*-*-*-final.ts"; + const adversarialPath = "src/" + "a-".repeat(2000) + "X"; // ~4,000 chars — the empirically dangerous length for 3 wildcards + const start = Date.now(); + const compiled = globToRegExp(pathological); + expect(compiled.test(adversarialPath)).toBe(false); + expect(compiled.test("completely/unrelated/path.md")).toBe(false); + expect(compiled.test("")).toBe(false); + expect(compiled.test("src/a-b-c-final.ts")).toBe(false); // even a "near miss" that would otherwise match + expect(Date.now() - start).toBeLessThan(1000); + }); + + it("a glob AT the safe cap (2 wildcards), called directly, still compiles and matches normally — proves the cap is inclusive, not exclusive", () => { + const atCap = "src/*/*.ts"; + expect(globToRegExp(atCap).test("src/a/f.ts")).toBe(true); + expect(globToRegExp(atCap).test("src/a/f.js")).toBe(false); + }); + + it("SECURITY (ReDoS, correctness of the group-vs-character count): a single `**` globstar is ONE wildcard group (not two), so it never gets anywhere near the cap on its own", () => { + expect(globToRegExp("scripts/**").test("scripts/deep/nested/build.mjs")).toBe(true); + }); + + it("a `**` globstar PLUS a single `*` — 3 raw star CHARACTERS but only 2 wildcard GROUPS — compiles and matches normally, not the fail-safe path. This is the real content-lane/spec-resolver.ts shape (e.g. an artifactGlob like \"public/**/*.json\"); counting raw `*` characters instead of groups would wrongly reject it", () => { + const mixed = "public/**/*.json"; + expect(globToRegExp(mixed).test("public/deep/nested/report.json")).toBe(true); + expect(globToRegExp(mixed).test("public/report.json")).toBe(true); // `**/` also matches zero segments + expect(globToRegExp(mixed).test("public/deep/nested/report.txt")).toBe(false); // wrong extension + }); +}); describe("change-guardrail glob matching", () => { it("`**` matches across path separators (a guarded dir guards its whole subtree)", () => { @@ -53,6 +93,44 @@ describe("change-guardrail glob matching", () => { // FAIL-SAFE (#1062): guardrails configured but the changed-file set is empty (unknown) ⇒ treat as a hit. expect(isGuardrailHit([], globs)).toBe(true); }); + + it("SECURITY (ReDoS): a glob with too many chained wildcards no longer risks catastrophic backtracking — it fails SAFE TOWARD GUARDING (matches every path) instead of ever compiling the pathological pattern", () => { + // 3 chained single-segment wildcards is already empirically dangerous (see MAX_GLOB_WILDCARD_GROUPS's rationale: + // over 2 seconds at a ~4,000-char adversarial path) — 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. + const pathological = "src/*-*-*-final.ts"; + const adversarialPath = "src/" + "a-".repeat(2000) + "X"; + const start = Date.now(); + // A pathological guardrail glob still HOLDS the PR for manual review (the safe direction for a guardrail — + // silently disabling protection would be far worse than an unnecessary hold). + expect(matchesAny(adversarialPath, [pathological])).toBe(true); + expect(matchesAny("completely/unrelated/path.md", [pathological])).toBe(true); + expect(matchesAny("", [pathological])).toBe(true); + expect(Date.now() - start).toBeLessThan(1000); + expect(changedPathsHittingGuardrail(["unrelated/file.ts"], [pathological])).toEqual(["unrelated/file.ts"]); + expect(isGuardrailHit(["unrelated/file.ts"], [pathological])).toBe(true); + }); + + it("SECURITY (ReDoS): a glob AT the safe cap (2 wildcards) still compiles and matches NORMALLY (not the fail-safe path)", () => { + // Exactly 2 stars: at the cap, still safely compiled/evaluated — proves the cap is inclusive, not exclusive, + // and that ordinary (non-pathological) multi-wildcard globs keep their real matching semantics. This is also + // the shape of nearly every real guardrail glob in production (a single `**` = 2 wildcard characters). + const atCap = "src/*/*.ts"; + expect(matchesAny("src/a/f.ts", [atCap])).toBe(true); + expect(matchesAny("src/a/f.js", [atCap])).toBe(false); // wrong extension — genuinely doesn't match + }); + + it("a wildcard-free literal glob (e.g. an exact guarded file like '.gittensory.yml') is never treated as unsafe", () => { + expect(matchesAny(".gittensory.yml", [".gittensory.yml"])).toBe(true); + expect(matchesAny("other-file.yml", [".gittensory.yml"])).toBe(false); + }); + + it("a mix of one pathological glob among otherwise-fine globs still forces a hold for ANY path (fail-safe dominates)", () => { + const globs = ["docs/**", "src/*-*-*-final.ts"]; + // "docs/**" alone would not match this path, but the pathological glob's fail-safe short-circuits matchesAny + // to true for every path once any configured glob is judged unsafe to compile. + expect(matchesAny("completely/unrelated.md", globs)).toBe(true); + }); }); // #flood-readiness: the live hard-guardrail globs must guard crucial files that live OUTSIDE the dir-prefix