From 6a80e9db058c591091fa8f010c9d3e7f8336acc6 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 1 Mar 2026 23:25:13 +0000 Subject: [PATCH] fix(browser): harden writable output paths --- CHANGELOG.md | 1 + src/browser/output-atomic.ts | 52 ++++++ src/browser/paths.test.ts | 23 +++ src/browser/paths.ts | 3 + src/browser/pw-tools-core.downloads.ts | 19 +- src/browser/pw-tools-core.trace.ts | 8 +- ...-core.waits-next-download-saves-it.test.ts | 166 ++++++++++++------ 7 files changed, 219 insertions(+), 53 deletions(-) create mode 100644 src/browser/output-atomic.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e768f61d..f529508c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,7 @@ Docs: https://docs.openclaw.ai - Browser/Extension relay shutdown: flush pending extension-request timers/rejections during relay `stop()` before socket/server teardown so in-flight extension waits do not survive shutdown windows. Landed from contributor PR #24142 by @kevinWangSheng. - Browser/Extension relay reconnect resilience: keep CDP clients alive across brief MV3 extension disconnect windows, wait briefly for extension reconnect before failing in-flight CDP commands, and only tear down relay target/client state after reconnect grace expires. Landed from contributor PR #27617 by @davidemanuelDEV. - Browser/Route decode hardening: guard malformed percent-encoding in relay target action routes and browser route-param decoding so crafted `%` paths return `400` instead of crashing/unhandled URI decode failures. Landed from contributor PR #11880 by @Yida-Dev. +- Browser/Writable output path hardening: reject existing hardlinked writable targets, and finalize browser download/trace outputs via sibling temp files plus atomic rename to block hardlink-alias overwrite paths under browser temp roots. - Feishu/Inbound message metadata: include inbound `message_id` in `BodyForAgent` on a dedicated metadata line so agents can reliably correlate and act on media/message operations that require message IDs, with regression coverage. (#27253) thanks @xss925175263. - Feishu/Doc tools: route `feishu_doc` and `feishu_app_scopes` through the active agent account context (with explicit `accountId` override support) so multi-account agents no longer default to the first configured app, with regression coverage for context routing and explicit override behavior. (#27338) thanks @AaronL725. - LINE/Inline directives auth: gate directive parsing (`/model`, `/think`, `/verbose`, `/reasoning`, `/queue`) on resolved authorization (`command.isAuthorizedSender`) so `commands.allowFrom`-authorized LINE senders are not silently stripped when raw `CommandAuthorized` is unset. Landed from contributor PR #27248 by @kevinWangSheng. (#27240) diff --git a/src/browser/output-atomic.ts b/src/browser/output-atomic.ts new file mode 100644 index 000000000..8cd782188 --- /dev/null +++ b/src/browser/output-atomic.ts @@ -0,0 +1,52 @@ +import crypto from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; + +function sanitizeFileNameTail(fileName: string): string { + const trimmed = String(fileName ?? "").trim(); + if (!trimmed) { + return "output.bin"; + } + let base = path.posix.basename(trimmed); + base = path.win32.basename(base); + let cleaned = ""; + for (let i = 0; i < base.length; i++) { + const code = base.charCodeAt(i); + if (code < 0x20 || code === 0x7f) { + continue; + } + cleaned += base[i]; + } + base = cleaned.trim(); + if (!base || base === "." || base === "..") { + return "output.bin"; + } + if (base.length > 200) { + base = base.slice(0, 200); + } + return base; +} + +function buildSiblingTempPath(targetPath: string): string { + const id = crypto.randomUUID(); + const safeTail = sanitizeFileNameTail(path.basename(targetPath)); + return path.join(path.dirname(targetPath), `.openclaw-output-${id}-${safeTail}.part`); +} + +export async function writeViaSiblingTempPath(params: { + targetPath: string; + writeTemp: (tempPath: string) => Promise; +}): Promise { + const targetPath = path.resolve(params.targetPath); + const tempPath = buildSiblingTempPath(targetPath); + let renameSucceeded = false; + try { + await params.writeTemp(tempPath); + await fs.rename(tempPath, targetPath); + renameSucceeded = true; + } finally { + if (!renameSucceeded) { + await fs.rm(tempPath, { force: true }).catch(() => {}); + } + } +} diff --git a/src/browser/paths.test.ts b/src/browser/paths.test.ts index 90ef8afaa..f3ed376c4 100644 --- a/src/browser/paths.test.ts +++ b/src/browser/paths.test.ts @@ -305,6 +305,29 @@ describe("resolveWritablePathWithinRoot", () => { }); }, ); + + it.runIf(process.platform !== "win32")( + "rejects existing hardlinked files under root", + async () => { + await withFixtureRoot(async ({ baseDir, uploadsDir }) => { + const outsidePath = path.join(baseDir, "outside-target.txt"); + await fs.writeFile(outsidePath, "outside", "utf8"); + const hardlinkedPath = path.join(uploadsDir, "linked.txt"); + await fs.link(outsidePath, hardlinkedPath); + + const result = await resolveWritablePathWithinRoot({ + rootDir: uploadsDir, + requestedPath: "linked.txt", + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("must stay within uploads directory"); + } + }); + }, + ); }); describe("resolvePathsWithinRoot", () => { diff --git a/src/browser/paths.ts b/src/browser/paths.ts index 422585695..1506a2e2e 100644 --- a/src/browser/paths.ts +++ b/src/browser/paths.ts @@ -54,6 +54,9 @@ async function validateCanonicalPathWithinRoot(params: { if (params.expect === "file" && !candidateLstat.isFile()) { return "invalid"; } + if (params.expect === "file" && candidateLstat.nlink > 1) { + return "invalid"; + } const candidateRealPath = await fs.realpath(params.candidatePath); return isPathInside(params.rootRealPath, candidateRealPath) ? "ok" : "invalid"; } catch (err) { diff --git a/src/browser/pw-tools-core.downloads.ts b/src/browser/pw-tools-core.downloads.ts index 4933c78b5..8afb3afd8 100644 --- a/src/browser/pw-tools-core.downloads.ts +++ b/src/browser/pw-tools-core.downloads.ts @@ -3,6 +3,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { Page } from "playwright-core"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; +import { writeViaSiblingTempPath } from "./output-atomic.js"; import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js"; import { ensurePageState, @@ -111,13 +112,25 @@ type DownloadPayload = { async function saveDownloadPayload(download: DownloadPayload, outPath: string) { const suggested = download.suggestedFilename?.() || "download.bin"; - const resolvedOutPath = outPath?.trim() || buildTempDownloadPath(suggested); + const requestedPath = outPath?.trim(); + const resolvedOutPath = path.resolve(requestedPath || buildTempDownloadPath(suggested)); await fs.mkdir(path.dirname(resolvedOutPath), { recursive: true }); - await download.saveAs?.(resolvedOutPath); + + if (!requestedPath) { + await download.saveAs?.(resolvedOutPath); + } else { + await writeViaSiblingTempPath({ + targetPath: resolvedOutPath, + writeTemp: async (tempPath) => { + await download.saveAs?.(tempPath); + }, + }); + } + return { url: download.url?.() || "", suggestedFilename: suggested, - path: path.resolve(resolvedOutPath), + path: resolvedOutPath, }; } diff --git a/src/browser/pw-tools-core.trace.ts b/src/browser/pw-tools-core.trace.ts index 0efa988ca..43d0dc0b6 100644 --- a/src/browser/pw-tools-core.trace.ts +++ b/src/browser/pw-tools-core.trace.ts @@ -1,3 +1,4 @@ +import { writeViaSiblingTempPath } from "./output-atomic.js"; import { ensureContextState, getPageForTargetId } from "./pw-session.js"; export async function traceStartViaPlaywright(opts: { @@ -32,6 +33,11 @@ export async function traceStopViaPlaywright(opts: { if (!ctxState.traceActive) { throw new Error("No active trace. Start a trace before stopping it."); } - await context.tracing.stop({ path: opts.path }); + await writeViaSiblingTempPath({ + targetPath: opts.path, + writeTemp: async (tempPath) => { + await context.tracing.stop({ path: tempPath }); + }, + }); ctxState.traceActive = false; } diff --git a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index d4e8ad263..fb2a2e486 100644 --- a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -1,3 +1,5 @@ +import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -23,6 +25,15 @@ describe("pw-tools-core", () => { tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw"); }); + async function withTempDir(run: (tempDir: string) => Promise): Promise { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-browser-download-test-")); + try { + return await run(tempDir); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + } + async function waitForImplicitDownloadOutput(params: { downloadUrl: string; suggestedFilename: string; @@ -67,64 +78,121 @@ describe("pw-tools-core", () => { }; } - it("waits for the next download and saves it", async () => { - const harness = createDownloadEventHarness(); + it("waits for the next download and atomically finalizes explicit output paths", async () => { + await withTempDir(async (tempDir) => { + const harness = createDownloadEventHarness(); + const targetPath = path.join(tempDir, "file.bin"); - const saveAs = vi.fn(async () => {}); - const download = { - url: () => "https://example.com/file.bin", - suggestedFilename: () => "file.bin", - saveAs, - }; + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "file-content", "utf8"); + }); + const download = { + url: () => "https://example.com/file.bin", + suggestedFilename: () => "file.bin", + saveAs, + }; - const targetPath = path.resolve("/tmp/file.bin"); - const p = mod.waitForDownloadViaPlaywright({ - cdpUrl: "http://127.0.0.1:18792", - targetId: "T1", - path: targetPath, - timeoutMs: 1000, + const p = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + path: targetPath, + timeoutMs: 1000, + }); + + await Promise.resolve(); + harness.expectArmed(); + harness.trigger(download); + + const res = await p; + const savedPath = saveAs.mock.calls[0]?.[0]; + expect(typeof savedPath).toBe("string"); + expect(savedPath).not.toBe(targetPath); + expect(path.dirname(String(savedPath))).toBe(tempDir); + expect(path.basename(String(savedPath))).toContain(".openclaw-output-"); + expect(path.basename(String(savedPath))).toContain(".part"); + expect(await fs.readFile(targetPath, "utf8")).toBe("file-content"); + expect(res.path).toBe(targetPath); }); - - await Promise.resolve(); - harness.expectArmed(); - harness.trigger(download); - - const res = await p; - expect(saveAs).toHaveBeenCalledWith(targetPath); - expect(res.path).toBe(targetPath); }); - it("clicks a ref and saves the resulting download", async () => { - const harness = createDownloadEventHarness(); + it("clicks a ref and atomically finalizes explicit download paths", async () => { + await withTempDir(async (tempDir) => { + const harness = createDownloadEventHarness(); - const click = vi.fn(async () => {}); - setPwToolsCoreCurrentRefLocator({ click }); + const click = vi.fn(async () => {}); + setPwToolsCoreCurrentRefLocator({ click }); - const saveAs = vi.fn(async () => {}); - const download = { - url: () => "https://example.com/report.pdf", - suggestedFilename: () => "report.pdf", - saveAs, - }; + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "report-content", "utf8"); + }); + const download = { + url: () => "https://example.com/report.pdf", + suggestedFilename: () => "report.pdf", + saveAs, + }; - const targetPath = path.resolve("/tmp/report.pdf"); - const p = mod.downloadViaPlaywright({ - cdpUrl: "http://127.0.0.1:18792", - targetId: "T1", - ref: "e12", - path: targetPath, - timeoutMs: 1000, + const targetPath = path.join(tempDir, "report.pdf"); + const p = mod.downloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + ref: "e12", + path: targetPath, + timeoutMs: 1000, + }); + + await Promise.resolve(); + harness.expectArmed(); + expect(click).toHaveBeenCalledWith({ timeout: 1000 }); + + harness.trigger(download); + + const res = await p; + const savedPath = saveAs.mock.calls[0]?.[0]; + expect(typeof savedPath).toBe("string"); + expect(savedPath).not.toBe(targetPath); + expect(path.dirname(String(savedPath))).toBe(tempDir); + expect(path.basename(String(savedPath))).toContain(".openclaw-output-"); + expect(path.basename(String(savedPath))).toContain(".part"); + expect(await fs.readFile(targetPath, "utf8")).toBe("report-content"); + expect(res.path).toBe(targetPath); }); - - await Promise.resolve(); - harness.expectArmed(); - expect(click).toHaveBeenCalledWith({ timeout: 1000 }); - - harness.trigger(download); - - const res = await p; - expect(saveAs).toHaveBeenCalledWith(targetPath); - expect(res.path).toBe(targetPath); }); + + it.runIf(process.platform !== "win32")( + "does not overwrite outside files when explicit output path is a hardlink alias", + async () => { + await withTempDir(async (tempDir) => { + const outsidePath = path.join(tempDir, "outside.txt"); + await fs.writeFile(outsidePath, "outside-before", "utf8"); + const linkedPath = path.join(tempDir, "linked.txt"); + await fs.link(outsidePath, linkedPath); + + const harness = createDownloadEventHarness(); + const saveAs = vi.fn(async (outPath: string) => { + await fs.writeFile(outPath, "download-content", "utf8"); + }); + const p = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + path: linkedPath, + timeoutMs: 1000, + }); + + await Promise.resolve(); + harness.expectArmed(); + harness.trigger({ + url: () => "https://example.com/file.bin", + suggestedFilename: () => "file.bin", + saveAs, + }); + + const res = await p; + expect(res.path).toBe(linkedPath); + expect(await fs.readFile(linkedPath, "utf8")).toBe("download-content"); + expect(await fs.readFile(outsidePath, "utf8")).toBe("outside-before"); + }); + }, + ); + it("uses preferred tmp dir when waiting for download without explicit path", async () => { tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw-preferred"); const { res, outPath } = await waitForImplicitDownloadOutput({