From 4a80311628fb6020deced138c6f9774dd8e1aaf1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 16:53:10 +0000 Subject: [PATCH] refactor(security): split sandbox media staging and stream safe copies --- ...bound-media-into-sandbox-workspace.test.ts | 33 ++ src/auto-reply/reply/stage-sandbox-media.ts | 330 ++++++++++-------- src/infra/fs-safe.test.ts | 37 ++ src/infra/fs-safe.ts | 100 +++++- 4 files changed, 350 insertions(+), 150 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 7c853fd1771..0b766e003f4 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 @@ -1,6 +1,7 @@ import fs from "node:fs/promises"; import { basename, join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { MEDIA_MAX_BYTES } from "../media/store.js"; import { createSandboxMediaContexts, createSandboxMediaStageConfig, @@ -141,4 +142,36 @@ describe("stageSandboxMedia", () => { expect(sessionCtx.MediaPath).toBe(mediaPath); }); }); + + it("skips oversized media staging and keeps original media paths", async () => { + await withSandboxMediaTempHome("openclaw-triggers-", async (home) => { + const cfg = createSandboxMediaStageConfig(home); + const workspaceDir = join(home, "openclaw"); + const sandboxDir = join(home, "sandboxes", "session"); + vi.mocked(ensureSandboxWorkspaceForSession).mockResolvedValue({ + workspaceDir: sandboxDir, + containerWorkdir: "/work", + }); + + const inboundDir = join(home, ".openclaw", "media", "inbound"); + await fs.mkdir(inboundDir, { recursive: true }); + const mediaPath = join(inboundDir, "oversized.bin"); + await fs.writeFile(mediaPath, Buffer.alloc(MEDIA_MAX_BYTES + 1, 0x41)); + + const { ctx, sessionCtx } = createSandboxMediaContexts(mediaPath); + await stageSandboxMedia({ + ctx, + sessionCtx, + cfg, + sessionKey: "agent:main:main", + workspaceDir, + }); + + await expect( + fs.stat(join(sandboxDir, "media", "inbound", basename(mediaPath))), + ).rejects.toThrow(); + expect(ctx.MediaPath).toBe(mediaPath); + expect(sessionCtx.MediaPath).toBe(mediaPath); + }); + }); }); diff --git a/src/auto-reply/reply/stage-sandbox-media.ts b/src/auto-reply/reply/stage-sandbox-media.ts index 12719c9b96c..d364fa6a554 100644 --- a/src/auto-reply/reply/stage-sandbox-media.ts +++ b/src/auto-reply/reply/stage-sandbox-media.ts @@ -6,17 +6,19 @@ import { assertSandboxPath } from "../../agents/sandbox-paths.js"; import { ensureSandboxWorkspaceForSession } from "../../agents/sandbox.js"; import type { OpenClawConfig } from "../../config/config.js"; import { logVerbose } from "../../globals.js"; -import { readLocalFileSafely, writeFileWithinRoot } from "../../infra/fs-safe.js"; +import { copyFileWithinRoot, SafeOpenError } from "../../infra/fs-safe.js"; import { normalizeScpRemoteHost } from "../../infra/scp-host.js"; import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; import { isInboundPathAllowed, resolveIMessageRemoteAttachmentRoots, } from "../../media/inbound-path-policy.js"; -import { getMediaDir } from "../../media/store.js"; +import { getMediaDir, MEDIA_MAX_BYTES } from "../../media/store.js"; import { CONFIG_DIR } from "../../utils.js"; import type { MsgContext, TemplateContext } from "../templating.js"; +const STAGED_MEDIA_MAX_BYTES = MEDIA_MAX_BYTES; + export async function stageSandboxMedia(params: { ctx: MsgContext; sessionCtx: TemplateContext; @@ -26,13 +28,7 @@ export async function stageSandboxMedia(params: { }) { const { ctx, sessionCtx, cfg, sessionKey, workspaceDir } = params; const hasPathsArray = Array.isArray(ctx.MediaPaths) && ctx.MediaPaths.length > 0; - const pathsFromArray = Array.isArray(ctx.MediaPaths) ? ctx.MediaPaths : undefined; - const rawPaths = - pathsFromArray && pathsFromArray.length > 0 - ? pathsFromArray - : ctx.MediaPath?.trim() - ? [ctx.MediaPath.trim()] - : []; + const rawPaths = resolveRawPaths(ctx); if (rawPaths.length === 0 || !sessionKey) { return; } @@ -52,165 +48,88 @@ export async function stageSandboxMedia(params: { return; } - const resolveAbsolutePath = (value: string): string | null => { - let resolved = value.trim(); - if (!resolved) { - return null; - } - if (resolved.startsWith("file://")) { - try { - resolved = fileURLToPath(resolved); - } catch { - return null; - } - } - if (!path.isAbsolute(resolved)) { - return null; - } - return resolved; - }; + await fs.mkdir(effectiveWorkspaceDir, { recursive: true }); + const remoteAttachmentRoots = resolveIMessageRemoteAttachmentRoots({ + cfg, + accountId: ctx.AccountId, + }); - try { - await fs.mkdir(effectiveWorkspaceDir, { recursive: true }); - const remoteAttachmentRoots = resolveIMessageRemoteAttachmentRoots({ - cfg, - accountId: ctx.AccountId, + const usedNames = new Set(); + const staged = new Map(); // absolute source -> relative sandbox path + + for (const raw of rawPaths) { + const source = resolveAbsolutePath(raw); + if (!source || staged.has(source)) { + continue; + } + const allowed = await isAllowedSourcePath({ + source, + mediaRemoteHost: ctx.MediaRemoteHost, + remoteAttachmentRoots, }); + if (!allowed) { + continue; + } + const fileName = allocateStagedFileName(source, usedNames); + if (!fileName) { + continue; + } + const relativeDest = sandbox ? path.join("media", "inbound", fileName) : fileName; + const dest = path.join(effectiveWorkspaceDir, relativeDest); - const usedNames = new Set(); - const staged = new Map(); // absolute source -> relative sandbox path - - for (const raw of rawPaths) { - const source = resolveAbsolutePath(raw); - if (!source) { - continue; - } - if (staged.has(source)) { - continue; - } - - if ( - ctx.MediaRemoteHost && - !isInboundPathAllowed({ - filePath: source, - roots: remoteAttachmentRoots, - }) - ) { - logVerbose(`Blocking remote media staging from disallowed attachment path: ${source}`); - continue; - } - - // Local paths must be restricted to the media directory. - if (!ctx.MediaRemoteHost) { - const mediaDir = getMediaDir(); - if ( - !isInboundPathAllowed({ - filePath: source, - roots: [mediaDir], - }) - ) { - logVerbose(`Blocking attempt to stage media from outside media directory: ${source}`); - continue; - } - try { - await assertSandboxPath({ - filePath: source, - cwd: mediaDir, - root: mediaDir, - }); - } catch { - logVerbose(`Blocking attempt to stage media from outside media directory: ${source}`); - continue; - } - } - - const baseName = path.basename(source); - if (!baseName) { - continue; - } - const parsed = path.parse(baseName); - let fileName = baseName; - let suffix = 1; - while (usedNames.has(fileName)) { - fileName = `${parsed.name}-${suffix}${parsed.ext}`; - suffix += 1; - } - usedNames.add(fileName); - - const relativeDest = sandbox ? path.join("media", "inbound", fileName) : fileName; - const dest = path.join(effectiveWorkspaceDir, relativeDest); + try { if (ctx.MediaRemoteHost) { - // Remote media arrives via SCP to a temp file, then we write into root with alias guards. await stageRemoteFileIntoRoot({ remoteHost: ctx.MediaRemoteHost, remotePath: source, rootDir: effectiveWorkspaceDir, relativeDestPath: relativeDest, + maxBytes: STAGED_MEDIA_MAX_BYTES, }); } else { await stageLocalFileIntoRoot({ sourcePath: source, rootDir: effectiveWorkspaceDir, relativeDestPath: relativeDest, + maxBytes: STAGED_MEDIA_MAX_BYTES, }); } - // For sandbox use relative path, for remote cache use absolute path - const stagedPath = sandbox ? path.posix.join("media", "inbound", fileName) : dest; - staged.set(source, stagedPath); + } catch (err) { + if (err instanceof SafeOpenError && err.code === "too-large") { + logVerbose( + `Blocking inbound media staging above ${STAGED_MEDIA_MAX_BYTES} bytes: ${source}`, + ); + } else { + logVerbose(`Failed to stage inbound media path ${source}: ${String(err)}`); + } + continue; } - const rewriteIfStaged = (value: string | undefined): string | undefined => { - const raw = value?.trim(); - if (!raw) { - return value; - } - const abs = resolveAbsolutePath(raw); - if (!abs) { - return value; - } - const mapped = staged.get(abs); - return mapped ?? value; - }; - - const nextMediaPaths = hasPathsArray ? rawPaths.map((p) => rewriteIfStaged(p) ?? p) : undefined; - if (nextMediaPaths) { - ctx.MediaPaths = nextMediaPaths; - sessionCtx.MediaPaths = nextMediaPaths; - ctx.MediaPath = nextMediaPaths[0]; - sessionCtx.MediaPath = nextMediaPaths[0]; - } else { - const rewritten = rewriteIfStaged(ctx.MediaPath); - if (rewritten && rewritten !== ctx.MediaPath) { - ctx.MediaPath = rewritten; - sessionCtx.MediaPath = rewritten; - } - } - - if (Array.isArray(ctx.MediaUrls) && ctx.MediaUrls.length > 0) { - const nextUrls = ctx.MediaUrls.map((u) => rewriteIfStaged(u) ?? u); - ctx.MediaUrls = nextUrls; - sessionCtx.MediaUrls = nextUrls; - } - const rewrittenUrl = rewriteIfStaged(ctx.MediaUrl); - if (rewrittenUrl && rewrittenUrl !== ctx.MediaUrl) { - ctx.MediaUrl = rewrittenUrl; - sessionCtx.MediaUrl = rewrittenUrl; - } - } catch (err) { - logVerbose(`Failed to stage inbound media for sandbox: ${String(err)}`); + // For sandbox use relative path, for remote cache use absolute path + const stagedPath = sandbox ? path.posix.join("media", "inbound", fileName) : dest; + staged.set(source, stagedPath); } + + rewriteStagedMediaPaths({ + ctx, + sessionCtx, + rawPaths, + staged, + hasPathsArray, + }); } async function stageLocalFileIntoRoot(params: { sourcePath: string; rootDir: string; relativeDestPath: string; + maxBytes?: number; }): Promise { - const safeRead = await readLocalFileSafely({ filePath: params.sourcePath }); - await writeFileWithinRoot({ + await copyFileWithinRoot({ + sourcePath: params.sourcePath, rootDir: params.rootDir, relativePath: params.relativeDestPath, - data: safeRead.buffer, + maxBytes: params.maxBytes, }); } @@ -219,6 +138,7 @@ async function stageRemoteFileIntoRoot(params: { remotePath: string; rootDir: string; relativeDestPath: string; + maxBytes?: number; }): Promise { const tmpRoot = resolvePreferredOpenClawTmpDir(); await fs.mkdir(tmpRoot, { recursive: true }); @@ -230,12 +150,144 @@ async function stageRemoteFileIntoRoot(params: { sourcePath: tmpPath, rootDir: params.rootDir, relativeDestPath: params.relativeDestPath, + maxBytes: params.maxBytes, }); } finally { await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); } } +function resolveRawPaths(ctx: MsgContext): string[] { + const pathsFromArray = Array.isArray(ctx.MediaPaths) ? ctx.MediaPaths : undefined; + return pathsFromArray && pathsFromArray.length > 0 + ? pathsFromArray + : ctx.MediaPath?.trim() + ? [ctx.MediaPath.trim()] + : []; +} + +function resolveAbsolutePath(value: string): string | null { + let resolved = value.trim(); + if (!resolved) { + return null; + } + if (resolved.startsWith("file://")) { + try { + resolved = fileURLToPath(resolved); + } catch { + return null; + } + } + if (!path.isAbsolute(resolved)) { + return null; + } + return resolved; +} + +async function isAllowedSourcePath(params: { + source: string; + mediaRemoteHost?: string; + remoteAttachmentRoots: string[]; +}): Promise { + if (params.mediaRemoteHost) { + if ( + !isInboundPathAllowed({ + filePath: params.source, + roots: params.remoteAttachmentRoots, + }) + ) { + logVerbose(`Blocking remote media staging from disallowed attachment path: ${params.source}`); + return false; + } + return true; + } + const mediaDir = getMediaDir(); + if ( + !isInboundPathAllowed({ + filePath: params.source, + roots: [mediaDir], + }) + ) { + logVerbose(`Blocking attempt to stage media from outside media directory: ${params.source}`); + return false; + } + try { + await assertSandboxPath({ + filePath: params.source, + cwd: mediaDir, + root: mediaDir, + }); + return true; + } catch { + logVerbose(`Blocking attempt to stage media from outside media directory: ${params.source}`); + return false; + } +} + +function allocateStagedFileName(source: string, usedNames: Set): string | null { + const baseName = path.basename(source); + if (!baseName) { + return null; + } + const parsed = path.parse(baseName); + let fileName = baseName; + let suffix = 1; + while (usedNames.has(fileName)) { + fileName = `${parsed.name}-${suffix}${parsed.ext}`; + suffix += 1; + } + usedNames.add(fileName); + return fileName; +} + +function rewriteStagedMediaPaths(params: { + ctx: MsgContext; + sessionCtx: TemplateContext; + rawPaths: string[]; + staged: Map; + hasPathsArray: boolean; +}): void { + const rewriteIfStaged = (value: string | undefined): string | undefined => { + const raw = value?.trim(); + if (!raw) { + return value; + } + const abs = resolveAbsolutePath(raw); + if (!abs) { + return value; + } + const mapped = params.staged.get(abs); + return mapped ?? value; + }; + + const nextMediaPaths = params.hasPathsArray + ? params.rawPaths.map((p) => rewriteIfStaged(p) ?? p) + : undefined; + if (nextMediaPaths) { + params.ctx.MediaPaths = nextMediaPaths; + params.sessionCtx.MediaPaths = nextMediaPaths; + params.ctx.MediaPath = nextMediaPaths[0]; + params.sessionCtx.MediaPath = nextMediaPaths[0]; + } else { + const rewritten = rewriteIfStaged(params.ctx.MediaPath); + if (rewritten && rewritten !== params.ctx.MediaPath) { + params.ctx.MediaPath = rewritten; + params.sessionCtx.MediaPath = rewritten; + } + } + + if (Array.isArray(params.ctx.MediaUrls) && params.ctx.MediaUrls.length > 0) { + const nextUrls = params.ctx.MediaUrls.map((u) => rewriteIfStaged(u) ?? u); + params.ctx.MediaUrls = nextUrls; + params.sessionCtx.MediaUrls = nextUrls; + } + const rewrittenUrl = rewriteIfStaged(params.ctx.MediaUrl); + if (rewrittenUrl && rewrittenUrl !== params.ctx.MediaUrl) { + params.ctx.MediaUrl = rewrittenUrl; + params.sessionCtx.MediaUrl = rewrittenUrl; + } +} + async function scpFile(remoteHost: string, remotePath: string, localPath: string): Promise { const safeRemoteHost = normalizeScpRemoteHost(remoteHost); if (!safeRemoteHost) { diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index 9235af9d659..ac9f3df78eb 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -3,6 +3,7 @@ import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { + copyFileWithinRoot, createRootScopedReadFile, SafeOpenError, openFileWithinRoot, @@ -176,6 +177,42 @@ describe("fs-safe", () => { await expect(fs.readFile(path.join(root, "nested", "out.txt"), "utf8")).resolves.toBe("hello"); }); + it("copies a file within root safely", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); + const sourcePath = path.join(sourceDir, "in.txt"); + await fs.writeFile(sourcePath, "copy-ok"); + + await copyFileWithinRoot({ + sourcePath, + rootDir: root, + relativePath: "nested/copied.txt", + }); + + await expect(fs.readFile(path.join(root, "nested", "copied.txt"), "utf8")).resolves.toBe( + "copy-ok", + ); + }); + + it("enforces maxBytes when copying into root", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); + const sourcePath = path.join(sourceDir, "big.bin"); + await fs.writeFile(sourcePath, Buffer.alloc(8)); + + await expect( + copyFileWithinRoot({ + sourcePath, + rootDir: root, + relativePath: "nested/big.bin", + maxBytes: 4, + }), + ).rejects.toMatchObject({ code: "too-large" }); + await expect(fs.stat(path.join(root, "nested", "big.bin"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + it("rejects write traversal outside root", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); await expect( diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index bc9d38c6d17..5c7628ace78 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -4,6 +4,7 @@ import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { pipeline } from "node:stream/promises"; import { sameFileIdentity } from "./file-identity.js"; import { expandHomePrefix } from "./home-dir.js"; import { assertNoPathAliasEscape } from "./path-alias-guards.js"; @@ -282,13 +283,15 @@ async function readOpenedFileSafely(params: { }; } -export async function writeFileWithinRoot(params: { +async function openWritableFileWithinRoot(params: { rootDir: string; relativePath: string; - data: string | Buffer; - encoding?: BufferEncoding; mkdir?: boolean; -}): Promise { +}): Promise<{ + handle: FileHandle; + createdForWrite: boolean; + openedRealPath: string; +}> { const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); try { await assertNoPathAliasEscape({ @@ -372,17 +375,92 @@ export async function writeFileWithinRoot(params: { if (!createdForWrite) { await handle.truncate(0); } - if (typeof params.data === "string") { - await handle.writeFile(params.data, params.encoding ?? "utf8"); - } else { - await handle.writeFile(params.data); - } + return { + handle, + createdForWrite, + openedRealPath: realPath, + }; } catch (err) { if (createdForWrite && err instanceof SafeOpenError && openedRealPath) { await fs.rm(openedRealPath, { force: true }).catch(() => {}); } - throw err; - } finally { await handle.close().catch(() => {}); + throw err; + } +} + +export async function writeFileWithinRoot(params: { + rootDir: string; + relativePath: string; + data: string | Buffer; + encoding?: BufferEncoding; + mkdir?: boolean; +}): Promise { + const target = await openWritableFileWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + mkdir: params.mkdir, + }); + try { + if (typeof params.data === "string") { + await target.handle.writeFile(params.data, params.encoding ?? "utf8"); + } else { + await target.handle.writeFile(params.data); + } + } finally { + await target.handle.close().catch(() => {}); + } +} + +export async function copyFileWithinRoot(params: { + sourcePath: string; + rootDir: string; + relativePath: string; + maxBytes?: number; + mkdir?: boolean; +}): Promise { + const source = await openVerifiedLocalFile(params.sourcePath); + if (params.maxBytes !== undefined && source.stat.size > params.maxBytes) { + await source.handle.close().catch(() => {}); + throw new SafeOpenError( + "too-large", + `file exceeds limit of ${params.maxBytes} bytes (got ${source.stat.size})`, + ); + } + + let target: { + handle: FileHandle; + createdForWrite: boolean; + openedRealPath: string; + } | null = null; + let sourceClosedByStream = false; + let targetClosedByStream = false; + try { + target = await openWritableFileWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + mkdir: params.mkdir, + }); + const sourceStream = source.handle.createReadStream(); + const targetStream = target.handle.createWriteStream(); + sourceStream.once("close", () => { + sourceClosedByStream = true; + }); + targetStream.once("close", () => { + targetClosedByStream = true; + }); + await pipeline(sourceStream, targetStream); + } catch (err) { + if (target?.createdForWrite) { + await fs.rm(target.openedRealPath, { force: true }).catch(() => {}); + } + throw err; + } finally { + if (!sourceClosedByStream) { + await source.handle.close().catch(() => {}); + } + if (target && !targetClosedByStream) { + await target.handle.close().catch(() => {}); + } } }