mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-10 12:14:58 +00:00
fix: harden sandbox writes and centralize atomic file writes
This commit is contained in:
@@ -89,6 +89,9 @@ function installDockerReadMock(params?: { canonicalPath?: string }) {
|
||||
if (script.includes('cat -- "$1"')) {
|
||||
return dockerExecResult("content");
|
||||
}
|
||||
if (script.includes("mktemp")) {
|
||||
return dockerExecResult("/workspace/.openclaw-write-b.txt.ABC123\n");
|
||||
}
|
||||
return dockerExecResult("");
|
||||
});
|
||||
}
|
||||
@@ -200,6 +203,37 @@ describe("sandbox fs bridge shell compatibility", () => {
|
||||
expect(mockedExecDockerRaw).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("writes via temp file + atomic rename (never direct truncation)", async () => {
|
||||
const bridge = createSandboxFsBridge({ sandbox: createSandbox() });
|
||||
|
||||
await bridge.writeFile({ filePath: "b.txt", data: "hello" });
|
||||
|
||||
const scripts = getScriptsFromCalls();
|
||||
expect(scripts.some((script) => script.includes('cat >"$1"'))).toBe(false);
|
||||
expect(scripts.some((script) => script.includes('cat >"$tmp"'))).toBe(true);
|
||||
expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(true);
|
||||
});
|
||||
|
||||
it("re-validates target before final rename and cleans temp file on failure", async () => {
|
||||
mockedOpenBoundaryFile
|
||||
.mockImplementationOnce(async () => ({ ok: false, reason: "path" }))
|
||||
.mockImplementationOnce(async () => ({
|
||||
ok: false,
|
||||
reason: "validation",
|
||||
error: new Error("Hardlinked path is not allowed"),
|
||||
}));
|
||||
|
||||
const bridge = createSandboxFsBridge({ sandbox: createSandbox() });
|
||||
await expect(bridge.writeFile({ filePath: "b.txt", data: "hello" })).rejects.toThrow(
|
||||
/hardlinked path/i,
|
||||
);
|
||||
|
||||
const scripts = getScriptsFromCalls();
|
||||
expect(scripts.some((script) => script.includes("mktemp"))).toBe(true);
|
||||
expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(false);
|
||||
expect(scripts.some((script) => script.includes('rm -f -- "$1"'))).toBe(true);
|
||||
});
|
||||
|
||||
it("allows mkdirp for existing in-boundary subdirectories", async () => {
|
||||
await withTempDir("openclaw-fs-bridge-mkdirp-", async (stateDir) => {
|
||||
const workspaceDir = path.join(stateDir, "workspace");
|
||||
|
||||
@@ -119,15 +119,23 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
||||
const buffer = Buffer.isBuffer(params.data)
|
||||
? params.data
|
||||
: Buffer.from(params.data, params.encoding ?? "utf8");
|
||||
const script =
|
||||
params.mkdir === false
|
||||
? 'set -eu; cat >"$1"'
|
||||
: 'set -eu; dir=$(dirname -- "$1"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; cat >"$1"';
|
||||
await this.runCommand(script, {
|
||||
args: [target.containerPath],
|
||||
stdin: buffer,
|
||||
const tempPath = await this.writeFileToTempPath({
|
||||
targetContainerPath: target.containerPath,
|
||||
mkdir: params.mkdir !== false,
|
||||
data: buffer,
|
||||
signal: params.signal,
|
||||
});
|
||||
|
||||
try {
|
||||
await this.assertPathSafety(target, { action: "write files", requireWritable: true });
|
||||
await this.runCommand('set -eu; mv -f -- "$1" "$2"', {
|
||||
args: [tempPath, target.containerPath],
|
||||
signal: params.signal,
|
||||
});
|
||||
} catch (error) {
|
||||
await this.cleanupTempPath(tempPath, params.signal);
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise<void> {
|
||||
@@ -351,6 +359,58 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
||||
return normalizeContainerPath(canonical);
|
||||
}
|
||||
|
||||
private async writeFileToTempPath(params: {
|
||||
targetContainerPath: string;
|
||||
mkdir: boolean;
|
||||
data: Buffer;
|
||||
signal?: AbortSignal;
|
||||
}): Promise<string> {
|
||||
const script = params.mkdir
|
||||
? [
|
||||
"set -eu",
|
||||
'target="$1"',
|
||||
'dir=$(dirname -- "$target")',
|
||||
'if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi',
|
||||
'base=$(basename -- "$target")',
|
||||
'tmp=$(mktemp "$dir/.openclaw-write-$base.XXXXXX")',
|
||||
'cat >"$tmp"',
|
||||
'printf "%s\\n" "$tmp"',
|
||||
].join("\n")
|
||||
: [
|
||||
"set -eu",
|
||||
'target="$1"',
|
||||
'dir=$(dirname -- "$target")',
|
||||
'base=$(basename -- "$target")',
|
||||
'tmp=$(mktemp "$dir/.openclaw-write-$base.XXXXXX")',
|
||||
'cat >"$tmp"',
|
||||
'printf "%s\\n" "$tmp"',
|
||||
].join("\n");
|
||||
const result = await this.runCommand(script, {
|
||||
args: [params.targetContainerPath],
|
||||
stdin: params.data,
|
||||
signal: params.signal,
|
||||
});
|
||||
const tempPath = result.stdout.toString("utf8").trim().split(/\r?\n/).at(-1)?.trim();
|
||||
if (!tempPath || !tempPath.startsWith("/")) {
|
||||
throw new Error(
|
||||
`Failed to create temporary sandbox write path for ${params.targetContainerPath}`,
|
||||
);
|
||||
}
|
||||
return normalizeContainerPath(tempPath);
|
||||
}
|
||||
|
||||
private async cleanupTempPath(tempPath: string, signal?: AbortSignal): Promise<void> {
|
||||
try {
|
||||
await this.runCommand('set -eu; rm -f -- "$1"', {
|
||||
args: [tempPath],
|
||||
signal,
|
||||
allowFailure: true,
|
||||
});
|
||||
} catch {
|
||||
// Best-effort cleanup only.
|
||||
}
|
||||
}
|
||||
|
||||
private ensureWriteAccess(target: SandboxResolvedFsPath, action: string) {
|
||||
if (!allowsWrites(this.sandbox.workspaceAccess) || !target.writable) {
|
||||
throw new Error(`Sandbox path is read-only; cannot ${action}: ${target.containerPath}`);
|
||||
|
||||
@@ -1,12 +1,7 @@
|
||||
import crypto from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { writeJsonAtomic } from "../../infra/json-files.js";
|
||||
import { acquireSessionWriteLock } from "../session-write-lock.js";
|
||||
import {
|
||||
SANDBOX_BROWSER_REGISTRY_PATH,
|
||||
SANDBOX_REGISTRY_PATH,
|
||||
SANDBOX_STATE_DIR,
|
||||
} from "./constants.js";
|
||||
import { SANDBOX_BROWSER_REGISTRY_PATH, SANDBOX_REGISTRY_PATH } from "./constants.js";
|
||||
|
||||
export type SandboxRegistryEntry = {
|
||||
containerName: string;
|
||||
@@ -111,20 +106,7 @@ async function writeRegistryFile<T extends RegistryEntry>(
|
||||
registryPath: string,
|
||||
registry: RegistryFile<T>,
|
||||
): Promise<void> {
|
||||
await fs.mkdir(SANDBOX_STATE_DIR, { recursive: true });
|
||||
const payload = `${JSON.stringify(registry, null, 2)}\n`;
|
||||
const registryDir = path.dirname(registryPath);
|
||||
const tempPath = path.join(
|
||||
registryDir,
|
||||
`${path.basename(registryPath)}.${crypto.randomUUID()}.tmp`,
|
||||
);
|
||||
await fs.writeFile(tempPath, payload, "utf-8");
|
||||
try {
|
||||
await fs.rename(tempPath, registryPath);
|
||||
} catch (error) {
|
||||
await fs.rm(tempPath, { force: true });
|
||||
throw error;
|
||||
}
|
||||
await writeJsonAtomic(registryPath, registry, { trailingNewline: true });
|
||||
}
|
||||
|
||||
export async function readRegistry(): Promise<SandboxRegistry> {
|
||||
|
||||
Reference in New Issue
Block a user