Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 42 additions & 6 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
36 changes: 36 additions & 0 deletions test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
],
},
});
Expand All @@ -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);
});
Expand Down Expand Up @@ -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/.
Expand Down
Loading