fix(agents): block workspaceOnly apply_patch delete symlink escape

This commit is contained in:
Peter Steinberger
2026-02-15 03:23:16 +01:00
parent 683aa09b55
commit 914b9d1e79
4 changed files with 52 additions and 8 deletions

View File

@@ -74,6 +74,7 @@ Docs: https://docs.openclaw.ai
- Feishu/Security: harden media URL fetching against SSRF and local file disclosure. (#16285) Thanks @mbelinky.
- Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent.
- Security/Agents: enforce workspace-root path bounds for `apply_patch` in non-sandbox mode to block traversal and symlink escape writes. Thanks @p80n-sec.
- Security/Agents: enforce symlink-escape checks for `apply_patch` delete hunks under `workspaceOnly`, while still allowing deleting the symlink itself. Thanks @p80n-sec.
- Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent.
- Security: fix Chutes manual OAuth login state validation by requiring the full redirect URL (reject code-only pastes) (thanks @aether-ai-agent).
- Security/Gateway: harden tool-supplied `gatewayUrl` overrides by restricting them to loopback or the configured `gateway.remote.url`. Thanks @p80n-sec.

View File

@@ -181,9 +181,7 @@ describe("applyPatch", () => {
*** End Patch`;
try {
await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(
/Symlink escapes sandbox root/,
);
await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Symlink escapes sandbox root/);
const stillThere = await fs.readFile(outsideFile, "utf8");
expect(stillThere).toBe("victim\n");
} finally {
@@ -216,4 +214,29 @@ describe("applyPatch", () => {
}
});
});
it("allows deleting a symlink itself even if it points outside cwd", async () => {
await withTempDir(async (dir) => {
const outsideDir = await fs.mkdtemp(path.join(path.dirname(dir), "openclaw-patch-outside-"));
try {
const outsideTarget = path.join(outsideDir, "target.txt");
await fs.writeFile(outsideTarget, "keep\n", "utf8");
const linkDir = path.join(dir, "link");
await fs.symlink(outsideDir, linkDir);
const patch = `*** Begin Patch
*** Delete File: link
*** End Patch`;
const result = await applyPatch(patch, { cwd: dir });
expect(result.summary.deleted).toEqual(["link"]);
await expect(fs.lstat(linkDir)).rejects.toBeDefined();
const outsideContents = await fs.readFile(outsideTarget, "utf8");
expect(outsideContents).toBe("keep\n");
} finally {
await fs.rm(outsideDir, { recursive: true, force: true });
}
});
});
});

View File

@@ -155,7 +155,7 @@ export async function applyPatch(
}
if (hunk.kind === "delete") {
const target = await resolvePatchPath(hunk.path, options);
const target = await resolvePatchPath(hunk.path, options, "unlink");
await fileOps.remove(target.resolved);
recordSummary(summary, seen, "deleted", target.display);
continue;
@@ -254,6 +254,7 @@ async function ensureDir(filePath: string, ops: PatchFileOps) {
async function resolvePatchPath(
filePath: string,
options: ApplyPatchOptions,
purpose: "readWrite" | "unlink" = "readWrite",
): Promise<{ resolved: string; display: string }> {
if (options.sandbox) {
const resolved = options.sandbox.bridge.resolvePath({
@@ -273,6 +274,7 @@ async function resolvePatchPath(
filePath,
cwd: options.cwd,
root: options.cwd,
allowFinalSymlink: purpose === "unlink",
})
).resolved
: resolvePathFromCwd(filePath, options.cwd);

View File

@@ -50,9 +50,16 @@ export function resolveSandboxPath(params: { filePath: string; cwd: string; root
return { resolved, relative };
}
export async function assertSandboxPath(params: { filePath: string; cwd: string; root: string }) {
export async function assertSandboxPath(params: {
filePath: string;
cwd: string;
root: string;
allowFinalSymlink?: boolean;
}) {
const resolved = resolveSandboxPath(params);
await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root));
await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root), {
allowFinalSymlink: params.allowFinalSymlink,
});
return resolved;
}
@@ -90,18 +97,29 @@ export async function resolveSandboxedMediaSource(params: {
return resolved.resolved;
}
async function assertNoSymlinkEscape(relative: string, root: string) {
async function assertNoSymlinkEscape(
relative: string,
root: string,
options?: { allowFinalSymlink?: boolean },
) {
if (!relative) {
return;
}
const rootReal = await tryRealpath(root);
const parts = relative.split(path.sep).filter(Boolean);
let current = root;
for (const part of parts) {
for (let idx = 0; idx < parts.length; idx += 1) {
const part = parts[idx];
const isLast = idx === parts.length - 1;
current = path.join(current, part);
try {
const stat = await fs.lstat(current);
if (stat.isSymbolicLink()) {
// Unlinking a symlink itself is safe even if it points outside the root. What we
// must prevent is traversing through a symlink to reach targets outside root.
if (options?.allowFinalSymlink && isLast) {
return;
}
const target = await tryRealpath(current);
if (!isPathInside(rootReal, target)) {
throw new Error(