mirror of
https://fastgit.cc/github.com/openclaw/openclaw
synced 2026-04-30 14:02:56 +08:00
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
This commit is contained in:
committed by
GitHub
parent
84185cb3eb
commit
1470de5d3e
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<typeof import("./abort.runtime.js")> | null = null;
|
||||
let ttsRuntimePromise: Promise<typeof import("../../tts/tts.runtime.js")> | null = null;
|
||||
let replyMediaPathsRuntimePromise: Promise<typeof import("./reply-media-paths.runtime.js")> | 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<Awaited<ReturnType<typeof loadTtsRuntime>>["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<ReplyPayload> => {
|
||||
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();
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user