fix: check worktree for external_directory permission in subdirs (#7811)
This commit is contained in:
@@ -279,7 +279,7 @@ export namespace File {
|
|||||||
|
|
||||||
// TODO: Filesystem.contains is lexical only - symlinks inside the project can escape.
|
// TODO: Filesystem.contains is lexical only - symlinks inside the project can escape.
|
||||||
// TODO: On Windows, cross-drive paths bypass this check. Consider realpath canonicalization.
|
// TODO: On Windows, cross-drive paths bypass this check. Consider realpath canonicalization.
|
||||||
if (!Filesystem.contains(Instance.directory, full)) {
|
if (!Instance.containsPath(full)) {
|
||||||
throw new Error(`Access denied: path escapes project directory`)
|
throw new Error(`Access denied: path escapes project directory`)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -339,7 +339,7 @@ export namespace File {
|
|||||||
|
|
||||||
// TODO: Filesystem.contains is lexical only - symlinks inside the project can escape.
|
// TODO: Filesystem.contains is lexical only - symlinks inside the project can escape.
|
||||||
// TODO: On Windows, cross-drive paths bypass this check. Consider realpath canonicalization.
|
// TODO: On Windows, cross-drive paths bypass this check. Consider realpath canonicalization.
|
||||||
if (!Filesystem.contains(Instance.directory, resolved)) {
|
if (!Instance.containsPath(resolved)) {
|
||||||
throw new Error(`Access denied: path escapes project directory`)
|
throw new Error(`Access denied: path escapes project directory`)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import { Project } from "./project"
|
|||||||
import { State } from "./state"
|
import { State } from "./state"
|
||||||
import { iife } from "@/util/iife"
|
import { iife } from "@/util/iife"
|
||||||
import { GlobalBus } from "@/bus/global"
|
import { GlobalBus } from "@/bus/global"
|
||||||
|
import { Filesystem } from "@/util/filesystem"
|
||||||
|
|
||||||
interface Context {
|
interface Context {
|
||||||
directory: string
|
directory: string
|
||||||
@@ -46,6 +47,18 @@ export const Instance = {
|
|||||||
get project() {
|
get project() {
|
||||||
return context.use().project
|
return context.use().project
|
||||||
},
|
},
|
||||||
|
/**
|
||||||
|
* Check if a path is within the project boundary.
|
||||||
|
* Returns true if path is inside Instance.directory OR Instance.worktree.
|
||||||
|
* Paths within the worktree but outside the working directory should not trigger external_directory permission.
|
||||||
|
*/
|
||||||
|
containsPath(filepath: string) {
|
||||||
|
if (Filesystem.contains(Instance.directory, filepath)) return true
|
||||||
|
// Non-git projects set worktree to "/" which would match ANY absolute path.
|
||||||
|
// Skip worktree check in this case to preserve external_directory permissions.
|
||||||
|
if (Instance.worktree === "/") return false
|
||||||
|
return Filesystem.contains(Instance.worktree, filepath)
|
||||||
|
},
|
||||||
state<S>(init: () => S, dispose?: (state: Awaited<S>) => Promise<void>): () => S {
|
state<S>(init: () => S, dispose?: (state: Awaited<S>) => Promise<void>): () => S {
|
||||||
return State.create(() => Instance.directory, init, dispose)
|
return State.create(() => Instance.directory, init, dispose)
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -85,7 +85,7 @@ export const BashTool = Tool.define("bash", async () => {
|
|||||||
throw new Error("Failed to parse command")
|
throw new Error("Failed to parse command")
|
||||||
}
|
}
|
||||||
const directories = new Set<string>()
|
const directories = new Set<string>()
|
||||||
if (!Filesystem.contains(Instance.directory, cwd)) directories.add(cwd)
|
if (!Instance.containsPath(cwd)) directories.add(cwd)
|
||||||
const patterns = new Set<string>()
|
const patterns = new Set<string>()
|
||||||
const always = new Set<string>()
|
const always = new Set<string>()
|
||||||
|
|
||||||
@@ -124,7 +124,7 @@ export const BashTool = Tool.define("bash", async () => {
|
|||||||
process.platform === "win32" && resolved.match(/^\/[a-z]\//)
|
process.platform === "win32" && resolved.match(/^\/[a-z]\//)
|
||||||
? resolved.replace(/^\/([a-z])\//, (_, drive) => `${drive.toUpperCase()}:\\`).replace(/\//g, "\\")
|
? resolved.replace(/^\/([a-z])\//, (_, drive) => `${drive.toUpperCase()}:\\`).replace(/\//g, "\\")
|
||||||
: resolved
|
: resolved
|
||||||
if (!Filesystem.contains(Instance.directory, normalized)) directories.add(normalized)
|
if (!Instance.containsPath(normalized)) directories.add(normalized)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,5 @@
|
|||||||
import path from "path"
|
import path from "path"
|
||||||
import type { Tool } from "./tool"
|
import type { Tool } from "./tool"
|
||||||
import { Filesystem } from "../util/filesystem"
|
|
||||||
import { Instance } from "../project/instance"
|
import { Instance } from "../project/instance"
|
||||||
|
|
||||||
type Kind = "file" | "directory"
|
type Kind = "file" | "directory"
|
||||||
@@ -15,7 +14,7 @@ export async function assertExternalDirectory(ctx: Tool.Context, target?: string
|
|||||||
|
|
||||||
if (options?.bypass) return
|
if (options?.bypass) return
|
||||||
|
|
||||||
if (Filesystem.contains(Instance.directory, target)) return
|
if (Instance.containsPath(target)) return
|
||||||
|
|
||||||
const kind = options?.kind ?? "file"
|
const kind = options?.kind ?? "file"
|
||||||
const parentDir = kind === "directory" ? target : path.dirname(target)
|
const parentDir = kind === "directory" ? target : path.dirname(target)
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { test, expect, describe } from "bun:test"
|
import { test, expect, describe } from "bun:test"
|
||||||
import path from "path"
|
import path from "path"
|
||||||
|
import fs from "fs/promises"
|
||||||
import { Filesystem } from "../../src/util/filesystem"
|
import { Filesystem } from "../../src/util/filesystem"
|
||||||
import { File } from "../../src/file"
|
import { File } from "../../src/file"
|
||||||
import { Instance } from "../../src/project/instance"
|
import { Instance } from "../../src/project/instance"
|
||||||
@@ -113,3 +114,85 @@ describe("File.list path traversal protection", () => {
|
|||||||
})
|
})
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe("Instance.containsPath", () => {
|
||||||
|
test("returns true for path inside directory", async () => {
|
||||||
|
await using tmp = await tmpdir({ git: true })
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: () => {
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "foo.txt"))).toBe(true)
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "src", "file.ts"))).toBe(true)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("returns true for path inside worktree but outside directory (monorepo subdirectory scenario)", async () => {
|
||||||
|
await using tmp = await tmpdir({ git: true })
|
||||||
|
const subdir = path.join(tmp.path, "packages", "lib")
|
||||||
|
await fs.mkdir(subdir, { recursive: true })
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: subdir,
|
||||||
|
fn: () => {
|
||||||
|
// .opencode at worktree root, but we're running from packages/lib
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, ".opencode", "state"))).toBe(true)
|
||||||
|
// sibling package should also be accessible
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "packages", "other", "file.ts"))).toBe(true)
|
||||||
|
// worktree root itself
|
||||||
|
expect(Instance.containsPath(tmp.path)).toBe(true)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("returns false for path outside both directory and worktree", async () => {
|
||||||
|
await using tmp = await tmpdir({ git: true })
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: () => {
|
||||||
|
expect(Instance.containsPath("/etc/passwd")).toBe(false)
|
||||||
|
expect(Instance.containsPath("/tmp/other-project")).toBe(false)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("returns false for path with .. escaping worktree", async () => {
|
||||||
|
await using tmp = await tmpdir({ git: true })
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: () => {
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "..", "escape.txt"))).toBe(false)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("handles directory === worktree (running from repo root)", async () => {
|
||||||
|
await using tmp = await tmpdir({ git: true })
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: () => {
|
||||||
|
expect(Instance.directory).toBe(Instance.worktree)
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "file.txt"))).toBe(true)
|
||||||
|
expect(Instance.containsPath("/etc/passwd")).toBe(false)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("non-git project does not allow arbitrary paths via worktree='/'", async () => {
|
||||||
|
await using tmp = await tmpdir() // no git: true
|
||||||
|
|
||||||
|
await Instance.provide({
|
||||||
|
directory: tmp.path,
|
||||||
|
fn: () => {
|
||||||
|
// worktree is "/" for non-git projects, but containsPath should NOT allow all paths
|
||||||
|
expect(Instance.containsPath(path.join(tmp.path, "file.txt"))).toBe(true)
|
||||||
|
expect(Instance.containsPath("/etc/passwd")).toBe(false)
|
||||||
|
expect(Instance.containsPath("/tmp/other")).toBe(false)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user