mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-07 20:51:23 +00:00
refactor(security): harden temp-path handling for inbound media
This commit is contained in:
@@ -1,17 +1,16 @@
|
||||
import crypto from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import type { MsgContext } from "../auto-reply/templating.js";
|
||||
import type { MediaUnderstandingAttachmentsConfig } from "../config/types.tools.js";
|
||||
import type { MediaAttachment, MediaUnderstandingCapability } from "./types.js";
|
||||
import { logVerbose, shouldLogVerbose } from "../globals.js";
|
||||
import { isAbortError } from "../infra/unhandled-rejections.js";
|
||||
import { fetchRemoteMedia, MediaFetchError } from "../media/fetch.js";
|
||||
import { detectMime, getFileExtension, isAudioFileName, kindFromMime } from "../media/mime.js";
|
||||
import { buildRandomTempFilePath } from "../plugin-sdk/temp-path.js";
|
||||
import { MediaUnderstandingSkipError } from "./errors.js";
|
||||
import { fetchWithTimeout } from "./providers/shared.js";
|
||||
import type { MediaAttachment, MediaUnderstandingCapability } from "./types.js";
|
||||
|
||||
type MediaBufferResult = {
|
||||
buffer: Buffer;
|
||||
@@ -352,7 +351,10 @@ export class MediaAttachmentCache {
|
||||
timeoutMs: params.timeoutMs,
|
||||
});
|
||||
const extension = path.extname(bufferResult.fileName || "") || "";
|
||||
const tmpPath = path.join(os.tmpdir(), `openclaw-media-${crypto.randomUUID()}${extension}`);
|
||||
const tmpPath = buildRandomTempFilePath({
|
||||
prefix: "openclaw-media",
|
||||
extension,
|
||||
});
|
||||
await fs.writeFile(tmpPath, bufferResult.buffer);
|
||||
entry.tempPath = tmpPath;
|
||||
entry.tempCleanup = async () => {
|
||||
|
||||
@@ -154,7 +154,7 @@ export { extractToolSend } from "./tool-send.js";
|
||||
export { resolveChannelAccountConfigBasePath } from "./config-paths.js";
|
||||
export { chunkTextForOutbound } from "./text-chunking.js";
|
||||
export { readJsonFileWithFallback, writeJsonFileAtomically } from "./json-store.js";
|
||||
export { buildRandomTempFilePath } from "./temp-path.js";
|
||||
export { buildRandomTempFilePath, withTempDownloadPath } from "./temp-path.js";
|
||||
export type { ChatType } from "../channels/chat-type.js";
|
||||
/** @deprecated Use ChatType instead */
|
||||
export type { RoutePeerKind } from "../routing/resolve-route.js";
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { buildRandomTempFilePath } from "./temp-path.js";
|
||||
import { buildRandomTempFilePath, withTempDownloadPath } from "./temp-path.js";
|
||||
|
||||
describe("buildRandomTempFilePath", () => {
|
||||
it("builds deterministic paths when now/uuid are provided", () => {
|
||||
@@ -30,3 +31,41 @@ describe("buildRandomTempFilePath", () => {
|
||||
expect(result).not.toContain("..");
|
||||
});
|
||||
});
|
||||
|
||||
describe("withTempDownloadPath", () => {
|
||||
it("creates a temp path under tmp dir and cleans up the temp directory", async () => {
|
||||
let capturedPath = "";
|
||||
await withTempDownloadPath(
|
||||
{
|
||||
prefix: "line-media",
|
||||
},
|
||||
async (tmpPath) => {
|
||||
capturedPath = tmpPath;
|
||||
await fs.writeFile(tmpPath, "ok");
|
||||
},
|
||||
);
|
||||
|
||||
expect(capturedPath).toContain(path.join(os.tmpdir(), "line-media-"));
|
||||
await expect(fs.stat(capturedPath)).rejects.toMatchObject({ code: "ENOENT" });
|
||||
});
|
||||
|
||||
it("sanitizes prefix and fileName", async () => {
|
||||
let capturedPath = "";
|
||||
await withTempDownloadPath(
|
||||
{
|
||||
prefix: "../../line/../media",
|
||||
fileName: "../../evil.bin",
|
||||
},
|
||||
async (tmpPath) => {
|
||||
capturedPath = tmpPath;
|
||||
},
|
||||
);
|
||||
|
||||
const tmpRoot = path.resolve(os.tmpdir());
|
||||
const resolved = path.resolve(capturedPath);
|
||||
const rel = path.relative(tmpRoot, resolved);
|
||||
expect(rel === ".." || rel.startsWith(`..${path.sep}`)).toBe(false);
|
||||
expect(path.basename(capturedPath)).toBe("evil.bin");
|
||||
expect(capturedPath).not.toContain("..");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import crypto from "node:crypto";
|
||||
import { mkdtemp, rm } from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
@@ -20,6 +21,12 @@ function sanitizeExtension(extension?: string): string {
|
||||
return `.${token}`;
|
||||
}
|
||||
|
||||
function sanitizeFileName(fileName: string): string {
|
||||
const base = path.basename(fileName).replace(/[^a-zA-Z0-9._-]+/g, "-");
|
||||
const normalized = base.replace(/^-+|-+$/g, "");
|
||||
return normalized || "download.bin";
|
||||
}
|
||||
|
||||
export function buildRandomTempFilePath(params: {
|
||||
prefix: string;
|
||||
extension?: string;
|
||||
@@ -37,3 +44,22 @@ export function buildRandomTempFilePath(params: {
|
||||
const uuid = params.uuid?.trim() || crypto.randomUUID();
|
||||
return path.join(params.tmpDir ?? os.tmpdir(), `${prefix}-${now}-${uuid}${extension}`);
|
||||
}
|
||||
|
||||
export async function withTempDownloadPath<T>(
|
||||
params: {
|
||||
prefix: string;
|
||||
fileName?: string;
|
||||
tmpDir?: string;
|
||||
},
|
||||
fn: (tmpPath: string) => Promise<T>,
|
||||
): Promise<T> {
|
||||
const tempRoot = params.tmpDir ?? os.tmpdir();
|
||||
const prefix = `${sanitizePrefix(params.prefix)}-`;
|
||||
const dir = await mkdtemp(path.join(tempRoot, prefix));
|
||||
const tmpPath = path.join(dir, sanitizeFileName(params.fileName ?? "download.bin"));
|
||||
try {
|
||||
return await fn(tmpPath);
|
||||
} finally {
|
||||
await rm(dir, { recursive: true, force: true }).catch(() => {});
|
||||
}
|
||||
}
|
||||
|
||||
63
src/security/temp-path-guard.test.ts
Normal file
63
src/security/temp-path-guard.test.ts
Normal file
@@ -0,0 +1,63 @@
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
const DYNAMIC_TMPDIR_JOIN_RE = /path\.join\(os\.tmpdir\(\),\s*`[^`]*\$\{[^`]*`/;
|
||||
const RUNTIME_ROOTS = ["src", "extensions"];
|
||||
const SKIP_PATTERNS = [
|
||||
/\.test\.tsx?$/,
|
||||
/\.e2e\.tsx?$/,
|
||||
/\.d\.ts$/,
|
||||
/[\\/](?:__tests__|tests)[\\/]/,
|
||||
/[\\/]test-helpers(?:\.[^\\/]+)?\.ts$/,
|
||||
];
|
||||
|
||||
function shouldSkip(relativePath: string): boolean {
|
||||
return SKIP_PATTERNS.some((pattern) => pattern.test(relativePath));
|
||||
}
|
||||
|
||||
async function listTsFiles(dir: string): Promise<string[]> {
|
||||
const entries = await fs.readdir(dir, { withFileTypes: true });
|
||||
const out: string[] = [];
|
||||
for (const entry of entries) {
|
||||
if (entry.name === "node_modules" || entry.name === "dist" || entry.name.startsWith(".")) {
|
||||
continue;
|
||||
}
|
||||
const fullPath = path.join(dir, entry.name);
|
||||
if (entry.isDirectory()) {
|
||||
out.push(...(await listTsFiles(fullPath)));
|
||||
continue;
|
||||
}
|
||||
if (!entry.isFile()) {
|
||||
continue;
|
||||
}
|
||||
if (fullPath.endsWith(".ts") || fullPath.endsWith(".tsx")) {
|
||||
out.push(fullPath);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
describe("temp path guard", () => {
|
||||
it("blocks dynamic template path.join(os.tmpdir(), ...) in runtime source files", async () => {
|
||||
const repoRoot = process.cwd();
|
||||
const offenders: string[] = [];
|
||||
|
||||
for (const root of RUNTIME_ROOTS) {
|
||||
const absRoot = path.join(repoRoot, root);
|
||||
const files = await listTsFiles(absRoot);
|
||||
for (const file of files) {
|
||||
const relativePath = path.relative(repoRoot, file);
|
||||
if (shouldSkip(relativePath)) {
|
||||
continue;
|
||||
}
|
||||
const source = await fs.readFile(file, "utf-8");
|
||||
if (DYNAMIC_TMPDIR_JOIN_RE.test(source)) {
|
||||
offenders.push(relativePath);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user