diff --git a/.changeset/stale-lemons-protect.md b/.changeset/stale-lemons-protect.md new file mode 100644 index 00000000000..8f54eb79d5f --- /dev/null +++ b/.changeset/stale-lemons-protect.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Prompt before agents access files outside the active directory when a workspace boundary resolves to a filesystem root. diff --git a/packages/opencode/src/tool/external-directory.ts b/packages/opencode/src/tool/external-directory.ts index 88b73da5096..c6518e20d2b 100644 --- a/packages/opencode/src/tool/external-directory.ts +++ b/packages/opencode/src/tool/external-directory.ts @@ -3,7 +3,6 @@ import { Effect } from "effect" import { EffectLogger } from "@/effect" import { InstanceState } from "@/effect" import type * as Tool from "./tool" -import { Instance } from "../project/instance" import { AppFileSystem } from "@opencode-ai/shared/filesystem" type Kind = "file" | "directory" @@ -13,6 +12,16 @@ type Options = { kind?: Kind } +// kilocode_change start - root boundaries must not auto-allow external_directory +function root(dir: string) { + return path.parse(dir).root === dir +} + +function inside(dir: string, file: string) { + return !root(dir) && AppFileSystem.contains(dir, file) +} +// kilocode_change end + export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirectory")(function* ( ctx: Tool.Context, target?: string, @@ -24,7 +33,9 @@ export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirec const ins = yield* InstanceState.context const full = process.platform === "win32" ? AppFileSystem.normalizePath(target) : target - if (Instance.containsPath(full, ins)) return + // kilocode_change start - keep root-workspace behavior intact outside permission prompts + if (inside(ins.directory, full) || inside(ins.worktree, full)) return + // kilocode_change end const kind = options?.kind ?? "file" const dir = kind === "directory" ? full : path.dirname(full) diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index 6c4d455224e..38b7e1b7ed8 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -1,7 +1,9 @@ import { chmod, mkdir, readFile, stat as statFile, writeFile } from "fs/promises" import { createWriteStream, existsSync, statSync } from "fs" import { realpathSync } from "fs" -import { dirname, join, relative, resolve as pathResolve, win32 } from "path" +// kilocode_change start - harden containment checks +import { dirname, isAbsolute, join, relative, resolve as pathResolve, sep, win32 } from "path" +// kilocode_change end import { Readable } from "stream" import { pipeline } from "stream/promises" import { Glob } from "@opencode-ai/shared/util/glob" @@ -162,7 +164,10 @@ export function overlaps(a: string, b: string) { } export function contains(parent: string, child: string) { - return !relative(parent, child).startsWith("..") + // kilocode_change start - reject cross-drive and escaped relative paths + const rel = relative(parent, child) + return rel === "" || (!isAbsolute(rel) && rel !== ".." && !rel.startsWith(`..${sep}`)) + // kilocode_change end } export async function findUp( diff --git a/packages/opencode/test/kilocode/external-directory-boundary.test.ts b/packages/opencode/test/kilocode/external-directory-boundary.test.ts new file mode 100644 index 00000000000..ea04fd71d53 --- /dev/null +++ b/packages/opencode/test/kilocode/external-directory-boundary.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, test } from "bun:test" +import { Effect } from "effect" +import path from "path" +import type { Permission } from "../../src/permission" +import { Instance } from "../../src/project/instance" +import { SessionID, MessageID } from "../../src/session/schema" +import { assertExternalDirectory } from "../../src/tool/external-directory" +import type { Tool } from "../../src/tool" +import { Filesystem } from "../../src/util" +import { AppFileSystem } from "@opencode-ai/shared/filesystem" +import { tmpdir } from "../fixture/fixture" + +const base: Omit = { + sessionID: SessionID.make("ses_test-boundary-session"), + messageID: MessageID.make(""), + callID: "", + agent: "code", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, +} + +const glob = (p: string) => + process.platform === "win32" ? AppFileSystem.normalizePathPattern(p) : p.replaceAll("\\", "/") + +const asks = () => { + const items: Array> = [] + const ctx: Tool.Context = { + ...base, + ask: (req) => + Effect.sync(() => { + items.push(req) + }), + } + return { items, ctx } +} + +describe("kilocode external directory boundaries", () => { + test("asks before accessing outside a repo-root session", async () => { + await using repo = await tmpdir({ git: true }) + await using outer = await tmpdir() + const file = path.join(outer.path, "outside.txt") + const { items, ctx } = asks() + + await Instance.provide({ + directory: repo.path, + fn: async () => { + try { + await assertExternalDirectory(ctx, file) + } finally { + await Instance.dispose() + } + }, + }) + + const ext = items.find((item) => item.permission === "external_directory") + expect(ext).toBeDefined() + expect(ext!.patterns).toEqual([glob(path.join(outer.path, "*"))]) + expect(ext!.always).toEqual([glob(path.join(outer.path, "*"))]) + expect(ext!.metadata).toMatchObject({ filepath: file, parentDir: outer.path }) + }) + + test("asks when the instance directory is a filesystem root", async () => { + await using outer = await tmpdir() + const root = path.parse(outer.path).root + const file = path.join(outer.path, "outside-root.txt") + const { items, ctx } = asks() + + await Instance.provide({ + directory: root, + fn: async () => { + try { + await assertExternalDirectory(ctx, file) + } finally { + await Instance.dispose() + } + }, + }) + + const ext = items.find((item) => item.permission === "external_directory") + expect(ext).toBeDefined() + expect(ext!.patterns).toEqual([glob(path.join(outer.path, "*"))]) + expect(ext!.metadata).toMatchObject({ filepath: file, parentDir: outer.path }) + }) + + test("contains helpers keep dot-prefixed child names internal", () => { + expect(Filesystem.contains("/project", "/project/..cache/file")).toBe(true) + expect(AppFileSystem.contains("/a/b", "/a/b/..cache/file")).toBe(true) + }) + + test("AppFileSystem.contains rejects cross-drive paths on Windows", () => { + if (process.platform !== "win32") return + expect(AppFileSystem.contains("C:\\repo", "D:\\outside\\file.txt")).toBe(false) + }) +}) diff --git a/packages/shared/src/filesystem.ts b/packages/shared/src/filesystem.ts index 44346be8f94..da0af299cb3 100644 --- a/packages/shared/src/filesystem.ts +++ b/packages/shared/src/filesystem.ts @@ -1,5 +1,12 @@ import { NodeFileSystem } from "@effect/platform-node" -import { dirname, join, relative, resolve as pathResolve } from "path" +import { + dirname, + isAbsolute, + join, + relative, + resolve as pathResolve, + sep, +} from "path" // kilocode_change - harden containment checks import { realpathSync } from "fs" import * as NFS from "fs/promises" import { lookup } from "mime-types" @@ -231,6 +238,9 @@ export namespace AppFileSystem { } export function contains(parent: string, child: string) { - return !relative(parent, child).startsWith("..") + // kilocode_change start - reject cross-drive and escaped relative paths + const rel = relative(parent, child) + return rel === "" || (!isAbsolute(rel) && rel !== ".." && !rel.startsWith(`..${sep}`)) + // kilocode_change end } } diff --git a/packages/shared/test/kilocode/filesystem-containment.test.ts b/packages/shared/test/kilocode/filesystem-containment.test.ts new file mode 100644 index 00000000000..9f98b926002 --- /dev/null +++ b/packages/shared/test/kilocode/filesystem-containment.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, test } from "bun:test" +import { AppFileSystem } from "@opencode-ai/shared/filesystem" + +describe("kilocode filesystem containment", () => { + test("keeps dot-prefixed child names internal", () => { + expect(AppFileSystem.contains("/a/b", "/a/b/..cache/file")).toBe(true) + }) + + test("rejects cross-drive paths on Windows", () => { + if (process.platform !== "win32") return + expect(AppFileSystem.contains("C:\\repo", "D:\\outside\\file.txt")).toBe(false) + }) +})