fix(sandbox): use absolute /bin/sh path for docker exec
Fixes intermittent 'sh not found' errors in nix2container-based sandbox
images. The OCI runtime PATH resolution can fail sporadically when using
bare 'sh' command. Using absolute path /bin/sh bypasses PATH lookup.
Also adds allowedReadPaths config option for bind mount access:
- Add allowedReadPaths?: string[] to SandboxDockerConfig
- Merge allowedReadPaths arrays from global + agent config (like binds)
- Update resolveSandboxPath/assertSandboxPath to validate paths against
root OR any allowedPath entry
Example config:
{
"sandbox": {
"docker": {
"binds": ["/host/skills:/workspace/.skills/tameson:ro"],
"allowedReadPaths": ["/workspace/.skills"]
}
}
}
This commit is contained in:
parent
6372242da7
commit
e229faf850
@ -72,7 +72,8 @@ export function buildDockerExecArgs(params: {
|
||||
const pathExport = hasCustomPath
|
||||
? 'export PATH="${CLAWDBOT_PREPEND_PATH}:$PATH"; unset CLAWDBOT_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;
|
||||
}
|
||||
|
||||
|
||||
@ -380,7 +380,7 @@ describe("buildDockerExecArgs", () => {
|
||||
tty: false,
|
||||
});
|
||||
|
||||
expect(args).toContain("sh");
|
||||
expect(args).toContain("/bin/sh");
|
||||
expect(args).toContain("-lc");
|
||||
});
|
||||
|
||||
|
||||
89
src/agents/sandbox-paths.test.ts
Normal file
89
src/agents/sandbox-paths.test.ts
Normal file
@ -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");
|
||||
});
|
||||
});
|
||||
@ -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;
|
||||
}
|
||||
|
||||
|
||||
@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@ -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]);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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[];
|
||||
};
|
||||
|
||||
Loading…
Reference in New Issue
Block a user