mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix(file-transfer): reject oversized inline file writes before node dispatch (#104556)
* fix(file-transfer): cap inline file write payloads * fix(file-transfer): validate inline base64 before decoding --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
f348b291bc
commit
fdf5812817
@@ -3,7 +3,7 @@ import crypto from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { handleFileWrite } from "./file-write.js";
|
||||
|
||||
let tmpRoot: string;
|
||||
@@ -361,6 +361,19 @@ describe("handleFileWrite — base64 round-trip validation", () => {
|
||||
const r = await handleFileWrite({ path: target, contentBase64: "-_8=" });
|
||||
expect(r.ok).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects whitespace and control characters before decoding", async () => {
|
||||
const bufferFrom = vi.spyOn(Buffer, "from");
|
||||
const target = path.join(tmpRoot, "whitespace.bin");
|
||||
const r = await handleFileWrite({
|
||||
path: target,
|
||||
contentBase64: " \n\t".repeat(1024 * 1024),
|
||||
});
|
||||
|
||||
expectFailure(r, "INVALID_BASE64");
|
||||
expect(bufferFrom).not.toHaveBeenCalled();
|
||||
await expectAccessMissing(target);
|
||||
});
|
||||
});
|
||||
|
||||
describe("handleFileWrite — size cap", () => {
|
||||
|
||||
@@ -8,6 +8,7 @@ import {
|
||||
resolveAbsolutePathForWrite,
|
||||
root,
|
||||
} from "openclaw/plugin-sdk/security-runtime";
|
||||
import { inspectStrictBase64 } from "../shared/base64.js";
|
||||
|
||||
const MAX_CONTENT_BYTES = 16 * 1024 * 1024; // 16 MB
|
||||
|
||||
@@ -104,7 +105,19 @@ export async function handleFileWrite(
|
||||
return err("INVALID_BASE64", "contentBase64 is required");
|
||||
}
|
||||
|
||||
// 2. Decode base64 → Buffer.
|
||||
// 2. Validate the payload and cap its decoded size before allocating a Buffer.
|
||||
const decodedBytes = inspectStrictBase64(contentBase64);
|
||||
if (decodedBytes === undefined) {
|
||||
return err("INVALID_BASE64", "contentBase64 is not valid base64");
|
||||
}
|
||||
if (decodedBytes > MAX_CONTENT_BYTES) {
|
||||
return err(
|
||||
"FILE_TOO_LARGE",
|
||||
`decoded content is ${decodedBytes} bytes; maximum is ${MAX_CONTENT_BYTES} bytes (16 MB)`,
|
||||
);
|
||||
}
|
||||
|
||||
// Decode base64 → Buffer.
|
||||
// Buffer.from(s, "base64") in Node never throws — it silently drops
|
||||
// non-base64 characters and returns whatever it could decode. That
|
||||
// means a typo or truncated input would land garbage on disk if we
|
||||
@@ -121,13 +134,6 @@ export async function handleFileWrite(
|
||||
return err("INVALID_BASE64", "contentBase64 is not valid base64");
|
||||
}
|
||||
|
||||
if (buf.length > MAX_CONTENT_BYTES) {
|
||||
return err(
|
||||
"FILE_TOO_LARGE",
|
||||
`decoded content is ${buf.length} bytes; maximum is ${MAX_CONTENT_BYTES} bytes (16 MB)`,
|
||||
);
|
||||
}
|
||||
|
||||
let targetPath: string;
|
||||
let parentDir: string;
|
||||
let parentExists: boolean;
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
// File Transfer tests cover strict base64 preflight validation.
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { inspectStrictBase64 } from "./base64.js";
|
||||
|
||||
describe("inspectStrictBase64", () => {
|
||||
it.each([
|
||||
["", 0],
|
||||
["aGk=", 2],
|
||||
["aGk", 2],
|
||||
["+/8=", 2],
|
||||
["-_8=", 2],
|
||||
])("accepts canonical padded, unpadded, and base64url input", (value, expected) => {
|
||||
expect(inspectStrictBase64(value)).toBe(expected);
|
||||
});
|
||||
|
||||
it.each(["A", "A===", "=AAA", "AA=A", "AAA@@@", "aG k=", "\tAAA="])(
|
||||
"rejects malformed input without decoding: %j",
|
||||
(value) => {
|
||||
expect(inspectStrictBase64(value)).toBeUndefined();
|
||||
},
|
||||
);
|
||||
});
|
||||
@@ -0,0 +1,51 @@
|
||||
// File Transfer plugin module implements strict base64 preflight validation.
|
||||
|
||||
function isBase64DataChar(code: number): boolean {
|
||||
return (
|
||||
(code >= 0x41 && code <= 0x5a) ||
|
||||
(code >= 0x61 && code <= 0x7a) ||
|
||||
(code >= 0x30 && code <= 0x39) ||
|
||||
code === 0x2b ||
|
||||
code === 0x2f ||
|
||||
code === 0x2d ||
|
||||
code === 0x5f
|
||||
);
|
||||
}
|
||||
|
||||
/** Validates base64 structure and returns its decoded size without allocating a decode buffer. */
|
||||
export function inspectStrictBase64(value: string): number | undefined {
|
||||
let dataChars = 0;
|
||||
let padding = 0;
|
||||
let sawPadding = false;
|
||||
|
||||
for (let index = 0; index < value.length; index += 1) {
|
||||
const code = value.charCodeAt(index);
|
||||
if (code === 0x3d) {
|
||||
padding += 1;
|
||||
if (padding > 2) {
|
||||
return undefined;
|
||||
}
|
||||
sawPadding = true;
|
||||
continue;
|
||||
}
|
||||
if (sawPadding || !isBase64DataChar(code)) {
|
||||
return undefined;
|
||||
}
|
||||
dataChars += 1;
|
||||
}
|
||||
|
||||
if (dataChars === 0) {
|
||||
return padding === 0 ? 0 : undefined;
|
||||
}
|
||||
const remainder = dataChars % 4;
|
||||
if (padding === 0) {
|
||||
return remainder === 1 ? undefined : Math.floor((dataChars * 3) / 4);
|
||||
}
|
||||
if (dataChars + padding < 4 || (dataChars + padding) % 4 !== 0) {
|
||||
return undefined;
|
||||
}
|
||||
if ((padding === 1 && remainder !== 3) || (padding === 2 && remainder !== 2)) {
|
||||
return undefined;
|
||||
}
|
||||
return Math.floor((dataChars * 3) / 4);
|
||||
}
|
||||
@@ -1,6 +1,12 @@
|
||||
// File Transfer tests cover file write tool plugin behavior.
|
||||
import { callGatewayTool } from "openclaw/plugin-sdk/agent-harness-runtime";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
callGatewayTool,
|
||||
listNodes,
|
||||
resolveNodeIdFromList,
|
||||
} from "openclaw/plugin-sdk/agent-harness-runtime";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { humanSize } from "../shared/params.js";
|
||||
import { FILE_WRITE_HARD_MAX_BYTES } from "./descriptors.js";
|
||||
import { createFileWriteTool } from "./file-write-tool.js";
|
||||
|
||||
vi.mock("openclaw/plugin-sdk/agent-harness-runtime", () => ({
|
||||
@@ -13,7 +19,15 @@ vi.mock("openclaw/plugin-sdk/media-store", () => ({
|
||||
readMediaBuffer: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("../shared/audit.js", () => ({
|
||||
appendFileTransferAudit: vi.fn(),
|
||||
}));
|
||||
|
||||
describe("file_write tool", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("rejects malformed inline base64 before invoking the node", async () => {
|
||||
const tool = createFileWriteTool();
|
||||
|
||||
@@ -27,4 +41,64 @@ describe("file_write tool", () => {
|
||||
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects oversized inline base64 before invoking the node", async () => {
|
||||
const tool = createFileWriteTool();
|
||||
const encodedLength = Math.ceil(((FILE_WRITE_HARD_MAX_BYTES + 1) * 4) / 3);
|
||||
const contentBase64 = "A".repeat(encodedLength);
|
||||
|
||||
await expect(
|
||||
tool.execute("tool-call-1", {
|
||||
node: "node-1",
|
||||
path: "/tmp/out.bin",
|
||||
contentBase64,
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
`decoded content is ${FILE_WRITE_HARD_MAX_BYTES + 1} bytes; maximum is ${FILE_WRITE_HARD_MAX_BYTES} bytes (${humanSize(FILE_WRITE_HARD_MAX_BYTES)})`,
|
||||
);
|
||||
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects whitespace-heavy malformed input without decoding or dispatching", async () => {
|
||||
const bufferFrom = vi.spyOn(Buffer, "from");
|
||||
const tool = createFileWriteTool();
|
||||
|
||||
await expect(
|
||||
tool.execute("tool-call-1", {
|
||||
node: "node-1",
|
||||
path: "/tmp/out.bin",
|
||||
contentBase64: " ".repeat(1024 * 1024),
|
||||
}),
|
||||
).rejects.toThrow("contentBase64 is not valid base64");
|
||||
|
||||
expect(bufferFrom).not.toHaveBeenCalled();
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("accepts exactly 16 MiB of inline data", async () => {
|
||||
vi.mocked(listNodes).mockResolvedValue([{ nodeId: "node-1", displayName: "Node 1" }]);
|
||||
vi.mocked(resolveNodeIdFromList).mockReturnValue("node-1");
|
||||
vi.mocked(callGatewayTool).mockResolvedValue({
|
||||
payload: {
|
||||
ok: true,
|
||||
path: "/tmp/out.bin",
|
||||
size: FILE_WRITE_HARD_MAX_BYTES,
|
||||
sha256: "a".repeat(64),
|
||||
overwritten: false,
|
||||
},
|
||||
});
|
||||
const tool = createFileWriteTool();
|
||||
const contentBase64 = Buffer.alloc(FILE_WRITE_HARD_MAX_BYTES).toString("base64");
|
||||
|
||||
await expect(
|
||||
tool.execute("tool-call-1", {
|
||||
node: "node-1",
|
||||
path: "/tmp/out.bin",
|
||||
contentBase64,
|
||||
}),
|
||||
).resolves.toMatchObject({ details: { size: FILE_WRITE_HARD_MAX_BYTES } });
|
||||
|
||||
expect(callGatewayTool).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3,6 +3,7 @@ import crypto from "node:crypto";
|
||||
import type { AnyAgentTool } from "openclaw/plugin-sdk/agent-harness-runtime";
|
||||
import { readMediaBuffer } from "openclaw/plugin-sdk/media-store";
|
||||
import { appendFileTransferAudit } from "../shared/audit.js";
|
||||
import { inspectStrictBase64 } from "../shared/base64.js";
|
||||
import { humanSize, readBoolean } from "../shared/params.js";
|
||||
import {
|
||||
FILE_TRANSFER_SUBDIR,
|
||||
@@ -16,6 +17,15 @@ function normalizeBase64ForCompare(value: string): string {
|
||||
}
|
||||
|
||||
function decodeStrictBase64(value: string): Buffer {
|
||||
const decodedBytes = inspectStrictBase64(value);
|
||||
if (decodedBytes === undefined) {
|
||||
throw new Error("contentBase64 is not valid base64");
|
||||
}
|
||||
if (decodedBytes > FILE_WRITE_HARD_MAX_BYTES) {
|
||||
throw new Error(
|
||||
`decoded content is ${decodedBytes} bytes; maximum is ${FILE_WRITE_HARD_MAX_BYTES} bytes (${humanSize(FILE_WRITE_HARD_MAX_BYTES)})`,
|
||||
);
|
||||
}
|
||||
const buffer = Buffer.from(value, "base64");
|
||||
if (normalizeBase64ForCompare(buffer.toString("base64")) !== normalizeBase64ForCompare(value)) {
|
||||
throw new Error("contentBase64 is not valid base64");
|
||||
|
||||
Reference in New Issue
Block a user