mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-10 15:24:58 +00:00
fix(sandbox): allow mkdirp boundary checks on existing directories (#31547)
This commit is contained in:
@@ -7,12 +7,22 @@ vi.mock("./docker.js", () => ({
|
|||||||
execDockerRaw: vi.fn(),
|
execDockerRaw: vi.fn(),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
vi.mock("../../infra/boundary-file-read.js", async (importOriginal) => {
|
||||||
|
const actual = await importOriginal<typeof import("../../infra/boundary-file-read.js")>();
|
||||||
|
return {
|
||||||
|
...actual,
|
||||||
|
openBoundaryFile: vi.fn(actual.openBoundaryFile),
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
import { openBoundaryFile } from "../../infra/boundary-file-read.js";
|
||||||
import { execDockerRaw } from "./docker.js";
|
import { execDockerRaw } from "./docker.js";
|
||||||
import { createSandboxFsBridge } from "./fs-bridge.js";
|
import { createSandboxFsBridge } from "./fs-bridge.js";
|
||||||
import { createSandboxTestContext } from "./test-fixtures.js";
|
import { createSandboxTestContext } from "./test-fixtures.js";
|
||||||
import type { SandboxContext } from "./types.js";
|
import type { SandboxContext } from "./types.js";
|
||||||
|
|
||||||
const mockedExecDockerRaw = vi.mocked(execDockerRaw);
|
const mockedExecDockerRaw = vi.mocked(execDockerRaw);
|
||||||
|
const mockedOpenBoundaryFile = vi.mocked(openBoundaryFile);
|
||||||
const DOCKER_SCRIPT_INDEX = 5;
|
const DOCKER_SCRIPT_INDEX = 5;
|
||||||
const DOCKER_FIRST_SCRIPT_ARG_INDEX = 7;
|
const DOCKER_FIRST_SCRIPT_ARG_INDEX = 7;
|
||||||
|
|
||||||
@@ -96,6 +106,7 @@ async function createHostEscapeFixture(stateDir: string) {
|
|||||||
describe("sandbox fs bridge shell compatibility", () => {
|
describe("sandbox fs bridge shell compatibility", () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
mockedExecDockerRaw.mockClear();
|
mockedExecDockerRaw.mockClear();
|
||||||
|
mockedOpenBoundaryFile.mockClear();
|
||||||
installDockerReadMock();
|
installDockerReadMock();
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -211,6 +222,34 @@ describe("sandbox fs bridge shell compatibility", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("allows mkdirp when boundary open reports io for an existing directory", async () => {
|
||||||
|
await withTempDir("openclaw-fs-bridge-mkdirp-io-", async (stateDir) => {
|
||||||
|
const workspaceDir = path.join(stateDir, "workspace");
|
||||||
|
const nestedDir = path.join(workspaceDir, "memory", "kemik");
|
||||||
|
await fs.mkdir(nestedDir, { recursive: true });
|
||||||
|
|
||||||
|
mockedOpenBoundaryFile.mockImplementationOnce(async () => ({
|
||||||
|
ok: false,
|
||||||
|
reason: "io",
|
||||||
|
error: Object.assign(new Error("EISDIR"), { code: "EISDIR" }),
|
||||||
|
}));
|
||||||
|
|
||||||
|
const bridge = createSandboxFsBridge({
|
||||||
|
sandbox: createSandbox({
|
||||||
|
workspaceDir,
|
||||||
|
agentWorkspaceDir: workspaceDir,
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined();
|
||||||
|
|
||||||
|
const mkdirCall = findCallByScriptFragment('mkdir -p -- "$1"');
|
||||||
|
expect(mkdirCall).toBeDefined();
|
||||||
|
const mkdirPath = mkdirCall ? getDockerPathArg(mkdirCall[0]) : "";
|
||||||
|
expect(mkdirPath).toBe("/workspace/memory/kemik");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("rejects mkdirp when target exists as a file", async () => {
|
it("rejects mkdirp when target exists as a file", async () => {
|
||||||
await withTempDir("openclaw-fs-bridge-mkdirp-file-", async (stateDir) => {
|
await withTempDir("openclaw-fs-bridge-mkdirp-file-", async (stateDir) => {
|
||||||
const workspaceDir = path.join(stateDir, "workspace");
|
const workspaceDir = path.join(stateDir, "workspace");
|
||||||
|
|||||||
@@ -267,25 +267,12 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
|||||||
});
|
});
|
||||||
if (!guarded.ok) {
|
if (!guarded.ok) {
|
||||||
if (guarded.reason !== "path") {
|
if (guarded.reason !== "path") {
|
||||||
// mkdirp may legally target an already-existing directory. Keep a
|
// Some platforms cannot open directories via openSync(O_RDONLY), even when
|
||||||
// directory-only fallback so boundary checks remain strict for files
|
// the path is a valid in-boundary directory. Allow mkdirp to proceed in that
|
||||||
// while avoiding false negatives from file-oriented open validation.
|
// narrow case by verifying the host path is an existing directory.
|
||||||
if (options.allowedType === "directory") {
|
const canFallbackToDirectoryStat =
|
||||||
try {
|
options.allowedType === "directory" && this.pathIsExistingDirectory(target.hostPath);
|
||||||
const st = fs.statSync(target.hostPath);
|
if (!canFallbackToDirectoryStat) {
|
||||||
if (!st.isDirectory()) {
|
|
||||||
throw new Error(
|
|
||||||
`Sandbox boundary checks failed; cannot ${options.action}: ${target.containerPath}`,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
} catch {
|
|
||||||
throw guarded.error instanceof Error
|
|
||||||
? guarded.error
|
|
||||||
: new Error(
|
|
||||||
`Sandbox boundary checks failed; cannot ${options.action}: ${target.containerPath}`,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
} else {
|
|
||||||
throw guarded.error instanceof Error
|
throw guarded.error instanceof Error
|
||||||
? guarded.error
|
? guarded.error
|
||||||
: new Error(
|
: new Error(
|
||||||
@@ -314,6 +301,14 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private pathIsExistingDirectory(hostPath: string): boolean {
|
||||||
|
try {
|
||||||
|
return fs.statSync(hostPath).isDirectory();
|
||||||
|
} catch {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private resolveMountByContainerPath(containerPath: string): SandboxFsMount | null {
|
private resolveMountByContainerPath(containerPath: string): SandboxFsMount | null {
|
||||||
const normalized = normalizeContainerPath(containerPath);
|
const normalized = normalizeContainerPath(containerPath);
|
||||||
for (const mount of this.mountsByContainer) {
|
for (const mount of this.mountsByContainer) {
|
||||||
|
|||||||
Reference in New Issue
Block a user