mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-11 03:14:36 +00:00
fix(browser): harden writable output paths
This commit is contained in:
@@ -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 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/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/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/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.
|
- 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)
|
- 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)
|
||||||
|
|||||||
52
src/browser/output-atomic.ts
Normal file
52
src/browser/output-atomic.ts
Normal file
@@ -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<void>;
|
||||||
|
}): Promise<void> {
|
||||||
|
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(() => {});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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", () => {
|
describe("resolvePathsWithinRoot", () => {
|
||||||
|
|||||||
@@ -54,6 +54,9 @@ async function validateCanonicalPathWithinRoot(params: {
|
|||||||
if (params.expect === "file" && !candidateLstat.isFile()) {
|
if (params.expect === "file" && !candidateLstat.isFile()) {
|
||||||
return "invalid";
|
return "invalid";
|
||||||
}
|
}
|
||||||
|
if (params.expect === "file" && candidateLstat.nlink > 1) {
|
||||||
|
return "invalid";
|
||||||
|
}
|
||||||
const candidateRealPath = await fs.realpath(params.candidatePath);
|
const candidateRealPath = await fs.realpath(params.candidatePath);
|
||||||
return isPathInside(params.rootRealPath, candidateRealPath) ? "ok" : "invalid";
|
return isPathInside(params.rootRealPath, candidateRealPath) ? "ok" : "invalid";
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import fs from "node:fs/promises";
|
|||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
import type { Page } from "playwright-core";
|
import type { Page } from "playwright-core";
|
||||||
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
|
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
|
||||||
|
import { writeViaSiblingTempPath } from "./output-atomic.js";
|
||||||
import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
|
import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
|
||||||
import {
|
import {
|
||||||
ensurePageState,
|
ensurePageState,
|
||||||
@@ -111,13 +112,25 @@ type DownloadPayload = {
|
|||||||
|
|
||||||
async function saveDownloadPayload(download: DownloadPayload, outPath: string) {
|
async function saveDownloadPayload(download: DownloadPayload, outPath: string) {
|
||||||
const suggested = download.suggestedFilename?.() || "download.bin";
|
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 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 {
|
return {
|
||||||
url: download.url?.() || "",
|
url: download.url?.() || "",
|
||||||
suggestedFilename: suggested,
|
suggestedFilename: suggested,
|
||||||
path: path.resolve(resolvedOutPath),
|
path: resolvedOutPath,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { writeViaSiblingTempPath } from "./output-atomic.js";
|
||||||
import { ensureContextState, getPageForTargetId } from "./pw-session.js";
|
import { ensureContextState, getPageForTargetId } from "./pw-session.js";
|
||||||
|
|
||||||
export async function traceStartViaPlaywright(opts: {
|
export async function traceStartViaPlaywright(opts: {
|
||||||
@@ -32,6 +33,11 @@ export async function traceStopViaPlaywright(opts: {
|
|||||||
if (!ctxState.traceActive) {
|
if (!ctxState.traceActive) {
|
||||||
throw new Error("No active trace. Start a trace before stopping it.");
|
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;
|
ctxState.traceActive = false;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,3 +1,5 @@
|
|||||||
|
import fs from "node:fs/promises";
|
||||||
|
import os from "node:os";
|
||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||||
import {
|
import {
|
||||||
@@ -23,6 +25,15 @@ describe("pw-tools-core", () => {
|
|||||||
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw");
|
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
async function withTempDir<T>(run: (tempDir: string) => Promise<T>): Promise<T> {
|
||||||
|
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: {
|
async function waitForImplicitDownloadOutput(params: {
|
||||||
downloadUrl: string;
|
downloadUrl: string;
|
||||||
suggestedFilename: string;
|
suggestedFilename: string;
|
||||||
@@ -67,64 +78,121 @@ describe("pw-tools-core", () => {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
it("waits for the next download and saves it", async () => {
|
it("waits for the next download and atomically finalizes explicit output paths", async () => {
|
||||||
const harness = createDownloadEventHarness();
|
await withTempDir(async (tempDir) => {
|
||||||
|
const harness = createDownloadEventHarness();
|
||||||
|
const targetPath = path.join(tempDir, "file.bin");
|
||||||
|
|
||||||
const saveAs = vi.fn(async () => {});
|
const saveAs = vi.fn(async (outPath: string) => {
|
||||||
const download = {
|
await fs.writeFile(outPath, "file-content", "utf8");
|
||||||
url: () => "https://example.com/file.bin",
|
});
|
||||||
suggestedFilename: () => "file.bin",
|
const download = {
|
||||||
saveAs,
|
url: () => "https://example.com/file.bin",
|
||||||
};
|
suggestedFilename: () => "file.bin",
|
||||||
|
saveAs,
|
||||||
|
};
|
||||||
|
|
||||||
const targetPath = path.resolve("/tmp/file.bin");
|
const p = mod.waitForDownloadViaPlaywright({
|
||||||
const p = mod.waitForDownloadViaPlaywright({
|
cdpUrl: "http://127.0.0.1:18792",
|
||||||
cdpUrl: "http://127.0.0.1:18792",
|
targetId: "T1",
|
||||||
targetId: "T1",
|
path: targetPath,
|
||||||
path: targetPath,
|
timeoutMs: 1000,
|
||||||
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 () => {
|
it("clicks a ref and atomically finalizes explicit download paths", async () => {
|
||||||
const harness = createDownloadEventHarness();
|
await withTempDir(async (tempDir) => {
|
||||||
|
const harness = createDownloadEventHarness();
|
||||||
|
|
||||||
const click = vi.fn(async () => {});
|
const click = vi.fn(async () => {});
|
||||||
setPwToolsCoreCurrentRefLocator({ click });
|
setPwToolsCoreCurrentRefLocator({ click });
|
||||||
|
|
||||||
const saveAs = vi.fn(async () => {});
|
const saveAs = vi.fn(async (outPath: string) => {
|
||||||
const download = {
|
await fs.writeFile(outPath, "report-content", "utf8");
|
||||||
url: () => "https://example.com/report.pdf",
|
});
|
||||||
suggestedFilename: () => "report.pdf",
|
const download = {
|
||||||
saveAs,
|
url: () => "https://example.com/report.pdf",
|
||||||
};
|
suggestedFilename: () => "report.pdf",
|
||||||
|
saveAs,
|
||||||
|
};
|
||||||
|
|
||||||
const targetPath = path.resolve("/tmp/report.pdf");
|
const targetPath = path.join(tempDir, "report.pdf");
|
||||||
const p = mod.downloadViaPlaywright({
|
const p = mod.downloadViaPlaywright({
|
||||||
cdpUrl: "http://127.0.0.1:18792",
|
cdpUrl: "http://127.0.0.1:18792",
|
||||||
targetId: "T1",
|
targetId: "T1",
|
||||||
ref: "e12",
|
ref: "e12",
|
||||||
path: targetPath,
|
path: targetPath,
|
||||||
timeoutMs: 1000,
|
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 () => {
|
it("uses preferred tmp dir when waiting for download without explicit path", async () => {
|
||||||
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw-preferred");
|
tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw-preferred");
|
||||||
const { res, outPath } = await waitForImplicitDownloadOutput({
|
const { res, outPath } = await waitForImplicitDownloadOutput({
|
||||||
|
|||||||
Reference in New Issue
Block a user