Skip to content

Commit ef326f5

Browse files
steipetesteipete
committed
fix(browser): revalidate upload paths at use time
1 parent 15cfba7 commit ef326f5

8 files changed

Lines changed: 264 additions & 26 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai
2121
- Security/Nextcloud Talk: stop treating DM pairing-store entries as group allowlist senders, so group authorization remains bounded to configured group allowlists. (#26116) Thanks @bmendonca3.
2222
- Security/IRC: keep pairing-store approvals DM-only and out of IRC group allowlist authorization, with policy regression tests for allowlist resolution. (#26112) Thanks @bmendonca3.
2323
- Security/Microsoft Teams: isolate group allowlist and command authorization from DM pairing-store entries to prevent cross-context authorization bleed. (#26111) Thanks @bmendonca3.
24+
- Security/Browser uploads: revalidate upload paths at use-time in Playwright file-chooser and direct-input flows so missing/rebound paths are rejected before `setFiles`, with regression coverage for strict missing-path handling.
2425
- Security/LINE: cap unsigned webhook body reads before auth/signature handling to bound unauthenticated body processing. (#26095) Thanks @bmendonca3.
2526
- Agents/Model fallback: keep explicit text + image fallback chains reachable even when `agents.defaults.models` allowlists are present, prefer explicit run `agentId` over session-key parsing for followup fallback override resolution (with session-key fallback), treat agent-level fallback overrides as configured in embedded runner preflight, and classify `model_cooldown` / `cooling down` errors as `rate_limit` so failover continues. (#11972, #24137, #17231)
2627
- Followups/Routing: when explicit origin routing fails, allow same-channel fallback dispatch (while still blocking cross-channel fallback) so followup replies do not get dropped on transient origin-adapter failures. (#26109) Thanks @Sid-Qin.

‎src/browser/paths.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
resolveExistingPathsWithinRoot,
77
resolvePathsWithinRoot,
88
resolvePathWithinRoot,
9+
resolveStrictExistingPathsWithinRoot,
910
} from "./paths.js";
1011

1112
async function createFixtureRoot(): Promise<{ baseDir: string; uploadsDir: string }> {
@@ -194,6 +195,29 @@ describe("resolveExistingPathsWithinRoot", () => {
194195
);
195196
});
196197

198+
describe("resolveStrictExistingPathsWithinRoot", () => {
199+
function expectInvalidResult(
200+
result: Awaited<ReturnType<typeof resolveStrictExistingPathsWithinRoot>>,
201+
expectedSnippet: string,
202+
) {
203+
expect(result.ok).toBe(false);
204+
if (!result.ok) {
205+
expect(result.error).toContain(expectedSnippet);
206+
}
207+
}
208+
209+
it("rejects missing files instead of returning lexical fallbacks", async () => {
210+
await withFixtureRoot(async ({ uploadsDir }) => {
211+
const result = await resolveStrictExistingPathsWithinRoot({
212+
rootDir: uploadsDir,
213+
requestedPaths: ["missing.txt"],
214+
scopeLabel: "uploads directory",
215+
});
216+
expectInvalidResult(result, "regular non-symlink file");
217+
});
218+
});
219+
});
220+
197221
describe("resolvePathWithinRoot", () => {
198222
it("uses default file name when requested path is blank", () => {
199223
const result = resolvePathWithinRoot({

‎src/browser/paths.ts‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,29 @@ export async function resolveExistingPathsWithinRoot(params: {
5454
rootDir: string;
5555
requestedPaths: string[];
5656
scopeLabel: string;
57+
}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> {
58+
return await resolveCheckedPathsWithinRoot({
59+
...params,
60+
allowMissingFallback: true,
61+
});
62+
}
63+
64+
export async function resolveStrictExistingPathsWithinRoot(params: {
65+
rootDir: string;
66+
requestedPaths: string[];
67+
scopeLabel: string;
68+
}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> {
69+
return await resolveCheckedPathsWithinRoot({
70+
...params,
71+
allowMissingFallback: false,
72+
});
73+
}
74+
75+
async function resolveCheckedPathsWithinRoot(params: {
76+
rootDir: string;
77+
requestedPaths: string[];
78+
scopeLabel: string;
79+
allowMissingFallback: boolean;
5780
}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> {
5881
const rootDir = path.resolve(params.rootDir);
5982
let rootRealPath: string | undefined;
@@ -119,7 +142,7 @@ export async function resolveExistingPathsWithinRoot(params: {
119142
});
120143
resolvedPaths.push(opened.realPath);
121144
} catch (err) {
122-
if (err instanceof SafeOpenError && err.code === "not-found") {
145+
if (params.allowMissingFallback && err instanceof SafeOpenError && err.code === "not-found") {
123146
// Preserve historical behavior for paths that do not exist yet.
124147
resolvedPaths.push(pathResult.fallbackPath);
125148
continue;

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

Lines changed: 15 additions & 1 deletion
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 { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
67
import {
78
ensurePageState,
89
getPageForTargetId,
@@ -166,7 +167,20 @@ export async function armFileUploadViaPlaywright(opts: {
166167
}
167168
return;
168169
}
169-
await fileChooser.setFiles(opts.paths);
170+
const uploadPathsResult = await resolveStrictExistingPathsWithinRoot({
171+
rootDir: DEFAULT_UPLOAD_DIR,
172+
requestedPaths: opts.paths,
173+
scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`,
174+
});
175+
if (!uploadPathsResult.ok) {
176+
try {
177+
await page.keyboard.press("Escape");
178+
} catch {
179+
// Best-effort.
180+
}
181+
return;
182+
}
183+
await fileChooser.setFiles(uploadPathsResult.paths);
170184
try {
171185
const input =
172186
typeof fileChooser.element === "function"

‎src/browser/pw-tools-core.interactions.set-input-files.test.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
let page: Record<string, unknown> | null = null;
4+
let locator: Record<string, unknown> | null = null;
5+
6+
const getPageForTargetId = vi.fn(async () => {
7+
if (!page) {
8+
throw new Error("test: page not set");
9+
}
10+
return page;
11+
});
12+
const ensurePageState = vi.fn(() => ({}));
13+
const restoreRoleRefsForTarget = vi.fn(() => {});
14+
const refLocator = vi.fn(() => {
15+
if (!locator) {
16+
throw new Error("test: locator not set");
17+
}
18+
return locator;
19+
});
20+
const forceDisconnectPlaywrightForTarget = vi.fn(async () => {});
21+
22+
const resolveStrictExistingPathsWithinRoot =
23+
vi.fn<typeof import("./paths.js").resolveStrictExistingPathsWithinRoot>();
24+
25+
vi.mock("./pw-session.js", () => {
26+
return {
27+
ensurePageState,
28+
forceDisconnectPlaywrightForTarget,
29+
getPageForTargetId,
30+
refLocator,
31+
restoreRoleRefsForTarget,
32+
};
33+
});
34+
35+
vi.mock("./paths.js", () => {
36+
return {
37+
DEFAULT_UPLOAD_DIR: "/tmp/openclaw/uploads",
38+
resolveStrictExistingPathsWithinRoot,
39+
};
40+
});
41+
42+
let setInputFilesViaPlaywright: typeof import("./pw-tools-core.interactions.js").setInputFilesViaPlaywright;
43+
44+
describe("setInputFilesViaPlaywright", () => {
45+
beforeAll(async () => {
46+
({ setInputFilesViaPlaywright } = await import("./pw-tools-core.interactions.js"));
47+
});
48+
49+
beforeEach(() => {
50+
vi.clearAllMocks();
51+
page = null;
52+
locator = null;
53+
resolveStrictExistingPathsWithinRoot.mockResolvedValue({
54+
ok: true,
55+
paths: ["/private/tmp/openclaw/uploads/ok.txt"],
56+
});
57+
});
58+
59+
it("revalidates upload paths and uses resolved canonical paths for inputRef", async () => {
60+
const setInputFiles = vi.fn(async () => {});
61+
locator = {
62+
setInputFiles,
63+
elementHandle: vi.fn(async () => null),
64+
};
65+
page = {
66+
locator: vi.fn(() => ({ first: () => locator })),
67+
};
68+
69+
await setInputFilesViaPlaywright({
70+
cdpUrl: "http://127.0.0.1:18792",
71+
targetId: "T1",
72+
inputRef: "e7",
73+
paths: ["/tmp/openclaw/uploads/ok.txt"],
74+
});
75+
76+
expect(resolveStrictExistingPathsWithinRoot).toHaveBeenCalledWith({
77+
rootDir: "/tmp/openclaw/uploads",
78+
requestedPaths: ["/tmp/openclaw/uploads/ok.txt"],
79+
scopeLabel: "uploads directory (/tmp/openclaw/uploads)",
80+
});
81+
expect(refLocator).toHaveBeenCalledWith(page, "e7");
82+
expect(setInputFiles).toHaveBeenCalledWith(["/private/tmp/openclaw/uploads/ok.txt"]);
83+
});
84+
85+
it("throws and skips setInputFiles when use-time validation fails", async () => {
86+
resolveStrictExistingPathsWithinRoot.mockResolvedValueOnce({
87+
ok: false,
88+
error: "Invalid path: must stay within uploads directory",
89+
});
90+
91+
const setInputFiles = vi.fn(async () => {});
92+
locator = {
93+
setInputFiles,
94+
elementHandle: vi.fn(async () => null),
95+
};
96+
page = {
97+
locator: vi.fn(() => ({ first: () => locator })),
98+
};
99+
100+
await expect(
101+
setInputFilesViaPlaywright({
102+
cdpUrl: "http://127.0.0.1:18792",
103+
targetId: "T1",
104+
element: "input[type=file]",
105+
paths: ["/tmp/openclaw/uploads/missing.txt"],
106+
}),
107+
).rejects.toThrow("Invalid path: must stay within uploads directory");
108+
109+
expect(setInputFiles).not.toHaveBeenCalled();
110+
});
111+
});

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { BrowserFormField } from "./client-actions-core.js";
2+
import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
23
import {
34
ensurePageState,
45
forceDisconnectPlaywrightForTarget,
@@ -626,9 +627,18 @@ export async function setInputFilesViaPlaywright(opts: {
626627
}
627628

628629
const locator = inputRef ? refLocator(page, inputRef) : page.locator(element).first();
630+
const uploadPathsResult = await resolveStrictExistingPathsWithinRoot({
631+
rootDir: DEFAULT_UPLOAD_DIR,
632+
requestedPaths: opts.paths,
633+
scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`,
634+
});
635+
if (!uploadPathsResult.ok) {
636+
throw new Error(uploadPathsResult.error);
637+
}
638+
const resolvedPaths = uploadPathsResult.paths;
629639

630640
try {
631-
await locator.setInputFiles(opts.paths);
641+
await locator.setInputFiles(resolvedPaths);
632642
} catch (err) {
633643
throw toAIFriendlyError(err, inputRef || element);
634644
}

‎src/browser/pw-tools-core.last-file-chooser-arm-wins.test.ts‎

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1+
import crypto from "node:crypto";
2+
import fs from "node:fs/promises";
3+
import path from "node:path";
14
import { describe, expect, it, vi } from "vitest";
5+
import { DEFAULT_UPLOAD_DIR } from "./paths.js";
26
import {
37
installPwToolsCoreTestHooks,
48
setPwToolsCoreCurrentPage,
@@ -9,6 +13,15 @@ const mod = await import("./pw-tools-core.js");
913

1014
describe("pw-tools-core", () => {
1115
it("last file-chooser arm wins", async () => {
16+
const firstPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-arm-1-${crypto.randomUUID()}.txt`);
17+
const secondPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-arm-2-${crypto.randomUUID()}.txt`);
18+
await fs.mkdir(DEFAULT_UPLOAD_DIR, { recursive: true });
19+
await Promise.all([
20+
fs.writeFile(firstPath, "1", "utf8"),
21+
fs.writeFile(secondPath, "2", "utf8"),
22+
]);
23+
const secondCanonicalPath = await fs.realpath(secondPath);
24+
1225
let resolve1: ((value: unknown) => void) | null = null;
1326
let resolve2: ((value: unknown) => void) | null = null;
1427

@@ -35,24 +48,30 @@ describe("pw-tools-core", () => {
3548
keyboard: { press: vi.fn(async () => {}) },
3649
});
3750

38-
await mod.armFileUploadViaPlaywright({
39-
cdpUrl: "http://127.0.0.1:18792",
40-
paths: ["/tmp/1"],
41-
});
42-
await mod.armFileUploadViaPlaywright({
43-
cdpUrl: "http://127.0.0.1:18792",
44-
paths: ["/tmp/2"],
45-
});
46-
47-
if (!resolve1 || !resolve2) {
48-
throw new Error("file chooser handlers were not registered");
51+
try {
52+
await mod.armFileUploadViaPlaywright({
53+
cdpUrl: "http://127.0.0.1:18792",
54+
paths: [firstPath],
55+
});
56+
await mod.armFileUploadViaPlaywright({
57+
cdpUrl: "http://127.0.0.1:18792",
58+
paths: [secondPath],
59+
});
60+
61+
if (!resolve1 || !resolve2) {
62+
throw new Error("file chooser handlers were not registered");
63+
}
64+
(resolve1 as (value: unknown) => void)(fc1);
65+
(resolve2 as (value: unknown) => void)(fc2);
66+
await Promise.resolve();
67+
68+
expect(fc1.setFiles).not.toHaveBeenCalled();
69+
await vi.waitFor(() => {
70+
expect(fc2.setFiles).toHaveBeenCalledWith([secondCanonicalPath]);
71+
});
72+
} finally {
73+
await Promise.all([fs.rm(firstPath, { force: true }), fs.rm(secondPath, { force: true })]);
4974
}
50-
(resolve1 as (value: unknown) => void)(fc1);
51-
(resolve2 as (value: unknown) => void)(fc2);
52-
await Promise.resolve();
53-
54-
expect(fc1.setFiles).not.toHaveBeenCalled();
55-
expect(fc2.setFiles).toHaveBeenCalledWith(["/tmp/2"]);
5675
});
5776
it("arms the next dialog and accepts/dismisses (default timeout)", async () => {
5877
const accept = vi.fn(async () => {});

‎src/browser/pw-tools-core.screenshots-element-selector.test.ts‎

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1+
import crypto from "node:crypto";
2+
import fs from "node:fs/promises";
3+
import path from "node:path";
14
import { describe, expect, it, vi } from "vitest";
5+
import { DEFAULT_UPLOAD_DIR } from "./paths.js";
26
import {
37
getPwToolsCoreSessionMocks,
48
installPwToolsCoreTestHooks,
@@ -81,26 +85,58 @@ describe("pw-tools-core", () => {
8185
).rejects.toThrow(/fullPage is not supported/i);
8286
});
8387
it("arms the next file chooser and sets files (default timeout)", async () => {
88+
const uploadPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-upload-${crypto.randomUUID()}.txt`);
89+
await fs.mkdir(path.dirname(uploadPath), { recursive: true });
90+
await fs.writeFile(uploadPath, "fixture", "utf8");
91+
const canonicalUploadPath = await fs.realpath(uploadPath);
8492
const fileChooser = { setFiles: vi.fn(async () => {}) };
8593
const waitForEvent = vi.fn(async (_event: string, _opts: unknown) => fileChooser);
8694
setPwToolsCoreCurrentPage({
8795
waitForEvent,
8896
keyboard: { press: vi.fn(async () => {}) },
8997
});
9098

99+
try {
100+
await mod.armFileUploadViaPlaywright({
101+
cdpUrl: "http://127.0.0.1:18792",
102+
targetId: "T1",
103+
paths: [uploadPath],
104+
});
105+
106+
// waitForEvent is awaited immediately; handler continues async.
107+
await Promise.resolve();
108+
109+
expect(waitForEvent).toHaveBeenCalledWith("filechooser", {
110+
timeout: 120_000,
111+
});
112+
await vi.waitFor(() => {
113+
expect(fileChooser.setFiles).toHaveBeenCalledWith([canonicalUploadPath]);
114+
});
115+
} finally {
116+
await fs.rm(uploadPath, { force: true });
117+
}
118+
});
119+
it("revalidates file-chooser paths at use-time and cancels missing files", async () => {
120+
const missingPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-missing-${crypto.randomUUID()}.txt`);
121+
const fileChooser = { setFiles: vi.fn(async () => {}) };
122+
const press = vi.fn(async () => {});
123+
const waitForEvent = vi.fn(async () => fileChooser);
124+
setPwToolsCoreCurrentPage({
125+
waitForEvent,
126+
keyboard: { press },
127+
});
128+
91129
await mod.armFileUploadViaPlaywright({
92130
cdpUrl: "http://127.0.0.1:18792",
93131
targetId: "T1",
94-
paths: ["/tmp/a.txt"],
132+
paths: [missingPath],
95133
});
96-
97-
// waitForEvent is awaited immediately; handler continues async.
98134
await Promise.resolve();
99135

100-
expect(waitForEvent).toHaveBeenCalledWith("filechooser", {
101-
timeout: 120_000,
136+
await vi.waitFor(() => {
137+
expect(press).toHaveBeenCalledWith("Escape");
102138
});
103-
expect(fileChooser.setFiles).toHaveBeenCalledWith(["/tmp/a.txt"]);
139+
expect(fileChooser.setFiles).not.toHaveBeenCalled();
104140
});
105141
it("arms the next file chooser and escapes if no paths provided", async () => {
106142
const fileChooser = { setFiles: vi.fn(async () => {}) };

0 commit comments

Comments
 (0)