From 1e94fce22fb1af22bdca78a9c492f0f64db0fb99 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 12:57:10 +0100 Subject: [PATCH] fix(browser): confine upload paths for file chooser --- CHANGELOG.md | 1 + docs/tools/browser.md | 4 +- src/agents/tools/browser-tool.ts | 21 +++++++- src/browser/routes/agent.act.ts | 20 +++++-- src/browser/routes/path-output.ts | 1 + ...-contract-form-layout-act-commands.test.ts | 53 +++++++++++++++++-- .../register.files-downloads.ts | 28 +++++++++- 7 files changed, 115 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d5ea9626aa..91df8427de3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,7 @@ Docs: https://docs.openclaw.ai - Security/Gateway: breaking default-behavior change - canvas IP-based auth fallback now only accepts machine-scoped addresses (RFC1918, link-local, ULA IPv6, CGNAT); public-source IP matches now require bearer token auth. (#14661) Thanks @sumleo. - Security/Link understanding: block loopback/internal host patterns and private/mapped IPv6 addresses in extracted URL handling to close SSRF bypasses in link CLI flows. (#15604) Thanks @AI-Reviewer-QS. - Security/Browser: constrain `POST /trace/stop`, `POST /wait/download`, and `POST /download` output paths to OpenClaw temp roots and reject traversal/escape paths. +- Security/Browser: confine `POST /hooks/file-chooser` upload paths to an OpenClaw temp uploads root and reject traversal/escape paths. Thanks @1seal. - Security/Canvas: serve A2UI assets via the shared safe-open path (`openFileWithinRoot`) to close traversal/TOCTOU gaps, with traversal and symlink regression coverage. (#10525) Thanks @abdelsfane. - Security/WhatsApp: enforce `0o600` on `creds.json` and `creds.json.bak` on save/backup/restore paths to reduce credential file exposure. (#10529) Thanks @abdelsfane. - Security/Gateway: sanitize and truncate untrusted WebSocket header values in pre-handshake close logs to reduce log-poisoning risk. Thanks @thewilloftheshadow. diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 107c92b9911..74f42472439 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -411,7 +411,7 @@ Actions: - `openclaw browser select 9 OptionA OptionB` - `openclaw browser download e12 report.pdf` - `openclaw browser waitfordownload report.pdf` -- `openclaw browser upload /tmp/file.pdf` +- `openclaw browser upload /tmp/openclaw/uploads/file.pdf` - `openclaw browser fill --fields '[{"ref":"1","type":"text","value":"Ada"}]'` - `openclaw browser dialog --accept` - `openclaw browser wait --text "Done"` @@ -447,6 +447,8 @@ Notes: - Download and trace output paths are constrained to OpenClaw temp roots: - traces: `/tmp/openclaw` (fallback: `${os.tmpdir()}/openclaw`) - downloads: `/tmp/openclaw/downloads` (fallback: `${os.tmpdir()}/openclaw/downloads`) +- Upload paths are constrained to an OpenClaw temp uploads root: + - uploads: `/tmp/openclaw/uploads` (fallback: `${os.tmpdir()}/openclaw/uploads`) - `upload` can also set file inputs directly via `--input-ref` or `--element`. - `snapshot`: - `--format ai` (default when Playwright is installed): returns an AI snapshot with numeric refs (`aria-ref=""`). diff --git a/src/agents/tools/browser-tool.ts b/src/agents/tools/browser-tool.ts index eeb2dae5026..71d55e3a99d 100644 --- a/src/agents/tools/browser-tool.ts +++ b/src/agents/tools/browser-tool.ts @@ -1,4 +1,5 @@ import crypto from "node:crypto"; +import path from "node:path"; import { browserAct, browserArmDialog, @@ -22,6 +23,7 @@ import { import { resolveBrowserConfig } from "../../browser/config.js"; import { DEFAULT_AI_SNAPSHOT_MAX_CHARS } from "../../browser/constants.js"; import { loadConfig } from "../../config/config.js"; +import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; import { saveMediaBuffer } from "../../media/store.js"; import { wrapExternalContent } from "../../security/external-content.js"; import { BrowserToolSchema } from "./browser-tool.schema.js"; @@ -724,6 +726,21 @@ export function createBrowserTool(opts?: { if (paths.length === 0) { throw new Error("paths required"); } + const uploadRoot = path.resolve(path.join(resolvePreferredOpenClawTmpDir(), "uploads")); + const normalizedPaths = paths.map((p) => { + const raw = String(p ?? "").trim(); + if (!raw) { + throw new Error("upload path is empty"); + } + const resolved = path.resolve(uploadRoot, raw); + const rel = path.relative(uploadRoot, resolved); + if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { + throw new Error( + `Invalid upload path: must stay within ${uploadRoot} (copy files there first)`, + ); + } + return resolved; + }); const ref = readStringParam(params, "ref"); const inputRef = readStringParam(params, "inputRef"); const element = readStringParam(params, "element"); @@ -738,7 +755,7 @@ export function createBrowserTool(opts?: { path: "/hooks/file-chooser", profile, body: { - paths, + paths: normalizedPaths, ref, inputRef, element, @@ -750,7 +767,7 @@ export function createBrowserTool(opts?: { } return jsonResult( await browserArmFileChooser(baseUrl, { - paths, + paths: normalizedPaths, ref, inputRef, element, diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index 6c6e31153b0..233271dd69f 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -14,7 +14,7 @@ import { resolveProfileContext, SELECTOR_UNSUPPORTED_MESSAGE, } from "./agent.shared.js"; -import { DEFAULT_DOWNLOAD_DIR, resolvePathWithinRoot } from "./path-output.js"; +import { DEFAULT_DOWNLOAD_DIR, DEFAULT_UPLOAD_DIR, resolvePathWithinRoot } from "./path-output.js"; import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js"; export function registerBrowserAgentActRoutes( @@ -355,6 +355,20 @@ export function registerBrowserAgentActRoutes( return jsonError(res, 400, "paths are required"); } try { + const resolvedPaths: string[] = []; + for (const raw of paths) { + const pathResult = resolvePathWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPath: raw, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!pathResult.ok) { + res.status(400).json({ error: pathResult.error }); + return; + } + resolvedPaths.push(pathResult.path); + } + const tab = await profileCtx.ensureTabAvailable(targetId); const pw = await requirePwAi(res, "file chooser hook"); if (!pw) { @@ -369,13 +383,13 @@ export function registerBrowserAgentActRoutes( targetId: tab.targetId, inputRef, element, - paths, + paths: resolvedPaths, }); } else { await pw.armFileUploadViaPlaywright({ cdpUrl: profileCtx.profile.cdpUrl, targetId: tab.targetId, - paths, + paths: resolvedPaths, timeoutMs: timeoutMs ?? undefined, }); if (ref) { diff --git a/src/browser/routes/path-output.ts b/src/browser/routes/path-output.ts index 137b625210e..38680076829 100644 --- a/src/browser/routes/path-output.ts +++ b/src/browser/routes/path-output.ts @@ -4,6 +4,7 @@ import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js" export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir(); export const DEFAULT_TRACE_DIR = DEFAULT_BROWSER_TMP_DIR; export const DEFAULT_DOWNLOAD_DIR = path.join(DEFAULT_BROWSER_TMP_DIR, "downloads"); +export const DEFAULT_UPLOAD_DIR = path.join(DEFAULT_BROWSER_TMP_DIR, "uploads"); export function resolvePathWithinRoot(params: { rootDir: string; diff --git a/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/src/browser/server.agent-contract-form-layout-act-commands.test.ts index 2c5c2234740..8809393818b 100644 --- a/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -1,6 +1,8 @@ import { type AddressInfo, createServer } from "node:net"; +import path from "node:path"; import { fetch as realFetch } from "undici"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_UPLOAD_DIR } from "./routes/path-output.js"; let testPort = 0; let cdpBaseUrl = ""; @@ -399,35 +401,55 @@ describe("browser control server", () => { it("agent contract: hooks + response + downloads + screenshot", async () => { const base = await startServerAndBase(); + const uploadA = path.join(DEFAULT_UPLOAD_DIR, "a.txt"); const upload = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/a.txt"], + paths: [uploadA], timeoutMs: 1234, }); expect(upload).toMatchObject({ ok: true }); expect(pwMocks.armFileUploadViaPlaywright).toHaveBeenCalledWith({ cdpUrl: cdpBaseUrl, targetId: "abcd1234", - paths: ["/tmp/a.txt"], + paths: [uploadA], timeoutMs: 1234, }); + const uploadB = path.join(DEFAULT_UPLOAD_DIR, "b.txt"); const uploadWithRef = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/b.txt"], + paths: [uploadB], ref: "e12", }); expect(uploadWithRef).toMatchObject({ ok: true }); + const uploadC = path.join(DEFAULT_UPLOAD_DIR, "c.txt"); const uploadWithInputRef = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/c.txt"], + paths: [uploadC], inputRef: "e99", }); expect(uploadWithInputRef).toMatchObject({ ok: true }); + expect(pwMocks.setInputFilesViaPlaywright).toHaveBeenCalledWith( + expect.objectContaining({ + cdpUrl: cdpBaseUrl, + targetId: "abcd1234", + inputRef: "e99", + paths: [uploadC], + }), + ); + const uploadD = path.join(DEFAULT_UPLOAD_DIR, "d.txt"); const uploadWithElement = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/d.txt"], + paths: [uploadD], element: "input[type=file]", }); expect(uploadWithElement).toMatchObject({ ok: true }); + expect(pwMocks.setInputFilesViaPlaywright).toHaveBeenCalledWith( + expect.objectContaining({ + cdpUrl: cdpBaseUrl, + targetId: "abcd1234", + element: "input[type=file]", + paths: [uploadD], + }), + ); const dialog = await postJson(`${base}/hooks/dialog`, { accept: true, @@ -508,6 +530,27 @@ describe("browser control server", () => { ); }); + it("hooks/file-chooser rejects traversal path outside uploads dir", async () => { + const base = await startServerAndBase(); + const res = await postJson<{ error?: string }>(`${base}/hooks/file-chooser`, { + paths: ["../../pwned.txt"], + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.armFileUploadViaPlaywright).not.toHaveBeenCalled(); + expect(pwMocks.setInputFilesViaPlaywright).not.toHaveBeenCalled(); + }); + + it("hooks/file-chooser rejects absolute path outside uploads dir", async () => { + const base = await startServerAndBase(); + const outside = path.resolve(DEFAULT_UPLOAD_DIR, "..", "..", "pwned.txt"); + const res = await postJson<{ error?: string }>(`${base}/hooks/file-chooser`, { + paths: [outside], + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.armFileUploadViaPlaywright).not.toHaveBeenCalled(); + expect(pwMocks.setInputFilesViaPlaywright).not.toHaveBeenCalled(); + }); + it("wait/download rejects traversal path outside downloads dir", async () => { const base = await startServerAndBase(); const waitRes = await postJson<{ error?: string }>(`${base}/wait/download`, { 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 7cb9728e239..8298d30cb06 100644 --- a/src/cli/browser-cli-actions-input/register.files-downloads.ts +++ b/src/cli/browser-cli-actions-input/register.files-downloads.ts @@ -1,10 +1,30 @@ import type { Command } from "commander"; +import path from "node:path"; import { danger } from "../../globals.js"; +import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.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 uploadRoot = path.resolve(path.join(resolvePreferredOpenClawTmpDir(), "uploads")); + return paths.map((p) => { + const raw = String(p ?? "").trim(); + if (!raw) { + throw new Error("upload path is empty"); + } + const resolved = path.resolve(uploadRoot, raw); + const rel = path.relative(uploadRoot, resolved); + if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { + throw new Error( + `Invalid upload path: must stay within ${uploadRoot} (copy files there first)`, + ); + } + return resolved; + }); +} + export function registerBrowserFilesAndDownloadsCommands( browser: Command, parentOpts: (cmd: Command) => BrowserParentOpts, @@ -12,7 +32,10 @@ export function registerBrowserFilesAndDownloadsCommands( browser .command("upload") .description("Arm file upload for the next file chooser") - .argument("", "File paths to upload") + .argument( + "", + "File paths to upload (must be within OpenClaw temp uploads dir, e.g. /tmp/openclaw/uploads/file.pdf)", + ) .option("--ref ", "Ref id from snapshot to click after arming") .option("--input-ref ", "Ref id for to set directly") .option("--element ", "CSS selector for ") @@ -25,6 +48,7 @@ export function registerBrowserFilesAndDownloadsCommands( .action(async (paths: string[], opts, cmd) => { const { parent, profile } = resolveBrowserActionContext(cmd, parentOpts); try { + const normalizedPaths = normalizeUploadPaths(paths); const timeoutMs = Number.isFinite(opts.timeoutMs) ? opts.timeoutMs : undefined; const result = await callBrowserRequest<{ download: { path: string } }>( parent, @@ -33,7 +57,7 @@ export function registerBrowserFilesAndDownloadsCommands( path: "/hooks/file-chooser", query: profile ? { profile } : undefined, body: { - paths, + paths: normalizedPaths, ref: opts.ref?.trim() || undefined, inputRef: opts.inputRef?.trim() || undefined, element: opts.element?.trim() || undefined,