From 1d7cb6fc03552bbba00e7cffb3aa9741f5556416 Mon Sep 17 00:00:00 2001 From: Devin Robison Date: Tue, 24 Mar 2026 16:28:53 -0700 Subject: [PATCH] fix: close sandbox media root bypass for mediaUrl/fileUrl aliases (#54034) * fix: close sandbox media root bypass for mediaUrl/fileUrl aliases * Address Greptile feedback * Fix windows test case failure --- .../outbound/message-action-params.test.ts | 81 +++++++++ src/infra/outbound/message-action-params.ts | 32 +++- .../message-action-runner.media.test.ts | 171 +++++++++++++++--- ...sage-action-runner.plugin-dispatch.test.ts | 7 + src/infra/outbound/message-action-runner.ts | 22 ++- 5 files changed, 282 insertions(+), 31 deletions(-) diff --git a/src/infra/outbound/message-action-params.test.ts b/src/infra/outbound/message-action-params.test.ts index a6cd67afcbc..510051f1cf1 100644 --- a/src/infra/outbound/message-action-params.test.ts +++ b/src/infra/outbound/message-action-params.test.ts @@ -185,6 +185,87 @@ describe("message action media helpers", () => { } }); + maybeIt("normalizes mediaUrl and fileUrl sandbox media params", async () => { + const sandboxRoot = await fs.mkdtemp(path.join(os.tmpdir(), "msg-params-alias-")); + try { + const args: Record = { + mediaUrl: " file:///workspace/assets/photo.png ", + fileUrl: "/workspace/docs/report.pdf", + }; + + await normalizeSandboxMediaParams({ + args, + mediaPolicy: { + mode: "sandbox", + sandboxRoot: ` ${sandboxRoot} `, + }, + }); + + expect(args).toMatchObject({ + mediaUrl: path.join(sandboxRoot, "assets", "photo.png"), + fileUrl: path.join(sandboxRoot, "docs", "report.pdf"), + }); + } finally { + await fs.rm(sandboxRoot, { recursive: true, force: true }); + } + }); + + maybeIt( + "keeps remote HTTP mediaUrl and fileUrl aliases unchanged under sandbox normalization", + async () => { + const sandboxRoot = await fs.mkdtemp(path.join(os.tmpdir(), "msg-params-remote-alias-")); + try { + const args: Record = { + mediaUrl: "https://example.com/assets/photo.png?sig=1", + fileUrl: "https://example.com/docs/report.pdf?sig=2", + }; + + await normalizeSandboxMediaParams({ + args, + mediaPolicy: { + mode: "sandbox", + sandboxRoot, + }, + }); + + expect(args).toMatchObject({ + mediaUrl: "https://example.com/assets/photo.png?sig=1", + fileUrl: "https://example.com/docs/report.pdf?sig=2", + }); + } finally { + await fs.rm(sandboxRoot, { recursive: true, force: true }); + } + }, + ); + + it("uses mediaUrl and fileUrl aliases when inferring attachment filenames", async () => { + const mediaArgs: Record = { + mediaUrl: "https://example.com/pic.png", + }; + await hydrateAttachmentParamsForAction({ + cfg, + channel: "slack", + args: mediaArgs, + action: "sendAttachment", + dryRun: true, + mediaPolicy: { mode: "host" }, + }); + expect(mediaArgs.filename).toBe("pic.png"); + + const fileArgs: Record = { + fileUrl: "https://example.com/docs/report.pdf", + }; + await hydrateAttachmentParamsForAction({ + cfg, + channel: "slack", + args: fileArgs, + action: "sendAttachment", + dryRun: true, + mediaPolicy: { mode: "host" }, + }); + expect(fileArgs.filename).toBe("report.pdf"); + }); + it("falls back to extension-based attachment names for remote-host file URLs", async () => { const args: Record = { media: "file://attacker/share/photo.png", diff --git a/src/infra/outbound/message-action-params.ts b/src/infra/outbound/message-action-params.ts index db9d2b26297..37119615520 100644 --- a/src/infra/outbound/message-action-params.ts +++ b/src/infra/outbound/message-action-params.ts @@ -10,6 +10,27 @@ import { readBooleanParam as readBooleanParamShared } from "../../plugin-sdk/boo export const readBooleanParam = readBooleanParamShared; +const SANDBOX_MEDIA_PARAM_KEYS = ["media", "path", "filePath", "mediaUrl", "fileUrl"] as const; + +function readMediaParam( + args: Record, + key: (typeof SANDBOX_MEDIA_PARAM_KEYS)[number], +): string | undefined { + return readStringParam(args, key, { trim: false }); +} + +function readAttachmentMediaHint(args: Record): string | undefined { + return readMediaParam(args, "media") ?? readMediaParam(args, "mediaUrl"); +} + +function readAttachmentFileHint(args: Record): string | undefined { + return ( + readMediaParam(args, "path") ?? + readMediaParam(args, "filePath") ?? + readMediaParam(args, "fileUrl") + ); +} + function resolveAttachmentMaxBytes(params: { cfg: OpenClawConfig; channel: ChannelId; @@ -190,9 +211,8 @@ export async function normalizeSandboxMediaParams(params: { }): Promise { const sandboxRoot = params.mediaPolicy.mode === "sandbox" ? params.mediaPolicy.sandboxRoot.trim() : undefined; - const mediaKeys: Array<"media" | "path" | "filePath"> = ["media", "path", "filePath"]; - for (const key of mediaKeys) { - const raw = readStringParam(params.args, key, { trim: false }); + for (const key of SANDBOX_MEDIA_PARAM_KEYS) { + const raw = readMediaParam(params.args, key); if (!raw) { continue; } @@ -242,10 +262,8 @@ async function hydrateAttachmentActionPayload(params: { allowMessageCaptionFallback?: boolean; mediaPolicy: AttachmentMediaPolicy; }): Promise { - const mediaHint = readStringParam(params.args, "media", { trim: false }); - const fileHint = - readStringParam(params.args, "path", { trim: false }) ?? - readStringParam(params.args, "filePath", { trim: false }); + const mediaHint = readAttachmentMediaHint(params.args); + const fileHint = readAttachmentFileHint(params.args); const contentTypeParam = readStringParam(params.args, "contentType") ?? readStringParam(params.args, "mimeType"); diff --git a/src/infra/outbound/message-action-runner.media.test.ts b/src/infra/outbound/message-action-runner.media.test.ts index 293dfffca69..2d341b861a5 100644 --- a/src/infra/outbound/message-action-runner.media.test.ts +++ b/src/infra/outbound/message-action-runner.media.test.ts @@ -56,6 +56,7 @@ const runDrySend = (params: { async function expectSandboxMediaRewrite(params: { sandboxDir: string; media?: string; + mediaField?: "media" | "mediaUrl" | "fileUrl"; message?: string; expectedRelativePath: string; }) { @@ -64,7 +65,11 @@ async function expectSandboxMediaRewrite(params: { actionParams: { channel: "slack", target: "#C12345678", - ...(params.media ? { media: params.media } : {}), + ...(params.media + ? { + [params.mediaField ?? "media"]: params.media, + } + : {}), ...(params.message ? { message: params.message } : {}), }, sandboxRoot: params.sandboxDir, @@ -196,6 +201,7 @@ describe("runMessageAction media behavior", () => { async function expectRejectsLocalAbsolutePathWithoutSandbox(params: { action: "sendAttachment" | "setGroupIcon"; target: string; + mediaField?: "media" | "mediaUrl" | "fileUrl"; message?: string; tempPrefix: string; }) { @@ -209,7 +215,7 @@ describe("runMessageAction media behavior", () => { const actionParams: Record = { channel: "bluebubbles", target: params.target, - media: outsidePath, + [params.mediaField ?? "media"]: outsidePath, }; if (params.message) { actionParams.message = params.message; @@ -270,6 +276,24 @@ describe("runMessageAction media behavior", () => { message: "caption", expectedPath: path.join("data", "pic.png"), }, + { + name: "sendAttachment mediaUrl rewrite", + action: "sendAttachment" as const, + target: "+15551234567", + mediaField: "mediaUrl" as const, + media: "./data/pic.png", + message: "caption", + expectedPath: path.join("data", "pic.png"), + }, + { + name: "sendAttachment fileUrl rewrite", + action: "sendAttachment" as const, + target: "+15551234567", + mediaField: "fileUrl" as const, + media: "/workspace/files/report.pdf", + message: "caption", + expectedPath: path.join("files", "report.pdf"), + }, { name: "setGroupIcon rewrite", action: "setGroupIcon" as const, @@ -286,7 +310,7 @@ describe("runMessageAction media behavior", () => { params: { channel: "bluebubbles", target: testCase.target, - media: testCase.media, + [testCase.mediaField ?? "media"]: testCase.media, ...(testCase.message ? { message: testCase.message } : {}), }, sandboxRoot: sandboxDir, @@ -309,6 +333,20 @@ describe("runMessageAction media behavior", () => { message: "caption", tempPrefix: "msg-attachment-", }, + { + action: "sendAttachment" as const, + target: "+15551234567", + mediaField: "mediaUrl" as const, + message: "caption", + tempPrefix: "msg-attachment-media-url-", + }, + { + action: "sendAttachment" as const, + target: "+15551234567", + mediaField: "fileUrl" as const, + message: "caption", + tempPrefix: "msg-attachment-file-url-", + }, { action: "setGroupIcon" as const, target: "group:123", @@ -337,25 +375,43 @@ describe("runMessageAction media behavior", () => { setActivePluginRegistry(createTestRegistry([])); }); - it.each(["/etc/passwd", "file:///etc/passwd"])( - "rejects out-of-sandbox media reference: %s", - async (media) => { - await withSandbox(async (sandboxDir) => { - await expect( - runDrySend({ - cfg: slackConfig, - actionParams: { - channel: "slack", - target: "#C12345678", - media, - message: "", - }, - sandboxRoot: sandboxDir, - }), - ).rejects.toThrow(/sandbox/i); - }); + it.each([ + { + name: "media absolute path", + mediaField: "media" as const, + media: "/etc/passwd", }, - ); + { + name: "mediaUrl absolute path", + mediaField: "mediaUrl" as const, + media: "/etc/passwd", + }, + { + name: "mediaUrl file URL", + mediaField: "mediaUrl" as const, + media: "file:///etc/passwd", + }, + { + name: "fileUrl file URL", + mediaField: "fileUrl" as const, + media: "file:///etc/passwd", + }, + ])("rejects out-of-sandbox media reference: $name", async ({ mediaField, media }) => { + await withSandbox(async (sandboxDir) => { + await expect( + runDrySend({ + cfg: slackConfig, + actionParams: { + channel: "slack", + target: "#C12345678", + [mediaField]: media, + message: "", + }, + sandboxRoot: sandboxDir, + }), + ).rejects.toThrow(/sandbox/i); + }); + }); it("rejects data URLs in media params", async () => { await expect( @@ -379,6 +435,20 @@ describe("runMessageAction media behavior", () => { message: "", expectedRelativePath: path.join("data", "file.txt"), }, + { + name: "relative mediaUrl path", + mediaField: "mediaUrl" as const, + media: "./data/file.txt", + message: "", + expectedRelativePath: path.join("data", "file.txt"), + }, + { + name: "/workspace fileUrl path", + mediaField: "fileUrl" as const, + media: "/workspace/data/file.txt", + message: "", + expectedRelativePath: path.join("data", "file.txt"), + }, { name: "/workspace media path", media: "/workspace/data/file.txt", @@ -390,11 +460,12 @@ describe("runMessageAction media behavior", () => { message: "Hello\nMEDIA: ./data/note.ogg", expectedRelativePath: path.join("data", "note.ogg"), }, - ]) { + ] as const) { await withSandbox(async (sandboxDir) => { await expectSandboxMediaRewrite({ sandboxDir, media: testCase.media, + mediaField: testCase.mediaField, message: testCase.message, expectedRelativePath: testCase.expectedRelativePath, }); @@ -402,6 +473,62 @@ describe("runMessageAction media behavior", () => { } }); + it("prefers media over mediaUrl when both aliases are present", async () => { + await withSandbox(async (sandboxDir) => { + const result = await runDrySend({ + cfg: slackConfig, + actionParams: { + channel: "slack", + target: "#C12345678", + media: "./data/primary.txt", + mediaUrl: "./data/secondary.txt", + message: "", + }, + sandboxRoot: sandboxDir, + }); + + expect(result.kind).toBe("send"); + if (result.kind !== "send") { + throw new Error("expected send result"); + } + expect(result.sendResult?.mediaUrl).toBe(path.join(sandboxDir, "data", "primary.txt")); + }); + }); + + it.each([ + { + name: "mediaUrl", + mediaField: "mediaUrl" as const, + }, + { + name: "fileUrl", + mediaField: "fileUrl" as const, + }, + ])( + "keeps remote HTTP $name aliases unchanged under sandbox validation", + async ({ mediaField }) => { + await withSandbox(async (sandboxDir) => { + const remoteUrl = "https://example.com/files/report.pdf?sig=1"; + const result = await runDrySend({ + cfg: slackConfig, + actionParams: { + channel: "slack", + target: "#C12345678", + [mediaField]: remoteUrl, + message: "", + }, + sandboxRoot: sandboxDir, + }); + + expect(result.kind).toBe("send"); + if (result.kind !== "send") { + throw new Error("expected send result"); + } + expect(result.sendResult?.mediaUrl).toBe(remoteUrl); + }); + }, + ); + it("allows media paths under preferred OpenClaw tmp root", async () => { const tmpRoot = resolvePreferredOpenClawTmpDir(); await fs.mkdir(tmpRoot, { recursive: true }); diff --git a/src/infra/outbound/message-action-runner.plugin-dispatch.test.ts b/src/infra/outbound/message-action-runner.plugin-dispatch.test.ts index 88f816c2d88..224e4a7f2cc 100644 --- a/src/infra/outbound/message-action-runner.plugin-dispatch.test.ts +++ b/src/infra/outbound/message-action-runner.plugin-dispatch.test.ts @@ -1,3 +1,4 @@ +import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { jsonResult } from "../../agents/tools/common.js"; import type { ChannelPlugin } from "../../channels/plugins/types.js"; @@ -58,6 +59,7 @@ describe("runMessageAction plugin dispatch", () => { afterEach(() => { setActivePluginRegistry(createTestRegistry([])); vi.clearAllMocks(); + vi.unstubAllEnvs(); }); it("dispatches messageId/chatId-based Feishu actions through the shared runner", async () => { @@ -114,6 +116,10 @@ describe("runMessageAction plugin dispatch", () => { }); it("routes execution context ids into plugin handleAction", async () => { + const stateDir = path.join("/tmp", "openclaw-plugin-dispatch-media-roots"); + const expectedWorkspaceRoot = path.resolve(stateDir, "workspace-alpha"); + vi.stubEnv("OPENCLAW_STATE_DIR", stateDir); + await runMessageAction({ cfg: { channels: { @@ -148,6 +154,7 @@ describe("runMessageAction plugin dispatch", () => { sessionKey: "agent:alpha:main", sessionId: "session-123", agentId: "alpha", + mediaLocalRoots: expect.arrayContaining([expectedWorkspaceRoot]), toolContext: expect.objectContaining({ currentChannelId: "chat:oc_123", currentThreadTs: "thread-456", diff --git a/src/infra/outbound/message-action-runner.ts b/src/infra/outbound/message-action-runner.ts index e7b37829981..7a5740f0cd3 100644 --- a/src/infra/outbound/message-action-runner.ts +++ b/src/infra/outbound/message-action-runner.ts @@ -258,6 +258,7 @@ type ResolvedActionContext = { cfg: OpenClawConfig; params: Record; channel: ChannelId; + mediaLocalRoots: readonly string[]; accountId?: string | null; dryRun: boolean; gateway?: MessageActionRunnerGateway; @@ -382,8 +383,10 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise 0; const hasCard = params.card != null && typeof params.card === "object"; const hasComponents = params.components != null && typeof params.components === "object"; @@ -620,7 +623,18 @@ async function handlePollAction(ctx: ResolvedActionContext): Promise { - const { cfg, params, channel, accountId, dryRun, gateway, input, abortSignal, agentId } = ctx; + const { + cfg, + params, + channel, + mediaLocalRoots, + accountId, + dryRun, + gateway, + input, + abortSignal, + agentId, + } = ctx; throwIfAborted(abortSignal); const action = input.action as Exclude; if (dryRun) { @@ -644,6 +658,7 @@ async function handlePluginAction(ctx: ResolvedActionContext): Promise