Skip to content

Commit 6a80e9d

Browse files
steipetesteipete
committed
fix(browser): harden writable output paths
1 parent 51bccaf commit 6a80e9d

7 files changed

Lines changed: 216 additions & 50 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ Docs: https://docs.openclaw.ai
201201
- 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.
202202
- 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.
203203
- 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.
204+
- 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.
204205
- 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.
205206
- 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.
206207
- 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)

‎src/browser/output-atomic.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import crypto from "node:crypto";
2+
import fs from "node:fs/promises";
3+
import path from "node:path";
4+
5+
function sanitizeFileNameTail(fileName: string): string {
6+
const trimmed = String(fileName ?? "").trim();
7+
if (!trimmed) {
8+
return "output.bin";
9+
}
10+
let base = path.posix.basename(trimmed);
11+
base = path.win32.basename(base);
12+
let cleaned = "";
13+
for (let i = 0; i < base.length; i++) {
14+
const code = base.charCodeAt(i);
15+
if (code < 0x20 || code === 0x7f) {
16+
continue;
17+
}
18+
cleaned += base[i];
19+
}
20+
base = cleaned.trim();
21+
if (!base || base === "." || base === "..") {
22+
return "output.bin";
23+
}
24+
if (base.length > 200) {
25+
base = base.slice(0, 200);
26+
}
27+
return base;
28+
}
29+
30+
function buildSiblingTempPath(targetPath: string): string {
31+
const id = crypto.randomUUID();
32+
const safeTail = sanitizeFileNameTail(path.basename(targetPath));
33+
return path.join(path.dirname(targetPath), `.openclaw-output-${id}-${safeTail}.part`);
34+
}
35+
36+
export async function writeViaSiblingTempPath(params: {
37+
targetPath: string;
38+
writeTemp: (tempPath: string) => Promise<void>;
39+
}): Promise<void> {
40+
const targetPath = path.resolve(params.targetPath);
41+
const tempPath = buildSiblingTempPath(targetPath);
42+
let renameSucceeded = false;
43+
try {
44+
await params.writeTemp(tempPath);
45+
await fs.rename(tempPath, targetPath);
46+
renameSucceeded = true;
47+
} finally {
48+
if (!renameSucceeded) {
49+
await fs.rm(tempPath, { force: true }).catch(() => {});
50+
}
51+
}
52+
}

‎src/browser/paths.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,29 @@ describe("resolveWritablePathWithinRoot", () => {
305305
});
306306
},
307307
);
308+
309+
it.runIf(process.platform !== "win32")(
310+
"rejects existing hardlinked files under root",
311+
async () => {
312+
await withFixtureRoot(async ({ baseDir, uploadsDir }) => {
313+
const outsidePath = path.join(baseDir, "outside-target.txt");
314+
await fs.writeFile(outsidePath, "outside", "utf8");
315+
const hardlinkedPath = path.join(uploadsDir, "linked.txt");
316+
await fs.link(outsidePath, hardlinkedPath);
317+
318+
const result = await resolveWritablePathWithinRoot({
319+
rootDir: uploadsDir,
320+
requestedPath: "linked.txt",
321+
scopeLabel: "uploads directory",
322+
});
323+
324+
expect(result.ok).toBe(false);
325+
if (!result.ok) {
326+
expect(result.error).toContain("must stay within uploads directory");
327+
}
328+
});
329+
},
330+
);
308331
});
309332

310333
describe("resolvePathsWithinRoot", () => {

‎src/browser/paths.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ async function validateCanonicalPathWithinRoot(params: {
5454
if (params.expect === "file" && !candidateLstat.isFile()) {
5555
return "invalid";
5656
}
57+
if (params.expect === "file" && candidateLstat.nlink > 1) {
58+
return "invalid";
59+
}
5760
const candidateRealPath = await fs.realpath(params.candidatePath);
5861
return isPathInside(params.rootRealPath, candidateRealPath) ? "ok" : "invalid";
5962
} catch (err) {

‎src/browser/pw-tools-core.downloads.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import fs from "node:fs/promises";
33
import path from "node:path";
44
import type { Page } from "playwright-core";
55
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
6+
import { writeViaSiblingTempPath } from "./output-atomic.js";
67
import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
78
import {
89
ensurePageState,
@@ -111,13 +112,25 @@ type DownloadPayload = {
111112

112113
async function saveDownloadPayload(download: DownloadPayload, outPath: string) {
113114
const suggested = download.suggestedFilename?.() || "download.bin";
114-
const resolvedOutPath = outPath?.trim() || buildTempDownloadPath(suggested);
115+
const requestedPath = outPath?.trim();
116+
const resolvedOutPath = path.resolve(requestedPath || buildTempDownloadPath(suggested));
115117
await fs.mkdir(path.dirname(resolvedOutPath), { recursive: true });
116-
await download.saveAs?.(resolvedOutPath);
118+
119+
if (!requestedPath) {
120+
await download.saveAs?.(resolvedOutPath);
121+
} else {
122+
await writeViaSiblingTempPath({
123+
targetPath: resolvedOutPath,
124+
writeTemp: async (tempPath) => {
125+
await download.saveAs?.(tempPath);
126+
},
127+
});
128+
}
129+
117130
return {
118131
url: download.url?.() || "",
119132
suggestedFilename: suggested,
120-
path: path.resolve(resolvedOutPath),
133+
path: resolvedOutPath,
121134
};
122135
}
123136

‎src/browser/pw-tools-core.trace.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { writeViaSiblingTempPath } from "./output-atomic.js";
12
import { ensureContextState, getPageForTargetId } from "./pw-session.js";
23

34
export async function traceStartViaPlaywright(opts: {
@@ -32,6 +33,11 @@ export async function traceStopViaPlaywright(opts: {
3233
if (!ctxState.traceActive) {
3334
throw new Error("No active trace. Start a trace before stopping it.");
3435
}
35-
await context.tracing.stop({ path: opts.path });
36+
await writeViaSiblingTempPath({
37+
targetPath: opts.path,
38+
writeTemp: async (tempPath) => {
39+
await context.tracing.stop({ path: tempPath });
40+
},
41+
});
3642
ctxState.traceActive = false;
3743
}

‎src/browser/pw-tools-core.waits-next-download-saves-it.test.ts‎

Lines changed: 114 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import fs from "node:fs/promises";
2+
import os from "node:os";
13
import path from "node:path";
24
import { beforeEach, describe, expect, it, vi } from "vitest";
35
import {
@@ -23,6 +25,15 @@ describe("pw-tools-core", () => {
2325
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw");
2426
});
2527

28+
async function withTempDir<T>(run: (tempDir: string) => Promise<T>): Promise<T> {
29+
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-browser-download-test-"));
30+
try {
31+
return await run(tempDir);
32+
} finally {
33+
await fs.rm(tempDir, { recursive: true, force: true });
34+
}
35+
}
36+
2637
async function waitForImplicitDownloadOutput(params: {
2738
downloadUrl: string;
2839
suggestedFilename: string;
@@ -67,64 +78,121 @@ describe("pw-tools-core", () => {
6778
};
6879
}
6980

70-
it("waits for the next download and saves it", async () => {
71-
const harness = createDownloadEventHarness();
81+
it("waits for the next download and atomically finalizes explicit output paths", async () => {
82+
await withTempDir(async (tempDir) => {
83+
const harness = createDownloadEventHarness();
84+
const targetPath = path.join(tempDir, "file.bin");
7285

73-
const saveAs = vi.fn(async () => {});
74-
const download = {
75-
url: () => "https://example.com/file.bin",
76-
suggestedFilename: () => "file.bin",
77-
saveAs,
78-
};
86+
const saveAs = vi.fn(async (outPath: string) => {
87+
await fs.writeFile(outPath, "file-content", "utf8");
88+
});
89+
const download = {
90+
url: () => "https://example.com/file.bin",
91+
suggestedFilename: () => "file.bin",
92+
saveAs,
93+
};
7994

80-
const targetPath = path.resolve("/tmp/file.bin");
81-
const p = mod.waitForDownloadViaPlaywright({
82-
cdpUrl: "http://127.0.0.1:18792",
83-
targetId: "T1",
84-
path: targetPath,
85-
timeoutMs: 1000,
86-
});
95+
const p = mod.waitForDownloadViaPlaywright({
96+
cdpUrl: "http://127.0.0.1:18792",
97+
targetId: "T1",
98+
path: targetPath,
99+
timeoutMs: 1000,
100+
});
87101

88-
await Promise.resolve();
89-
harness.expectArmed();
90-
harness.trigger(download);
102+
await Promise.resolve();
103+
harness.expectArmed();
104+
harness.trigger(download);
91105

92-
const res = await p;
93-
expect(saveAs).toHaveBeenCalledWith(targetPath);
94-
expect(res.path).toBe(targetPath);
106+
const res = await p;
107+
const savedPath = saveAs.mock.calls[0]?.[0];
108+
expect(typeof savedPath).toBe("string");
109+
expect(savedPath).not.toBe(targetPath);
110+
expect(path.dirname(String(savedPath))).toBe(tempDir);
111+
expect(path.basename(String(savedPath))).toContain(".openclaw-output-");
112+
expect(path.basename(String(savedPath))).toContain(".part");
113+
expect(await fs.readFile(targetPath, "utf8")).toBe("file-content");
114+
expect(res.path).toBe(targetPath);
115+
});
95116
});
96-
it("clicks a ref and saves the resulting download", async () => {
97-
const harness = createDownloadEventHarness();
117+
it("clicks a ref and atomically finalizes explicit download paths", async () => {
118+
await withTempDir(async (tempDir) => {
119+
const harness = createDownloadEventHarness();
98120

99-
const click = vi.fn(async () => {});
100-
setPwToolsCoreCurrentRefLocator({ click });
121+
const click = vi.fn(async () => {});
122+
setPwToolsCoreCurrentRefLocator({ click });
101123

102-
const saveAs = vi.fn(async () => {});
103-
const download = {
104-
url: () => "https://example.com/report.pdf",
105-
suggestedFilename: () => "report.pdf",
106-
saveAs,
107-
};
124+
const saveAs = vi.fn(async (outPath: string) => {
125+
await fs.writeFile(outPath, "report-content", "utf8");
126+
});
127+
const download = {
128+
url: () => "https://example.com/report.pdf",
129+
suggestedFilename: () => "report.pdf",
130+
saveAs,
131+
};
108132

109-
const targetPath = path.resolve("/tmp/report.pdf");
110-
const p = mod.downloadViaPlaywright({
111-
cdpUrl: "http://127.0.0.1:18792",
112-
targetId: "T1",
113-
ref: "e12",
114-
path: targetPath,
115-
timeoutMs: 1000,
116-
});
133+
const targetPath = path.join(tempDir, "report.pdf");
134+
const p = mod.downloadViaPlaywright({
135+
cdpUrl: "http://127.0.0.1:18792",
136+
targetId: "T1",
137+
ref: "e12",
138+
path: targetPath,
139+
timeoutMs: 1000,
140+
});
117141

118-
await Promise.resolve();
119-
harness.expectArmed();
120-
expect(click).toHaveBeenCalledWith({ timeout: 1000 });
142+
await Promise.resolve();
143+
harness.expectArmed();
144+
expect(click).toHaveBeenCalledWith({ timeout: 1000 });
121145

122-
harness.trigger(download);
146+
harness.trigger(download);
123147

124-
const res = await p;
125-
expect(saveAs).toHaveBeenCalledWith(targetPath);
126-
expect(res.path).toBe(targetPath);
148+
const res = await p;
149+
const savedPath = saveAs.mock.calls[0]?.[0];
150+
expect(typeof savedPath).toBe("string");
151+
expect(savedPath).not.toBe(targetPath);
152+
expect(path.dirname(String(savedPath))).toBe(tempDir);
153+
expect(path.basename(String(savedPath))).toContain(".openclaw-output-");
154+
expect(path.basename(String(savedPath))).toContain(".part");
155+
expect(await fs.readFile(targetPath, "utf8")).toBe("report-content");
156+
expect(res.path).toBe(targetPath);
157+
});
127158
});
159+
160+
it.runIf(process.platform !== "win32")(
161+
"does not overwrite outside files when explicit output path is a hardlink alias",
162+
async () => {
163+
await withTempDir(async (tempDir) => {
164+
const outsidePath = path.join(tempDir, "outside.txt");
165+
await fs.writeFile(outsidePath, "outside-before", "utf8");
166+
const linkedPath = path.join(tempDir, "linked.txt");
167+
await fs.link(outsidePath, linkedPath);
168+
169+
const harness = createDownloadEventHarness();
170+
const saveAs = vi.fn(async (outPath: string) => {
171+
await fs.writeFile(outPath, "download-content", "utf8");
172+
});
173+
const p = mod.waitForDownloadViaPlaywright({
174+
cdpUrl: "http://127.0.0.1:18792",
175+
targetId: "T1",
176+
path: linkedPath,
177+
timeoutMs: 1000,
178+
});
179+
180+
await Promise.resolve();
181+
harness.expectArmed();
182+
harness.trigger({
183+
url: () => "https://example.com/file.bin",
184+
suggestedFilename: () => "file.bin",
185+
saveAs,
186+
});
187+
188+
const res = await p;
189+
expect(res.path).toBe(linkedPath);
190+
expect(await fs.readFile(linkedPath, "utf8")).toBe("download-content");
191+
expect(await fs.readFile(outsidePath, "utf8")).toBe("outside-before");
192+
});
193+
},
194+
);
195+
128196
it("uses preferred tmp dir when waiting for download without explicit path", async () => {
129197
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw-preferred");
130198
const { res, outPath } = await waitForImplicitDownloadOutput({

0 commit comments

Comments
 (0)