mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix(file-transfer): audit dir fetch archive size (#104536)
* fix(file-transfer): audit dir fetch archive size * test(file-transfer): guard dir fetch audit archive test on Windows * test(file-transfer): format dir fetch audit guard * fix(file-transfer): verify directory archive audit metadata --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
18e8713b99
commit
9000c9bfe3
@@ -1,3 +1,4 @@
|
||||
import crypto from "node:crypto";
|
||||
// File Transfer tests cover archive-policy child-output failures.
|
||||
import { EventEmitter } from "node:events";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
@@ -64,11 +65,17 @@ describe("dir.fetch archive policy output lifecycle", () => {
|
||||
});
|
||||
const { testing } = await importWithSpawn(spawnMock);
|
||||
|
||||
const archive = Buffer.from("archive");
|
||||
await expect(
|
||||
testing.listDirFetchArchiveEntries({
|
||||
tarBase64: Buffer.from("archive").toString("base64"),
|
||||
tarBase64: archive.toString("base64"),
|
||||
}),
|
||||
).resolves.toEqual({ ok: true, entries: ["ok.txt"] });
|
||||
).resolves.toEqual({
|
||||
ok: true,
|
||||
entries: ["ok.txt"],
|
||||
sizeBytes: archive.byteLength,
|
||||
sha256: crypto.createHash("sha256").update(archive).digest("hex"),
|
||||
});
|
||||
});
|
||||
|
||||
it("surfaces UTF-16 safe tar stderr tail when archive listing fails with emoji at projection boundary", async () => {
|
||||
|
||||
@@ -1,9 +1,11 @@
|
||||
// File Transfer tests cover node invoke policy plugin behavior.
|
||||
import crypto from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import { gzipSync } from "node:zlib";
|
||||
import type { OpenClawPluginNodeInvokePolicyContext } from "openclaw/plugin-sdk/plugin-entry";
|
||||
import { afterAll, afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { createFileTransferNodeInvokePolicy } from "./node-invoke-policy.js";
|
||||
import { appendFileTransferAudit } from "./audit.js";
|
||||
import { createFileTransferNodeInvokePolicy, testing } from "./node-invoke-policy.js";
|
||||
|
||||
vi.mock("./audit.js", () => ({
|
||||
appendFileTransferAudit: vi.fn(async () => undefined),
|
||||
@@ -45,6 +47,14 @@ function tarEntries(entries: Record<string, string>): string {
|
||||
return gzipSync(Buffer.concat(blocks)).toString("base64");
|
||||
}
|
||||
|
||||
function archiveMetadata(tarBase64: string): { tarBytes: number; sha256: string } {
|
||||
const buffer = Buffer.from(tarBase64, "base64");
|
||||
return {
|
||||
tarBytes: buffer.byteLength,
|
||||
sha256: crypto.createHash("sha256").update(buffer).digest("hex"),
|
||||
};
|
||||
}
|
||||
|
||||
function writeTarString(header: Buffer, offset: number, length: number, value: string): void {
|
||||
header.write(value.slice(0, length), offset, length, "utf8");
|
||||
}
|
||||
@@ -145,6 +155,13 @@ function requireInvokeParams(
|
||||
}
|
||||
|
||||
describe("file-transfer node invoke policy", () => {
|
||||
it("maps only transfer payload sizes into audit records", () => {
|
||||
expect(testing.readAuditSizeBytes("file.fetch", { size: 3 })).toBe(3);
|
||||
expect(testing.readAuditSizeBytes("file.write", { size: 4 })).toBe(4);
|
||||
expect(testing.readAuditSizeBytes("dir.fetch", { tarBytes: 999 }, 5)).toBe(5);
|
||||
expect(testing.readAuditSizeBytes("dir.list", { size: 6 })).toBeUndefined();
|
||||
});
|
||||
|
||||
it("injects policy-owned limits before invoking the node", async () => {
|
||||
const policy = createFileTransferNodeInvokePolicy();
|
||||
const { ctx, invokeNode } = createCtx({
|
||||
@@ -610,8 +627,7 @@ describe("file-transfer node invoke policy", () => {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
tarBase64,
|
||||
tarBytes: 7,
|
||||
sha256: "c".repeat(64),
|
||||
...archiveMetadata(tarBase64),
|
||||
fileCount: 2,
|
||||
entries: ["a.txt", "sub/b.txt"],
|
||||
},
|
||||
@@ -629,6 +645,97 @@ describe("file-transfer node invoke policy", () => {
|
||||
},
|
||||
);
|
||||
|
||||
testUnlessWindows("audits dir.fetch archive bytes after a successful transfer", async () => {
|
||||
const policy = createFileTransferNodeInvokePolicy();
|
||||
const tarBase64 = tarEntries({
|
||||
"a.txt": "a",
|
||||
});
|
||||
const { tarBytes, sha256 } = archiveMetadata(tarBase64);
|
||||
const { ctx } = createCtx({
|
||||
command: "dir.fetch",
|
||||
params: { path: "/tmp/project" },
|
||||
});
|
||||
const invokeNode = vi.mocked(ctx.invokeNode);
|
||||
invokeNode
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
payload: {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
entries: ["a.txt"],
|
||||
fileCount: 1,
|
||||
preflightOnly: true,
|
||||
},
|
||||
})
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
payload: {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
tarBase64,
|
||||
tarBytes,
|
||||
sha256,
|
||||
fileCount: 1,
|
||||
},
|
||||
});
|
||||
|
||||
const result = await policy.handle(ctx);
|
||||
|
||||
expectResultFields(result, { ok: true });
|
||||
expect(appendFileTransferAudit).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({
|
||||
op: "dir.fetch",
|
||||
requestedPath: "/tmp/project",
|
||||
canonicalPath: "/tmp/project",
|
||||
decision: "allowed",
|
||||
sizeBytes: tarBytes,
|
||||
sha256,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
testUnlessWindows("rejects mismatched dir.fetch archive integrity metadata", async () => {
|
||||
const policy = createFileTransferNodeInvokePolicy();
|
||||
const tarBase64 = tarEntries({ "a.txt": "a" });
|
||||
const { ctx, invokeNode } = createCtx({
|
||||
command: "dir.fetch",
|
||||
params: { path: "/tmp/project" },
|
||||
});
|
||||
invokeNode
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
payload: {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
entries: ["a.txt"],
|
||||
fileCount: 1,
|
||||
preflightOnly: true,
|
||||
},
|
||||
})
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
payload: {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
tarBase64,
|
||||
tarBytes: 1,
|
||||
sha256: "c".repeat(64),
|
||||
fileCount: 1,
|
||||
},
|
||||
});
|
||||
|
||||
const result = await policy.handle(ctx);
|
||||
|
||||
expectResultFields(result, { ok: false, code: "ARCHIVE_SIZE_MISMATCH" });
|
||||
expect(appendFileTransferAudit).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({
|
||||
op: "dir.fetch",
|
||||
decision: "error",
|
||||
errorCode: "ARCHIVE_SIZE_MISMATCH",
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
testUnlessWindows(
|
||||
"checks final dir.fetch archive entries before returning the archive",
|
||||
async () => {
|
||||
@@ -666,8 +773,7 @@ describe("file-transfer node invoke policy", () => {
|
||||
ok: true,
|
||||
path: "/home/me",
|
||||
tarBase64,
|
||||
tarBytes: 7,
|
||||
sha256: "c".repeat(64),
|
||||
...archiveMetadata(tarBase64),
|
||||
fileCount: 2,
|
||||
},
|
||||
});
|
||||
@@ -715,8 +821,7 @@ describe("file-transfer node invoke policy", () => {
|
||||
ok: true,
|
||||
path: "/tmp/project",
|
||||
tarBase64,
|
||||
tarBytes: 7,
|
||||
sha256: "c".repeat(64),
|
||||
...archiveMetadata(tarBase64),
|
||||
fileCount: 5001,
|
||||
},
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
// File Transfer plugin module implements node invoke policy behavior.
|
||||
import { spawn } from "node:child_process";
|
||||
import crypto from "node:crypto";
|
||||
import { readPositiveIntegerParam } from "openclaw/plugin-sdk/param-readers";
|
||||
import type {
|
||||
OpenClawPluginNodeInvokePolicy,
|
||||
@@ -272,6 +273,20 @@ function readResultPayload(result: { payload?: unknown }): Record<string, unknow
|
||||
: null;
|
||||
}
|
||||
|
||||
function readAuditSizeBytes(
|
||||
command: FileTransferCommand,
|
||||
payload: Record<string, unknown> | null,
|
||||
verifiedDirFetchBytes?: number,
|
||||
): number | undefined {
|
||||
if (command === "dir.fetch") {
|
||||
return verifiedDirFetchBytes;
|
||||
}
|
||||
if (command === "dir.list") {
|
||||
return undefined;
|
||||
}
|
||||
return typeof payload?.size === "number" ? payload.size : undefined;
|
||||
}
|
||||
|
||||
function joinRemotePolicyPath(root: string, relPath: string): string {
|
||||
const rel = relPath.replace(/\\/gu, "/").replace(/^\.\//u, "");
|
||||
if (!rel || rel === ".") {
|
||||
@@ -309,7 +324,10 @@ function normalizeTarEntryPath(entry: string): string | null {
|
||||
|
||||
async function listDirFetchArchiveEntries(
|
||||
payload: Record<string, unknown> | null,
|
||||
): Promise<{ ok: true; entries: string[] } | { ok: false; code: string; reason: string }> {
|
||||
): Promise<
|
||||
| { ok: true; entries: string[]; sizeBytes: number; sha256: string }
|
||||
| { ok: false; code: string; reason: string }
|
||||
> {
|
||||
const tarBase64 = typeof payload?.tarBase64 === "string" ? payload.tarBase64 : "";
|
||||
if (!tarBase64) {
|
||||
return {
|
||||
@@ -319,8 +337,25 @@ async function listDirFetchArchiveEntries(
|
||||
};
|
||||
}
|
||||
const tarBuffer = Buffer.from(tarBase64, "base64");
|
||||
const sizeBytes = tarBuffer.byteLength;
|
||||
if (typeof payload?.tarBytes === "number" && payload.tarBytes !== sizeBytes) {
|
||||
return {
|
||||
ok: false,
|
||||
code: "ARCHIVE_SIZE_MISMATCH",
|
||||
reason: `dir.fetch archive size mismatch: payload says ${payload.tarBytes} bytes, decoded ${sizeBytes}`,
|
||||
};
|
||||
}
|
||||
const sha256 = crypto.createHash("sha256").update(tarBuffer).digest("hex");
|
||||
if (typeof payload?.sha256 === "string" && payload.sha256.toLowerCase() !== sha256) {
|
||||
return {
|
||||
ok: false,
|
||||
code: "ARCHIVE_INTEGRITY_FAILURE",
|
||||
reason: `dir.fetch archive sha256 mismatch: payload says ${payload.sha256.toLowerCase()}, decoded ${sha256}`,
|
||||
};
|
||||
}
|
||||
return await new Promise<
|
||||
{ ok: true; entries: string[] } | { ok: false; code: string; reason: string }
|
||||
| { ok: true; entries: string[]; sizeBytes: number; sha256: string }
|
||||
| { ok: false; code: string; reason: string }
|
||||
>((resolve) => {
|
||||
const tarBin = process.platform !== "win32" ? "/usr/bin/tar" : "tar";
|
||||
const child = spawn(tarBin, ["-tzf", "-"], { stdio: ["pipe", "pipe", "pipe"] });
|
||||
@@ -330,7 +365,9 @@ async function listDirFetchArchiveEntries(
|
||||
let stderr = "";
|
||||
let settled = false;
|
||||
const finish = (
|
||||
result: { ok: true; entries: string[] } | { ok: false; code: string; reason: string },
|
||||
result:
|
||||
| { ok: true; entries: string[]; sizeBytes: number; sha256: string }
|
||||
| { ok: false; code: string; reason: string },
|
||||
): void => {
|
||||
if (settled) {
|
||||
return;
|
||||
@@ -430,7 +467,7 @@ async function listDirFetchArchiveEntries(
|
||||
return;
|
||||
}
|
||||
}
|
||||
finish({ ok: true, entries });
|
||||
finish({ ok: true, entries, sizeBytes, sha256 });
|
||||
});
|
||||
child.on("error", (error) => {
|
||||
finish({
|
||||
@@ -902,6 +939,7 @@ async function handleFileTransferInvoke(
|
||||
};
|
||||
}
|
||||
}
|
||||
let verifiedDirFetchArchive: { sizeBytes: number; sha256: string } | undefined;
|
||||
if (command === "dir.fetch") {
|
||||
const archiveEntries = await listDirFetchArchiveEntries(payload);
|
||||
if (!archiveEntries.ok) {
|
||||
@@ -935,6 +973,10 @@ async function handleFileTransferInvoke(
|
||||
if (archiveDeny) {
|
||||
return archiveDeny;
|
||||
}
|
||||
verifiedDirFetchArchive = {
|
||||
sizeBytes: archiveEntries.sizeBytes,
|
||||
sha256: archiveEntries.sha256,
|
||||
};
|
||||
}
|
||||
|
||||
await appendFileTransferAudit({
|
||||
@@ -944,8 +986,13 @@ async function handleFileTransferInvoke(
|
||||
requestedPath,
|
||||
canonicalPath,
|
||||
decision: "allowed",
|
||||
sizeBytes: typeof payload?.size === "number" ? payload.size : undefined,
|
||||
sha256: typeof payload?.sha256 === "string" ? payload.sha256 : undefined,
|
||||
sizeBytes: readAuditSizeBytes(command, payload, verifiedDirFetchArchive?.sizeBytes),
|
||||
sha256:
|
||||
command === "dir.fetch"
|
||||
? verifiedDirFetchArchive?.sha256
|
||||
: typeof payload?.sha256 === "string"
|
||||
? payload.sha256
|
||||
: undefined,
|
||||
durationMs: Date.now() - startedAt,
|
||||
});
|
||||
|
||||
@@ -961,4 +1008,5 @@ export function createFileTransferNodeInvokePolicy(): OpenClawPluginNodeInvokePo
|
||||
|
||||
export const testing = {
|
||||
listDirFetchArchiveEntries,
|
||||
readAuditSizeBytes,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user