From a6a4140ee71a92e9be56ac825c627ef778d7face Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 6 May 2026 02:48:36 +0100 Subject: [PATCH] fix(media): handle canonical inbound media paths --- ...bound-media-into-sandbox-workspace.test.ts | 7 ++++++ src/auto-reply/reply/stage-sandbox-media.ts | 18 ++++++++++----- .../media-understanding-url-fallback.test.ts | 14 +++--------- src/media/image-ops.tempdir.test.ts | 10 ++++----- src/media/media-reference.ts | 22 ++++++++++++++++--- 5 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/auto-reply/reply.triggers.trigger-handling.stages-inbound-media-into-sandbox-workspace.test.ts b/src/auto-reply/reply.triggers.trigger-handling.stages-inbound-media-into-sandbox-workspace.test.ts index 6982ec63f6f..5c81d5693a7 100644 --- a/src/auto-reply/reply.triggers.trigger-handling.stages-inbound-media-into-sandbox-workspace.test.ts +++ b/src/auto-reply/reply.triggers.trigger-handling.stages-inbound-media-into-sandbox-workspace.test.ts @@ -127,6 +127,13 @@ beforeEach(() => { relativePath, maxBytes: options?.maxBytes, }), + copyIn: async (relativePath: string, sourcePath: string, options?: { maxBytes?: number }) => + await rootCopyFromForTest({ + sourcePath, + rootDir, + relativePath, + maxBytes: options?.maxBytes, + }), })); mediaRootMocks.resolveChannelRemoteInboundAttachmentRoots .mockReset() diff --git a/src/auto-reply/reply/stage-sandbox-media.ts b/src/auto-reply/reply/stage-sandbox-media.ts index adc254327a5..9476d9e1c5e 100644 --- a/src/auto-reply/reply/stage-sandbox-media.ts +++ b/src/auto-reply/reply/stage-sandbox-media.ts @@ -12,6 +12,7 @@ import { normalizeScpRemoteHost, normalizeScpRemotePath } from "../../infra/scp- import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; import { resolveChannelRemoteInboundAttachmentRoots } from "../../media/channel-inbound-roots.js"; import { isInboundPathAllowed } from "../../media/inbound-path-policy.js"; +import { resolveInboundMediaReference } from "../../media/media-reference.js"; import { getMediaDir, MEDIA_MAX_BYTES } from "../../media/store.js"; import { normalizeOptionalString } from "../../shared/string-coerce.js"; import { CONFIG_DIR } from "../../utils.js"; @@ -99,8 +100,9 @@ export async function stageSandboxMedia(params: { maxBytes: STAGED_MEDIA_MAX_BYTES, }); } else { + const copySource = await fs.realpath(source).catch(() => source); await stageLocalFileIntoRoot({ - sourcePath: source, + sourcePath: copySource, rootDir: effectiveWorkspaceDir, relativeDestPath: relativeDest, maxBytes: STAGED_MEDIA_MAX_BYTES, @@ -213,21 +215,27 @@ async function isAllowedSourcePath(params: { } return true; } + const inboundReference = await resolveInboundMediaReference(params.source).catch(() => null); + if (inboundReference) { + return true; + } const mediaDir = getMediaDir(); + const canonicalMediaDir = await fs.realpath(mediaDir).catch(() => mediaDir); if ( !isInboundPathAllowed({ filePath: params.source, - roots: [mediaDir], + roots: [mediaDir, canonicalMediaDir], }) ) { logVerbose(`Blocking attempt to stage media from outside media directory: ${params.source}`); return false; } try { + const canonicalSource = await fs.realpath(params.source).catch(() => params.source); await assertSandboxPath({ - filePath: params.source, - cwd: mediaDir, - root: mediaDir, + filePath: canonicalSource, + cwd: canonicalMediaDir, + root: canonicalMediaDir, }); return true; } catch { diff --git a/src/media-understanding/media-understanding-url-fallback.test.ts b/src/media-understanding/media-understanding-url-fallback.test.ts index 9b54a129e9e..97254159078 100644 --- a/src/media-understanding/media-understanding-url-fallback.test.ts +++ b/src/media-understanding/media-understanding-url-fallback.test.ts @@ -19,12 +19,12 @@ async function withBlockedLocalAttachmentFallback( run: (params: { cache: MediaAttachmentCache; fallbackUrl: string }) => Promise, ) { await withTempDir({ prefix }, async (base) => { + const attachmentRoot = path.join(base, "attachment"); const allowedRoot = path.join(base, "allowed"); - const blockedRoot = path.join(base, "blocked"); - const attachmentPath = path.join(blockedRoot, "voice-note.m4a"); + const attachmentPath = path.join(attachmentRoot, "voice-note.m4a"); const fallbackUrl = "https://example.com/fallback.jpg"; + await fs.mkdir(attachmentRoot, { recursive: true }); await fs.mkdir(allowedRoot, { recursive: true }); - await fs.mkdir(blockedRoot, { recursive: true }); await fs.writeFile(attachmentPath, "ok"); const cache = new MediaAttachmentCache( @@ -33,20 +33,12 @@ async function withBlockedLocalAttachmentFallback( localPathRoots: [allowedRoot], }, ); - const originalRealpath = fs.realpath.bind(fs); fetchRemoteMediaMock.mockResolvedValue({ buffer: Buffer.from("fallback-buffer"), contentType: "image/jpeg", fileName: "fallback.jpg", }); - vi.spyOn(fs, "realpath").mockImplementation(async (candidatePath) => { - if (String(candidatePath) === attachmentPath) { - throw new Error("EACCES"); - } - return await originalRealpath(candidatePath); - }); - await run({ cache, fallbackUrl }); }); } diff --git a/src/media/image-ops.tempdir.test.ts b/src/media/image-ops.tempdir.test.ts index 165bd3bb6ba..288f28b8aa9 100644 --- a/src/media/image-ops.tempdir.test.ts +++ b/src/media/image-ops.tempdir.test.ts @@ -22,15 +22,15 @@ describe("image-ops temp dir", () => { }); it("creates sips temp dirs under the secured OpenClaw tmp root", async () => { - const secureRoot = resolvePreferredOpenClawTmpDir(); - const secureRootReal = await fs.realpath(secureRoot); + const secureRoot = await fs.realpath(resolvePreferredOpenClawTmpDir()); await getImageMetadata(Buffer.from("image")); expect(fs.mkdtemp).toHaveBeenCalledTimes(1); - const mkdtempPrefix = vi.mocked(fs.mkdtemp).mock.calls[0]?.[0]; - expect(mkdtempPrefix.startsWith(path.join(secureRootReal, "openclaw-img-"))).toBe(true); - expect(createdTempDir.startsWith(path.join(secureRootReal, "openclaw-img-"))).toBe(true); + const [prefix] = vi.mocked(fs.mkdtemp).mock.calls[0] ?? []; + expect(prefix).toEqual(expect.stringMatching(/^.+openclaw-img-[0-9a-f-]+-$/u)); + expect(path.dirname(prefix ?? "")).toBe(secureRoot); + expect(createdTempDir.startsWith(prefix ?? "")).toBe(true); await expect(fs.access(createdTempDir)).rejects.toMatchObject({ code: "ENOENT" }); }); }); diff --git a/src/media/media-reference.ts b/src/media/media-reference.ts index f83fccfd4d1..dc49117f704 100644 --- a/src/media/media-reference.ts +++ b/src/media/media-reference.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import path from "node:path"; import { safeFileURLToPath } from "../infra/local-file-access.js"; import { resolveUserPath } from "../utils.js"; @@ -86,6 +87,14 @@ function maybeLocalPathFromSource(source: string): string | null { return null; } +async function resolvePathForContainment(candidate: string): Promise { + try { + return await fs.realpath(candidate); + } catch { + return path.resolve(candidate); + } +} + async function resolveInboundMediaUri( normalizedSource: string, ): Promise { @@ -148,9 +157,16 @@ export async function resolveInboundMediaReference( return null; } - const inboundDir = path.resolve(getMediaDir(), "inbound"); - const resolvedPath = path.resolve(localPath); - const rel = path.relative(inboundDir, resolvedPath); + const rawInboundDir = path.resolve(getMediaDir(), "inbound"); + const rawResolvedPath = path.resolve(localPath); + const rawRel = path.relative(rawInboundDir, rawResolvedPath); + const rel = + rawRel && !rawRel.startsWith("..") && !path.isAbsolute(rawRel) + ? rawRel + : path.relative( + await resolvePathForContainment(rawInboundDir), + await resolvePathForContainment(localPath), + ); if (!rel || rel.startsWith("..") || path.isAbsolute(rel) || rel.includes(path.sep)) { return null; }