mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 15:20:44 +00:00
fix(security): prevent workspace PATH injection via service env and trash helpers (#73264)
* fix: address issue * fix: address PR review feedback * fix: address review-pr skill feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address build feedback * fix: address PR review feedback * docs: add changelog entry for PR merge
This commit is contained in:
Pavan Kumar Gondhi
committed by
GitHub
GitHub
parent
b79e617ad1
commit
230f7122dd
@@ -8,36 +8,197 @@ vi.mock("../process/exec.js", () => ({
|
||||
runExec,
|
||||
}));
|
||||
|
||||
function mockTrashContainer(...suffixes: string[]) {
|
||||
let call = 0;
|
||||
return vi.spyOn(fs, "mkdtempSync").mockImplementation((prefix) => {
|
||||
const suffix = suffixes[call] ?? "secure";
|
||||
call += 1;
|
||||
return `${prefix}${suffix}`;
|
||||
});
|
||||
}
|
||||
|
||||
describe("browser trash", () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
runExec.mockReset();
|
||||
vi.spyOn(Date, "now").mockReturnValue(123);
|
||||
vi.spyOn(os, "homedir").mockReturnValue("/home/test");
|
||||
vi.spyOn(os, "tmpdir").mockReturnValue("/tmp");
|
||||
vi.spyOn(fs, "lstatSync").mockReturnValue({
|
||||
isDirectory: () => true,
|
||||
isSymbolicLink: () => false,
|
||||
} as fs.Stats);
|
||||
vi.spyOn(fs.realpathSync, "native").mockImplementation((candidate) => String(candidate));
|
||||
});
|
||||
|
||||
it("returns the target path when trash exits successfully", async () => {
|
||||
it("moves paths to a reserved user trash container without invoking a PATH-resolved command", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
runExec.mockResolvedValue(undefined);
|
||||
const mkdirSync = vi.spyOn(fs, "mkdirSync");
|
||||
const renameSync = vi.spyOn(fs, "renameSync");
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe("/tmp/demo");
|
||||
expect(runExec).toHaveBeenCalledWith("trash", ["/tmp/demo"], { timeoutMs: 10_000 });
|
||||
expect(mkdirSync).not.toHaveBeenCalled();
|
||||
expect(renameSync).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("falls back to rename when trash exits non-zero", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
runExec.mockRejectedValue(new Error("permission denied"));
|
||||
const mkdirSync = vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
const existsSync = vi.spyOn(fs, "existsSync").mockReturnValue(false);
|
||||
const mkdtempSync = mockTrashContainer("secure");
|
||||
const renameSync = vi.spyOn(fs, "renameSync").mockImplementation(() => undefined);
|
||||
const cpSync = vi.spyOn(fs, "cpSync");
|
||||
const rmSync = vi.spyOn(fs, "rmSync");
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe(
|
||||
"/home/test/.Trash/demo-123-secure/demo",
|
||||
);
|
||||
expect(runExec).not.toHaveBeenCalled();
|
||||
expect(mkdirSync).toHaveBeenCalledWith("/home/test/.Trash", {
|
||||
recursive: true,
|
||||
mode: 0o700,
|
||||
});
|
||||
expect(mkdtempSync).toHaveBeenCalledWith("/home/test/.Trash/demo-123-");
|
||||
expect(renameSync).toHaveBeenCalledWith("/tmp/demo", "/home/test/.Trash/demo-123-secure/demo");
|
||||
expect(cpSync).not.toHaveBeenCalled();
|
||||
expect(rmSync).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("uses the resolved trash directory for reserved destinations", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
vi.spyOn(fs.realpathSync, "native").mockImplementation((candidate) => {
|
||||
const value = String(candidate);
|
||||
if (value === "/home/test") {
|
||||
return "/real/home/test";
|
||||
}
|
||||
if (value === "/home/test/.Trash") {
|
||||
return "/real/home/test/.Trash";
|
||||
}
|
||||
return value;
|
||||
});
|
||||
const mkdtempSync = mockTrashContainer("secure");
|
||||
const renameSync = vi.spyOn(fs, "renameSync").mockImplementation(() => undefined);
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe("/home/test/.Trash/demo-123");
|
||||
expect(mkdirSync).toHaveBeenCalledWith("/home/test/.Trash", { recursive: true });
|
||||
expect(existsSync).toHaveBeenCalledWith("/home/test/.Trash/demo-123");
|
||||
expect(renameSync).toHaveBeenCalledWith("/tmp/demo", "/home/test/.Trash/demo-123");
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe(
|
||||
"/real/home/test/.Trash/demo-123-secure/demo",
|
||||
);
|
||||
expect(mkdtempSync).toHaveBeenCalledWith("/real/home/test/.Trash/demo-123-");
|
||||
expect(renameSync).toHaveBeenCalledWith(
|
||||
"/tmp/demo",
|
||||
"/real/home/test/.Trash/demo-123-secure/demo",
|
||||
);
|
||||
});
|
||||
|
||||
it("refuses to trash filesystem roots", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
|
||||
await expect(movePathToTrash("/")).rejects.toThrow("Refusing to trash root path");
|
||||
});
|
||||
|
||||
it("refuses to trash paths outside allowed roots", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
|
||||
await expect(movePathToTrash("/etc/openclaw-demo")).rejects.toThrow(
|
||||
"Refusing to trash path outside allowed roots",
|
||||
);
|
||||
});
|
||||
|
||||
it("refuses to use a symlinked trash directory", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
vi.spyOn(fs, "lstatSync").mockReturnValue({
|
||||
isDirectory: () => true,
|
||||
isSymbolicLink: () => true,
|
||||
} as fs.Stats);
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).rejects.toThrow(
|
||||
"Refusing to use non-directory/symlink trash directory",
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to copy and remove when rename crosses filesystems", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
const exdev = Object.assign(new Error("cross-device"), { code: "EXDEV" });
|
||||
vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
mockTrashContainer("secure");
|
||||
vi.spyOn(fs, "renameSync").mockImplementation(() => {
|
||||
throw exdev;
|
||||
});
|
||||
const cpSync = vi.spyOn(fs, "cpSync").mockImplementation(() => undefined);
|
||||
const rmSync = vi.spyOn(fs, "rmSync").mockImplementation(() => undefined);
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe(
|
||||
"/home/test/.Trash/demo-123-secure/demo",
|
||||
);
|
||||
expect(cpSync).toHaveBeenCalledWith("/tmp/demo", "/home/test/.Trash/demo-123-secure/demo", {
|
||||
recursive: true,
|
||||
force: false,
|
||||
errorOnExist: true,
|
||||
});
|
||||
expect(rmSync).toHaveBeenCalledWith("/tmp/demo", { recursive: true, force: false });
|
||||
});
|
||||
|
||||
it("retries copy fallback when the copy destination is created concurrently", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
const exdev = Object.assign(new Error("cross-device"), { code: "EXDEV" });
|
||||
const copyCollision = Object.assign(new Error("copy exists"), {
|
||||
code: "ERR_FS_CP_EEXIST",
|
||||
});
|
||||
vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
mockTrashContainer("first", "second");
|
||||
vi.spyOn(fs, "renameSync").mockImplementation(() => {
|
||||
throw exdev;
|
||||
});
|
||||
const cpSync = vi
|
||||
.spyOn(fs, "cpSync")
|
||||
.mockImplementationOnce(() => {
|
||||
throw copyCollision;
|
||||
})
|
||||
.mockImplementation(() => undefined);
|
||||
const rmSync = vi.spyOn(fs, "rmSync").mockImplementation(() => undefined);
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe(
|
||||
"/home/test/.Trash/demo-123-second/demo",
|
||||
);
|
||||
expect(cpSync).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
"/tmp/demo",
|
||||
"/home/test/.Trash/demo-123-first/demo",
|
||||
{
|
||||
recursive: true,
|
||||
force: false,
|
||||
errorOnExist: true,
|
||||
},
|
||||
);
|
||||
expect(cpSync).toHaveBeenNthCalledWith(
|
||||
2,
|
||||
"/tmp/demo",
|
||||
"/home/test/.Trash/demo-123-second/demo",
|
||||
{
|
||||
recursive: true,
|
||||
force: false,
|
||||
errorOnExist: true,
|
||||
},
|
||||
);
|
||||
expect(rmSync).toHaveBeenCalledTimes(1);
|
||||
expect(Date.now).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("retries with the same timestamp when the destination is created concurrently", async () => {
|
||||
const { movePathToTrash } = await import("./trash.js");
|
||||
const collision = Object.assign(new Error("exists"), { code: "EEXIST" });
|
||||
vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined);
|
||||
mockTrashContainer("first", "second");
|
||||
const renameSync = vi
|
||||
.spyOn(fs, "renameSync")
|
||||
.mockImplementationOnce(() => {
|
||||
throw collision;
|
||||
})
|
||||
.mockImplementation(() => undefined);
|
||||
|
||||
await expect(movePathToTrash("/tmp/demo")).resolves.toBe(
|
||||
"/home/test/.Trash/demo-123-second/demo",
|
||||
);
|
||||
expect(renameSync).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
"/tmp/demo",
|
||||
"/home/test/.Trash/demo-123-first/demo",
|
||||
);
|
||||
expect(renameSync).toHaveBeenNthCalledWith(
|
||||
2,
|
||||
"/tmp/demo",
|
||||
"/home/test/.Trash/demo-123-second/demo",
|
||||
);
|
||||
expect(Date.now).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,22 +1,141 @@
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { generateSecureToken } from "../infra/secure-random.js";
|
||||
import { runExec } from "../process/exec.js";
|
||||
|
||||
export async function movePathToTrash(targetPath: string): Promise<string> {
|
||||
try {
|
||||
await runExec("trash", [targetPath], { timeoutMs: 10_000 });
|
||||
return targetPath;
|
||||
} catch {
|
||||
const trashDir = path.join(os.homedir(), ".Trash");
|
||||
fs.mkdirSync(trashDir, { recursive: true });
|
||||
const base = path.basename(targetPath);
|
||||
let dest = path.join(trashDir, `${base}-${Date.now()}`);
|
||||
if (fs.existsSync(dest)) {
|
||||
dest = path.join(trashDir, `${base}-${Date.now()}-${generateSecureToken(6)}`);
|
||||
const TRASH_DESTINATION_COLLISION_CODES = new Set(["EEXIST", "ENOTEMPTY", "ERR_FS_CP_EEXIST"]);
|
||||
const TRASH_DESTINATION_RETRY_LIMIT = 4;
|
||||
|
||||
function getFsErrorCode(error: unknown): string | undefined {
|
||||
if (!error || typeof error !== "object" || !("code" in error)) {
|
||||
return undefined;
|
||||
}
|
||||
const code = (error as NodeJS.ErrnoException).code;
|
||||
return typeof code === "string" ? code : undefined;
|
||||
}
|
||||
|
||||
function isTrashDestinationCollision(error: unknown): boolean {
|
||||
const code = getFsErrorCode(error);
|
||||
return Boolean(code && TRASH_DESTINATION_COLLISION_CODES.has(code));
|
||||
}
|
||||
|
||||
function isSameOrChildPath(candidate: string, parent: string): boolean {
|
||||
return candidate === parent || candidate.startsWith(`${parent}${path.sep}`);
|
||||
}
|
||||
|
||||
function resolveAllowedTrashRoots(): string[] {
|
||||
const roots = [os.homedir(), os.tmpdir()].map((root) => {
|
||||
try {
|
||||
return path.resolve(fs.realpathSync.native(root));
|
||||
} catch {
|
||||
return path.resolve(root);
|
||||
}
|
||||
fs.renameSync(targetPath, dest);
|
||||
return dest;
|
||||
});
|
||||
return [...new Set(roots)];
|
||||
}
|
||||
|
||||
function assertAllowedTrashTarget(targetPath: string): void {
|
||||
let resolvedTargetPath = path.resolve(targetPath);
|
||||
try {
|
||||
resolvedTargetPath = path.resolve(fs.realpathSync.native(targetPath));
|
||||
} catch {
|
||||
// The subsequent move will surface missing or inaccessible targets.
|
||||
}
|
||||
const isAllowed = resolveAllowedTrashRoots().some(
|
||||
(root) => resolvedTargetPath !== root && isSameOrChildPath(resolvedTargetPath, root),
|
||||
);
|
||||
if (!isAllowed) {
|
||||
throw new Error(`Refusing to trash path outside allowed roots: ${targetPath}`);
|
||||
}
|
||||
}
|
||||
|
||||
function resolveTrashDir(): string {
|
||||
const homeDir = os.homedir();
|
||||
const trashDir = path.join(homeDir, ".Trash");
|
||||
fs.mkdirSync(trashDir, { recursive: true, mode: 0o700 });
|
||||
const trashDirStat = fs.lstatSync(trashDir);
|
||||
if (!trashDirStat.isDirectory() || trashDirStat.isSymbolicLink()) {
|
||||
throw new Error(`Refusing to use non-directory/symlink trash directory: ${trashDir}`);
|
||||
}
|
||||
const realHome = path.resolve(fs.realpathSync.native(homeDir));
|
||||
const resolvedTrashDir = path.resolve(fs.realpathSync.native(trashDir));
|
||||
if (resolvedTrashDir === realHome || !isSameOrChildPath(resolvedTrashDir, realHome)) {
|
||||
throw new Error(`Trash directory escaped home directory: ${trashDir}`);
|
||||
}
|
||||
return resolvedTrashDir;
|
||||
}
|
||||
|
||||
function trashBaseName(targetPath: string): string {
|
||||
const resolvedTargetPath = path.resolve(targetPath);
|
||||
if (resolvedTargetPath === path.parse(resolvedTargetPath).root) {
|
||||
throw new Error(`Refusing to trash root path: ${targetPath}`);
|
||||
}
|
||||
const base = path.basename(resolvedTargetPath).replace(/[\\/]+/g, "");
|
||||
if (!base) {
|
||||
throw new Error(`Unable to derive safe trash basename for: ${targetPath}`);
|
||||
}
|
||||
return base;
|
||||
}
|
||||
|
||||
function resolveContainedPath(root: string, leaf: string): string {
|
||||
const resolvedRoot = path.resolve(root);
|
||||
const resolvedPath = path.resolve(resolvedRoot, leaf);
|
||||
if (!isSameOrChildPath(resolvedPath, resolvedRoot) || resolvedPath === resolvedRoot) {
|
||||
throw new Error(`Trash destination escaped trash directory: ${resolvedPath}`);
|
||||
}
|
||||
return resolvedPath;
|
||||
}
|
||||
|
||||
function reserveTrashDestination(trashDir: string, base: string, timestamp: number): string {
|
||||
const containerPrefix = resolveContainedPath(trashDir, `${base}-${timestamp}-`);
|
||||
const container = fs.mkdtempSync(containerPrefix);
|
||||
const resolvedContainer = path.resolve(container);
|
||||
const resolvedTrashDir = path.resolve(trashDir);
|
||||
if (
|
||||
resolvedContainer === resolvedTrashDir ||
|
||||
!isSameOrChildPath(resolvedContainer, resolvedTrashDir)
|
||||
) {
|
||||
throw new Error(`Trash destination escaped trash directory: ${container}`);
|
||||
}
|
||||
return resolveContainedPath(container, base);
|
||||
}
|
||||
|
||||
function movePathToDestination(targetPath: string, dest: string): boolean {
|
||||
try {
|
||||
fs.renameSync(targetPath, dest);
|
||||
return true;
|
||||
} catch (error) {
|
||||
if (getFsErrorCode(error) !== "EXDEV") {
|
||||
if (isTrashDestinationCollision(error)) {
|
||||
return false;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
fs.cpSync(targetPath, dest, { recursive: true, force: false, errorOnExist: true });
|
||||
fs.rmSync(targetPath, { recursive: true, force: false });
|
||||
return true;
|
||||
} catch (error) {
|
||||
if (isTrashDestinationCollision(error)) {
|
||||
return false;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
export async function movePathToTrash(targetPath: string): Promise<string> {
|
||||
// Avoid resolving external trash helpers through the service PATH during cleanup.
|
||||
const base = trashBaseName(targetPath);
|
||||
assertAllowedTrashTarget(targetPath);
|
||||
const trashDir = resolveTrashDir();
|
||||
const timestamp = Date.now();
|
||||
for (let attempt = 0; attempt < TRASH_DESTINATION_RETRY_LIMIT; attempt += 1) {
|
||||
const dest = reserveTrashDestination(trashDir, base, timestamp);
|
||||
if (movePathToDestination(targetPath, dest)) {
|
||||
return dest;
|
||||
}
|
||||
}
|
||||
|
||||
throw new Error(`Unable to choose a unique trash destination for ${targetPath}`);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user