diff --git a/lib/protected-patterns.ts b/lib/protected-patterns.ts index a59605bc..15d1535c 100644 --- a/lib/protected-patterns.ts +++ b/lib/protected-patterns.ts @@ -1,5 +1,9 @@ function normalizePath(input: string): string { - return input.replaceAll("\\\\", "/") + // A single backslash. In source, "\\" is the one-character string; the + // previous "\\\\" was a *two*-character string, so it only ever matched a + // doubled separator -- which a real Windows path does not contain. The + // normalisation was therefore a no-op on the only platform that needs it. + return input.replaceAll("\\", "/") } function escapeRegExpChar(ch: string): string { diff --git a/tests/protected-patterns.test.ts b/tests/protected-patterns.test.ts new file mode 100644 index 00000000..49c941a1 --- /dev/null +++ b/tests/protected-patterns.test.ts @@ -0,0 +1,104 @@ +import assert from "node:assert/strict" +import test from "node:test" +import { + getFilePathsFromParameters, + isFilePathProtected, + isToolNameProtected, + matchesGlob, +} from "../lib/protected-patterns" + +// A single backslash, built from its char code so the intent survives any later +// reformatting of this file. The bug being pinned here was precisely an escaping +// mistake in a string literal, so the tests avoid writing one by hand. +const BS = String.fromCharCode(92) +const winPath = (...segments: string[]) => segments.join(BS) + +test("matchesGlob treats a Windows separator as a path separator", () => { + // `protectedFilePatterns` is documented with forward slashes ("**/*.config.ts"), + // but on Windows the tool parameters carry backslashes. Both sides are + // normalised, so the same pattern must match either spelling of the same file. + const path = winPath("C:", "repo", "src", "config", "secrets.ts") + + assert.equal(matchesGlob(path, "**/secrets.ts"), true) + assert.equal(matchesGlob(path, "**/config/*.ts"), true) + assert.equal(matchesGlob(path, "C:/repo/src/**"), true) + assert.equal(matchesGlob(path, "**/*.ts"), true) +}) + +test("matchesGlob accepts a pattern written with Windows separators", () => { + // Normalisation applies to the pattern too, so a user who copies a path out of + // Explorer and uses it as a pattern gets the same result as the documented form. + const pattern = winPath("**", "config", "*.ts") + + assert.equal(matchesGlob("C:/repo/src/config/secrets.ts", pattern), true) + assert.equal(matchesGlob(winPath("C:", "repo", "src", "config", "secrets.ts"), pattern), true) +}) + +test("a single-segment wildcard still does not cross a Windows separator", () => { + // `*` is defined as "[^/]*". Normalisation must convert separators rather than + // erase them, or `*` would silently start matching across directories. + assert.equal(matchesGlob(winPath("src", "config", "secrets.ts"), "src/*"), false) + assert.equal(matchesGlob(winPath("src", "config", "secrets.ts"), "src/*/*.ts"), true) + assert.equal(matchesGlob(winPath("src", "secrets.ts"), "src/*"), true) +}) + +test("isFilePathProtected protects a Windows path the user configured", () => { + // The end-to-end shape: a `read` tool call on Windows, checked against the + // documented pattern style. This is the assertion that failed before the fix -- + // the file was protected on POSIX and unprotected on Windows. + const patterns = ["**/secrets.ts", "**/.env"] + + for (const path of [ + "C:/repo/src/config/secrets.ts", + winPath("C:", "repo", "src", "config", "secrets.ts"), + ]) { + const paths = getFilePathsFromParameters("read", { filePath: path }) + assert.equal(isFilePathProtected(paths, patterns), true, `not protected: ${path}`) + } +}) + +test("isFilePathProtected still returns false for a genuinely unmatched path", () => { + // The fix must widen matching only for separators, not for anything else. + const paths = getFilePathsFromParameters("read", { + filePath: winPath("C:", "repo", "src", "main.ts"), + }) + + assert.equal(isFilePathProtected(paths, ["**/secrets.ts"]), false) + assert.equal(isFilePathProtected(paths, []), false) + assert.equal(isFilePathProtected([], ["**/*.ts"]), false) +}) + +test("multiedit and apply_patch paths are protected on Windows too", () => { + // These two tools carry paths in shapes of their own, so they need their own + // coverage: a nested `edits` array and paths embedded in patch text. + const patterns = ["**/secrets.ts"] + + const multiedit = getFilePathsFromParameters("multiedit", { + filePath: winPath("src", "main.ts"), + edits: [{ filePath: winPath("src", "config", "secrets.ts") }], + }) + assert.equal(isFilePathProtected(multiedit, patterns), true) + + const patch = getFilePathsFromParameters("apply_patch", { + patchText: `*** Update File: ${winPath("src", "config", "secrets.ts")}\n@@\n-a\n+b\n`, + }) + assert.equal(isFilePathProtected(patch, patterns), true) +}) + +test("isToolNameProtected is unaffected by separator normalisation", () => { + // Tool names contain no separators; this pins that the shared helper does not + // change their behaviour. + assert.equal(isToolNameProtected("bash", ["bash"]), true) + assert.equal(isToolNameProtected("bash", ["ba*"]), true) + assert.equal(isToolNameProtected("bash", ["read"]), false) + assert.equal(isToolNameProtected("bash", []), false) + assert.equal(isToolNameProtected("", ["bash"]), false) +}) + +test("matchesGlob rejects an empty pattern and handles regex metacharacters", () => { + // Pattern text is interpolated into a RegExp, so characters that mean something + // there must be matched literally. + assert.equal(matchesGlob("a/b.ts", ""), false) + assert.equal(matchesGlob("src/a+b(1).ts", "src/a+b(1).ts"), true) + assert.equal(matchesGlob("src/axb1.ts", "src/a+b(1).ts"), false) +})