diff --git a/src/agents/bash-tools.shared.ts b/src/agents/bash-tools.shared.ts index 8c2d16fb2..f90ae20d0 100644 --- a/src/agents/bash-tools.shared.ts +++ b/src/agents/bash-tools.shared.ts @@ -72,7 +72,8 @@ export function buildDockerExecArgs(params: { const pathExport = hasCustomPath ? 'export PATH="${OPENCLAW_PREPEND_PATH}:$PATH"; unset OPENCLAW_PREPEND_PATH; ' : ""; - args.push(params.containerName, "sh", "-lc", `${pathExport}${params.command}`); + // Use absolute path /bin/sh to avoid OCI runtime PATH resolution issues in nix containers + args.push(params.containerName, "/bin/sh", "-lc", `${pathExport}${params.command}`); return args; } diff --git a/src/agents/bash-tools.test.ts b/src/agents/bash-tools.test.ts index bfb7605ef..f1acfe6e3 100644 --- a/src/agents/bash-tools.test.ts +++ b/src/agents/bash-tools.test.ts @@ -399,7 +399,7 @@ describe("buildDockerExecArgs", () => { tty: false, }); - expect(args).toContain("sh"); + expect(args).toContain("/bin/sh"); expect(args).toContain("-lc"); }); diff --git a/src/agents/sandbox-paths.test.ts b/src/agents/sandbox-paths.test.ts new file mode 100644 index 000000000..ec170f6cd --- /dev/null +++ b/src/agents/sandbox-paths.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; +import { resolveSandboxPath, assertSandboxPath } from "./sandbox-paths.js"; +import path from "node:path"; + +describe("resolveSandboxPath", () => { + const root = "/workspace"; + const cwd = "/workspace"; + + it("allows paths within root", () => { + const result = resolveSandboxPath({ + filePath: "subdir/file.txt", + cwd, + root, + }); + expect(result.resolved).toBe("/workspace/subdir/file.txt"); + expect(result.relative).toBe("subdir/file.txt"); + expect(result.base).toBe("/workspace"); + }); + + it("rejects paths escaping root without allowedPaths", () => { + expect(() => + resolveSandboxPath({ + filePath: "/other/path/file.txt", + cwd, + root, + }), + ).toThrow(/escapes sandbox root/); + }); + + it("allows paths in allowedPaths when they escape root", () => { + // /external/skills is outside /workspace, but allowed via allowedPaths + const result = resolveSandboxPath({ + filePath: "/external/skills/tameson/SKILL.md", + cwd, + root, + allowedPaths: ["/external/skills"], + }); + expect(result.resolved).toBe("/external/skills/tameson/SKILL.md"); + expect(result.relative).toBe("tameson/SKILL.md"); + expect(result.base).toBe("/external/skills"); + }); + + it("still rejects paths not in root or allowedPaths", () => { + expect(() => + resolveSandboxPath({ + filePath: "/etc/passwd", + cwd, + root, + allowedPaths: ["/external/skills"], + }), + ).toThrow(/escapes sandbox root/); + }); + + it("allows exact match of allowedPath", () => { + const result = resolveSandboxPath({ + filePath: "/external/skills", + cwd, + root, + allowedPaths: ["/external/skills"], + }); + expect(result.resolved).toBe("/external/skills"); + expect(result.relative).toBe(""); + expect(result.base).toBe("/external/skills"); + }); + + it("prefers root over allowedPaths for paths inside root", () => { + // Path is inside root - should use root as base, not allowedPaths + const result = resolveSandboxPath({ + filePath: "/workspace/file.txt", + cwd, + root, + allowedPaths: ["/workspace"], // redundant, but shouldn't matter + }); + expect(result.base).toBe("/workspace"); + expect(result.relative).toBe("file.txt"); + }); + + it("handles multiple allowedPaths", () => { + const result = resolveSandboxPath({ + filePath: "/data/files/test.txt", + cwd, + root, + allowedPaths: ["/external/skills", "/data/files"], + }); + expect(result.resolved).toBe("/data/files/test.txt"); + expect(result.relative).toBe("test.txt"); + expect(result.base).toBe("/data/files"); + }); +}); diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index f3dc787b7..e40e176cc 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -25,25 +25,53 @@ function resolveToCwd(filePath: string, cwd: string): string { return path.resolve(cwd, expanded); } -export function resolveSandboxPath(params: { filePath: string; cwd: string; root: string }): { +export function resolveSandboxPath(params: { + filePath: string; + cwd: string; + root: string; + allowedPaths?: string[]; +}): { resolved: string; relative: string; + base: string; } { const resolved = resolveToCwd(params.filePath, params.cwd); const rootResolved = path.resolve(params.root); const relative = path.relative(rootResolved, resolved); + + // Check if path is within the main root if (!relative || relative === "") { - return { resolved, relative: "" }; + return { resolved, relative: "", base: rootResolved }; } - if (relative.startsWith("..") || path.isAbsolute(relative)) { - throw new Error(`Path escapes sandbox root (${shortPath(rootResolved)}): ${params.filePath}`); + if (!relative.startsWith("..") && !path.isAbsolute(relative)) { + return { resolved, relative, base: rootResolved }; } - return { resolved, relative }; + + // Path escapes main root - check allowedPaths + if (params.allowedPaths?.length) { + for (const allowedPath of params.allowedPaths) { + const allowedResolved = path.resolve(allowedPath); + const relativeToAllowed = path.relative(allowedResolved, resolved); + if ( + relativeToAllowed === "" || + (!relativeToAllowed.startsWith("..") && !path.isAbsolute(relativeToAllowed)) + ) { + return { resolved, relative: relativeToAllowed, base: allowedResolved }; + } + } + } + + throw new Error(`Path escapes sandbox root (${shortPath(rootResolved)}): ${params.filePath}`); } -export async function assertSandboxPath(params: { filePath: string; cwd: string; root: string }) { +export async function assertSandboxPath(params: { + filePath: string; + cwd: string; + root: string; + allowedPaths?: string[]; +}) { const resolved = resolveSandboxPath(params); - await assertNoSymlink(resolved.relative, path.resolve(params.root)); + await assertNoSymlink(resolved.relative, resolved.base); return resolved; } diff --git a/src/agents/sandbox/config.ts b/src/agents/sandbox/config.ts index de3596b09..fb71f1cc8 100644 --- a/src/agents/sandbox/config.ts +++ b/src/agents/sandbox/config.ts @@ -51,6 +51,10 @@ export function resolveSandboxDockerConfig(params: { : globalDocker?.ulimits; const binds = [...(globalDocker?.binds ?? []), ...(agentDocker?.binds ?? [])]; + const allowedReadPaths = [ + ...(globalDocker?.allowedReadPaths ?? []), + ...(agentDocker?.allowedReadPaths ?? []), + ]; return { image: agentDocker?.image ?? globalDocker?.image ?? DEFAULT_SANDBOX_IMAGE, @@ -76,6 +80,7 @@ export function resolveSandboxDockerConfig(params: { dns: agentDocker?.dns ?? globalDocker?.dns, extraHosts: agentDocker?.extraHosts ?? globalDocker?.extraHosts, binds: binds.length ? binds : undefined, + allowedReadPaths: allowedReadPaths.length ? allowedReadPaths : undefined, }; } diff --git a/src/agents/sandbox/docker.ts b/src/agents/sandbox/docker.ts index 2f904420d..fb647e11f 100644 --- a/src/agents/sandbox/docker.ts +++ b/src/agents/sandbox/docker.ts @@ -203,7 +203,8 @@ async function createSandboxContainer(params: { await execDocker(["start", name]); if (cfg.setupCommand?.trim()) { - await execDocker(["exec", "-i", name, "sh", "-lc", cfg.setupCommand]); + // Use absolute path /bin/sh to avoid OCI runtime PATH resolution issues in nix containers + await execDocker(["exec", "-i", name, "/bin/sh", "-lc", cfg.setupCommand]); } } diff --git a/src/agents/sandbox/types.docker.ts b/src/agents/sandbox/types.docker.ts index 51e1a6b8c..6d16fa74a 100644 --- a/src/agents/sandbox/types.docker.ts +++ b/src/agents/sandbox/types.docker.ts @@ -19,4 +19,10 @@ export type SandboxDockerConfig = { dns?: string[]; extraHosts?: string[]; binds?: string[]; + /** + * Additional paths (inside the container) that are allowed for read operations. + * Useful for bind-mounted paths outside the workspace root. + * Example: ["/workspace/.skills"] to allow reading from bind-mounted skills. + */ + allowedReadPaths?: string[]; };