From 7f0489e4731c8d965d78d6eac4a60312e46a9426 Mon Sep 17 00:00:00 2001 From: Mariano <132747814+mbelinky@users.noreply.github.com> Date: Fri, 13 Feb 2026 19:24:33 +0000 Subject: [PATCH] Security/Browser: constrain trace and download output paths to OpenClaw temp roots (#15652) * Browser/Security: constrain trace and download output paths to temp roots * Changelog: remove advisory ID from pre-public security note * Browser/Security: constrain trace and download output paths to temp roots * Changelog: remove advisory ID from pre-public security note * test(bluebubbles): align timeout status expectation to 408 * test(discord): remove unused race-condition counter in threading test * test(bluebubbles): align timeout status expectation to 408 --- CHANGELOG.md | 1 + docs/tools/browser.md | 7 +- extensions/bluebubbles/src/monitor.test.ts | 4 +- src/browser/routes/agent.act.ts | 29 ++++++- src/browser/routes/agent.debug.ts | 16 +++- src/browser/routes/path-output.ts | 28 +++++++ ...-contract-form-layout-act-commands.test.ts | 84 ++++++++++++++++++- .../register.files-downloads.ts | 7 +- src/cli/browser-cli-debug.ts | 5 +- src/discord/monitor/threading.test.ts | 1 + 10 files changed, 166 insertions(+), 16 deletions(-) create mode 100644 src/browser/routes/path-output.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a7cdf28d1..1d88975b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai - Security/WhatsApp: enforce `0o600` on `creds.json` and `creds.json.bak` on save/backup/restore paths to reduce credential file exposure. (#10529) Thanks @abdelsfane. - WhatsApp: preserve outbound document filenames for web-session document sends instead of always sending `"file"`. (#15594) Thanks @TsekaLuk. - Security/Gateway + ACP: block high-risk tools (`sessions_spawn`, `sessions_send`, `gateway`, `whatsapp_login`) from HTTP `/tools/invoke` by default with `gateway.tools.{allow,deny}` overrides, and harden ACP permission selection to fail closed when tool identity/options are ambiguous while supporting `allow_always`/`reject_always`. (#15390) Thanks @aether-ai-agent. +- Security/Browser: constrain `POST /trace/stop`, `POST /wait/download`, and `POST /download` output paths to OpenClaw temp roots and reject traversal/escape paths. - Gateway/Tools Invoke: sanitize `/tools/invoke` execution failures while preserving `400` for tool input errors and returning `500` for unexpected runtime failures, with regression coverage and docs updates. (#13185) Thanks @davidrudduck. - MS Teams: preserve parsed mention entities/text when appending OneDrive fallback file links, and accept broader real-world Teams mention ID formats (`29:...`, `8:orgid:...`) while still rejecting placeholder patterns. (#15436) Thanks @hyojin. - Security/Audit: distinguish external webhooks (`hooks.enabled`) from internal hooks (`hooks.internal.enabled`) in attack-surface summaries to avoid false exposure signals when only internal hooks are enabled. (#13474) Thanks @mcaxtr. diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 743092314..107c92b99 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -409,8 +409,8 @@ Actions: - `openclaw browser scrollintoview e12` - `openclaw browser drag 10 11` - `openclaw browser select 9 OptionA OptionB` -- `openclaw browser download e12 /tmp/report.pdf` -- `openclaw browser waitfordownload /tmp/report.pdf` +- `openclaw browser download e12 report.pdf` +- `openclaw browser waitfordownload report.pdf` - `openclaw browser upload /tmp/file.pdf` - `openclaw browser fill --fields '[{"ref":"1","type":"text","value":"Ada"}]'` - `openclaw browser dialog --accept` @@ -444,6 +444,9 @@ Notes: - `upload` and `dialog` are **arming** calls; run them before the click/press that triggers the chooser/dialog. +- 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` 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/extensions/bluebubbles/src/monitor.test.ts b/extensions/bluebubbles/src/monitor.test.ts index a1b3c843b..6aae7e7c5 100644 --- a/extensions/bluebubbles/src/monitor.test.ts +++ b/extensions/bluebubbles/src/monitor.test.ts @@ -404,7 +404,7 @@ describe("BlueBubbles webhook monitor", () => { expect(res.statusCode).toBe(400); }); - it("returns 400 when request body times out (Slow-Loris protection)", async () => { + it("returns 408 when request body times out (Slow-Loris protection)", async () => { vi.useFakeTimers(); try { const account = createMockAccount(); @@ -439,7 +439,7 @@ describe("BlueBubbles webhook monitor", () => { const handled = await handledPromise; expect(handled).toBe(true); - expect(res.statusCode).toBe(400); + expect(res.statusCode).toBe(408); expect(req.destroy).toHaveBeenCalled(); } finally { vi.useRealTimers(); diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index da692997c..6c6e31153 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -14,6 +14,7 @@ import { resolveProfileContext, SELECTOR_UNSUPPORTED_MESSAGE, } from "./agent.shared.js"; +import { DEFAULT_DOWNLOAD_DIR, resolvePathWithinRoot } from "./path-output.js"; import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js"; export function registerBrowserAgentActRoutes( @@ -430,7 +431,7 @@ export function registerBrowserAgentActRoutes( } const body = readBody(req); const targetId = toStringOrEmpty(body.targetId) || undefined; - const out = toStringOrEmpty(body.path) || undefined; + const out = toStringOrEmpty(body.path) || ""; const timeoutMs = toNumber(body.timeoutMs); try { const tab = await profileCtx.ensureTabAvailable(targetId); @@ -438,10 +439,23 @@ export function registerBrowserAgentActRoutes( if (!pw) { return; } + let downloadPath: string | undefined; + if (out.trim()) { + const downloadPathResult = resolvePathWithinRoot({ + rootDir: DEFAULT_DOWNLOAD_DIR, + requestedPath: out, + scopeLabel: "downloads directory", + }); + if (!downloadPathResult.ok) { + res.status(400).json({ error: downloadPathResult.error }); + return; + } + downloadPath = downloadPathResult.path; + } const result = await pw.waitForDownloadViaPlaywright({ cdpUrl: profileCtx.profile.cdpUrl, targetId: tab.targetId, - path: out, + path: downloadPath, timeoutMs: timeoutMs ?? undefined, }); res.json({ ok: true, targetId: tab.targetId, download: result }); @@ -467,6 +481,15 @@ export function registerBrowserAgentActRoutes( return jsonError(res, 400, "path is required"); } try { + const downloadPathResult = resolvePathWithinRoot({ + rootDir: DEFAULT_DOWNLOAD_DIR, + requestedPath: out, + scopeLabel: "downloads directory", + }); + if (!downloadPathResult.ok) { + res.status(400).json({ error: downloadPathResult.error }); + return; + } const tab = await profileCtx.ensureTabAvailable(targetId); const pw = await requirePwAi(res, "download"); if (!pw) { @@ -476,7 +499,7 @@ export function registerBrowserAgentActRoutes( cdpUrl: profileCtx.profile.cdpUrl, targetId: tab.targetId, ref, - path: out, + path: downloadPathResult.path, timeoutMs: timeoutMs ?? undefined, }); res.json({ ok: true, targetId: tab.targetId, download: result }); diff --git a/src/browser/routes/agent.debug.ts b/src/browser/routes/agent.debug.ts index 7ba0ed52a..f5a1a3ae9 100644 --- a/src/browser/routes/agent.debug.ts +++ b/src/browser/routes/agent.debug.ts @@ -3,12 +3,10 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { BrowserRouteContext } from "../server-context.js"; import type { BrowserRouteRegistrar } from "./types.js"; -import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; import { handleRouteError, readBody, requirePwAi, resolveProfileContext } from "./agent.shared.js"; +import { DEFAULT_TRACE_DIR, resolvePathWithinRoot } from "./path-output.js"; import { toBoolean, toStringOrEmpty } from "./utils.js"; -const DEFAULT_TRACE_DIR = resolvePreferredOpenClawTmpDir(); - export function registerBrowserAgentDebugRoutes( app: BrowserRouteRegistrar, ctx: BrowserRouteContext, @@ -136,7 +134,17 @@ export function registerBrowserAgentDebugRoutes( const id = crypto.randomUUID(); const dir = DEFAULT_TRACE_DIR; await fs.mkdir(dir, { recursive: true }); - const tracePath = out.trim() || path.join(dir, `browser-trace-${id}.zip`); + const tracePathResult = resolvePathWithinRoot({ + rootDir: dir, + requestedPath: out, + scopeLabel: "trace directory", + defaultFileName: `browser-trace-${id}.zip`, + }); + if (!tracePathResult.ok) { + res.status(400).json({ error: tracePathResult.error }); + return; + } + const tracePath = tracePathResult.path; await pw.traceStopViaPlaywright({ cdpUrl: profileCtx.profile.cdpUrl, targetId: tab.targetId, diff --git a/src/browser/routes/path-output.ts b/src/browser/routes/path-output.ts new file mode 100644 index 000000000..137b62521 --- /dev/null +++ b/src/browser/routes/path-output.ts @@ -0,0 +1,28 @@ +import path from "node:path"; +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 function resolvePathWithinRoot(params: { + rootDir: string; + requestedPath: string; + scopeLabel: string; + defaultFileName?: string; +}): { ok: true; path: string } | { ok: false; error: string } { + const root = path.resolve(params.rootDir); + const raw = params.requestedPath.trim(); + if (!raw) { + if (!params.defaultFileName) { + return { ok: false, error: "path is required" }; + } + return { ok: true, path: path.join(root, params.defaultFileName) }; + } + const resolved = path.resolve(root, raw); + const rel = path.relative(root, resolved); + if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { + return { ok: false, error: `Invalid path: must stay within ${params.scopeLabel}` }; + } + return { ok: true, path: resolved }; +} 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 d1ea49b9f..a63eef29c 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 @@ -49,6 +49,7 @@ const pwMocks = vi.hoisted(() => ({ selectOptionViaPlaywright: vi.fn(async () => {}), setInputFilesViaPlaywright: vi.fn(async () => {}), snapshotAiViaPlaywright: vi.fn(async () => ({ snapshot: "ok" })), + traceStopViaPlaywright: vi.fn(async () => {}), takeScreenshotViaPlaywright: vi.fn(async () => ({ buffer: Buffer.from("png"), })), @@ -434,14 +435,14 @@ describe("browser control server", () => { expect(dialog).toMatchObject({ ok: true }); const waitDownload = await postJson(`${base}/wait/download`, { - path: "/tmp/report.pdf", + path: "report.pdf", timeoutMs: 1111, }); expect(waitDownload).toMatchObject({ ok: true }); const download = await postJson(`${base}/download`, { ref: "e12", - path: "/tmp/report.pdf", + path: "report.pdf", }); expect(download).toMatchObject({ ok: true }); @@ -480,4 +481,83 @@ describe("browser control server", () => { expect(stopped.ok).toBe(true); expect(stopped.stopped).toBe(true); }); + + it("trace stop rejects traversal path outside trace dir", async () => { + const base = await startServerAndBase(); + const res = await postJson<{ error?: string }>(`${base}/trace/stop`, { + path: "../../pwned.zip", + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.traceStopViaPlaywright).not.toHaveBeenCalled(); + }); + + it("trace stop accepts in-root relative output path", async () => { + const base = await startServerAndBase(); + const res = await postJson<{ ok?: boolean; path?: string }>(`${base}/trace/stop`, { + path: "safe-trace.zip", + }); + expect(res.ok).toBe(true); + expect(res.path).toContain("safe-trace.zip"); + expect(pwMocks.traceStopViaPlaywright).toHaveBeenCalledWith( + expect.objectContaining({ + cdpUrl: cdpBaseUrl, + targetId: "abcd1234", + path: expect.stringContaining("safe-trace.zip"), + }), + ); + }); + + it("wait/download rejects traversal path outside downloads dir", async () => { + const base = await startServerAndBase(); + const waitRes = await postJson<{ error?: string }>(`${base}/wait/download`, { + path: "../../pwned.pdf", + }); + expect(waitRes.error).toContain("Invalid path"); + expect(pwMocks.waitForDownloadViaPlaywright).not.toHaveBeenCalled(); + }); + + it("download rejects traversal path outside downloads dir", async () => { + const base = await startServerAndBase(); + const downloadRes = await postJson<{ error?: string }>(`${base}/download`, { + ref: "e12", + path: "../../pwned.pdf", + }); + expect(downloadRes.error).toContain("Invalid path"); + expect(pwMocks.downloadViaPlaywright).not.toHaveBeenCalled(); + }); + + it("wait/download accepts in-root relative output path", async () => { + const base = await startServerAndBase(); + const res = await postJson<{ ok?: boolean; download?: { path?: string } }>( + `${base}/wait/download`, + { + path: "safe-wait.pdf", + }, + ); + expect(res.ok).toBe(true); + expect(pwMocks.waitForDownloadViaPlaywright).toHaveBeenCalledWith( + expect.objectContaining({ + cdpUrl: cdpBaseUrl, + targetId: "abcd1234", + path: expect.stringContaining("safe-wait.pdf"), + }), + ); + }); + + it("download accepts in-root relative output path", async () => { + const base = await startServerAndBase(); + const res = await postJson<{ ok?: boolean; download?: { path?: string } }>(`${base}/download`, { + ref: "e12", + path: "safe-download.pdf", + }); + expect(res.ok).toBe(true); + expect(pwMocks.downloadViaPlaywright).toHaveBeenCalledWith( + expect.objectContaining({ + cdpUrl: cdpBaseUrl, + targetId: "abcd1234", + ref: "e12", + path: expect.stringContaining("safe-download.pdf"), + }), + ); + }); }); 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 0827079ba..7cb9728e2 100644 --- a/src/cli/browser-cli-actions-input/register.files-downloads.ts +++ b/src/cli/browser-cli-actions-input/register.files-downloads.ts @@ -59,7 +59,7 @@ export function registerBrowserFilesAndDownloadsCommands( .description("Wait for the next download (and save it)") .argument( "[path]", - "Save path (default: /tmp/openclaw/downloads/...; fallback: os.tmpdir()/openclaw/downloads/...)", + "Save path within openclaw temp downloads dir (default: /tmp/openclaw/downloads/...; fallback: os.tmpdir()/openclaw/downloads/...)", ) .option("--target-id ", "CDP target id (or unique prefix)") .option( @@ -100,7 +100,10 @@ export function registerBrowserFilesAndDownloadsCommands( .command("download") .description("Click a ref and save the resulting download") .argument("", "Ref id from snapshot to click") - .argument("", "Save path") + .argument( + "", + "Save path within openclaw temp downloads dir (e.g. report.pdf or /tmp/openclaw/downloads/report.pdf)", + ) .option("--target-id ", "CDP target id (or unique prefix)") .option( "--timeout-ms ", diff --git a/src/cli/browser-cli-debug.ts b/src/cli/browser-cli-debug.ts index 58ae72cdf..2c4537438 100644 --- a/src/cli/browser-cli-debug.ts +++ b/src/cli/browser-cli-debug.ts @@ -179,7 +179,10 @@ export function registerBrowserDebugCommands( trace .command("stop") .description("Stop trace recording and write a .zip") - .option("--out ", "Output path for the trace zip") + .option( + "--out ", + "Output path within openclaw temp dir (e.g. trace.zip or /tmp/openclaw/trace.zip)", + ) .option("--target-id ", "CDP target id (or unique prefix)") .action(async (opts, cmd) => { const parent = parentOpts(cmd); diff --git a/src/discord/monitor/threading.test.ts b/src/discord/monitor/threading.test.ts index 530d9730e..587aca8bb 100644 --- a/src/discord/monitor/threading.test.ts +++ b/src/discord/monitor/threading.test.ts @@ -115,6 +115,7 @@ describe("resolveDiscordReplyDeliveryPlan", () => { describe("maybeCreateDiscordAutoThread", () => { it("returns existing thread ID when creation fails due to race condition", async () => { + // First call succeeds (simulating another agent creating the thread) const client = { rest: { post: async () => {