diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index bd440512ce..95f246dbe7 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -123,9 +123,23 @@ export const globalSettingsSchema = z.object({ autoApprovalEnabled: z.boolean().optional(), alwaysAllowReadOnly: z.boolean().optional(), alwaysAllowReadOnlyOutsideWorkspace: z.boolean().optional(), + /** + * Gitignore-style patterns naming the files that may be read without + * approval, even when `alwaysAllowReadOnly` is off. Resolved relative to the + * workspace root; absolute patterns are also accepted. + */ + allowedReadFiles: z.array(z.string()).optional(), alwaysAllowWrite: z.boolean().optional(), alwaysAllowWriteOutsideWorkspace: z.boolean().optional(), alwaysAllowWriteProtected: z.boolean().optional(), + /** + * Gitignore-style path patterns, relative to the workspace root, whose files + * may be created/edited without approval even when `alwaysAllowWrite` is off. + * + * Lets a user grant a narrow, path-scoped write permission (for example a + * scratchpad file) without auto-approving writes to the whole workspace. + */ + allowedWriteFiles: z.array(z.string()).optional(), writeDelayMs: z.number().min(0).optional(), /** * Fuzzy matching threshold for the multi-search-replace diff strategy. diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index ea52c09599..6f3ae38ec9 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -270,9 +270,11 @@ export type ExtensionState = Pick< | "autoApprovalEnabled" | "alwaysAllowReadOnly" | "alwaysAllowReadOnlyOutsideWorkspace" + | "allowedReadFiles" | "alwaysAllowWrite" | "alwaysAllowWriteOutsideWorkspace" | "alwaysAllowWriteProtected" + | "allowedWriteFiles" | "alwaysAllowMcp" | "alwaysAllowModeSwitch" | "alwaysAllowSubtasks" diff --git a/src/core/auto-approval/__tests__/allowedReadFiles.spec.ts b/src/core/auto-approval/__tests__/allowedReadFiles.spec.ts new file mode 100644 index 0000000000..de868136f0 --- /dev/null +++ b/src/core/auto-approval/__tests__/allowedReadFiles.spec.ts @@ -0,0 +1,214 @@ +// npx vitest run core/auto-approval/__tests__/allowedReadFiles.spec.ts + +import { checkAutoApproval } from ".." +import { CWD, baseState, type State } from "./fixtures" + +const askToRead = async ({ + state, + tool = "readFile", + cwd = CWD, + ...payload +}: { + state: Partial + tool?: string + cwd?: string + path?: string + batchFiles?: Array<{ path: string }> + batchDirs?: Array<{ path: string }> + additionalFileCount?: number + isOutsideWorkspace?: boolean +}) => + checkAutoApproval({ + state: { ...baseState, ...state }, + cwd, + ask: "tool", + text: JSON.stringify({ tool, ...payload }), + }) + +describe("allowedReadFiles auto-approval", () => { + it("asks when the file is not listed", async () => { + expect(await askToRead({ path: "src/index.ts", state: { allowedReadFiles: ["notes.md"] } })).toEqual({ + decision: "ask", + }) + }) + + it("approves a listed file even though alwaysAllowReadOnly is off", async () => { + expect(await askToRead({ path: "notes.md", state: { allowedReadFiles: ["notes.md"] } })).toEqual({ + decision: "approve", + }) + }) + + it("approves a listed file outside the workspace without the outside-workspace toggle", async () => { + expect( + await askToRead({ + path: "/tmp/notes.md", + isOutsideWorkspace: true, + state: { allowedReadFiles: ["/tmp/notes.md"] }, + }), + ).toEqual({ decision: "approve" }) + }) + + it("approves a file covered by a glob", async () => { + expect( + await askToRead({ path: "docs/scratch/a.md", state: { allowedReadFiles: ["docs/scratch/**"] } }), + ).toEqual({ decision: "approve" }) + }) + + // Write permission implies read permission. + it("approves a file listed only in the write allowlist", async () => { + expect(await askToRead({ path: "notes.md", state: { allowedWriteFiles: ["notes.md"] } })).toEqual({ + decision: "approve", + }) + }) + + it("does not grant write permission for a read-listed file", async () => { + expect( + await checkAutoApproval({ + state: { ...baseState, allowedReadFiles: ["notes.md"] }, + cwd: CWD, + ask: "tool", + text: JSON.stringify({ tool: "newFileCreated", path: "notes.md" }), + }), + ).toEqual({ decision: "ask" }) + }) + + // The patterns and the path have to be resolved against the same root, which is + // the one the task runs in rather than the one the window currently shows. + it("resolves the patterns against the cwd it is given", async () => { + const state = { allowedReadFiles: ["/path/to/repo/notes.md"] } + + expect(await askToRead({ path: "notes.md", cwd: "/path/to/repo", state })).toEqual({ + decision: "approve", + }) + + expect(await askToRead({ path: "notes.md", cwd: "/path/to/other-repo", state })).toEqual({ + decision: "ask", + }) + }) + + describe("batch reads", () => { + it("approves when every file in the batch is listed", async () => { + expect( + await askToRead({ + batchFiles: [{ path: "notes.md" }, { path: "todo.md" }], + state: { allowedReadFiles: ["*.md"] }, + }), + ).toEqual({ decision: "approve" }) + }) + + // One approval answers for the whole batch, so a single unlisted file + // must not be carried in by its listed siblings. + it("asks when only some files in the batch are listed", async () => { + expect( + await askToRead({ + batchFiles: [{ path: "notes.md" }, { path: "src/index.ts" }], + state: { allowedReadFiles: ["notes.md"] }, + }), + ).toEqual({ decision: "ask" }) + }) + + it("draws on both allowlists across a batch", async () => { + expect( + await askToRead({ + batchFiles: [{ path: "notes.md" }, { path: "todo.md" }], + state: { allowedReadFiles: ["notes.md"], allowedWriteFiles: ["todo.md"] }, + }), + ).toEqual({ decision: "approve" }) + }) + + it("does not let a listed batch carry an unlisted path", async () => { + expect( + await askToRead({ + path: "src/index.ts", + batchFiles: [{ path: "notes.md" }], + state: { allowedReadFiles: ["notes.md"] }, + }), + ).toEqual({ decision: "ask" }) + }) + + it("asks when the read names no file at all", async () => { + expect(await askToRead({ state: { allowedReadFiles: ["*", "**", "notes.md"] } })).toEqual({ + decision: "ask", + }) + }) + + // `additionalFileCount` reports files the message does not name, and the + // approval would cover them as well, so no pattern can vouch for them. + it("asks when the read carries files it does not name", async () => { + expect( + await askToRead({ + path: "notes.md", + additionalFileCount: 2, + state: { allowedReadFiles: ["notes.md"] }, + }), + ).toEqual({ decision: "ask" }) + + expect( + await askToRead({ + batchFiles: [{ path: "notes.md" }], + additionalFileCount: 1, + state: { allowedReadFiles: ["*.md"] }, + }), + ).toEqual({ decision: "ask" }) + }) + }) + + // The allowlist names files, but these tools act on directories and report + // on files no pattern named, so a pattern must not approve them. + describe("tools that are not file reads", () => { + it.each(["listFiles", "listFilesTopLevel", "listFilesRecursive", "searchFiles", "codebaseSearch"])( + "asks for %s even when the path is listed", + async (tool) => { + expect( + await askToRead({ + tool, + path: "docs", + state: { allowedReadFiles: ["docs", "docs/**", "**"] }, + }), + ).toEqual({ decision: "ask" }) + }, + ) + + it("still approves those tools when alwaysAllowReadOnly is on", async () => { + expect(await askToRead({ tool: "listFiles", path: "docs", state: { alwaysAllowReadOnly: true } })).toEqual({ + decision: "approve", + }) + }) + + // Listing tools report their directories in `batchDirs`. They are turned away + // by the tool name, but assert it here as well so the two reasons cannot both + // disappear unnoticed. + it("asks for a directory listing batch even when the directories are listed", async () => { + expect( + await askToRead({ + tool: "listFilesTopLevel", + batchDirs: [{ path: "docs" }, { path: "src" }], + state: { allowedReadFiles: ["docs", "src", "**"] }, + }), + ).toEqual({ decision: "ask" }) + }) + }) + + it("asks when auto-approval is disabled entirely", async () => { + expect( + await askToRead({ + path: "notes.md", + state: { allowedReadFiles: ["notes.md"], autoApprovalEnabled: false }, + }), + ).toEqual({ decision: "ask" }) + }) + + it("leaves the alwaysAllowReadOnly behaviour unchanged when nothing is listed", async () => { + expect(await askToRead({ path: "src/index.ts", state: { alwaysAllowReadOnly: true } })).toEqual({ + decision: "approve", + }) + + expect( + await askToRead({ + path: "/tmp/notes.md", + isOutsideWorkspace: true, + state: { alwaysAllowReadOnly: true }, + }), + ).toEqual({ decision: "ask" }) + }) +}) diff --git a/src/core/auto-approval/__tests__/allowedWriteFiles.spec.ts b/src/core/auto-approval/__tests__/allowedWriteFiles.spec.ts new file mode 100644 index 0000000000..8013a3b715 --- /dev/null +++ b/src/core/auto-approval/__tests__/allowedWriteFiles.spec.ts @@ -0,0 +1,182 @@ +// npx vitest run core/auto-approval/__tests__/allowedWriteFiles.spec.ts + +import { checkAutoApproval } from ".." +import { CWD, baseState, type State } from "./fixtures" + +const askToWrite = async ({ + path, + state, + tool = "newFileCreated", + isProtected, + isOutsideWorkspace, + cwd = CWD, + batchDiffs, +}: { + path?: string + state: Partial + tool?: string + isProtected?: boolean + isOutsideWorkspace?: boolean + cwd?: string + batchDiffs?: { path: string }[] +}) => + checkAutoApproval({ + state: { ...baseState, ...state }, + cwd, + ask: "tool", + text: JSON.stringify({ tool, path, isOutsideWorkspace, isProtected, batchDiffs }), + isProtected, + }) + +describe("allowedWriteFiles auto-approval", () => { + it("asks when the file is not listed", async () => { + expect(await askToWrite({ path: "src/index.ts", state: { allowedWriteFiles: ["notes.md"] } })).toEqual({ + decision: "ask", + }) + }) + + it("approves a listed file even though alwaysAllowWrite is off", async () => { + expect(await askToWrite({ path: "notes.md", state: { allowedWriteFiles: ["notes.md"] } })).toEqual({ + decision: "approve", + }) + }) + + it("approves each write tool action for a listed file", async () => { + for (const tool of ["editedExistingFile", "appliedDiff", "newFileCreated", "generateImage"]) { + expect(await askToWrite({ path: "notes.md", state: { allowedWriteFiles: ["notes.md"] }, tool })).toEqual({ + decision: "approve", + }) + } + }) + + it("approves a listed file outside the workspace without the outside-workspace toggle", async () => { + expect( + await askToWrite({ + path: "/tmp/notes.md", + isOutsideWorkspace: true, + state: { allowedWriteFiles: ["/tmp/notes.md"] }, + }), + ).toEqual({ decision: "approve" }) + }) + + it("still asks for a protected file, even when listed", async () => { + expect( + await askToWrite({ + path: "AGENTS.md", + isProtected: true, + state: { allowedWriteFiles: ["*.md"] }, + }), + ).toEqual({ decision: "ask" }) + }) + + it("approves a listed protected file once protected writes are allowed", async () => { + expect( + await askToWrite({ + path: "AGENTS.md", + isProtected: true, + state: { allowedWriteFiles: ["*.md"], alwaysAllowWriteProtected: true }, + }), + ).toEqual({ decision: "approve" }) + }) + + // Write permission implies read permission. + it("grants read permission for a listed file", async () => { + expect( + await checkAutoApproval({ + state: { ...baseState, allowedWriteFiles: ["notes.md"] }, + cwd: CWD, + ask: "tool", + text: JSON.stringify({ tool: "readFile", path: "notes.md" }), + }), + ).toEqual({ decision: "approve" }) + }) + + it("does not grant read permission for an unlisted file", async () => { + expect( + await checkAutoApproval({ + state: { ...baseState, allowedWriteFiles: ["notes.md"] }, + cwd: CWD, + ask: "tool", + text: JSON.stringify({ tool: "readFile", path: "src/index.ts" }), + }), + ).toEqual({ decision: "ask" }) + }) + + it("asks when auto-approval is disabled entirely", async () => { + expect( + await askToWrite({ + path: "notes.md", + state: { allowedWriteFiles: ["notes.md"], autoApprovalEnabled: false }, + }), + ).toEqual({ decision: "ask" }) + }) + + it("leaves the alwaysAllowWrite behaviour unchanged when nothing is listed", async () => { + expect(await askToWrite({ path: "src/index.ts", state: { alwaysAllowWrite: true } })).toEqual({ + decision: "approve", + }) + + expect( + await askToWrite({ + path: "/tmp/notes.md", + isOutsideWorkspace: true, + state: { alwaysAllowWrite: true }, + }), + ).toEqual({ decision: "ask" }) + }) + + // A workspace-relative pattern and a workspace-relative path are only about the + // same file if both are resolved against the root the task actually runs in. A + // resumed or child task can run in a different one than the window shows, so the + // caller passes the task's own `cwd` rather than the provider's. + it("resolves the patterns against the cwd it is given", async () => { + const state = { allowedWriteFiles: ["/path/to/repo/notes.md"] } + + expect(await askToWrite({ path: "notes.md", cwd: "/path/to/repo", state })).toEqual({ + decision: "approve", + }) + + // The same relative path, in another workspace, is another file. + expect(await askToWrite({ path: "notes.md", cwd: "/path/to/other-repo", state })).toEqual({ + decision: "ask", + }) + }) + + describe("a write naming several files", () => { + // One approval covers the whole action, so a pattern has to cover all of it. + it("approves only when every file in the batch is listed", async () => { + expect( + await askToWrite({ + state: { allowedWriteFiles: ["docs/**"] }, + tool: "appliedDiff", + batchDiffs: [{ path: "docs/a.md" }, { path: "docs/b.md" }], + }), + ).toEqual({ decision: "approve" }) + + expect( + await askToWrite({ + state: { allowedWriteFiles: ["docs/**"] }, + tool: "appliedDiff", + batchDiffs: [{ path: "docs/a.md" }, { path: "src/index.ts" }], + }), + ).toEqual({ decision: "ask" }) + }) + + it("does not let a listed batch carry an unlisted path", async () => { + expect( + await askToWrite({ + path: "src/index.ts", + state: { allowedWriteFiles: ["docs/**"] }, + tool: "appliedDiff", + batchDiffs: [{ path: "docs/a.md" }], + }), + ).toEqual({ decision: "ask" }) + }) + + it("asks when the action names no file at all", async () => { + expect( + await askToWrite({ state: { allowedWriteFiles: ["docs/**", "*", "**"] }, tool: "appliedDiff" }), + ).toEqual({ decision: "ask" }) + }) + }) +}) diff --git a/src/core/auto-approval/__tests__/filePatterns.spec.ts b/src/core/auto-approval/__tests__/filePatterns.spec.ts new file mode 100644 index 0000000000..067a543f48 --- /dev/null +++ b/src/core/auto-approval/__tests__/filePatterns.spec.ts @@ -0,0 +1,285 @@ +// npx vitest run core/auto-approval/__tests__/filePatterns.spec.ts + +import { isFileMatchedByPatterns, toMatcherPattern } from "../filePatterns" + +const CWD = "/path/to/repo" + +// Both platforms' rules are exercised on whichever platform the tests run on, by +// passing `isWindows` and `homeDir` explicitly instead of reading the real +// platform. Otherwise these assertions would encode the CI runner's OS: on +// Windows a workspace lives on a drive, and `os.homedir()` starts with one. +const HOME = "/home/me" +const WINDOWS_CWD = "C:/path/to/repo" +const WINDOWS_HOME = "C:\\Users\\me" + +// `cwd` is spelled out at every call rather than defaulted, since `undefined` is +// itself a case under test (no folder open) and a default would silently replace +// it with a workspace root. +const matches = (filePath: string, patterns: string[], cwd: string | undefined = CWD) => + isFileMatchedByPatterns({ filePath, cwd, patterns, isWindows: false, homeDir: HOME }) + +const matchesOnWindows = (filePath: string, patterns: string[], cwd: string | undefined = WINDOWS_CWD) => + isFileMatchedByPatterns({ filePath, cwd, patterns, isWindows: true, homeDir: WINDOWS_HOME }) + +const toPattern = (pattern: string, cwd: string | undefined) => toMatcherPattern(pattern, cwd, false, HOME) + +const toWindowsPattern = (pattern: string, cwd: string | undefined = WINDOWS_CWD) => + toMatcherPattern(pattern, cwd, true, WINDOWS_HOME) + +describe("toMatcherPattern", () => { + it("prefixes the workspace root and lets a bare filename match in any directory", () => { + expect(toPattern("notes.md", CWD)).toBe("/path/to/repo/**/notes.md") + }) + + it("anchors a pattern containing a slash to the workspace root", () => { + expect(toPattern("docs/notes.md", CWD)).toBe("/path/to/repo/docs/notes.md") + }) + + it("anchors an explicitly workspace-root-relative pattern", () => { + expect(toPattern("./notes.md", CWD)).toBe("/path/to/repo/notes.md") + }) + + it("keeps backslashes, which gitignore uses to escape rather than to separate", () => { + expect(toPattern("notes.md\\ ", CWD)).toBe("/path/to/repo/**/notes.md\\ ") + expect(toPattern("\\#hash.md", CWD)).toBe("/path/to/repo/**/\\#hash.md") + }) + + it("resolves a workspace-escaping pattern against the workspace root", () => { + expect(toPattern("../shared/notes.md", CWD)).toBe("/path/to/shared/notes.md") + }) + + it("expands a leading ~ to the home directory", () => { + expect(toPattern("~/notes.md", CWD)).toBe("/home/me/notes.md") + }) + + it("anchors a negation exactly like the pattern it cancels", () => { + expect(toPattern("!notes.md", CWD)).toBe("!/path/to/repo/**/notes.md") + expect(toPattern("!/tmp/notes.md", CWD)).toBe("!/tmp/notes.md") + }) + + it.each([ + ["an empty pattern", ""], + ["a whitespace-only pattern", " "], + ["the workspace root itself", "."], + ["the home directory itself", "~"], + ["a directory pattern", "mydir/"], + ])("rejects %s", (_label, pattern) => { + expect(toPattern(pattern, CWD)).toBeUndefined() + }) + + it("preserves whitespace, which gitignore syntax treats as significant", () => { + expect(toPattern(" notes.md", CWD)).toBe("/path/to/repo/**/ notes.md") + expect(toPattern("my notes.md", CWD)).toBe("/path/to/repo/**/my notes.md") + }) + + // See noWorkspaceRoot.spec.ts for why this fails closed. + it("rejects a workspace-relative pattern when the workspace root is unknown", () => { + expect(toPattern("../shared/notes.md", undefined)).toBeUndefined() + expect(toPattern("notes.md", undefined)).toBeUndefined() + }) + + it("keeps an absolute pattern usable when the workspace root is unknown", () => { + expect(toPattern("/tmp/notes.md", undefined)).toBe("/tmp/notes.md") + }) + + // A drive letter is a Windows concept. Elsewhere `C:` is an ordinary directory + // name, so `C:/tmp/notes.md` names a file inside it, relative to the workspace. + describe("on Windows", () => { + it("keeps the drive of an absolute pattern as its first path segment", () => { + expect(toWindowsPattern("D:/tmp/notes.md")).toBe("/D:/tmp/notes.md") + }) + + it("gives a drive-less absolute pattern the workspace's drive", () => { + // The OS reads `/tmp/notes.md` as being on the current drive, so a + // pattern and a path spelled that way have to end up on one drive; + // otherwise they could never match. + expect(toWindowsPattern("/tmp/notes.md")).toBe("/C:/tmp/notes.md") + }) + + it("prefixes the workspace root, drive included", () => { + expect(toWindowsPattern("notes.md")).toBe("/C:/path/to/repo/**/notes.md") + expect(toWindowsPattern("docs/notes.md")).toBe("/C:/path/to/repo/docs/notes.md") + }) + + it("expands ~ to a home directory that has a drive", () => { + expect(toWindowsPattern("~/notes.md")).toBe("/C:/Users/me/notes.md") + }) + + it("reads a backslash as a directory separator", () => { + expect(toWindowsPattern("docs\\notes.md")).toBe("/C:/path/to/repo/docs/notes.md") + }) + + it("treats a drive-looking pattern as a directory off Windows", () => { + expect(toPattern("C:/tmp/notes.md", CWD)).toBe("/path/to/repo/C:/tmp/notes.md") + }) + }) +}) + +describe("isFileMatchedByPatterns", () => { + it("does not match when no patterns are configured", () => { + expect(matches("notes.md", [])).toBe(false) + expect(isFileMatchedByPatterns({ filePath: "notes.md", cwd: CWD })).toBe(false) + }) + + it("does not match when no path is given", () => { + expect(isFileMatchedByPatterns({ filePath: undefined, cwd: CWD, patterns: ["notes.md"] })).toBe(false) + }) + + it("matches an exact workspace-relative path", () => { + expect(matches("docs/notes.md", ["docs/notes.md"])).toBe(true) + }) + + it("does not match a different file", () => { + expect(matches("docs/other.md", ["docs/notes.md"])).toBe(false) + }) + + it("matches a bare filename in any directory", () => { + expect(matches("notes.md", ["notes.md"])).toBe(true) + expect(matches("deeply/nested/notes.md", ["notes.md"])).toBe(true) + }) + + it("restricts an anchored pattern to the workspace root", () => { + expect(matches("notes.md", ["./notes.md"])).toBe(true) + expect(matches("deeply/nested/notes.md", ["./notes.md"])).toBe(false) + }) + + it("matches everything under a directory glob", () => { + expect(matches("docs/scratch/a.md", ["docs/scratch/**"])).toBe(true) + expect(matches("docs/scratch/nested/b.md", ["docs/scratch/**"])).toBe(true) + expect(matches("docs/elsewhere/a.md", ["docs/scratch/**"])).toBe(false) + }) + + it("matches an extension glob", () => { + expect(matches("docs/notes.md", ["*.md"])).toBe(true) + expect(matches("docs/notes.txt", ["*.md"])).toBe(false) + }) + + it("matches an absolute path against a workspace-relative pattern", () => { + expect(matches(`${CWD}/docs/notes.md`, ["docs/notes.md"])).toBe(true) + }) + + it("matches a workspace-relative path against an absolute pattern", () => { + expect(matches("docs/notes.md", [`${CWD}/docs/notes.md`])).toBe(true) + }) + + it("matches a file outside the workspace via an absolute pattern", () => { + expect(matches("/tmp/notes.md", ["/tmp/notes.md"])).toBe(true) + }) + + it("matches a file outside the workspace via a workspace-escaping pattern", () => { + expect(matches("../shared/notes.md", ["../shared/notes.md"])).toBe(true) + expect(matches("/path/to/shared/notes.md", ["../shared/notes.md"])).toBe(true) + }) + + it("does not match a file outside the workspace via a workspace-relative pattern", () => { + // "notes.md" is scoped to the workspace, so an unrelated absolute path + // of the same name must not be approved by it. + expect(matches("/tmp/notes.md", ["notes.md"])).toBe(false) + }) + + it("keeps Windows drives apart", () => { + expect(matchesOnWindows("C:/tmp/notes.md", ["c:/tmp/notes.md"])).toBe(true) + expect(matchesOnWindows("D:/tmp/notes.md", ["c:/tmp/notes.md"])).toBe(false) + }) + + it("matches a path that uses Windows separators, on Windows", () => { + expect(matchesOnWindows("docs\\notes.md", ["docs/notes.md"])).toBe(true) + }) + + // A backslash is a legal character in a filename on other platforms, where + // `my\file` is one file rather than `file` inside `my`, so it must not be read + // as a separator there. + it("treats a backslash in a path as part of the filename off Windows", () => { + expect(matches("docs\\notes.md", ["docs/notes.md"])).toBe(false) + }) + + it("honours an escaped glob character in a pattern", () => { + expect(matches("docs/a*b.md", ["docs/a\\*b.md"])).toBe(true) + expect(matches("docs/axb.md", ["docs/a\\*b.md"])).toBe(false) + }) + + it("still matches valid patterns when other entries are unusable", () => { + expect(matches("docs/notes.md", ["", "mydir/", "docs/notes.md"])).toBe(true) + }) + + it("matches filenames containing spaces", () => { + expect(matches("docs/my notes.md", ["docs/my notes.md"])).toBe(true) + expect(matches("docs/ notes.md", ["docs/ notes.md"])).toBe(true) + }) + + it("applies gitignore's trailing-whitespace rule", () => { + // An unescaped trailing space is dropped from the pattern, so it names + // the space-free file; escaping it keeps the space. + expect(matches("docs/notes.md", ["docs/notes.md "])).toBe(true) + expect(matches("docs/notes.md ", ["docs/notes.md\\ "])).toBe(true) + }) + + it("matches a workspace-relative path when the workspace root is unknown", () => { + expect(matches("docs/notes.md", ["docs/notes.md"], undefined)).toBe(true) + }) + + it("honours a negation that excludes a file from a broader pattern", () => { + expect(matches("docs/secret.md", ["docs/**", "!docs/secret.md"])).toBe(false) + expect(matches("docs/notes.md", ["docs/**", "!docs/secret.md"])).toBe(true) + }) + + // A pattern grants access to a named file, so it must not also grant the + // different file that differs only in case. + describe("case sensitivity", () => { + it("does not match a differently-cased name off Windows", () => { + expect(matches("NOTES.md", ["notes.md"])).toBe(false) + expect(matches("Notes.Md", ["notes.md"])).toBe(false) + expect(matches("DOCS/notes.md", ["docs/notes.md"])).toBe(false) + expect(matches("notes.md", ["notes.md"])).toBe(true) + }) + + it("ignores case on Windows, whose filesystem does too", () => { + expect(matchesOnWindows("NOTES.md", ["notes.md"])).toBe(true) + expect(matchesOnWindows("DOCS/notes.md", ["docs/notes.md"])).toBe(true) + }) + }) + + // A match on a directory must not decide the verdict of the files below it: + // see the note of the same name in filePatterns.ts. + describe("patterns that match a directory", () => { + it("does not grant a directory's contents", () => { + expect(matches("docs/notes.md", ["docs"])).toBe(false) + expect(matches("docs/nested/notes.md", ["docs"])).toBe(false) + expect(matches("elsewhere/docs/notes.md", ["docs"])).toBe(false) + }) + + it("still grants a file that has the pattern's name", () => { + expect(matches("docs", ["docs"])).toBe(true) + expect(matches("nested/docs", ["docs"])).toBe(true) + }) + + // A glob that matches a directory's *name* used to grant everything under + // it. It now grants only the files it matches itself, which for `*.d` are + // files ending in `.d` rather than the contents of a `build.d/` directory. + it("does not grant the contents of a directory whose name a glob matches", () => { + expect(matches("build.d/notes.md", ["*.d"])).toBe(false) + expect(matches("build.d", ["*.d"])).toBe(true) + }) + + // `*` names any file in any directory, exactly as `notes.md` and `*.md` do, + // so it does reach every file in the workspace. That is the documented + // meaning of a pattern without a slash, not the directory behaviour above: + // every one of those matches is against the file's own path. + it("keeps a bare wildcard matching files at any depth", () => { + expect(matches("notes.md", ["*"])).toBe(true) + expect(matches("nested/notes.md", ["*"])).toBe(true) + expect(matches("nested/notes.md", ["*.md"])).toBe(true) + }) + + it("keeps a directory glob granting everything below it", () => { + expect(matches("docs/notes.md", ["docs/**"])).toBe(true) + expect(matches("docs/nested/deeply/notes.md", ["docs/**"])).toBe(true) + }) + + it("honours a negation at any depth below a directory glob", () => { + expect(matches("docs/secret.md", ["docs/**", "!docs/secret.md"])).toBe(false) + expect(matches("docs/private/secret.md", ["docs/**", "!docs/private/secret.md"])).toBe(false) + expect(matches("docs/private/notes.md", ["docs/**", "!docs/private/secret.md"])).toBe(true) + }) + }) +}) diff --git a/src/core/auto-approval/__tests__/fixtures.ts b/src/core/auto-approval/__tests__/fixtures.ts new file mode 100644 index 0000000000..de19c95986 --- /dev/null +++ b/src/core/auto-approval/__tests__/fixtures.ts @@ -0,0 +1,50 @@ +// Shared fixtures for the auto-approval allowlist specs. +// +// The three allowlist specs each need a fully-populated auto-approval state with +// every toggle off, so that a test's own overrides are the only thing that can +// approve anything. Keeping one copy of it prevents the drift that comes from +// maintaining near-identical literals side by side. + +import type { ExtensionState } from "@roo-code/types" + +import type { AutoApprovalState, AutoApprovalStateOptions } from ".." + +export type State = Pick + +/** Workspace root the specs resolve their patterns and paths against. */ +export const CWD = "/path/to/repo" + +/** + * Home directory `~` stands for in the specs. + * + * Passed explicitly wherever `~` is involved, so the expectations do not depend + * on the machine running them: a real `os.homedir()` is `/home/someone` on Linux + * but `C:\Users\someone` on Windows, and `process.env.HOME` is normally unset + * there altogether. + */ +export const HOME = "/home/me" + +/** + * Auto-approval enabled, every permission off, both allowlists empty. + * + * Every field is listed rather than relying on optionality, so that a test which + * approves something can only be doing so through what it overrides. + */ +export const baseState: State = { + autoApprovalEnabled: true, + alwaysAllowReadOnly: false, + alwaysAllowReadOnlyOutsideWorkspace: false, + allowedReadFiles: [], + alwaysAllowWrite: false, + alwaysAllowWriteOutsideWorkspace: false, + alwaysAllowWriteProtected: false, + allowedWriteFiles: [], + alwaysAllowMcp: false, + alwaysAllowModeSwitch: false, + alwaysAllowSubtasks: false, + alwaysAllowExecute: false, + alwaysAllowFollowupQuestions: false, + destructiveCommandGuardEnabled: false, + allowedCommands: [], + deniedCommands: [], +} diff --git a/src/core/auto-approval/__tests__/negation.spec.ts b/src/core/auto-approval/__tests__/negation.spec.ts new file mode 100644 index 0000000000..2e51b80c97 --- /dev/null +++ b/src/core/auto-approval/__tests__/negation.spec.ts @@ -0,0 +1,97 @@ +// npx vitest run core/auto-approval/__tests__/negation.spec.ts + +import { isFileMatchedByPatterns } from "../filePatterns" +import { checkAutoApproval } from ".." +import { CWD, HOME, baseState, type State } from "./fixtures" + +// `isWindows` and `homeDir` are fixed rather than read from the platform, so the +// expectations mean the same thing on every CI runner. +const matches = (filePath: string, patterns: string[]) => + isFileMatchedByPatterns({ filePath, cwd: CWD, patterns, isWindows: false, homeDir: HOME }) + +const readDecision = async (state: Partial, path = "docs/secret.md") => + checkAutoApproval({ + state: { ...baseState, ...state }, + cwd: CWD, + ask: "tool", + text: JSON.stringify({ tool: "readFile", path }), + }) + +describe("pattern negation", () => { + it("excludes a workspace-relative file from a workspace-relative glob", () => { + expect(matches("docs/secret.md", ["docs/**", "!docs/secret.md"])).toBe(false) + expect(matches("docs/notes.md", ["docs/**", "!docs/secret.md"])).toBe(true) + }) + + // A negation is scoped by the path it names, not by the "!", so it lands in + // the same scope as the pattern it is meant to cancel. + it("excludes an absolute file from an absolute glob", () => { + expect(matches("/tmp/x/secret.md", ["/tmp/x/**", "!/tmp/x/secret.md"])).toBe(false) + expect(matches("/tmp/x/notes.md", ["/tmp/x/**", "!/tmp/x/secret.md"])).toBe(true) + }) + + // A deny pattern has to be effective whenever it names the file, whatever + // spelling was used for it or for the pattern it cancels. All patterns are + // therefore rewritten into one form and matched together. + it("cancels across the workspace-relative and absolute spellings", () => { + expect(matches("docs/secret.md", ["docs/**", `!${CWD}/docs/secret.md`])).toBe(false) + expect(matches("docs/secret.md", [`${CWD}/docs/**`, "!docs/secret.md"])).toBe(false) + }) + + it("cancels when both are written in the same form", () => { + expect(matches("docs/secret.md", [`${CWD}/docs/**`, `!${CWD}/docs/secret.md`])).toBe(false) + }) + + it("cancels a bare-filename pattern with an anchored negation", () => { + expect(matches("docs/secret.md", ["secret.md", "!docs/secret.md"])).toBe(false) + expect(matches("other/secret.md", ["secret.md", "!docs/secret.md"])).toBe(true) + }) + + // Asserting only the exclusion would also pass if `~/**` matched nothing at + // all, so the sibling file has to come out granted. + it("excludes via a home-directory negation", () => { + expect(matches(`${HOME}/notes.md`, ["~/**", "!~/notes.md"])).toBe(false) + expect(matches(`${HOME}/other.md`, ["~/**", "!~/notes.md"])).toBe(true) + }) + + it("grants nothing when only negations are configured", () => { + expect(matches("docs/secret.md", ["!docs/secret.md"])).toBe(false) + }) + + describe("across the two allowlists", () => { + // Each list is matched independently, so which box a pattern was typed + // into cannot change the outcome by reordering a concatenation. + it("does not let a write-list negation revoke read access", async () => { + expect( + await readDecision({ allowedReadFiles: ["docs/**"], allowedWriteFiles: ["!docs/secret.md"] }), + ).toEqual({ decision: "approve" }) + }) + + it("does not let a read-list negation revoke read access granted by the write list", async () => { + // Write permission implies read permission, so the write pattern + // still grants the read; a negation only narrows its own list. + expect( + await readDecision({ allowedReadFiles: ["!docs/secret.md"], allowedWriteFiles: ["docs/**"] }), + ).toEqual({ decision: "approve" }) + }) + + it("asks when each list's own negation excludes the file", async () => { + expect( + await readDecision({ + allowedReadFiles: ["docs/**", "!docs/secret.md"], + allowedWriteFiles: ["docs/**", "!docs/secret.md"], + }), + ).toEqual({ decision: "ask" }) + + expect( + await readDecision( + { + allowedReadFiles: ["docs/**", "!docs/secret.md"], + allowedWriteFiles: ["docs/**", "!docs/secret.md"], + }, + "docs/notes.md", + ), + ).toEqual({ decision: "approve" }) + }) + }) +}) diff --git a/src/core/auto-approval/__tests__/noWorkspaceRoot.spec.ts b/src/core/auto-approval/__tests__/noWorkspaceRoot.spec.ts new file mode 100644 index 0000000000..fb56d7d563 --- /dev/null +++ b/src/core/auto-approval/__tests__/noWorkspaceRoot.spec.ts @@ -0,0 +1,83 @@ +// npx vitest run core/auto-approval/__tests__/noWorkspaceRoot.spec.ts + +import { isFileMatchedByPatterns, toMatcherPattern } from "../filePatterns" + +const matchesWithoutWorkspace = (filePath: string, patterns: string[]) => + isFileMatchedByPatterns({ filePath, cwd: undefined, patterns }) + +describe("patterns without a workspace root", () => { + // A bare gitignore pattern matches in any directory. Left unprefixed, it would + // reach the whole filesystem, so a workspace-relative pattern must not be + // usable at all while there is no workspace to confine it to. + describe("does not let a workspace-relative pattern reach outside a workspace", () => { + it.each([ + ["a bare filename", "passwd"], + ["a star", "*"], + ["a double star", "**"], + ["an extension glob", "*.conf"], + ["a workspace-root-anchored path", "./passwd"], + ["a nested path", "etc/passwd"], + ])("rejects %s", (_label, pattern) => { + expect(toMatcherPattern(pattern, undefined)).toBeUndefined() + }) + + it.each([ + ["passwd", "/etc/passwd"], + ["*", "/etc/passwd"], + ["**", "/etc/passwd"], + ["*.conf", "/etc/nginx/nginx.conf"], + ["etc/passwd", "/etc/passwd"], + ])("does not match %s against %s", (pattern, filePath) => { + expect(matchesWithoutWorkspace(filePath, [pattern])).toBe(false) + }) + + it("does not match a workspace-relative path either", () => { + expect(matchesWithoutWorkspace("notes.md", ["notes.md"])).toBe(false) + }) + }) + + // An absolute pattern names its location outright, so it needs no workspace. + describe("keeps absolute patterns usable", () => { + it("matches an absolute pattern against that absolute path", () => { + expect(matchesWithoutWorkspace("/tmp/notes.md", ["/tmp/notes.md"])).toBe(true) + }) + + it("does not match a different absolute path", () => { + expect(matchesWithoutWorkspace("/etc/passwd", ["/tmp/notes.md"])).toBe(false) + }) + + it("matches an absolute glob", () => { + expect(matchesWithoutWorkspace("/tmp/scratch/notes.md", ["/tmp/scratch/**"])).toBe(true) + }) + + it("honours an absolute negation", () => { + expect( + matchesWithoutWorkspace("/tmp/scratch/secret.md", ["/tmp/scratch/**", "!/tmp/scratch/secret.md"]), + ).toBe(false) + }) + }) + + // With a workspace root the same patterns are confined to it, which is the + // behaviour the rejection above preserves. + describe("for contrast, with a workspace root", () => { + it("confines a bare filename to the workspace", () => { + expect( + isFileMatchedByPatterns({ filePath: "/etc/passwd", cwd: "/path/to/repo", patterns: ["passwd"] }), + ).toBe(false) + + expect( + isFileMatchedByPatterns({ + filePath: "/path/to/repo/etc/passwd", + cwd: "/path/to/repo", + patterns: ["passwd"], + }), + ).toBe(true) + }) + + it("confines a star to the workspace", () => { + expect(isFileMatchedByPatterns({ filePath: "/etc/passwd", cwd: "/path/to/repo", patterns: ["*"] })).toBe( + false, + ) + }) + }) +}) diff --git a/src/core/auto-approval/__tests__/windowsPaths.spec.ts b/src/core/auto-approval/__tests__/windowsPaths.spec.ts new file mode 100644 index 0000000000..90006ad2d3 --- /dev/null +++ b/src/core/auto-approval/__tests__/windowsPaths.spec.ts @@ -0,0 +1,101 @@ +// npx vitest run core/auto-approval/__tests__/windowsPaths.spec.ts + +import { isFileMatchedByPatterns } from "../filePatterns" + +// On Windows the workspace lives on a drive, so every path the matcher sees +// carries one. That used to break matching outright: patterns went through +// `path.resolve`, which stamps the *current* drive onto a drive-less path, while +// an absolute path reported by a tool kept whatever drive it already had. A +// pattern and a path could therefore end up on different drives and never match, +// which is what made the whole allowlist inert on Windows. +// +// These tests fix the platform explicitly, so they assert Windows behaviour on +// every CI runner rather than only on the Windows one. + +const WINDOWS_CWD = "C:\\path\\to\\repo" +const WINDOWS_HOME = "C:\\Users\\me" + +const matches = (filePath: string, patterns: string[], cwd: string | undefined = WINDOWS_CWD) => + isFileMatchedByPatterns({ filePath, cwd, patterns, isWindows: true, homeDir: WINDOWS_HOME }) + +describe("matching on Windows", () => { + it("matches a workspace-relative path against a bare pattern", () => { + expect(matches("notes.md", ["notes.md"])).toBe(true) + expect(matches("docs\\notes.md", ["notes.md"])).toBe(true) + }) + + it("matches an absolute in-workspace path against a workspace-relative pattern", () => { + expect(matches("C:\\path\\to\\repo\\docs\\notes.md", ["docs/notes.md"])).toBe(true) + expect(matches("C:/path/to/repo/docs/notes.md", ["docs/notes.md"])).toBe(true) + }) + + it("does not match a file outside the workspace against a workspace-relative pattern", () => { + expect(matches("C:\\other\\notes.md", ["notes.md"])).toBe(false) + }) + + it("matches an absolute pattern that names the drive", () => { + expect(matches("C:\\tmp\\notes.md", ["C:/tmp/notes.md"])).toBe(true) + expect(matches("C:\\tmp\\notes.md", ["c:/tmp/notes.md"])).toBe(true) + }) + + // The OS reads a drive-less absolute path as being on the current drive, so + // the pattern and the path have to be brought onto one drive before matching. + it("matches a drive-less absolute pattern against a path on the workspace drive", () => { + expect(matches("C:\\tmp\\notes.md", ["/tmp/notes.md"])).toBe(true) + expect(matches("/tmp/notes.md", ["/tmp/notes.md"])).toBe(true) + expect(matches("/tmp/notes.md", ["C:/tmp/notes.md"])).toBe(true) + }) + + it("keeps drives apart", () => { + expect(matches("D:\\tmp\\notes.md", ["C:/tmp/notes.md"])).toBe(false) + expect(matches("D:\\tmp\\notes.md", ["/tmp/notes.md"])).toBe(false) + // A workspace on D: makes the drive-less pattern name D:, not C:. + expect(matches("D:\\tmp\\notes.md", ["/tmp/notes.md"], "D:\\repo")).toBe(true) + }) + + it("expands ~ to a home directory that carries a drive", () => { + expect(matches("C:\\Users\\me\\notes.md", ["~/notes.md"])).toBe(true) + expect(matches("C:\\Users\\other\\notes.md", ["~/notes.md"])).toBe(false) + }) + + it("ignores case, as the filesystem does", () => { + expect(matches("C:\\path\\to\\repo\\NOTES.md", ["notes.md"])).toBe(true) + expect(matches("c:\\path\\to\\repo\\notes.md", ["notes.md"])).toBe(true) + }) + + it("resolves a workspace-escaping pattern on the workspace drive", () => { + expect(matches("C:\\path\\to\\shared\\notes.md", ["../shared/notes.md"])).toBe(true) + }) + + it("still honours negations", () => { + expect(matches("C:\\path\\to\\repo\\docs\\secret.md", ["docs/**", "!docs/secret.md"])).toBe(false) + expect(matches("C:\\path\\to\\repo\\docs\\notes.md", ["docs/**", "!docs/secret.md"])).toBe(true) + }) + + // The tests above pass `isWindows` explicitly, which leaves the production + // default untested. These reproduce the cases the Windows CI run reported as + // failing, with nothing passed but a workspace on a drive, so that the defaults + // are what decides them. + describe("with the platform reported as Windows", () => { + const realPlatform = process.platform + + beforeAll(() => Object.defineProperty(process, "platform", { value: "win32" })) + afterAll(() => Object.defineProperty(process, "platform", { value: realPlatform })) + + const matchesByDefault = (filePath: string, patterns: string[]) => + isFileMatchedByPatterns({ filePath, cwd: WINDOWS_CWD, patterns }) + + it("matches a bare pattern against a workspace-relative path", () => { + expect(matchesByDefault("notes.md", ["notes.md"])).toBe(true) + }) + + it("matches a directory glob", () => { + expect(matchesByDefault("docs/scratch/a.md", ["docs/scratch/**"])).toBe(true) + }) + + it("confines a bare pattern to the workspace", () => { + expect(matchesByDefault("C:/path/to/repo/etc/passwd", ["passwd"])).toBe(true) + expect(matchesByDefault("C:/etc/passwd", ["passwd"])).toBe(false) + }) + }) +}) diff --git a/src/core/auto-approval/filePatterns.ts b/src/core/auto-approval/filePatterns.ts new file mode 100644 index 0000000000..f882f07465 --- /dev/null +++ b/src/core/auto-approval/filePatterns.ts @@ -0,0 +1,428 @@ +import os from "os" +import path from "path" + +import ignore from "ignore" + +/** + * Matching of file paths against user-configured file patterns. + * + * This is used to grant an access permission for a few named files instead of + * for the whole workspace, for example `allowedWriteFiles`, which lists the + * files Zoo may create or edit without asking. + * + * The syntax is gitignore-inspired, but deliberately differs from it in how a + * path is anchored, because these patterns can name files anywhere on the + * filesystem rather than only inside one repository: + * + * | Pattern | Matches | + * | --------------------- | ------------------------------------------------ | + * | `notes.md` | that filename in any directory of the workspace | + * | `*.md` | any such file in any directory of the workspace | + * | `docs/notes.md` | that path relative to the workspace root | + * | `docs/scratch/**` | everything below that workspace directory | + * | `./notes.md` | that file in the workspace root only | + * | `../shared/notes.md` | that path next to the workspace | + * | `/tmp/notes.md` | that absolute path (`/` is the filesystem root) | + * | `C:/tmp/notes.md` | that absolute path on Windows | + * | `~/notes.md` | that path in the user's home directory | + * | `!docs/secret.md` | excludes a file matched by an earlier pattern | + * + * The differences from gitignore are all about reaching outside the workspace, + * which gitignore has no need to do. Of the three spellings below, gitignore + * accepts each without complaint and then matches nothing with it: + * - `/notes.md` is an absolute filesystem path, whereas gitignore would read it + * as the workspace root. Absolute paths have to be expressible, and `/` is the + * spelling users expect for them. + * - `./notes.md` anchors to the workspace root, the role gitignore gives to + * `/notes.md`. + * - `../shared/notes.md` is resolved against the workspace root, so it names a + * path outside the workspace. For gitignore, `..` is a literal segment that no + * path inside the repository can match. + * + * `/` is the only directory separator, also on Windows, since a backslash + * escapes the character after it. + * + * Matching is case-sensitive, except on Windows, so that a pattern is as + * case-sensitive as the filesystem whose files it names. + * Pattern `notes.md` must not hand out access to `NOTES.md`, + * which on Linux is a different file. + * This follows `git`, which compares case-sensitively unless `core.ignoreCase` + * is set (`git init` sets it when it detects a case-insensitive filesystem). + * It does *not* follow the `ignore` library, whose default is `ignorecase: true` + * regardless of platform, so the option has to be passed explicitly. + * + * A pattern always names files, never directories: `docs/` is rejected, and + * `docs` grants access to a *file* named `docs`, not to the directory's + * contents. Use `docs/**` to name everything below a directory. See + * "Why a match on a directory must not decide a file's verdict" below. + * + * # Why patterns and paths are rewritten before matching + * + * The `ignore` library matches a path against patterns as `git` does against + * `.gitignore` entries, so both have to be relative to, and below, one base + * directory. It therefore cannot handle: + * - absolute paths (`/tmp/notes.md`, `C:/tmp/notes.md`), or + * - paths that climb out of the base directory (`../shared/notes.md`). + * Running `ignore`'s matching (`ignores()`) against such a path throws + * `RangeError` ("path should be a `path.relative()`d string"). + * Passing such a *pattern* to `add()` throws nothing at all: + * It is accepted and then quietly never matches, which would + * turn a mistyped permission into a silent no-op. + * + * Both the configured patterns and the path being checked are therefore + * rewritten into a single form the library accepts: a path relative to the + * filesystem root (`/`), with any Windows drive as its first segment. + * + * An alternative implementation would be to remember for each pattern whether + * it's a workspace-scoped pattern or an absolute-filesystem pattern, and then + * run `ignore` twice, with different base dirs. We do not do that because it + * makes implementing negation patterns (`!`) harder, as `ignore` checks for + * negations only within the given set of patterns; if we had 2 such sets (one + * for workspace, one for absolute), then an absolute negation pattern like + * `!/path/to/my/repo/secret.txt` would surprisingly not deny a relative pattern + * such as `*.txt`, because those would be handled by different `ignore` + * instances. + * + * # Why a match on a directory must not decide a file's verdict + * + * Git's `gitignore` decides a path by walking it from the top: it tests each ancestor + * directory first and, once one is excluded, stops and reports the file as + * excluded too, because + * > It is not possible to re-include a file if a parent directory of that file + * > is excluded. + * (`man 5 gitignore`). `ignore` reproduces this, testing each ancestor as a + * directory path (with a trailing slash) before the file path itself. + * + * Here a match grants a permission rather than withholding one, so that same + * sentence reads: it is not possible to deny a file if a parent directory of + * that file is granted. Which is exactly the objection: for a permission that + * is the wrong default, in two ways. + * - Any pattern that matches a *directory* would grant its whole subtree, + * which is not apparent from the pattern: `docs` would grant every file under + * every `docs/` directory, and a bare `*` the entire workspace, even though + * the table above documents patterns as naming files and `docs/` is rejected + * outright for naming a directory. + * - A negation below such a directory would be silently ineffective: with + * `docs/**` followed by `!docs/private/secret.md`, the walk grants the + * `docs/private/` directory and never gets to the negation for the file. + * + * (In the below, `(star-star)` stands for the `**` wildcard, + * which cannot be written literally here because its trailing slash + * would close the JSDoc comment block.) + + * The ancestor walk is therefore neutralised by appending one rule that matches + * directories only, and nothing else, using `!(star-star)/` + * Being last, it decides every ancestor probe, + * since those are the only paths carrying a trailing slash, and it leaves the + * file's own verdict to the user's patterns. A pattern can therefore still name + * a *file* called `docs`, and `docs/**` still grants everything below `docs/`, + * because those rules match the file path directly rather than an ancestor. + * + * Example: + * For workspace root `/path/to/repo`, the two patterns + * docs/(star-star) + * !docs/private/secret.md + * are handed to `ignore` as + * /path/to/repo/docs/(star-star) + * !/path/to/repo/docs/private/secret.md + * !(star-star)/ <- appended + * and `docs/private/secret.md` is checked as `path/to/repo/docs/private/secret.md`: + * the ancestor probes (`path/`, ..., `path/to/repo/docs/private/`) all end in a + * slash, so the appended rule has the last word and reports them as not granted; + * the file path is then matched by rule 1 (granted) and rule 2 (denied), and as + * the last match wins, the file is denied. Without the appended rule, the + * ancestor `path/to/repo/docs/private/` would match rule 1 and end the walk + * there, granting the file the negation was written to withhold. + */ +const MATCH_DIRECTORIES_ONLY_PATTERN = "!**/" + +/** + * Whether we're on Windows. + */ +const runningOnWindows = () => process.platform === "win32" + +/** + * Convert Windows path separators so patterns and paths share one syntax. + * + * Only on Windows: everywhere else a backslash is an ordinary character in a + * filename (`touch 'my\file'` creates a single file, not a directory), so + * converting it would rewrite a path into a different one, and let the pattern + * `my/file` grant access to the unrelated file `my\file`. + */ +function pathsepsToPosix(value: string, isWindows: boolean): string { + return isWindows ? value.replace(/\\/g, "/") : value +} + +/** A Windows drive prefix, as the first thing in a path: `C:` in `C:/tmp/x`. */ +const DRIVE_PREFIX = /^([a-zA-Z]:)(\/.*)?$/ + +/** + * Split a leading Windows drive off a path, leaving a POSIX-absolute remainder. + * + * Only when reading paths as Windows does. Elsewhere `C:` is an ordinary + * directory name (`mkdir 'C:'` succeeds on Linux), so `C:/notes.md` is a + * *relative* path naming a file in it, and taking it for a drive would anchor it + * to the filesystem root instead of the workspace. + * + * - `"C:/tmp/x"` -> `{ drive: "C:", rest: "/tmp/x" }` + * - `"C:"` -> `{ drive: "C:", rest: "/" }` + * - `"/tmp/x"` -> `{ drive: undefined, rest: "/tmp/x" }` + * - `"C:/tmp/x"` off Windows -> `{ drive: undefined, rest: "C:/tmp/x" }` + */ +function splitDrive(value: string, isWindows: boolean): { drive?: string; rest: string } { + const drive = isWindows ? DRIVE_PREFIX.exec(value) : null + + return drive ? { drive: drive[1], rest: drive[2] || "/" } : { rest: value } +} + +function isAbsolutePosixPath(value: string, isWindows: boolean): boolean { + // Posix ("/tmp/x"), or on Windows also a drive letter ("C:/tmp/x"). + return value.startsWith("/") || splitDrive(value, isWindows).drive !== undefined +} + +function escapesWorkspace(posixPath: string): boolean { + return posixPath.split("/").includes("..") +} + +/** + * Rewrite a path into the single form the matcher works in: relative to the + * filesystem root, since the `ignore` library rejects paths starting with `/`. + * + * `absolutePath` is absolute in one of the two senses of `isAbsolutePosixPath`. + * A POSIX path loses its leading slash. A Windows path keeps its drive as the + * first path segment, which is what holds drives apart. + * + * On Windows a path naming no drive takes the workspace's, as the OS would read + * it, so that a `/tmp/notes.md` pattern and a `C:/tmp/notes.md` path still + * describe the same file. The drive's case is left as typed: a drive only occurs + * on Windows, where the matcher ignores case anyway (see the case-sensitivity + * note at the top), so `C:/x` and `c:/x` already agree. + * + * - `"/tmp/notes.md"` -> `"tmp/notes.md"` + * - `"C:/tmp/notes.md"` -> `"C:/tmp/notes.md"` + * - `"/tmp/notes.md"` with `workspaceDrive` `"C:"` -> `"C:/tmp/notes.md"` + */ +function toRootRelativePath(absolutePath: string, isWindows: boolean, workspaceDrive?: string): string { + const { drive, rest } = splitDrive(absolutePath, isWindows) + const effectiveDrive = drive ?? workspaceDrive + + return effectiveDrive ? `${effectiveDrive}${rest}` : rest.slice(1) +} + +/** + * Rewrite a user-supplied pattern so that it matches the root-relative paths + * produced by `toMatcherPath()`. + * + * Patterns that name a directory rather than a file are rejected (`undefined`). + * + * The workspace root (`cwd`) is normally known, but is absent when no folder is + * open in VS Code, and in contexts that build the extension state without one. + * Without it, a workspace-relative pattern is rejected rather than matched + * loosely: there is no workspace for it to be relative to, and a bare gitignore + * pattern matches in *any* directory, so `passwd` would otherwise match + * `/etc/passwd`, and a bare `*` would match every file on the machine. + * That would be quite a footgun. + * Only absolute patterns remain usable in that situation. + * + * When the root is known, a workspace-relative pattern has that root prefixed + * onto it, which puts it in the same form as an absolute one. gitignore anchors + * any pattern that has a separator at its beginning or middle, so prefixing turns + * a bare filename such as `notes.md`, which is meant to match in any directory, + * into a workspace-root-only match. Its reach is restored by inserting a `/**` + * segment (whose trailing slash cannot be written in this comment, as it would + * close the comment block) between the root and the filename: that wildcard stands + * for any number of directories including none, so the result matches + * `base/notes.md` as well as `base/a/b/notes.md`. + * + * In the below examples, `(star-star)` stands for the `**` wildcard, + * which cannot be written literally here because its trailing slash + * would close the JSDoc comment block. + * + * Examples (home directory `/home/me`). + * - For workspace root `cwd = "/path/to/repo"`: + * - `"notes.md"` -> `"/path/to/repo/(star-star)/notes.md"` + * - `"*.md"` -> `"/path/to/repo/(star-star)/*.md"` + * - `"docs/notes.md"` -> `"/path/to/repo/docs/notes.md"` + * - `"docs/scratch/**"` -> `"/path/to/repo/docs/scratch/**"` + * - `"./notes.md"` -> `"/path/to/repo/notes.md"` + * - `"../shared/notes.md"` -> `"/path/to/shared/notes.md"` + * resolved against the workspace root, so it lands outside the workspace + * - `"~/notes.md"` -> `"/home/me/notes.md"` + * - `"C:/tmp/notes.md"` -> `"/c:/tmp/notes.md"` + * - `"!docs/secret.md"` -> `"!/path/to/repo/docs/secret.md"` + * - `"mydir/"` -> `undefined` (because it names a dir) + * - `"~"` -> `undefined` (because it names a dir) + * - For workspace root `cwd = undefined`: + * - `"notes.md"` with no workspace root -> `undefined` + * (as is any workspace-relative pattern, see above) + * + * Whitespace and backslashes are left as typed, because the `ignore` library + * applies gitignore's own rules to them: leading whitespace is part of the + * filename, trailing whitespace is dropped unless escaped (`"notes.md\ "`), and + * a backslash escapes the character after it. Only patterns consisting solely of + * whitespace are rejected, since they name no file. + * + * @param pattern - Raw pattern as typed by the user. + * @param cwd - Workspace root, used to resolve workspace-relative patterns. + * @param isWindows - Whether to read paths by Windows' rules; see `pathsepsToPosix`. + * @param homeDir - Directory `~` expands to. Defaults to the real one; tests pass + * a Windows-shaped path to exercise that platform's rules. + * @returns The rewritten pattern, or `undefined` when the pattern can never + * match a file (empty, a directory, or escaping an unknown workspace root). + */ +export function toMatcherPattern( + pattern: string, + cwd?: string, + isWindows = runningOnWindows(), + homeDir = os.homedir(), +): string | undefined { + // Set gitignore's negation aside so the path is rewritten on its own merits, + // then restore it, so that a negation is anchored exactly like the pattern it + // is written to cancel. + const negation = pattern.startsWith("!") ? "!" : "" + // On Windows a backslash the user typed separates directories, so it becomes a + // slash before anything else looks at the pattern. Elsewhere it is left alone, + // as gitignore's escape character and a legal filename character. + let normalized = pathsepsToPosix(pattern.slice(negation.length), isWindows) + + if (!normalized.trim() || normalized === "." || normalized === "~" || normalized.endsWith("/")) { + return undefined + } + + const workspace = splitDrive(pathsepsToPosix(cwd ?? "", isWindows), isWindows) + const workspaceDrive = workspace.drive + + if (normalized.startsWith("~/")) { + const home = splitDrive(pathsepsToPosix(homeDir, isWindows), isWindows) + normalized = `${home.drive ?? ""}${path.posix.join(home.rest, normalized.slice(2))}` + } + + if (!isAbsolutePosixPath(normalized, isWindows) && escapesWorkspace(normalized)) { + if (!cwd) { + return undefined + } + + normalized = `${workspaceDrive ?? ""}${path.posix.resolve(workspace.rest, normalized)}` + } + + if (isAbsolutePosixPath(normalized, isWindows)) { + return `${negation}/${toRootRelativePath(normalized, isWindows, workspaceDrive)}` + } + + // "./notes.md" names the workspace root explicitly. + const isWorkspaceRootAnchored = normalized.startsWith("./") + + if (isWorkspaceRootAnchored) { + normalized = normalized.slice(2) + } + + if (!cwd) { + // No workspace to be relative to; see the note above on why this is not + // matched loosely instead. + return undefined + } + + const workspaceBase = toRootRelativePath(path.posix.resolve(workspace.rest), isWindows, workspaceDrive) + + // gitignore anchors a pattern to the base directory as soon as it has a + // separator "at the beginning or middle (or both)" (gitignore(5)), and only a + // pattern with no separator at all matches at any level below. Prefixing the + // workspace root necessarily adds separators, which would silently turn a bare + // filename into a root-only match; a double-star segment, standing for any + // number of directories including none, restores its reach. + const matchesInAnyDirectory = !isWorkspaceRootAnchored && !normalized.includes("/") + + return `${negation}/${workspaceBase}/${matchesInAnyDirectory ? "**/" : ""}${normalized}` +} + +/** + * Rewrite the path of the file being checked into the same root-relative form + * that `toMatcherPattern` produces. + * + * @returns The rewritten path, or `undefined` when it names no file, or when it + * is relative and there is no workspace root to resolve it against. + */ +function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boolean): string | undefined { + // Not trimmed: whitespace can be part of a filename. + const normalized = pathsepsToPosix(filePath, isWindows) + + if (!normalized.trim() || normalized === ".") { + return undefined + } + + const workspace = splitDrive(pathsepsToPosix(cwd ?? "", isWindows), isWindows) + + if (isAbsolutePosixPath(normalized, isWindows)) { + return toRootRelativePath(normalized, isWindows, workspace.drive) + } + + if (!cwd) { + // A relative path cannot be placed on the filesystem without a root, and + // the only patterns that survive without one are absolute, which such a + // path could never match anyway. + return undefined + } + + const { drive, rest } = splitDrive(normalized, isWindows) + + return toRootRelativePath(`${drive ?? ""}${path.posix.resolve(workspace.rest, rest)}`, isWindows, workspace.drive) +} + +/** + * Check whether a file path is covered by any of the configured patterns. + * + * Patterns are applied in the order given and the last one to match decides, so + * a `!` pattern excludes files matched by the patterns before it. A pattern only + * ever decides the file it names: matching one of its parent directories grants + * nothing, see "Why a match on a directory must not decide a file's verdict". + * + * @param filePath - Path of the file, either absolute or relative to `cwd`. + * @param cwd - Workspace root. + * @param patterns - Raw patterns as configured by the user. + * @param isWindows - Whether to read paths by Windows' rules: `\` separates + * directories, a leading `C:` is a drive, and case is ignored. Defaults to the + * platform in use; tests pass it explicitly to exercise either platform's rules. + * @param homeDir - Directory `~` expands to. Defaults to the real one. + */ +export function isFileMatchedByPatterns({ + filePath, + cwd, + patterns, + isWindows = runningOnWindows(), + homeDir = os.homedir(), +}: { + filePath?: string + cwd?: string + patterns?: string[] + isWindows?: boolean + homeDir?: string +}): boolean { + if (!filePath || !Array.isArray(patterns) || !patterns.length) { + return false + } + + const candidate = toMatcherPath(filePath, cwd, isWindows) + + if (!candidate) { + return false + } + + const matcherPatterns = patterns + .map((pattern) => toMatcherPattern(pattern, cwd, isWindows, homeDir)) + .filter((pattern): pattern is string => !!pattern) + + if (!matcherPatterns.length) { + return false + } + + try { + return ignore({ ignoreCase: isWindows }) + .add([...matcherPatterns, MATCH_DIRECTORIES_ONLY_PATTERN]) + .ignores(candidate) + } catch (error) { + // A path the matcher rejects cannot be confirmed as matching, so treat it + // as unmatched. + console.error(`[auto-approval] Failed to match path ${filePath}:`, error) + return false + } +} diff --git a/src/core/auto-approval/index.ts b/src/core/auto-approval/index.ts index 3d24c7497f..751b5c0674 100644 --- a/src/core/auto-approval/index.ts +++ b/src/core/auto-approval/index.ts @@ -12,6 +12,7 @@ import { ClineAskResponse } from "../../shared/WebviewMessage" import { isWriteToolAction, isReadOnlyToolAction } from "./tools" import { isMcpToolAlwaysAllowed } from "./mcp" import { getCommandDecision } from "./commands" +import { isFileMatchedByPatterns } from "./filePatterns" // We have auto-approval actions for different categories. export type AutoApprovalState = @@ -27,14 +28,111 @@ export type AutoApprovalState = export type AutoApprovalStateOptions = | "autoApprovalEnabled" | "alwaysAllowReadOnlyOutsideWorkspace" // For `alwaysAllowReadOnly`. + | "allowedReadFiles" // Grants reads per file, without `alwaysAllowReadOnly`. | "alwaysAllowWriteOutsideWorkspace" // For `alwaysAllowWrite`. | "alwaysAllowWriteProtected" + | "allowedWriteFiles" // Grants writes per file, without `alwaysAllowWrite`. | "followupAutoApproveTimeoutMs" // For `alwaysAllowFollowupQuestions`. | "mcpServers" // For `alwaysAllowMcp`. | "allowedCommands" // For `alwaysAllowExecute`. | "deniedCommands" | "destructiveCommandGuardEnabled" +/** + * Every file a tool action names, as far as the allowlists are concerned. + * + * One approval answers for the whole action, so every file it touches has to be + * covered by the patterns. Rather than deciding which of these fields a given + * message is expected to use, all of them are collected and all have to match: + * that way no field can grant a permission by being overlooked, and a field + * added later can only ever make the check stricter. + * + * `additionalFileCount` counts files that the message does *not* name (the chat + * row renders it as "and N more"). They cannot be matched against a pattern, so + * the caller must refuse the whole action rather than approve the named ones. + */ +function namedFiles(tool: ClineSayTool): { paths: string[]; hasUnnamedFiles: boolean } { + const batched = [...(tool.batchFiles ?? []), ...(tool.batchDiffs ?? []), ...(tool.batchDirs ?? [])] + + return { + paths: [...(tool.path === undefined ? [] : [tool.path]), ...batched.map((file) => file.path)], + hasUnnamedFiles: !!tool.additionalFileCount, + } +} + +/** + * Whether every file named by a tool action is covered by `matchFun`. + * + * Returns `false` for an action naming no file at all, since patterns can only + * grant access to files they name, and for one that carries unnamed files. + */ +function areAllNamedFilesMatched(tool: ClineSayTool, matchFun: (filePath: string) => boolean): boolean { + const { paths, hasUnnamedFiles } = namedFiles(tool) + + if (hasUnnamedFiles) { + return false + } + + // Bail on `!paths.length` defensively in case new paths are introduced + // in the future that are forgotten to be added to `namedFiles()`. + if (!paths.length) { + return false + } + + return paths.every((filePath) => matchFun(filePath)) +} + +/** + * Whether a read-only tool action is fully covered by the read allowlist patterns. + * + * The allowlist names individual files, so it only ever approves `read_file`: + * the other read-only actions (directory listings, searches, codebase queries) + * work on directories, not files, and are turned away by the `tool` check below. + * + * A `read_file` call can cover several files at once, in which case a single + * approval answers for all of them, so ALL of them have to be allowed. + * + * Write permission implies read permission, so both lists are consulted, each + * matched on its own rather than concatenated: gitignore negation is + * order-sensitive ("the last matching pattern wins"), so concatenating would let + * a `!` typed into one list cancel a pattern typed into the other, with the + * outcome depending on which list that was. A negation therefore only ever + * narrows the list it appears in. This also means that negating a read is + * ineffective while a non-negated write pattern still matches the file. + */ +function isReadAllowedByPatterns( + tool: ClineSayTool, + cwd: string | undefined, + state: Pick, +): boolean { + if (tool.tool !== "readFile") { + return false + } + + return areAllNamedFilesMatched( + tool, + (filePath) => + isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedReadFiles }) || + isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedWriteFiles }), + ) +} + +/** + * Whether a write tool action is fully covered by the write allowlist patterns. + * + * As for reads, one approval covers every file the action names, so every one of + * them has to be matched. + */ +function isWriteAllowedByPatterns( + tool: ClineSayTool, + cwd: string | undefined, + state: Pick, +): boolean { + return areAllNamedFilesMatched(tool, (filePath) => + isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedWriteFiles }), + ) +} + export type CheckAutoApprovalResult = | { decision: "approve" } | { decision: "deny" } @@ -47,11 +145,25 @@ export type CheckAutoApprovalResult = export async function checkAutoApproval({ state, + cwd, ask, text, isProtected, }: { state?: Pick + /** + * Workspace root the allowlist patterns and the checked path are resolved + * against. + * + * Must be the `cwd` of the task this ask belongs to, which is the root the + * path in `text` was made relative to. It is not read from `state`, because + * the provider's `cwd` follows the window (the focused editor in a multi-root + * workspace, or a `refreshWorkspace()` while the task runs) and a resumed or + * child task can run against another root entirely. Resolving against the + * wrong one would let a pattern written for one workspace approve a write + * landing in another. + */ + cwd?: string ask: ClineAsk text?: string isProtected?: boolean @@ -177,16 +289,37 @@ export async function checkAutoApproval({ const isOutsideWorkspace = !!tool.isOutsideWorkspace if (isReadOnlyToolAction(tool)) { - return state.alwaysAllowReadOnly === true && - (!isOutsideWorkspace || state.alwaysAllowReadOnlyOutsideWorkspace === true) - ? { decision: "approve" } - : { decision: "ask" } + // A file listed in `allowedReadFiles` may be read without the blanket + // `alwaysAllowReadOnly` permission. Such a pattern names its + // location, including outside the workspace, so it also stands in for + // `alwaysAllowReadOnlyOutsideWorkspace`. + const isAllowedReadFile = isReadAllowedByPatterns(tool, cwd, state) + + const isReadAllowed = + isAllowedReadFile || + (state.alwaysAllowReadOnly === true && + (!isOutsideWorkspace || state.alwaysAllowReadOnlyOutsideWorkspace === true)) + + return isReadAllowed ? { decision: "approve" } : { decision: "ask" } } if (isWriteToolAction(tool)) { - return state.alwaysAllowWrite === true && - (!isOutsideWorkspace || state.alwaysAllowWriteOutsideWorkspace === true) && - (!isProtected || state.alwaysAllowWriteProtected === true) + // A file listed in `allowedWriteFiles` may be written without the + // blanket `alwaysAllowWrite` permission. Such a pattern names its + // location, including outside the workspace, so it also stands in for + // `alwaysAllowWriteOutsideWorkspace`. + // + // It deliberately does not stand in for `alwaysAllowWriteProtected`: + // a broad pattern such as `*.md` would otherwise silently cover + // protected files like `AGENTS.md`. + const isAllowedWriteFile = isWriteAllowedByPatterns(tool, cwd, state) + + const isWriteAllowed = + isAllowedWriteFile || + (state.alwaysAllowWrite === true && + (!isOutsideWorkspace || state.alwaysAllowWriteOutsideWorkspace === true)) + + return isWriteAllowed && (!isProtected || state.alwaysAllowWriteProtected === true) ? { decision: "approve" } : { decision: "ask" } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4be087394e..27758a5e0e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1182,7 +1182,11 @@ export class Task extends EventEmitter implements TaskLike { // rendered, leaving them stuck on-screen). const provider = this.providerRef.deref() const state = provider ? await provider.getState() : undefined - const approval = await checkAutoApproval({ state, ask: type, text, isProtected }) + // `this.cwd`, not `provider.cwd`: + // The path inside `text` was made relative to this task's workspace, + // which for a resumed or child task need not be the one the provider + // currently reports. + const approval = await checkAutoApproval({ state, cwd: this.cwd, ask: type, text, isProtected }) const isAutoAnswered = approval.decision === "approve" || approval.decision === "deny" const autoApprovalDecision = isAutoAnswered ? approval.decision : undefined diff --git a/src/core/task/__tests__/ask-allowlist-cwd.spec.ts b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts new file mode 100644 index 0000000000..3b3682f969 --- /dev/null +++ b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts @@ -0,0 +1,105 @@ +// npx vitest run core/task/__tests__/ask-allowlist-cwd.spec.ts + +import type { ExtensionState } from "@roo-code/types" + +import { Task } from "../Task" + +// The allowlist patterns are resolved against a workspace root, and the path in +// the tool message was made relative by `getReadablePath(task.cwd, ...)`. So the +// root used for matching has to be the task's own `cwd`. +// +// The provider's `cwd` is a different value: it follows the window (the focused +// editor under multi-root workspaces, or a `refreshWorkspace()` mid-task), while a +// task resumed from history or created as a child keeps the workspace it belongs +// to. When the two diverge, resolving against the provider's would let a pattern +// written for one workspace approve a write landing in another. + +/** The parts of the provider that `Task.ask` reaches for. */ +type ProviderStub = { + getState: () => Promise> + postMessageToWebview: ReturnType + cwd: string +} + +function buildTask(provider: ProviderStub, taskCwd: string) { + const task = Object.create(Task.prototype) as Task + task["abort"] = false + task["clineMessages"] = [] + task["askResponse"] = undefined + task["askResponseText"] = undefined + task["askResponseImages"] = undefined + task["lastMessageTs"] = undefined + task["addToClineMessages"] = vi.fn(async () => {}) + task["saveClineMessages"] = vi.fn(async () => true) + task["updateClineMessage"] = vi.fn(async () => {}) + task["cancelAutoApprovalTimeout"] = vi.fn(() => {}) + task["checkpointSave"] = vi.fn(async () => {}) + task["emit"] = vi.fn() + // A double assertion is unavoidable here: `providerRef` is a `WeakRef`, + // and the stub is neither a `WeakRef` nor a whole `ClineProvider`. Constructing + // either would drag in the extension host, when `Task.ask` only ever calls + // `deref()`, `getState()` and `postMessageToWebview()` on it. + task["providerRef"] = { deref: () => provider } as unknown as Task["providerRef"] + // `Task.cwd` reads `workspacePath`, which is `historyItem.workspace` for a + // resumed task and the parent's path for a child one. It is `readonly`, so the + // stub installs it the way the constructor would. + Object.defineProperty(task, "workspacePath", { value: taskCwd }) + + return task +} + +async function attachQueue(task: Task) { + const { MessageQueueService } = await import("../../message-queue/MessageQueueService") + Object.defineProperty(task, "messageQueueService", { value: new MessageQueueService() }) +} + +const TASK_CWD = "/path/to/task-workspace" +const PROVIDER_CWD = "/path/to/window-workspace" + +const buildProvider = (allowedWriteFiles: string[]): ProviderStub => ({ + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + cwd: PROVIDER_CWD, + getState: async () => ({ + autoApprovalEnabled: true, + alwaysAllowWrite: false, + alwaysAllowWriteProtected: false, + allowedReadFiles: [], + allowedWriteFiles, + }), +}) + +/** + * Ask to write a workspace-relative path and report whether it was auto-approved. + * + * An ask that is not auto-answered blocks until the user responds, so the outcome + * is read from the message instead of the returned promise: `Task.ask` stamps + * `autoApprovalDecision` on it when it resolves the ask itself. The ask is then + * answered so nothing is left pending. + */ +const askToWriteRelativePath = async (allowedWriteFiles: string[]) => { + const task = buildTask(buildProvider(allowedWriteFiles), TASK_CWD) + await attachQueue(task) + + // A relative path, as `getReadablePath(task.cwd, relPath)` produces for a file + // inside the task's own workspace. + const asked = task.ask("tool", JSON.stringify({ tool: "newFileCreated", path: "notes.md" }), false) + + const addToClineMessages = task["addToClineMessages"] as ReturnType + await vi.waitUntil(() => addToClineMessages.mock.calls.length > 0) + const message = addToClineMessages.mock.calls[0][0] + + task.approveAsk() + await asked + + return message.autoApprovalDecision ?? ("ask" as const) +} + +describe("Task.ask resolves allowlists against the task's workspace", () => { + it("approves a file the task's own workspace root makes match", async () => { + expect(await askToWriteRelativePath([`${TASK_CWD}/notes.md`])).toBe("approve") + }) + + it("does not approve a pattern that only matches under the provider's workspace root", async () => { + expect(await askToWriteRelativePath([`${PROVIDER_CWD}/notes.md`])).toBe("ask") + }) +}) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6f70a19946..093b8c05d6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2486,9 +2486,11 @@ export class ClineProvider customInstructions, alwaysAllowReadOnly, alwaysAllowReadOnlyOutsideWorkspace, + allowedReadFiles, alwaysAllowWrite, alwaysAllowWriteOutsideWorkspace, alwaysAllowWriteProtected, + allowedWriteFiles, alwaysAllowExecute, destructiveCommandGuardEnabled, allowedCommands, @@ -2635,9 +2637,11 @@ export class ClineProvider customInstructions, alwaysAllowReadOnly: alwaysAllowReadOnly ?? false, alwaysAllowReadOnlyOutsideWorkspace: alwaysAllowReadOnlyOutsideWorkspace ?? false, + allowedReadFiles: allowedReadFiles ?? [], alwaysAllowWrite: alwaysAllowWrite ?? false, alwaysAllowWriteOutsideWorkspace: alwaysAllowWriteOutsideWorkspace ?? false, alwaysAllowWriteProtected: alwaysAllowWriteProtected ?? false, + allowedWriteFiles: allowedWriteFiles ?? [], alwaysAllowExecute: alwaysAllowExecute ?? false, destructiveCommandGuardEnabled, alwaysAllowMcp: alwaysAllowMcp ?? false, @@ -2870,9 +2874,11 @@ export class ClineProvider apiModelId: stateValues.apiModelId, alwaysAllowReadOnly: stateValues.alwaysAllowReadOnly ?? false, alwaysAllowReadOnlyOutsideWorkspace: stateValues.alwaysAllowReadOnlyOutsideWorkspace ?? false, + allowedReadFiles: stateValues.allowedReadFiles ?? [], alwaysAllowWrite: stateValues.alwaysAllowWrite ?? false, alwaysAllowWriteOutsideWorkspace: stateValues.alwaysAllowWriteOutsideWorkspace ?? false, alwaysAllowWriteProtected: stateValues.alwaysAllowWriteProtected ?? false, + allowedWriteFiles: stateValues.allowedWriteFiles ?? [], alwaysAllowExecute: stateValues.alwaysAllowExecute ?? false, destructiveCommandGuardEnabled: stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index e336ac8fac..ce43d3aa45 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1258,6 +1258,68 @@ describe("ClineProvider", () => { expect(state.destructiveCommandGuardEnabled).toBe(true) }) + test("getState returns the saved allowed read files", async () => { + await provider.contextProxy.setValue("allowedReadFiles", ["notes.md"]) + + const state = await provider.getState() + + expect(state.allowedReadFiles).toEqual(["notes.md"]) + }) + + test("getState defaults allowed read files to an empty list", async () => { + const state = await provider.getState() + + expect(state.allowedReadFiles).toEqual([]) + }) + + test("getStateToPostToWebview returns the saved allowed read files", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("allowedReadFiles", ["notes.md"]) + + const state = await provider.getStateToPostToWebview() + + expect(state.allowedReadFiles).toEqual(["notes.md"]) + }) + + test("getStateToPostToWebview defaults allowed read files to an empty list", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.allowedReadFiles).toEqual([]) + }) + + test("getState returns the saved allowed write files", async () => { + await provider.contextProxy.setValue("allowedWriteFiles", ["notes.md"]) + + const state = await provider.getState() + + expect(state.allowedWriteFiles).toEqual(["notes.md"]) + }) + + test("getState defaults allowed write files to an empty list", async () => { + const state = await provider.getState() + + expect(state.allowedWriteFiles).toEqual([]) + }) + + test("getStateToPostToWebview returns the saved allowed write files", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("allowedWriteFiles", ["notes.md"]) + + const state = await provider.getStateToPostToWebview() + + expect(state.allowedWriteFiles).toEqual(["notes.md"]) + }) + + test("getStateToPostToWebview defaults allowed write files to an empty list", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.allowedWriteFiles).toEqual([]) + }) + test("getStateToPostToWebview returns the saved destructive command guard setting", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("destructiveCommandGuardEnabled", true) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index bc92522790..a2b6a33b28 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1227,6 +1227,91 @@ describe("webviewMessageHandler - destructiveCommandGuardEnabled", () => { }) }) +// Both allowlists are normalized by the same branch, so both are held to the +// same contract. +describe.each(["allowedReadFiles", "allowedWriteFiles"] as const)("webviewMessageHandler - %s", (key) => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("persists the configured patterns", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { [key]: ["notes.md", "docs/scratch/**"] }, + }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith(key, ["notes.md", "docs/scratch/**"]) + }) + + it("drops entries that cannot name a file", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // The double assertion stands in for an untyped payload: the message + // arrives as JSON from the webview, so a non-string can reach the + // handler even though the type says otherwise. That is what the + // handler's `typeof` filter is there to catch, so the test has to be + // able to express it. + updatedSettings: { [key]: ["notes.md", "", " ", 42 as unknown as string] }, + }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith(key, ["notes.md"]) + }) + + // Whitespace is significant in gitignore syntax, so it must survive saving. + it("keeps whitespace within a pattern", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { [key]: [" notes.md", "my notes.md", "notes.md\\ "] }, + }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith(key, [ + " notes.md", + "my notes.md", + "notes.md\\ ", + ]) + }) + + it("persists an empty list when the setting is cleared", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { [key]: [] }, + }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith(key, []) + }) + + // Unlike allowed/denied commands, these settings have no + // workspace-configuration counterpart, so nothing should be written to VS + // Code settings. + it("does not write to the VS Code workspace configuration", async () => { + const update = vi.fn() + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ update } as never) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { [key]: ["notes.md"] }, + }) + + expect(update).not.toHaveBeenCalled() + }) +}) + +describe("webviewMessageHandler - allowlists together", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("persists both allowlists from one save", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { allowedReadFiles: ["read.md"], allowedWriteFiles: ["write.md"] }, + }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedReadFiles", ["read.md"]) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedWriteFiles", ["write.md"]) + }) +}) + describe("webviewMessageHandler - terminalProfile", () => { beforeEach(() => { vi.clearAllMocks() diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index e88fd864cd..3d83ea0265 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -730,6 +730,17 @@ export const webviewMessageHandler = async ( await vscode.workspace .getConfiguration(Package.name) .update("deniedCommands", newValue, vscode.ConfigurationTarget.Global) + } else if (key === "allowedReadFiles" || key === "allowedWriteFiles") { + const patterns = value ?? [] + + // Blank lines, which the textarea editor produces freely, + // name no file and are dropped here. Patterns are + // otherwise not `.trim()`ed: leading whitespace is + // significant in gitignore syntax, and trailing + // whitespace has to be escaped by the user to be kept. + newValue = Array.isArray(patterns) + ? patterns.filter((pattern) => typeof pattern === "string" && pattern.trim().length > 0) + : [] } else if (key === "ttsEnabled") { newValue = value ?? true setTtsEnabled(newValue as boolean) diff --git a/webview-ui/src/components/settings/AutoApproveSettings.tsx b/webview-ui/src/components/settings/AutoApproveSettings.tsx index 29676f2299..6276ea6514 100644 --- a/webview-ui/src/components/settings/AutoApproveSettings.tsx +++ b/webview-ui/src/components/settings/AutoApproveSettings.tsx @@ -12,6 +12,7 @@ import { SetCachedStateField } from "./types" import { SectionHeader } from "./SectionHeader" import { Section } from "./Section" import { SearchableSetting } from "./SearchableSetting" +import { FilePatternAllowlist } from "./FilePatternAllowlist" import { AutoApproveToggle } from "./AutoApproveToggle" import { MaxLimitInputs } from "./MaxLimitInputs" import { useExtensionState } from "@/context/ExtensionStateContext" @@ -21,9 +22,11 @@ import { useAutoApprovalToggles } from "@/hooks/useAutoApprovalToggles" type AutoApproveSettingsProps = HTMLAttributes & { alwaysAllowReadOnly?: boolean alwaysAllowReadOnlyOutsideWorkspace?: boolean + allowedReadFiles?: string[] alwaysAllowWrite?: boolean alwaysAllowWriteOutsideWorkspace?: boolean alwaysAllowWriteProtected?: boolean + allowedWriteFiles?: string[] alwaysAllowMcp?: boolean alwaysAllowModeSwitch?: boolean alwaysAllowSubtasks?: boolean @@ -38,9 +41,11 @@ type AutoApproveSettingsProps = HTMLAttributes & { setCachedStateField: SetCachedStateField< | "alwaysAllowReadOnly" | "alwaysAllowReadOnlyOutsideWorkspace" + | "allowedReadFiles" | "alwaysAllowWrite" | "alwaysAllowWriteOutsideWorkspace" | "alwaysAllowWriteProtected" + | "allowedWriteFiles" | "alwaysAllowMcp" | "alwaysAllowModeSwitch" | "alwaysAllowSubtasks" @@ -58,9 +63,11 @@ type AutoApproveSettingsProps = HTMLAttributes & { export const AutoApproveSettings = ({ alwaysAllowReadOnly, alwaysAllowReadOnlyOutsideWorkspace, + allowedReadFiles, alwaysAllowWrite, alwaysAllowWriteOutsideWorkspace, alwaysAllowWriteProtected, + allowedWriteFiles, alwaysAllowMcp, alwaysAllowModeSwitch, alwaysAllowSubtasks, @@ -242,6 +249,41 @@ export const AutoApproveSettings = ({ )} + {/* + * Shown regardless of the category toggles above: listing a file + * here is what grants access to it, so it must be reachable + * without first granting that access to everything. + */} +
+
+ +
{t("settings:autoApprove.allowlists.label")}
+
+ + {/* The pattern syntax is shared by every allowlist below. */} +
+ {t("settings:autoApprove.allowlists.description")} +
+ + + + +
+ {alwaysAllowFollowupQuestions && (
diff --git a/webview-ui/src/components/settings/FilePatternAllowlist.tsx b/webview-ui/src/components/settings/FilePatternAllowlist.tsx new file mode 100644 index 0000000000..86774a79c0 --- /dev/null +++ b/webview-ui/src/components/settings/FilePatternAllowlist.tsx @@ -0,0 +1,90 @@ +import type { FormEvent } from "react" +import { VSCodeTextArea } from "@vscode/webview-ui-toolkit/react" + +import { useAppTranslation } from "@/i18n/TranslationContext" + +import { SetCachedStateField } from "./types" +import { SearchableSetting } from "./SearchableSetting" + +type AllowlistField = "allowedReadFiles" | "allowedWriteFiles" + +interface FilePatternAllowlistProps { + /** Setting the patterns are buffered into. */ + field: AllowlistField + /** Key under `settings:autoApprove.allowlists` holding this list's strings. */ + translationKey: "readFiles" | "writeFiles" + /** Id this list is registered under in the settings search index. */ + settingId: string + /** Prefix for this list's `data-testid`s, so the two lists stay distinguishable. */ + testIdPrefix: string + patterns?: string[] + setCachedStateField: SetCachedStateField +} + +/** + * An editable list of gitignore-style file patterns granting one kind of + * auto-approved access. + * + * Edited as text, one pattern per line, because the order of the lines is + * meaningful: as in a `.gitignore` file, a later pattern overrides an earlier + * one, which a set of individually-added chips could not express. It also lets a + * list be pasted in or copied out in one go. + * + * Blank lines are kept while editing so that a line can be cleared without the + * cursor jumping; they are dropped when the settings are saved. + * + * The pattern syntax itself is explained once by the enclosing Allowlists + * section, so each list only carries what is specific to it. + */ +/** + * Read the current text out of a `VSCodeTextArea`'s input event. + * + * The toolkit component wraps a native `