diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 6bf695540d..46912a37e9 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -625,6 +625,10 @@ function parseManifestGlobList(value: JsonValue | undefined, fieldLabel: string, warnings.push(`Manifest "${fieldLabel}[${index}]" must be a non-empty string; ignoring it.`); continue; } + if (glob.length > MAX_ITEM_LENGTH) { + warnings.push(`Manifest "${fieldLabel}[${index}]" exceeds ${MAX_ITEM_LENGTH} chars; ignoring it.`); + continue; + } out.push(glob); } return out; @@ -660,6 +664,10 @@ function parseReviewPathInstructions(value: JsonValue | undefined, warnings: str warnings.push(`Manifest "review.path_instructions[${index}].path" must be a non-empty string; ignoring the entry.`); continue; } + if (path.length > MAX_ITEM_LENGTH) { + warnings.push(`Manifest "review.path_instructions[${index}].path" exceeds ${MAX_ITEM_LENGTH} chars; ignoring the entry.`); + continue; + } if (e.instructions === undefined || e.instructions === null) { warnings.push(`Manifest "review.path_instructions[${index}].instructions" is required; ignoring the entry.`); continue; @@ -859,19 +867,47 @@ function normalizePathForMatch(path: string): string { return String(path).replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\/+/, "").toLowerCase(); } +/** + * LINEAR-TIME wildcard matcher for a `*`-glob pattern over an already-normalized path. `*` (and a collapsed + * run of `*`) matches any run of characters INCLUDING `/` (gittensory globs cross slashes). Implemented as a + * prefix + suffix + ordered-substring (indexOf) scan rather than a `.*`-per-star regex: the old regex + * (`^.*a.*a...$`) backtracks catastrophically on a near-miss path and could hang the gate for an entire repo + * (a manifest glob with many non-adjacent `*`). This algorithm is O(path × parts) with NO backtracking. + */ +function linearGlobMatcher(pattern: string): (path: string) => boolean { + // The caller only compiles this for a pattern that contains a wildcard, so split always yields >= 2 parts. + const parts = pattern.split(/\*+/); // literal segments between (collapsed) wildcard runs + const first = parts[0]!; + const last = parts[parts.length - 1]!; + const middles = parts.slice(1, -1).filter((part) => part.length > 0); + return (path) => { + if (!path.startsWith(first) || !path.endsWith(last)) return false; + let idx = first.length; + for (const part of middles) { + const found = path.indexOf(part, idx); + if (found === -1) return false; + idx = found + part.length; + } + return path.length - last.length >= idx; // the suffix must not overlap the consumed prefix/middles + }; +} + /** * Compile a manifest path pattern into a predicate over an ALREADY-normalized path. Supports exact paths, - * directory prefixes (`src/` or `src`), and `*` wildcards (`**` collapses to `*`). Compiling once (the - * wildcard regex in particular) lets a caller test many paths against one pattern without recompiling per - * path — see {@link matchedPatterns}. An empty/blank pattern never matches. + * directory prefixes (`src/` or `src`), and `*` wildcards (`*` and a double-star both match any run of chars + * across `/`). A double-star-then-separator prefix means "zero or more path segments", so the mandatory slash + * is absorbed and a double-star glob also matches a ROOT-level (zero-depth) file, not only nested ones. + * Compiling once lets a caller test many paths against one pattern without recompiling per path — see + * {@link matchedPatterns}. An empty/blank pattern never matches. */ function compileManifestPathMatcher(pattern: string): (normalizedPath: string) => boolean { const normalizedPattern = normalizePathForMatch(pattern); if (!normalizedPattern) return () => false; if (normalizedPattern.includes("*")) { - const escaped = normalizedPattern.replace(/[.+?^${}()|[\]\\]/g, "\\$&").replace(/\*+/g, ".*"); - const regex = new RegExp(`^${escaped}$`); - return (normalizedPath) => regex.test(normalizedPath); + // A double-star-then-slash run collapses the mandatory separator into the wildcard so the glob matches + // zero-depth/root too (e.g. a leading double-star glob matches a root-level file). Then run the linear matcher. + const globbed = normalizedPattern.replace(/\*\*\//g, "*"); + return linearGlobMatcher(globbed); } const dirPattern = normalizedPattern.endsWith("/") ? normalizedPattern : `${normalizedPattern}/`; return (normalizedPath) => normalizedPath === normalizedPattern || normalizedPath.startsWith(dirPattern); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index c506c4c50f..6cd102ab9e 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -185,6 +185,33 @@ describe("matchesManifestPath", () => { expect(matchesManifestPath("", "src/")).toBe(false); expect(matchesManifestPath("src/x.ts", "")).toBe(false); }); + + it("**/ matches at the repo ROOT too (zero-depth), not only nested files (#review-audit)", () => { + expect(matchesManifestPath("app.test.ts", "**/*.test.ts")).toBe(true); // root-level (was a bug: required a slash) + expect(matchesManifestPath("dir/app.test.ts", "**/*.test.ts")).toBe(true); // nested still matches + expect(matchesManifestPath("foo", "**/foo")).toBe(true); + expect(matchesManifestPath("a/b/foo", "**/foo")).toBe(true); + expect(matchesManifestPath("a/b/c.ts", "**/*.ts")).toBe(true); + }); + + it("multi-wildcard matching is correct (ordered substrings, suffix cannot overlap)", () => { + expect(matchesManifestPath("xayybzzc", "*a*b*c")).toBe(true); + expect(matchesManifestPath("aXbXc", "a*b*c")).toBe(true); + expect(matchesManifestPath("ab", "a*b")).toBe(true); // * matches empty + expect(matchesManifestPath("ba", "a*b")).toBe(false); // wrong order + expect(matchesManifestPath("ac", "a*b*c")).toBe(false); // 'b' missing between a and c + expect(matchesManifestPath("ab", "a*b*c")).toBe(false); // missing trailing c + }); + + it("is LINEAR on a hostile multi-star glob — no catastrophic backtracking (ReDoS, #review-audit)", () => { + const evilGlob = "*a".repeat(20); // 20 non-adjacent stars; the old code compiled this to a backtracking regex + const nearMiss = "a".repeat(300) + "b"; // long run then a non-a tail the glob cannot satisfy + const start = performance.now(); + const result = matchesManifestPath(nearMiss, evilGlob); + const elapsed = performance.now() - start; + expect(result).toBe(false); + expect(elapsed).toBeLessThan(100); // the old per-star regex did not return within 30s on this input + }); }); // Regression tests for the three compileManifestPathMatcher branches: exact, @@ -1072,6 +1099,7 @@ describe("parseFocusManifest review config", () => { "nope", // non-mapping → dropped { path: "y/**" }, // missing instructions → dropped { path: 42, instructions: "non-string path" }, // path not a string → dropped + { path: `${"a".repeat(400)}/x`, instructions: "over-long path" }, // > MAX_ITEM_LENGTH → dropped (#review-audit) ], }, }); @@ -1084,6 +1112,7 @@ describe("parseFocusManifest review config", () => { expect(m.warnings.some((w) => /path_instructions\[4\]/.test(w))).toBe(true); expect(m.warnings.some((w) => /path_instructions\[5\]\.instructions/.test(w))).toBe(true); expect(m.warnings.some((w) => /path_instructions\[6\]\.path/.test(w))).toBe(true); // non-string path + expect(m.warnings.some((w) => /path_instructions\[7\]\.path.*exceeds/.test(w))).toBe(true); // over-long path // Round-trips through the cache serializer. expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.pathInstructions).toEqual(m.review.pathInstructions); }); @@ -1154,6 +1183,13 @@ describe("review.exclude_paths (#review-exclude-paths)", () => { expect(many.warnings.some((w) => /exclude_paths.*capped/.test(w))).toBe(true); }); + it("drops an over-long glob (defense-in-depth length cap) (#review-audit)", () => { + const huge = `${"a".repeat(400)}/x.ts`; // > MAX_ITEM_LENGTH (300) + const m = parseFocusManifest({ review: { exclude_paths: [huge, "dist/**"] } }); + expect(m.review.excludePaths).toEqual(["dist/**"]); // the over-long glob is dropped, the valid one kept + expect(m.warnings.some((w) => /exclude_paths\[0\].*exceeds/.test(w))).toBe(true); + }); + it("excludeReviewPaths filters matching files; empty globs return the same array (byte-identical)", () => { const files = [{ path: "src/a.ts" }, { path: "pnpm-lock.yaml" }, { path: "dist/bundle.js" }]; // `*` collapses to `.*` (crosses slashes), so `*.yaml` matches a top-level lockfile; `dist/**` matches under dist/.