From 8e4f6c03841bda492f827982d68ce5bd26c0e7fc Mon Sep 17 00:00:00 2001 From: Mariano <132747814+mbelinky@users.noreply.github.com> Date: Fri, 20 Feb 2026 16:36:25 +0000 Subject: [PATCH] fix(browser): block upload symlink escapes (#21972) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: 4381ef9a4d9107798c9c7c00aac62ee81a878789 Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Reviewed-by: @mbelinky --- CHANGELOG.md | 1 + src/agents/tools/browser-tool.ts | 4 +- src/browser/paths.test.ts | 105 ++++++++++++++++++ src/browser/paths.ts | 43 +++++++ src/browser/routes/agent.act.ts | 4 +- .../register.files-downloads.ts | 8 +- 6 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 src/browser/paths.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a40d0b39bf..cd534d80efc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Docs: https://docs.openclaw.ai - iOS/Security: force `https://` for non-loopback manual gateway hosts during iOS onboarding to block insecure remote transport URLs. (#21969) Thanks @mbelinky. - Shared/Security: reject insecure deep links that use `ws://` non-loopback gateway URLs to prevent plaintext remote websocket configuration. (#21970) Thanks @mbelinky. - macOS/Security: reject non-loopback `ws://` remote gateway URLs in macOS remote config to block insecure plaintext websocket endpoints. (#21971) Thanks @mbelinky. +- Browser/Security: block upload path symlink escapes so browser upload sources cannot traverse outside the allowed workspace via symlinked paths. (#21972) Thanks @mbelinky. - CLI/Onboarding: fix Anthropic-compatible custom provider verification by normalizing base URLs to avoid duplicate `/v1` paths during setup checks. (#21336) Thanks @17jmumford. - Security/Dependencies: bump transitive `hono` usage to `4.11.10` to incorporate timing-safe authentication comparison hardening for `basicAuth`/`bearerAuth` (`GHSA-gq3j-xvxp-8hrf`). Thanks @vincentkoc. diff --git a/src/agents/tools/browser-tool.ts b/src/agents/tools/browser-tool.ts index e7fb904b2be..9545ca3bcaa 100644 --- a/src/agents/tools/browser-tool.ts +++ b/src/agents/tools/browser-tool.ts @@ -21,7 +21,7 @@ import { } from "../../browser/client.js"; import { resolveBrowserConfig } from "../../browser/config.js"; import { DEFAULT_AI_SNAPSHOT_MAX_CHARS } from "../../browser/constants.js"; -import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js"; +import { DEFAULT_UPLOAD_DIR, resolveExistingPathsWithinRoot } from "../../browser/paths.js"; import { applyBrowserProxyPaths, persistBrowserProxyFiles } from "../../browser/proxy-files.js"; import { loadConfig } from "../../config/config.js"; import { wrapExternalContent } from "../../security/external-content.js"; @@ -700,7 +700,7 @@ export function createBrowserTool(opts?: { if (paths.length === 0) { throw new Error("paths required"); } - const uploadPathsResult = resolvePathsWithinRoot({ + const uploadPathsResult = await resolveExistingPathsWithinRoot({ rootDir: DEFAULT_UPLOAD_DIR, requestedPaths: paths, scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, diff --git a/src/browser/paths.test.ts b/src/browser/paths.test.ts new file mode 100644 index 00000000000..0ece74c4893 --- /dev/null +++ b/src/browser/paths.test.ts @@ -0,0 +1,105 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { resolveExistingPathsWithinRoot } from "./paths.js"; + +async function createFixtureRoot(): Promise<{ baseDir: string; uploadsDir: string }> { + const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-browser-paths-")); + const uploadsDir = path.join(baseDir, "uploads"); + await fs.mkdir(uploadsDir, { recursive: true }); + return { baseDir, uploadsDir }; +} + +describe("resolveExistingPathsWithinRoot", () => { + const cleanupDirs = new Set(); + + afterEach(async () => { + await Promise.all( + Array.from(cleanupDirs).map(async (dir) => { + await fs.rm(dir, { recursive: true, force: true }); + }), + ); + cleanupDirs.clear(); + }); + + it("accepts existing files under the upload root", async () => { + const { baseDir, uploadsDir } = await createFixtureRoot(); + cleanupDirs.add(baseDir); + + const nestedDir = path.join(uploadsDir, "nested"); + await fs.mkdir(nestedDir, { recursive: true }); + const filePath = path.join(nestedDir, "ok.txt"); + await fs.writeFile(filePath, "ok", "utf8"); + + const result = await resolveExistingPathsWithinRoot({ + rootDir: uploadsDir, + requestedPaths: [filePath], + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.paths).toEqual([await fs.realpath(filePath)]); + } + }); + + it("rejects traversal outside the upload root", async () => { + const { baseDir, uploadsDir } = await createFixtureRoot(); + cleanupDirs.add(baseDir); + + const outsidePath = path.join(baseDir, "outside.txt"); + await fs.writeFile(outsidePath, "nope", "utf8"); + + const result = await resolveExistingPathsWithinRoot({ + rootDir: uploadsDir, + requestedPaths: ["../outside.txt"], + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("must stay within uploads directory"); + } + }); + + it("keeps lexical in-root paths when files do not exist yet", async () => { + const { baseDir, uploadsDir } = await createFixtureRoot(); + cleanupDirs.add(baseDir); + + const result = await resolveExistingPathsWithinRoot({ + rootDir: uploadsDir, + requestedPaths: ["missing.txt"], + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.paths).toEqual([path.join(uploadsDir, "missing.txt")]); + } + }); + + it.runIf(process.platform !== "win32")( + "rejects symlink escapes outside upload root", + async () => { + const { baseDir, uploadsDir } = await createFixtureRoot(); + cleanupDirs.add(baseDir); + + const outsidePath = path.join(baseDir, "secret.txt"); + await fs.writeFile(outsidePath, "secret", "utf8"); + const symlinkPath = path.join(uploadsDir, "leak.txt"); + await fs.symlink(outsidePath, symlinkPath); + + const result = await resolveExistingPathsWithinRoot({ + rootDir: uploadsDir, + requestedPaths: ["leak.txt"], + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("regular non-symlink file"); + } + }, + ); +}); diff --git a/src/browser/paths.ts b/src/browser/paths.ts index 5d91c8287b6..3af2bd149e1 100644 --- a/src/browser/paths.ts +++ b/src/browser/paths.ts @@ -1,4 +1,5 @@ import path from "node:path"; +import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir(); @@ -47,3 +48,45 @@ export function resolvePathsWithinRoot(params: { } return { ok: true, paths: resolvedPaths }; } + +export async function resolveExistingPathsWithinRoot(params: { + rootDir: string; + requestedPaths: string[]; + scopeLabel: string; +}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> { + const resolvedPaths: string[] = []; + for (const raw of params.requestedPaths) { + const pathResult = resolvePathWithinRoot({ + rootDir: params.rootDir, + requestedPath: raw, + scopeLabel: params.scopeLabel, + }); + if (!pathResult.ok) { + return { ok: false, error: pathResult.error }; + } + + const rootDir = path.resolve(params.rootDir); + const relativePath = path.relative(rootDir, pathResult.path); + let opened: Awaited> | undefined; + try { + opened = await openFileWithinRoot({ + rootDir, + relativePath, + }); + resolvedPaths.push(opened.realPath); + } catch (err) { + if (err instanceof SafeOpenError && err.code === "not-found") { + // Preserve historical behavior for paths that do not exist yet. + resolvedPaths.push(pathResult.path); + continue; + } + return { + ok: false, + error: `Invalid path: must stay within ${params.scopeLabel} and be a regular non-symlink file`, + }; + } finally { + await opened?.handle.close().catch(() => {}); + } + } + return { ok: true, paths: resolvedPaths }; +} diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index b0c393eae6a..78fa2f6856c 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -16,7 +16,7 @@ import { DEFAULT_DOWNLOAD_DIR, DEFAULT_UPLOAD_DIR, resolvePathWithinRoot, - resolvePathsWithinRoot, + resolveExistingPathsWithinRoot, } from "./path-output.js"; import type { BrowserResponse, BrowserRouteRegistrar } from "./types.js"; import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js"; @@ -382,7 +382,7 @@ export function registerBrowserAgentActRoutes( targetId, feature: "file chooser hook", run: async ({ cdpUrl, tab, pw }) => { - const uploadPathsResult = resolvePathsWithinRoot({ + const uploadPathsResult = await resolveExistingPathsWithinRoot({ rootDir: DEFAULT_UPLOAD_DIR, requestedPaths: paths, scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, diff --git a/src/cli/browser-cli-actions-input/register.files-downloads.ts b/src/cli/browser-cli-actions-input/register.files-downloads.ts index 38c2c089d5c..af12682e31e 100644 --- a/src/cli/browser-cli-actions-input/register.files-downloads.ts +++ b/src/cli/browser-cli-actions-input/register.files-downloads.ts @@ -1,13 +1,13 @@ import type { Command } from "commander"; -import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js"; +import { DEFAULT_UPLOAD_DIR, resolveExistingPathsWithinRoot } from "../../browser/paths.js"; import { danger } from "../../globals.js"; import { defaultRuntime } from "../../runtime.js"; import { shortenHomePath } from "../../utils.js"; import { callBrowserRequest, type BrowserParentOpts } from "../browser-cli-shared.js"; import { resolveBrowserActionContext } from "./shared.js"; -function normalizeUploadPaths(paths: string[]): string[] { - const result = resolvePathsWithinRoot({ +async function normalizeUploadPaths(paths: string[]): Promise { + const result = await resolveExistingPathsWithinRoot({ rootDir: DEFAULT_UPLOAD_DIR, requestedPaths: paths, scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, @@ -81,7 +81,7 @@ export function registerBrowserFilesAndDownloadsCommands( .action(async (paths: string[], opts, cmd) => { const { parent, profile } = resolveBrowserActionContext(cmd, parentOpts); try { - const normalizedPaths = normalizeUploadPaths(paths); + const normalizedPaths = await normalizeUploadPaths(paths); const { timeoutMs, targetId } = resolveTimeoutAndTarget(opts); const result = await callBrowserRequest<{ download: { path: string } }>( parent,