mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-10-29 18:52:45 +08:00
fix(cli): guard external directory boundaries (#9484)
* fix(cli): guard external directory boundaries * test(cli): move boundary regressions to kilocode tests * fix(cli): scope root guard to permission prompts * test(cli): isolate boundary regression fixtures * test(cli): simplify boundary regression coverage
This commit is contained in:
@@ -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.
|
||||
@@ -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)
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<Tool.Context, "ask"> = {
|
||||
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<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
|
||||
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)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user