mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix(file-transfer): reject oversized dir.fetch preflights (#106293)
* fix(file-transfer): reject oversized dir fetch preflights * test(file-transfer): cover real oversized dir preflight * test(file-transfer): wrap oversized preflight fixture * test(file-transfer): cover preflight du fallback * ci: restore max-lines baseline entry * fix(file-transfer): make dir preflight size check authoritative * ci: drop stale max-lines baseline restore * test(file-transfer): gate tar-backed preflight cases * fix(file-transfer): preserve preflight bounds and errors --------- Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
This commit is contained in:
co-authored by
Omar Shahine
parent
e9fc23ad1f
commit
7440a1ae83
@@ -54,6 +54,77 @@ describe("dir.fetch process wrapper", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("uses capped tar output for preflight-only requests without returning the archive", async () => {
|
||||
runCommandBufferedMock.mockResolvedValueOnce(commandResult({ stdout: Buffer.from("archive") }));
|
||||
|
||||
await expect(
|
||||
handleDirFetch({ path: tmpRoot, maxBytes: 1024, preflightOnly: true }),
|
||||
).resolves.toMatchObject({
|
||||
ok: true,
|
||||
entries: ["ok.txt"],
|
||||
fileCount: 1,
|
||||
preflightOnly: true,
|
||||
});
|
||||
expect(runCommandBufferedMock).toHaveBeenCalledOnce();
|
||||
expect(runCommandBufferedMock).toHaveBeenCalledWith(
|
||||
[process.platform !== "win32" ? "/usr/bin/tar" : "tar", "-czf", "-", "-C", tmpRoot, "."],
|
||||
expect.objectContaining({
|
||||
discardOutput: { stderr: true },
|
||||
maxOutputBytes: { stdout: 1024, stderr: 64 * 1024 },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects oversized preflight-only requests using the encoded tar limit", async () => {
|
||||
runCommandBufferedMock.mockResolvedValueOnce(
|
||||
commandResult({
|
||||
code: null,
|
||||
termination: "output-limit",
|
||||
outputLimitStream: "stdout",
|
||||
}),
|
||||
);
|
||||
|
||||
await expect(
|
||||
handleDirFetch({ path: tmpRoot, maxBytes: 1024, preflightOnly: true }),
|
||||
).resolves.toMatchObject({
|
||||
ok: false,
|
||||
code: "TREE_TOO_LARGE",
|
||||
message: "tarball exceeded 1024 byte limit during preflight",
|
||||
});
|
||||
expect(runCommandBufferedMock).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("rejects preflight-only requests once filesystem listing crosses the entry cap", async () => {
|
||||
await Promise.all(
|
||||
Array.from({ length: 5001 }, (_, index) =>
|
||||
fs.writeFile(path.join(tmpRoot, `file-${index}.txt`), ""),
|
||||
),
|
||||
);
|
||||
|
||||
await expect(
|
||||
handleDirFetch({ path: tmpRoot, maxBytes: 1024, preflightOnly: true }),
|
||||
).resolves.toMatchObject({
|
||||
ok: false,
|
||||
code: "TREE_TOO_LARGE",
|
||||
message: "directory tree exceeds 5000 entries during preflight",
|
||||
});
|
||||
expect(runCommandBufferedMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("preserves filesystem classification when a preflight tar race removes the directory", async () => {
|
||||
runCommandBufferedMock.mockImplementationOnce(async () => {
|
||||
await fs.rm(tmpRoot, { recursive: true, force: true });
|
||||
return commandResult({ code: 1 });
|
||||
});
|
||||
|
||||
await expect(
|
||||
handleDirFetch({ path: tmpRoot, maxBytes: 1024, preflightOnly: true }),
|
||||
).resolves.toMatchObject({
|
||||
ok: false,
|
||||
code: "NOT_FOUND",
|
||||
});
|
||||
});
|
||||
|
||||
it("fails tar entry listing closed on wrapper errors", async () => {
|
||||
runCommandBufferedMock
|
||||
.mockResolvedValueOnce(commandResult({ stdout: Buffer.from("1\tproject\n") }))
|
||||
|
||||
@@ -56,7 +56,7 @@ describe("handleDirFetch — fs errors", () => {
|
||||
});
|
||||
|
||||
describe("handleDirFetch — happy path", () => {
|
||||
it("preflights directory entries without creating a tarball", async () => {
|
||||
it.runIf(HAS_TAR)("preflights directory entries without returning a tarball", async () => {
|
||||
await fs.writeFile(path.join(tmpRoot, "a.txt"), "alpha\n");
|
||||
await fs.mkdir(path.join(tmpRoot, ".ssh"));
|
||||
await fs.writeFile(path.join(tmpRoot, ".ssh", "id_rsa"), "secret\n");
|
||||
@@ -77,6 +77,24 @@ describe("handleDirFetch — happy path", () => {
|
||||
expect(r.fileCount).toBe(r.entries?.length);
|
||||
});
|
||||
|
||||
it.runIf(HAS_TAR && process.platform !== "win32")(
|
||||
"preflights symlinks without following their targets",
|
||||
async () => {
|
||||
await fs.writeFile(path.join(tmpRoot, "a.txt"), "alpha\n");
|
||||
await fs.symlink("missing-target", path.join(tmpRoot, "dangling-link"));
|
||||
|
||||
const r = await handleDirFetch({ path: tmpRoot, preflightOnly: true });
|
||||
if (!r.ok) {
|
||||
throw new Error(`expected ok, got ${r.code}: ${r.message}`);
|
||||
}
|
||||
|
||||
expect(r.tarBase64).toBe("");
|
||||
expect(r.entries).toContain("a.txt");
|
||||
expect(r.entries).toContain("dangling-link");
|
||||
expect(r.fileCount).toBe(r.entries?.length);
|
||||
},
|
||||
);
|
||||
|
||||
it.runIf(HAS_TAR)("returns a gzipped tar with byte count and sha256", async () => {
|
||||
await fs.writeFile(path.join(tmpRoot, "a.txt"), "alpha\n");
|
||||
await fs.writeFile(path.join(tmpRoot, "b.txt"), "beta\n");
|
||||
@@ -113,6 +131,16 @@ describe("handleDirFetch — happy path", () => {
|
||||
});
|
||||
|
||||
describe("handleDirFetch — size cap", () => {
|
||||
it.runIf(HAS_TAR)("returns TREE_TOO_LARGE for oversized preflight-only directories", async () => {
|
||||
const largePath = path.join(tmpRoot, "large.bin");
|
||||
await fs.writeFile(largePath, crypto.randomBytes(1024 * 1024));
|
||||
|
||||
await expectDirFetchError(
|
||||
{ path: tmpRoot, maxBytes: 64 * 1024, preflightOnly: true },
|
||||
"TREE_TOO_LARGE",
|
||||
);
|
||||
});
|
||||
|
||||
it.runIf(HAS_TAR)(
|
||||
"returns TREE_TOO_LARGE when content exceeds the cap mid-stream",
|
||||
async () => {
|
||||
|
||||
@@ -95,11 +95,22 @@ async function listTarEntries(tarBuffer: Buffer): Promise<string[] | null> {
|
||||
if (!result || result.termination !== "exit" || result.code !== 0) {
|
||||
return null;
|
||||
}
|
||||
return result.stdout
|
||||
.toString("utf8")
|
||||
.split("\n")
|
||||
.map((line) => line.replace(/\\/gu, "/").replace(/^\.\//u, "").replace(/\/$/u, ""))
|
||||
.filter((line) => line.length > 0);
|
||||
const entries: string[] = [];
|
||||
const output = result.stdout.toString("utf8");
|
||||
let start = 0;
|
||||
while (start <= output.length) {
|
||||
const end = output.indexOf("\n", start);
|
||||
const rawLine = output.slice(start, end === -1 ? output.length : end);
|
||||
const line = rawLine.replace(/\\/gu, "/").replace(/^\.\//u, "").replace(/\/$/u, "");
|
||||
if (line.length > 0) {
|
||||
entries.push(line);
|
||||
}
|
||||
if (end === -1) {
|
||||
break;
|
||||
}
|
||||
start = end + 1;
|
||||
}
|
||||
return entries.toSorted((left, right) => left.localeCompare(right));
|
||||
}
|
||||
|
||||
type TarArchiveResult = Buffer | "TOO_LARGE" | "TIMEOUT" | "ERROR";
|
||||
@@ -134,8 +145,7 @@ async function listTreeEntries(root: string, maxEntries: number): Promise<string
|
||||
const rootHandle = await fsRoot(root);
|
||||
async function visit(relativeDir: string): Promise<boolean> {
|
||||
const entries = await rootHandle.list(relativeDir, { withFileTypes: true });
|
||||
entries.sort((left, right) => left.name.localeCompare(right.name));
|
||||
for (const entry of entries) {
|
||||
for (const entry of entries.toSorted((left, right) => left.name.localeCompare(right.name))) {
|
||||
const rel = path.posix.join(relativeDir === "." ? "" : relativeDir, entry.name);
|
||||
results.push(rel);
|
||||
if (results.length > maxEntries) {
|
||||
@@ -180,26 +190,9 @@ export async function handleDirFetch(params: DirFetchParams): Promise<DirFetchRe
|
||||
}
|
||||
|
||||
if (preflightOnly) {
|
||||
let entries: string[] | "TOO_MANY";
|
||||
try {
|
||||
const entries = await listTreeEntries(canonical, 5000);
|
||||
if (entries === "TOO_MANY") {
|
||||
return {
|
||||
ok: false,
|
||||
code: "TREE_TOO_LARGE",
|
||||
message: "directory tree exceeds 5000 entries during preflight",
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
path: canonical,
|
||||
tarBase64: "",
|
||||
tarBytes: 0,
|
||||
sha256: "",
|
||||
fileCount: entries.length,
|
||||
entries,
|
||||
preflightOnly: true,
|
||||
};
|
||||
entries = await listTreeEntries(canonical, 5000);
|
||||
} catch (err) {
|
||||
const code = classifyFsError(err);
|
||||
return {
|
||||
@@ -209,6 +202,54 @@ export async function handleDirFetch(params: DirFetchParams): Promise<DirFetchRe
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
if (entries === "TOO_MANY") {
|
||||
return {
|
||||
ok: false,
|
||||
code: "TREE_TOO_LARGE",
|
||||
message: "directory tree exceeds 5000 entries during preflight",
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
|
||||
const tarBuffer = await createTarArchive(canonical, maxBytes);
|
||||
if (tarBuffer === "TOO_LARGE") {
|
||||
return {
|
||||
ok: false,
|
||||
code: "TREE_TOO_LARGE",
|
||||
message: `tarball exceeded ${maxBytes} byte limit during preflight`,
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
if (tarBuffer === "TIMEOUT") {
|
||||
return {
|
||||
ok: false,
|
||||
code: "READ_ERROR",
|
||||
message: "tar command exceeded 60s wall-clock timeout (slow filesystem or symlink loop?)",
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
if (tarBuffer === "ERROR") {
|
||||
const currentDirectory = await statRequiredDirectory(canonical, classifyFsError);
|
||||
if (!currentDirectory.ok) {
|
||||
return currentDirectory;
|
||||
}
|
||||
return {
|
||||
ok: false,
|
||||
code: "READ_ERROR",
|
||||
message: "tar command failed",
|
||||
canonicalPath: canonical,
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
path: canonical,
|
||||
tarBase64: "",
|
||||
tarBytes: 0,
|
||||
sha256: "",
|
||||
fileCount: entries.length,
|
||||
entries,
|
||||
preflightOnly: true,
|
||||
};
|
||||
}
|
||||
|
||||
// Preflight size check using du
|
||||
|
||||
Reference in New Issue
Block a user