From 4baa43384a14ecd8ca089402b07d96dddfb7d66e Mon Sep 17 00:00:00 2001 From: buddyh <31752869+buddyh@users.noreply.github.com> Date: Sun, 8 Feb 2026 23:52:11 -0500 Subject: [PATCH] fix(media): guard local media reads + accept all path types in MEDIA directive --- src/infra/outbound/message-action-runner.ts | 6 ++- src/media/parse.test.ts | 53 ++++++++++++++++----- src/media/parse.ts | 45 +++++++++++++---- src/web/media.test.ts | 40 ++++++++++++++++ src/web/media.ts | 52 ++++++++++++++++++-- 5 files changed, 170 insertions(+), 26 deletions(-) diff --git a/src/infra/outbound/message-action-runner.ts b/src/infra/outbound/message-action-runner.ts index eddc771870..fc842a7efc 100644 --- a/src/infra/outbound/message-action-runner.ts +++ b/src/infra/outbound/message-action-runner.ts @@ -443,7 +443,8 @@ async function hydrateSetGroupIconParams(params: { channel: params.channel, accountId: params.accountId, }); - const media = await loadWebMedia(mediaSource, maxBytes); + // localRoots: "any" — media paths are already validated by normalizeSandboxMediaList above. + const media = await loadWebMedia(mediaSource, maxBytes, { localRoots: "any" }); params.args.buffer = media.buffer.toString("base64"); if (!contentTypeParam && media.contentType) { params.args.contentType = media.contentType; @@ -507,7 +508,8 @@ async function hydrateSendAttachmentParams(params: { channel: params.channel, accountId: params.accountId, }); - const media = await loadWebMedia(mediaSource, maxBytes); + // localRoots: "any" — media paths are already validated by normalizeSandboxMediaList above. + const media = await loadWebMedia(mediaSource, maxBytes, { localRoots: "any" }); params.args.buffer = media.buffer.toString("base64"); if (!contentTypeParam && media.contentType) { params.args.contentType = media.contentType; diff --git a/src/media/parse.test.ts b/src/media/parse.test.ts index 5475ae2815..856e7216e1 100644 --- a/src/media/parse.test.ts +++ b/src/media/parse.test.ts @@ -8,28 +8,28 @@ describe("splitMediaFromOutput", () => { expect(result.text).toBe("Hello world"); }); - it("rejects absolute media paths to prevent LFI", () => { + it("accepts absolute media paths", () => { const result = splitMediaFromOutput("MEDIA:/Users/pete/My File.png"); - expect(result.mediaUrls).toBeUndefined(); - expect(result.text).toBe("MEDIA:/Users/pete/My File.png"); + expect(result.mediaUrls).toEqual(["/Users/pete/My File.png"]); + expect(result.text).toBe(""); }); - it("rejects quoted absolute media paths to prevent LFI", () => { + it("accepts quoted absolute media paths", () => { const result = splitMediaFromOutput('MEDIA:"/Users/pete/My File.png"'); - expect(result.mediaUrls).toBeUndefined(); - expect(result.text).toBe('MEDIA:"/Users/pete/My File.png"'); + expect(result.mediaUrls).toEqual(["/Users/pete/My File.png"]); + expect(result.text).toBe(""); }); - it("rejects tilde media paths to prevent LFI", () => { + it("accepts tilde media paths", () => { const result = splitMediaFromOutput("MEDIA:~/Pictures/My File.png"); - expect(result.mediaUrls).toBeUndefined(); - expect(result.text).toBe("MEDIA:~/Pictures/My File.png"); + expect(result.mediaUrls).toEqual(["~/Pictures/My File.png"]); + expect(result.text).toBe(""); }); - it("rejects directory traversal media paths to prevent LFI", () => { + it("accepts traversal-like media paths (validated at load time)", () => { const result = splitMediaFromOutput("MEDIA:../../etc/passwd"); - expect(result.mediaUrls).toBeUndefined(); - expect(result.text).toBe("MEDIA:../../etc/passwd"); + expect(result.mediaUrls).toEqual(["../../etc/passwd"]); + expect(result.text).toBe(""); }); it("captures safe relative media paths", () => { @@ -38,6 +38,12 @@ describe("splitMediaFromOutput", () => { expect(result.text).toBe(""); }); + it("accepts sandbox-relative media paths", () => { + const result = splitMediaFromOutput("MEDIA:media/inbound/image.png"); + expect(result.mediaUrls).toEqual(["media/inbound/image.png"]); + expect(result.text).toBe(""); + }); + it("keeps audio_as_voice detection stable across calls", () => { const input = "Hello [[audio_as_voice]]"; const first = splitMediaFromOutput(input); @@ -58,4 +64,27 @@ describe("splitMediaFromOutput", () => { expect(result.mediaUrls).toEqual(["./screenshot.png"]); expect(result.text).toBe(""); }); + + it("accepts Windows-style paths", () => { + const result = splitMediaFromOutput("MEDIA:C:\\Users\\pete\\Pictures\\snap.png"); + expect(result.mediaUrls).toEqual(["C:\\Users\\pete\\Pictures\\snap.png"]); + expect(result.text).toBe(""); + }); + + it("accepts TTS temp file paths", () => { + const result = splitMediaFromOutput("MEDIA:/tmp/tts-fAJy8C/voice-1770246885083.opus"); + expect(result.mediaUrls).toEqual(["/tmp/tts-fAJy8C/voice-1770246885083.opus"]); + expect(result.text).toBe(""); + }); + + it("accepts bare filenames with extensions", () => { + const result = splitMediaFromOutput("MEDIA:image.png"); + expect(result.mediaUrls).toEqual(["image.png"]); + expect(result.text).toBe(""); + }); + + it("rejects bare words without file extensions", () => { + const result = splitMediaFromOutput("MEDIA:screenshot"); + expect(result.mediaUrls).toBeUndefined(); + }); }); diff --git a/src/media/parse.ts b/src/media/parse.ts index b8fe22864e..693940a0ae 100644 --- a/src/media/parse.ts +++ b/src/media/parse.ts @@ -14,7 +14,29 @@ function cleanCandidate(raw: string) { return raw.replace(/^[`"'[{(]+/, "").replace(/[`"'\\})\],]+$/, ""); } -function isValidMedia(candidate: string, opts?: { allowSpaces?: boolean }) { +const WINDOWS_DRIVE_RE = /^[a-zA-Z]:[\\/]/; +const SCHEME_RE = /^[a-zA-Z][a-zA-Z0-9+.-]*:/; +const HAS_FILE_EXT = /\.\w{1,10}$/; + +// Recognize local file path patterns. Security validation is deferred to the +// load layer (loadWebMedia / resolveSandboxedMediaSource) which has the context +// needed to enforce sandbox roots and allowed directories. +function isLikelyLocalPath(candidate: string): boolean { + return ( + candidate.startsWith("/") || + candidate.startsWith("./") || + candidate.startsWith("../") || + candidate.startsWith("~") || + WINDOWS_DRIVE_RE.test(candidate) || + candidate.startsWith("\\\\") || + (!SCHEME_RE.test(candidate) && (candidate.includes("/") || candidate.includes("\\"))) + ); +} + +function isValidMedia( + candidate: string, + opts?: { allowSpaces?: boolean; allowBareFilename?: boolean }, +) { if (!candidate) { return false; } @@ -28,8 +50,17 @@ function isValidMedia(candidate: string, opts?: { allowSpaces?: boolean }) { return true; } - // Local paths: only allow safe relative paths starting with ./ that do not traverse upwards. - return candidate.startsWith("./") && !candidate.includes(".."); + if (isLikelyLocalPath(candidate)) { + return true; + } + + // Accept bare filenames (e.g. "image.png") only when the caller opts in. + // This avoids treating space-split path fragments as separate media items. + if (opts?.allowBareFilename && !SCHEME_RE.test(candidate) && HAS_FILE_EXT.test(candidate)) { + return true; + } + + return false; } function unwrapQuoted(value: string): string | undefined { @@ -128,11 +159,7 @@ export function splitMediaFromOutput(raw: string): { const trimmedPayload = payloadValue.trim(); const looksLikeLocalPath = - trimmedPayload.startsWith("/") || - trimmedPayload.startsWith("./") || - trimmedPayload.startsWith("../") || - trimmedPayload.startsWith("~") || - trimmedPayload.startsWith("file://"); + isLikelyLocalPath(trimmedPayload) || trimmedPayload.startsWith("file://"); if ( !unwrapped && validCount === 1 && @@ -152,7 +179,7 @@ export function splitMediaFromOutput(raw: string): { if (!hasValidMedia) { const fallback = normalizeMediaSource(cleanCandidate(payloadValue)); - if (isValidMedia(fallback, { allowSpaces: true })) { + if (isValidMedia(fallback, { allowSpaces: true, allowBareFilename: true })) { media.push(fallback); hasValidMedia = true; foundMediaToken = true; diff --git a/src/web/media.test.ts b/src/web/media.test.ts index ff40ef0c74..861ca9da45 100644 --- a/src/web/media.test.ts +++ b/src/web/media.test.ts @@ -292,3 +292,43 @@ describe("web media loading", () => { expect(result.buffer.length).toBeLessThanOrEqual(cap); }); }); + +describe("local media root guard", () => { + it("rejects local paths outside allowed roots", async () => { + const pngBuffer = await sharp({ + create: { width: 10, height: 10, channels: 3, background: "#00ff00" }, + }) + .png() + .toBuffer(); + const file = await writeTempFile(pngBuffer, ".png"); + + // Explicit roots that don't contain the temp file. + await expect( + loadWebMedia(file, 1024 * 1024, { localRoots: ["/nonexistent-root"] }), + ).rejects.toThrow(/not under an allowed directory/i); + }); + + it("allows local paths under an explicit root", async () => { + const pngBuffer = await sharp({ + create: { width: 10, height: 10, channels: 3, background: "#00ff00" }, + }) + .png() + .toBuffer(); + const file = await writeTempFile(pngBuffer, ".png"); + + const result = await loadWebMedia(file, 1024 * 1024, { localRoots: [os.tmpdir()] }); + expect(result.kind).toBe("image"); + }); + + it("allows any path when localRoots is 'any'", async () => { + const pngBuffer = await sharp({ + create: { width: 10, height: 10, channels: 3, background: "#00ff00" }, + }) + .png() + .toBuffer(); + const file = await writeTempFile(pngBuffer, ".png"); + + const result = await loadWebMedia(file, 1024 * 1024, { localRoots: "any" }); + expect(result.kind).toBe("image"); + }); +}); diff --git a/src/web/media.ts b/src/web/media.ts index edc172f35a..bed9bafe18 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -1,4 +1,5 @@ import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import type { SsrFPolicy } from "../infra/net/ssrf.js"; @@ -25,8 +26,48 @@ type WebMediaOptions = { maxBytes?: number; optimizeImages?: boolean; ssrfPolicy?: SsrFPolicy; + /** Allowed root directories for local path reads. "any" skips the check (caller already validated). */ + localRoots?: string[] | "any"; }; +function getDefaultLocalRoots(): string[] { + const home = os.homedir(); + return [ + os.tmpdir(), + path.join(home, ".openclaw", "media"), + path.join(home, ".openclaw", "agents"), + ]; +} + +async function assertLocalMediaAllowed( + mediaPath: string, + localRoots: string[] | "any" | undefined, +): Promise { + if (localRoots === "any") { + return; + } + const roots = localRoots ?? getDefaultLocalRoots(); + // Resolve symlinks so a symlink under /tmp pointing to /etc/passwd is caught. + let resolved: string; + try { + resolved = await fs.realpath(mediaPath); + } catch { + resolved = path.resolve(mediaPath); + } + for (const root of roots) { + let resolvedRoot: string; + try { + resolvedRoot = await fs.realpath(root); + } catch { + resolvedRoot = path.resolve(root); + } + if (resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep)) { + return; + } + } + throw new Error(`Local media path is not under an allowed directory: ${mediaPath}`); +} + const HEIC_MIME_RE = /^image\/hei[cf]$/i; const HEIC_EXT_RE = /\.(heic|heif)$/i; const MB = 1024 * 1024; @@ -124,7 +165,7 @@ async function loadWebMediaInternal( mediaUrl: string, options: WebMediaOptions = {}, ): Promise { - const { maxBytes, optimizeImages = true, ssrfPolicy } = options; + const { maxBytes, optimizeImages = true, ssrfPolicy, localRoots } = options; // Use fileURLToPath for proper handling of file:// URLs (handles file://localhost/path, etc.) if (mediaUrl.startsWith("file://")) { try { @@ -222,6 +263,9 @@ async function loadWebMediaInternal( mediaUrl = resolveUserPath(mediaUrl); } + // Guard local reads against allowed directory roots to prevent file exfiltration. + await assertLocalMediaAllowed(mediaUrl, localRoots); + // Local path const data = await fs.readFile(mediaUrl); const mime = await detectMime({ buffer: data, filePath: mediaUrl }); @@ -244,24 +288,26 @@ async function loadWebMediaInternal( export async function loadWebMedia( mediaUrl: string, maxBytes?: number, - options?: { ssrfPolicy?: SsrFPolicy }, + options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, ): Promise { return await loadWebMediaInternal(mediaUrl, { maxBytes, optimizeImages: true, ssrfPolicy: options?.ssrfPolicy, + localRoots: options?.localRoots, }); } export async function loadWebMediaRaw( mediaUrl: string, maxBytes?: number, - options?: { ssrfPolicy?: SsrFPolicy }, + options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, ): Promise { return await loadWebMediaInternal(mediaUrl, { maxBytes, optimizeImages: false, ssrfPolicy: options?.ssrfPolicy, + localRoots: options?.localRoots, }); }