From 1470de5d3e0970856d86cd99336bb8ada3fe87da Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Wed, 15 Apr 2026 23:58:01 +0530 Subject: [PATCH] fix(webchat): reject remote-host file:// URLs in media embedding path [AI-assisted] (#67293) * fix: address issue * fix: address PR review feedback * fix: address PR review feedback * docs: add changelog entry for PR merge --- CHANGELOG.md | 2 + ...ispatch-from-config.shared.test-harness.ts | 11 ++++ .../reply/dispatch-from-config.test.ts | 58 +++++++++++++++++++ src/auto-reply/reply/dispatch-from-config.ts | 47 +++++++++++++-- .../server-methods/chat-webchat-media.test.ts | 19 ++++++ .../server-methods/chat-webchat-media.ts | 9 ++- 6 files changed, 138 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35fde7c2477..ce41ff1ddd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(webchat): reject remote-host file:// URLs in media embedding path [AI-assisted]. (#67293) Thanks @pgondhi987. - fix(gateway): enforce localRoots containment on webchat audio embedding path [AI-assisted]. (#67298) Thanks @pgondhi987. - fix(matrix): block DM pairing-store entries from authorizing room control commands [AI-assisted]. (#67294) Thanks @pgondhi987. - Docker/build: verify `@matrix-org/matrix-sdk-crypto-nodejs` native bindings with `find` under `node_modules` instead of a hardcoded `.pnpm/...` path so pnpm v10+ virtual-store layouts no longer fail the image build. (#67143) thanks @ly85206559. @@ -31,6 +32,7 @@ Docs: https://docs.openclaw.ai - Docs/showcase: add a scannable hero, complete section jump links, and a responsive video grid for community examples. (#48493) Thanks @jchopard69. ### Fixes + - Security/approvals: redact secrets in exec approval prompts so inline approval review can no longer leak credential material in rendered prompt content. (#61077, #64790) - CLI/configure: re-read the persisted config hash after writes so config updates stop failing with stale-hash races. (#64188, #66528) - CLI/update: prune stale packaged `dist` chunks after npm upgrades and keep downgrade/verify inventory checks compat-safe so global upgrades stop failing on stale chunk imports. (#66959) Thanks @obviyus. diff --git a/src/auto-reply/reply/dispatch-from-config.shared.test-harness.ts b/src/auto-reply/reply/dispatch-from-config.shared.test-harness.ts index 697ec4d8b9d..92bd947d01b 100644 --- a/src/auto-reply/reply/dispatch-from-config.shared.test-harness.ts +++ b/src/auto-reply/reply/dispatch-from-config.shared.test-harness.ts @@ -107,6 +107,9 @@ const ttsMocks = vi.hoisted(() => ({ normalizeTtsAutoMode: vi.fn((value: unknown) => (typeof value === "string" ? value : undefined)), resolveTtsConfig: vi.fn((_cfg: OpenClawConfig) => ({ mode: "final" })), })); +const replyMediaPathMocks = vi.hoisted(() => ({ + createReplyMediaPathNormalizer: vi.fn(() => async (payload: ReplyPayload) => payload), +})); const threadInfoMocks = vi.hoisted(() => ({ parseSessionThreadInfo: vi.fn< (sessionKey: string | undefined) => { @@ -127,6 +130,7 @@ export { pluginConversationBindingMocks, sessionBindingMocks, sessionStoreMocks, + replyMediaPathMocks, threadInfoMocks, ttsMocks, }; @@ -263,6 +267,10 @@ vi.mock("../../tts/tts.js", () => ({ vi.mock("../../tts/tts.runtime.js", () => ({ maybeApplyTtsToPayload: (params: unknown) => ttsMocks.maybeApplyTtsToPayload(params), })); +vi.mock("./reply-media-paths.runtime.js", () => ({ + createReplyMediaPathNormalizer: (params: unknown) => + replyMediaPathMocks.createReplyMediaPathNormalizer(params), +})); vi.mock("../../tts/status-config.js", () => ({ resolveStatusTtsSnapshot: () => ({ autoMode: "always", @@ -311,6 +319,9 @@ export function resetPluginTtsAndThreadMocks() { .mockReset() .mockImplementation((value: unknown) => (typeof value === "string" ? value : undefined)); ttsMocks.resolveTtsConfig.mockReset().mockReturnValue({ mode: "final" }); + replyMediaPathMocks.createReplyMediaPathNormalizer + .mockReset() + .mockReturnValue(async (payload: ReplyPayload) => payload); threadInfoMocks.parseSessionThreadInfo .mockReset() .mockImplementation(parseGenericThreadSessionInfo); diff --git a/src/auto-reply/reply/dispatch-from-config.test.ts b/src/auto-reply/reply/dispatch-from-config.test.ts index 244cf120682..c67a4e1d4f7 100644 --- a/src/auto-reply/reply/dispatch-from-config.test.ts +++ b/src/auto-reply/reply/dispatch-from-config.test.ts @@ -142,6 +142,9 @@ const ttsMocks = vi.hoisted(() => { resolveTtsConfig: vi.fn((_cfg: OpenClawConfig) => ({ mode: "final" })), }; }); +const replyMediaPathMocks = vi.hoisted(() => ({ + createReplyMediaPathNormalizer: vi.fn(() => async (payload: ReplyPayload) => payload), +})); const threadInfoMocks = vi.hoisted(() => ({ parseSessionThreadInfo: vi.fn< (sessionKey: string | undefined) => { @@ -326,6 +329,10 @@ vi.mock("../../tts/tts.js", () => ({ vi.mock("../../tts/tts.runtime.js", () => ({ maybeApplyTtsToPayload: (params: unknown) => ttsMocks.maybeApplyTtsToPayload(params), })); +vi.mock("./reply-media-paths.runtime.js", () => ({ + createReplyMediaPathNormalizer: (params: unknown) => + replyMediaPathMocks.createReplyMediaPathNormalizer(params), +})); vi.mock("../../tts/status-config.js", () => ({ resolveStatusTtsSnapshot: () => ({ autoMode: "always", @@ -642,6 +649,10 @@ describe("dispatchReplyFromConfig", () => { ttsMocks.resolveTtsConfig.mockReturnValue({ mode: "final", }); + replyMediaPathMocks.createReplyMediaPathNormalizer.mockReset(); + replyMediaPathMocks.createReplyMediaPathNormalizer.mockReturnValue( + async (payload: ReplyPayload) => payload, + ); }); it("does not route when Provider matches OriginatingChannel (even if Surface is missing)", async () => { setNoAbort(); @@ -968,6 +979,12 @@ describe("dispatchReplyFromConfig", () => { await dispatchReplyFromConfig({ ctx, cfg, dispatcher, replyResolver }); + expect(replyMediaPathMocks.createReplyMediaPathNormalizer).toHaveBeenCalledWith( + expect.objectContaining({ + cfg, + messageProvider: "telegram", + }), + ); expect(dispatcher.sendToolResult).not.toHaveBeenCalled(); expect(dispatcher.sendFinalReply).not.toHaveBeenCalled(); expect(mocks.routeReply).toHaveBeenCalledTimes(1); @@ -1032,6 +1049,47 @@ describe("dispatchReplyFromConfig", () => { expect(dispatcher.sendFinalReply).toHaveBeenCalledTimes(1); }); + it("normalizes tool-result media before delivery and drops blocked file URLs", async () => { + setNoAbort(); + replyMediaPathMocks.createReplyMediaPathNormalizer.mockReturnValue( + async (payload: ReplyPayload) => ({ + ...payload, + mediaUrl: undefined, + mediaUrls: undefined, + }), + ); + const cfg = emptyConfig; + const dispatcher = createDispatcher(); + const ctx = buildTestCtx({ + Provider: "webchat", + Surface: "webchat", + ChatType: "group", + }); + + const replyResolver = async ( + _ctx: MsgContext, + opts?: GetReplyOptions, + _cfg?: OpenClawConfig, + ) => { + await opts?.onToolResult?.({ + text: "NO_REPLY", + mediaUrls: ["file://attacker/share/probe.mp3"], + }); + return { text: "done" } satisfies ReplyPayload; + }; + + await dispatchReplyFromConfig({ ctx, cfg, dispatcher, replyResolver }); + + expect(replyMediaPathMocks.createReplyMediaPathNormalizer).toHaveBeenCalledWith( + expect.objectContaining({ + cfg, + messageProvider: "webchat", + }), + ); + expect(dispatcher.sendToolResult).not.toHaveBeenCalled(); + expect(dispatcher.sendFinalReply).toHaveBeenCalledWith({ text: "done" }); + }); + it("delivers tool summaries in forum topic sessions (group + IsForum)", async () => { setNoAbort(); const cfg = emptyConfig; diff --git a/src/auto-reply/reply/dispatch-from-config.ts b/src/auto-reply/reply/dispatch-from-config.ts index 79049eea8ea..5c9e5a5500d 100644 --- a/src/auto-reply/reply/dispatch-from-config.ts +++ b/src/auto-reply/reply/dispatch-from-config.ts @@ -1,6 +1,10 @@ import { resolveSendableOutboundReplyParts } from "openclaw/plugin-sdk/reply-payload"; import { isParentOwnedBackgroundAcpSession } from "../../acp/session-interaction-mode.js"; -import { resolveAgentConfig, resolveSessionAgentId } from "../../agents/agent-scope.js"; +import { + resolveAgentConfig, + resolveAgentWorkspaceDir, + resolveSessionAgentId, +} from "../../agents/agent-scope.js"; import { resolveConversationBindingRecord, touchConversationBindingRecord, @@ -73,6 +77,8 @@ let getReplyFromConfigRuntimePromise: Promise< > | null = null; let abortRuntimePromise: Promise | null = null; let ttsRuntimePromise: Promise | null = null; +let replyMediaPathsRuntimePromise: Promise | null = + null; function loadRouteReplyRuntime() { routeReplyRuntimePromise ??= import("./route-reply.runtime.js"); @@ -94,6 +100,11 @@ function loadTtsRuntime() { return ttsRuntimePromise; } +function loadReplyMediaPathsRuntime() { + replyMediaPathsRuntimePromise ??= import("./reply-media-paths.runtime.js"); + return replyMediaPathsRuntimePromise; +} + async function maybeApplyTtsToReplyPayload( params: Parameters>["maybeApplyTtsToPayload"]>[0], ) { @@ -336,6 +347,27 @@ export async function dispatchReplyFromConfig( }); const originatingTo = ctx.OriginatingTo; const ttsChannel = shouldRouteToOriginating ? originatingChannel : currentSurface; + const { createReplyMediaPathNormalizer } = await loadReplyMediaPathsRuntime(); + const normalizeReplyMediaPaths = createReplyMediaPathNormalizer({ + cfg, + sessionKey: acpDispatchSessionKey, + workspaceDir: resolveAgentWorkspaceDir(cfg, sessionAgentId), + messageProvider: ttsChannel, + accountId: ctx.AccountId, + groupId, + groupChannel: ctx.GroupChannel, + groupSpace: ctx.GroupSpace, + requesterSenderId: ctx.SenderId, + requesterSenderName: ctx.SenderName, + requesterSenderUsername: ctx.SenderUsername, + requesterSenderE164: ctx.SenderE164, + }); + const normalizeReplyMediaPayload = async (payload: ReplyPayload): Promise => { + if (!resolveSendableOutboundReplyParts(payload).hasMedia) { + return payload; + } + return await normalizeReplyMediaPaths(payload); + }; const routeReplyToOriginating = async ( payload: ReplyPayload, @@ -610,7 +642,8 @@ export async function dispatchReplyFromConfig( inboundAudio, ttsAuto: sessionTtsAuto, }); - const result = await routeReplyToOriginating(ttsPayload); + const normalizedPayload = await normalizeReplyMediaPayload(ttsPayload); + const result = await routeReplyToOriginating(normalizedPayload); if (result) { if (!result.ok) { logVerbose( @@ -623,7 +656,7 @@ export async function dispatchReplyFromConfig( }; } return { - queuedFinal: dispatcher.sendFinalReply(ttsPayload), + queuedFinal: dispatcher.sendFinalReply(normalizedPayload), routedFinalCount: 0, }; }; @@ -868,7 +901,8 @@ export async function dispatchReplyFromConfig( inboundAudio, ttsAuto: sessionTtsAuto, }); - const deliveryPayload = resolveToolDeliveryPayload(ttsPayload); + const normalizedPayload = await normalizeReplyMediaPayload(ttsPayload); + const deliveryPayload = resolveToolDeliveryPayload(normalizedPayload); if (!deliveryPayload) { return; } @@ -947,10 +981,11 @@ export async function dispatchReplyFromConfig( inboundAudio, ttsAuto: sessionTtsAuto, }); + const normalizedPayload = await normalizeReplyMediaPayload(ttsPayload); if (shouldRouteToOriginating) { - await sendPayloadAsync(ttsPayload, context?.abortSignal, false); + await sendPayloadAsync(normalizedPayload, context?.abortSignal, false); } else { - dispatcher.sendBlockReply(ttsPayload); + dispatcher.sendBlockReply(normalizedPayload); } }; return run(); diff --git a/src/gateway/server-methods/chat-webchat-media.test.ts b/src/gateway/server-methods/chat-webchat-media.test.ts index 747ba96beab..33cc2662eb7 100644 --- a/src/gateway/server-methods/chat-webchat-media.test.ts +++ b/src/gateway/server-methods/chat-webchat-media.test.ts @@ -86,6 +86,25 @@ describe("buildWebchatAudioContentBlocksFromReplyPayloads", () => { expect((blocks[0] as { type?: string }).type).toBe("audio"); }); + it("drops tool-result file:// URLs with remote hosts before touching the filesystem", async () => { + const statSpy = vi.spyOn(fs, "statSync"); + const readSpy = vi.spyOn(fs, "readFileSync"); + + const blocks = await buildWebchatAudioContentBlocksFromReplyPayloads([ + { + text: "MEDIA:file://attacker/share/probe.mp3", + mediaUrl: "file://attacker/share/probe.mp3", + }, + ]); + + expect(blocks).toHaveLength(0); + expect(statSpy).not.toHaveBeenCalled(); + expect(readSpy).not.toHaveBeenCalled(); + + statSpy.mockRestore(); + readSpy.mockRestore(); + }); + it("rejects a local audio file outside configured localRoots", async () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-webchat-audio-")); const allowedRoot = path.join(tmpDir, "allowed"); diff --git a/src/gateway/server-methods/chat-webchat-media.ts b/src/gateway/server-methods/chat-webchat-media.ts index ee52c8c4b80..d3b8745fc16 100644 --- a/src/gateway/server-methods/chat-webchat-media.ts +++ b/src/gateway/server-methods/chat-webchat-media.ts @@ -1,8 +1,8 @@ import fs from "node:fs"; import path from "node:path"; -import { fileURLToPath } from "node:url"; import type { ReplyPayload } from "../../auto-reply/reply-payload.js"; import { assertLocalMediaAllowed, LocalMediaAccessError } from "../../media/local-media-access.js"; +import { assertNoWindowsNetworkPath, safeFileURLToPath } from "../../infra/local-file-access.js"; import { isAudioFileName } from "../../media/mime.js"; import { resolveSendableOutboundReplyParts } from "../../plugin-sdk/reply-payload.js"; import { normalizeLowercaseStringOrEmpty } from "../../shared/string-coerce.js"; @@ -40,7 +40,7 @@ function resolveLocalMediaPathForEmbedding(raw: string): string | null { } if (trimmed.startsWith("file:")) { try { - const p = fileURLToPath(trimmed); + const p = safeFileURLToPath(trimmed); if (!path.isAbsolute(p)) { return null; } @@ -52,6 +52,11 @@ function resolveLocalMediaPathForEmbedding(raw: string): string | null { if (!path.isAbsolute(trimmed)) { return null; } + try { + assertNoWindowsNetworkPath(trimmed, "Local media path"); + } catch { + return null; + } return trimmed; }