diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index c5c8c68214..0881889817 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -23,14 +23,18 @@ function canonicalize(value: string): string { // matchesAny below) so every caller — present or future, direct or indirect, including // content-lane/spec-resolver.ts's config-driven globs — is protected automatically. A caller with a STRICTER // fail direction (e.g. matchesAny's fail-toward-guarding for a security guardrail) overrides at its own layer. -const MAX_GLOB_WILDCARD_GROUPS = 2; +// Exported so any OTHER glob-safety check in this codebase (e.g. focus-manifest.ts's parse-time +// normalizeOptionalGlob, which rejects an over-complex contentLane.*Glob before it ever reaches globToRegExp) +// shares this exact threshold instead of drifting with its own re-derived number. +export 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 { + * "public/**\/*.json" — 2 real groups — as if they were 3-groups-dangerous). Exported for reuse by any other + * glob-safety check in this codebase — see MAX_GLOB_WILDCARD_GROUPS above. */ +export function countWildcardGroups(glob: string): number { let count = 0; for (let i = 0; i < glob.length; i += 1) { if (glob.charAt(i) !== "*") continue; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 3e637b30eb..a440d9d5b6 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -3,6 +3,7 @@ import type { GatePolicyPack, GateRuleMode, JsonValue, RepositorySettings } from import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../settings/autonomy"; import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; +import { countWildcardGroups, MAX_GLOB_WILDCARD_GROUPS } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; @@ -618,20 +619,15 @@ function normalizeOptionalPositiveInteger(value: JsonValue | undefined, field: s return null; } -// A glob compiled to RegExp (review/content-lane/spec-resolver.ts's globToRegExp reuse of the guardrail-path -// compiler) chains a `[^/]*` per `*` — MULTIPLE chained wildcards separated by literal characters can -// catastrophically backtrack on an adversarial near-miss input (verified empirically: 5 chained wildcards against -// a maximally-adversarial 300-char input took ~19 SECONDS; 3 stays under 5ms even at that same length). No -// legitimate single-purpose entry-file glob for this feature needs more than a couple of wildcards, so this caps -// wildcard count at parse time — well before the string ever reaches RegExp compilation — rather than trying to -// make the compiled pattern itself provably safe. -const MAX_GLOB_WILDCARDS = 3; - /** Normalize + bound a maintainer-supplied glob string: trims/length-caps like any other string field, AND caps - * the number of `*` wildcard characters (see MAX_GLOB_WILDCARDS) so it can never compile into a - * catastrophically-backtracking RegExp downstream. A glob over the cap is REJECTED (warns, returns null) rather - * than truncated — silently cutting wildcards out of a maintainer's pattern would silently change its meaning, - * which is worse than making them fix an over-complex glob. */ + * the number of wildcard GROUPS (see change-guardrail.ts's MAX_GLOB_WILDCARD_GROUPS / countWildcardGroups — + * the SAME group-based count globToRegExp itself enforces, reused here rather than re-derived, so this file + * can never drift from the actual safe boundary) so it can never compile into a catastrophically-backtracking + * RegExp downstream. A `**` pair is ONE group, not two raw `*` characters — counting raw characters would both + * wrongly reject a legitimate 2-group glob like "public/**\/*.json" (3 characters, 2 groups) and wrongly admit + * a genuinely dangerous 3-single-star glob at the same raw-character count. A glob over the cap is REJECTED + * (warns, returns null) rather than truncated — silently cutting wildcards out of a maintainer's pattern would + * silently change its meaning, which is worse than making them fix an over-complex glob. */ function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warnings: string[]): string | null { const normalized = normalizeOptionalString(value, field, warnings); if (normalized === null) return null; @@ -639,9 +635,9 @@ function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warn warnings.push(`Manifest field "${field}" truncated an over-long glob.`); } const bounded = normalized.slice(0, MAX_ITEM_LENGTH); - const wildcardCount = (bounded.match(/\*/g) ?? []).length; - if (wildcardCount > MAX_GLOB_WILDCARDS) { - warnings.push(`Manifest field "${field}" has too many wildcards (${wildcardCount} > ${MAX_GLOB_WILDCARDS}); ignoring it.`); + const wildcardGroupCount = countWildcardGroups(bounded); + if (wildcardGroupCount > MAX_GLOB_WILDCARD_GROUPS) { + warnings.push(`Manifest field "${field}" has too many wildcards (${wildcardGroupCount} > ${MAX_GLOB_WILDCARD_GROUPS}); ignoring it.`); return null; } return bounded; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index c0142fe76e..538269667a 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1090,26 +1090,34 @@ describe("parseFocusManifest gate config", () => { expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*truncated/.test(w))).toBe(true); }); - it("SECURITY (ReDoS): a glob with too many wildcards is REJECTED at parse time rather than ever reaching RegExp compilation", () => { - // 5 chained single-segment wildcards is empirically catastrophic against an adversarial input (verified - // ~19s in manual testing) — must never survive parsing to reach globToRegExp at all. + it("SECURITY (ReDoS): a glob with too many wildcard GROUPS is REJECTED at parse time rather than ever reaching RegExp compilation", () => { + // Shares change-guardrail.ts's countWildcardGroups/MAX_GLOB_WILDCARD_GROUPS — see that file for the + // benchmark proving 3+ chained wildcard groups risk catastrophic backtracking. 5 single-segment groups here + // is well over the cap; must never survive parsing to reach globToRegExp at all. const pathological = "registry/*-*-*-*-*-final.json"; const m = parseFocusManifest({ contentLane: { entryFileGlob: pathological, collectionField: "items" } }); expect(m.contentLane.entryFileGlob).toBeNull(); expect(m.contentLane.present).toBe(false); // entryFileGlob is REQUIRED — a rejected glob degrades to absent expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*too many wildcards/.test(w))).toBe(true); - // A glob AT the cap (3 wildcards) is accepted; the optional providerFileGlob/artifactGlob fields are dropped - // individually (with a warning) without invalidating the whole block, since only entryFileGlob/collectionField - // are required. + // A glob AT the cap (2 wildcard groups) is accepted; the optional providerFileGlob/artifactGlob fields are + // dropped individually (with a warning) without invalidating the whole block, since only + // entryFileGlob/collectionField are required. const atCap = parseFocusManifest({ - contentLane: { entryFileGlob: "registry/*/*/*.json", providerFileGlob: "providers/*-*-*-*-*.json", collectionField: "items" }, + contentLane: { entryFileGlob: "registry/*/*.json", providerFileGlob: "providers/*-*-*-*-*.json", collectionField: "items" }, }); expect(atCap.contentLane.present).toBe(true); - expect(atCap.contentLane.entryFileGlob).toBe("registry/*/*/*.json"); + expect(atCap.contentLane.entryFileGlob).toBe("registry/*/*.json"); expect(atCap.contentLane.providerFileGlob).toBeNull(); expect(atCap.warnings.some((w) => /contentLane\.providerFileGlob.*too many wildcards/.test(w))).toBe(true); }); + it("REGRESSION: a `**` globstar plus a single `*` — 3 raw star CHARACTERS but only 2 wildcard GROUPS — is accepted, not rejected (counting raw characters instead of groups would wrongly reject this legitimate shape)", () => { + const m = parseFocusManifest({ contentLane: { entryFileGlob: "public/**/*.json", collectionField: "items" } }); + expect(m.contentLane.present).toBe(true); + expect(m.contentLane.entryFileGlob).toBe("public/**/*.json"); + expect(m.warnings.some((w) => /entryFileGlob.*too many wildcards/.test(w))).toBe(false); + }); + it("contentLaneConfigToJson returns null for an absent config, and omits unset optional fields", () => { expect(contentLaneConfigToJson(parseFocusManifest(null).contentLane)).toBeNull(); const m = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json", collectionField: "items" } });