diff --git a/CHANGELOG.md b/CHANGELOG.md index 963892e9ff5..c8d520827bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai - 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. - 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. - Security/Microsoft Teams: isolate group allowlist and command authorization from DM pairing-store entries to prevent cross-context authorization bleed. (#26111) Thanks @bmendonca3. +- 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. - Security/LINE: cap unsigned webhook body reads before auth/signature handling to bound unauthenticated body processing. (#26095) Thanks @bmendonca3. - 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) - 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. diff --git a/src/browser/paths.test.ts b/src/browser/paths.test.ts index 441ee05b869..1599c3895b2 100644 --- a/src/browser/paths.test.ts +++ b/src/browser/paths.test.ts @@ -6,6 +6,7 @@ import { resolveExistingPathsWithinRoot, resolvePathsWithinRoot, resolvePathWithinRoot, + resolveStrictExistingPathsWithinRoot, } from "./paths.js"; async function createFixtureRoot(): Promise<{ baseDir: string; uploadsDir: string }> { @@ -194,6 +195,29 @@ describe("resolveExistingPathsWithinRoot", () => { ); }); +describe("resolveStrictExistingPathsWithinRoot", () => { + function expectInvalidResult( + result: Awaited>, + expectedSnippet: string, + ) { + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain(expectedSnippet); + } + } + + it("rejects missing files instead of returning lexical fallbacks", async () => { + await withFixtureRoot(async ({ uploadsDir }) => { + const result = await resolveStrictExistingPathsWithinRoot({ + rootDir: uploadsDir, + requestedPaths: ["missing.txt"], + scopeLabel: "uploads directory", + }); + expectInvalidResult(result, "regular non-symlink file"); + }); + }); +}); + describe("resolvePathWithinRoot", () => { it("uses default file name when requested path is blank", () => { const result = resolvePathWithinRoot({ diff --git a/src/browser/paths.ts b/src/browser/paths.ts index 0b458e44dec..88a541b75dc 100644 --- a/src/browser/paths.ts +++ b/src/browser/paths.ts @@ -54,6 +54,29 @@ export async function resolveExistingPathsWithinRoot(params: { rootDir: string; requestedPaths: string[]; scopeLabel: string; +}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> { + return await resolveCheckedPathsWithinRoot({ + ...params, + allowMissingFallback: true, + }); +} + +export async function resolveStrictExistingPathsWithinRoot(params: { + rootDir: string; + requestedPaths: string[]; + scopeLabel: string; +}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> { + return await resolveCheckedPathsWithinRoot({ + ...params, + allowMissingFallback: false, + }); +} + +async function resolveCheckedPathsWithinRoot(params: { + rootDir: string; + requestedPaths: string[]; + scopeLabel: string; + allowMissingFallback: boolean; }): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> { const rootDir = path.resolve(params.rootDir); let rootRealPath: string | undefined; @@ -119,7 +142,7 @@ export async function resolveExistingPathsWithinRoot(params: { }); resolvedPaths.push(opened.realPath); } catch (err) { - if (err instanceof SafeOpenError && err.code === "not-found") { + if (params.allowMissingFallback && err instanceof SafeOpenError && err.code === "not-found") { // Preserve historical behavior for paths that do not exist yet. resolvedPaths.push(pathResult.fallbackPath); continue; diff --git a/src/browser/pw-tools-core.downloads.ts b/src/browser/pw-tools-core.downloads.ts index 12be321653b..4933c78b5e4 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 { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js"; import { ensurePageState, getPageForTargetId, @@ -166,7 +167,20 @@ export async function armFileUploadViaPlaywright(opts: { } return; } - await fileChooser.setFiles(opts.paths); + const uploadPathsResult = await resolveStrictExistingPathsWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPaths: opts.paths, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!uploadPathsResult.ok) { + try { + await page.keyboard.press("Escape"); + } catch { + // Best-effort. + } + return; + } + await fileChooser.setFiles(uploadPathsResult.paths); try { const input = typeof fileChooser.element === "function" diff --git a/src/browser/pw-tools-core.interactions.set-input-files.test.ts b/src/browser/pw-tools-core.interactions.set-input-files.test.ts new file mode 100644 index 00000000000..dfbd6f58563 --- /dev/null +++ b/src/browser/pw-tools-core.interactions.set-input-files.test.ts @@ -0,0 +1,111 @@ +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +let page: Record | null = null; +let locator: Record | null = null; + +const getPageForTargetId = vi.fn(async () => { + if (!page) { + throw new Error("test: page not set"); + } + return page; +}); +const ensurePageState = vi.fn(() => ({})); +const restoreRoleRefsForTarget = vi.fn(() => {}); +const refLocator = vi.fn(() => { + if (!locator) { + throw new Error("test: locator not set"); + } + return locator; +}); +const forceDisconnectPlaywrightForTarget = vi.fn(async () => {}); + +const resolveStrictExistingPathsWithinRoot = + vi.fn(); + +vi.mock("./pw-session.js", () => { + return { + ensurePageState, + forceDisconnectPlaywrightForTarget, + getPageForTargetId, + refLocator, + restoreRoleRefsForTarget, + }; +}); + +vi.mock("./paths.js", () => { + return { + DEFAULT_UPLOAD_DIR: "/tmp/openclaw/uploads", + resolveStrictExistingPathsWithinRoot, + }; +}); + +let setInputFilesViaPlaywright: typeof import("./pw-tools-core.interactions.js").setInputFilesViaPlaywright; + +describe("setInputFilesViaPlaywright", () => { + beforeAll(async () => { + ({ setInputFilesViaPlaywright } = await import("./pw-tools-core.interactions.js")); + }); + + beforeEach(() => { + vi.clearAllMocks(); + page = null; + locator = null; + resolveStrictExistingPathsWithinRoot.mockResolvedValue({ + ok: true, + paths: ["/private/tmp/openclaw/uploads/ok.txt"], + }); + }); + + it("revalidates upload paths and uses resolved canonical paths for inputRef", async () => { + const setInputFiles = vi.fn(async () => {}); + locator = { + setInputFiles, + elementHandle: vi.fn(async () => null), + }; + page = { + locator: vi.fn(() => ({ first: () => locator })), + }; + + await setInputFilesViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + inputRef: "e7", + paths: ["/tmp/openclaw/uploads/ok.txt"], + }); + + expect(resolveStrictExistingPathsWithinRoot).toHaveBeenCalledWith({ + rootDir: "/tmp/openclaw/uploads", + requestedPaths: ["/tmp/openclaw/uploads/ok.txt"], + scopeLabel: "uploads directory (/tmp/openclaw/uploads)", + }); + expect(refLocator).toHaveBeenCalledWith(page, "e7"); + expect(setInputFiles).toHaveBeenCalledWith(["/private/tmp/openclaw/uploads/ok.txt"]); + }); + + it("throws and skips setInputFiles when use-time validation fails", async () => { + resolveStrictExistingPathsWithinRoot.mockResolvedValueOnce({ + ok: false, + error: "Invalid path: must stay within uploads directory", + }); + + const setInputFiles = vi.fn(async () => {}); + locator = { + setInputFiles, + elementHandle: vi.fn(async () => null), + }; + page = { + locator: vi.fn(() => ({ first: () => locator })), + }; + + await expect( + setInputFilesViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + element: "input[type=file]", + paths: ["/tmp/openclaw/uploads/missing.txt"], + }), + ).rejects.toThrow("Invalid path: must stay within uploads directory"); + + expect(setInputFiles).not.toHaveBeenCalled(); + }); +}); diff --git a/src/browser/pw-tools-core.interactions.ts b/src/browser/pw-tools-core.interactions.ts index 55e130c580e..cd6ad0e165c 100644 --- a/src/browser/pw-tools-core.interactions.ts +++ b/src/browser/pw-tools-core.interactions.ts @@ -1,4 +1,5 @@ import type { BrowserFormField } from "./client-actions-core.js"; +import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js"; import { ensurePageState, forceDisconnectPlaywrightForTarget, @@ -626,9 +627,18 @@ export async function setInputFilesViaPlaywright(opts: { } const locator = inputRef ? refLocator(page, inputRef) : page.locator(element).first(); + const uploadPathsResult = await resolveStrictExistingPathsWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPaths: opts.paths, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!uploadPathsResult.ok) { + throw new Error(uploadPathsResult.error); + } + const resolvedPaths = uploadPathsResult.paths; try { - await locator.setInputFiles(opts.paths); + await locator.setInputFiles(resolvedPaths); } catch (err) { throw toAIFriendlyError(err, inputRef || element); } diff --git a/src/browser/pw-tools-core.last-file-chooser-arm-wins.test.ts b/src/browser/pw-tools-core.last-file-chooser-arm-wins.test.ts index 3afbb2b9d40..16264ba9eb3 100644 --- a/src/browser/pw-tools-core.last-file-chooser-arm-wins.test.ts +++ b/src/browser/pw-tools-core.last-file-chooser-arm-wins.test.ts @@ -1,4 +1,8 @@ +import crypto from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; import { describe, expect, it, vi } from "vitest"; +import { DEFAULT_UPLOAD_DIR } from "./paths.js"; import { installPwToolsCoreTestHooks, setPwToolsCoreCurrentPage, @@ -9,6 +13,15 @@ const mod = await import("./pw-tools-core.js"); describe("pw-tools-core", () => { it("last file-chooser arm wins", async () => { + const firstPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-arm-1-${crypto.randomUUID()}.txt`); + const secondPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-arm-2-${crypto.randomUUID()}.txt`); + await fs.mkdir(DEFAULT_UPLOAD_DIR, { recursive: true }); + await Promise.all([ + fs.writeFile(firstPath, "1", "utf8"), + fs.writeFile(secondPath, "2", "utf8"), + ]); + const secondCanonicalPath = await fs.realpath(secondPath); + let resolve1: ((value: unknown) => void) | null = null; let resolve2: ((value: unknown) => void) | null = null; @@ -35,24 +48,30 @@ describe("pw-tools-core", () => { keyboard: { press: vi.fn(async () => {}) }, }); - await mod.armFileUploadViaPlaywright({ - cdpUrl: "http://127.0.0.1:18792", - paths: ["/tmp/1"], - }); - await mod.armFileUploadViaPlaywright({ - cdpUrl: "http://127.0.0.1:18792", - paths: ["/tmp/2"], - }); + try { + await mod.armFileUploadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + paths: [firstPath], + }); + await mod.armFileUploadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + paths: [secondPath], + }); - if (!resolve1 || !resolve2) { - throw new Error("file chooser handlers were not registered"); + if (!resolve1 || !resolve2) { + throw new Error("file chooser handlers were not registered"); + } + (resolve1 as (value: unknown) => void)(fc1); + (resolve2 as (value: unknown) => void)(fc2); + await Promise.resolve(); + + expect(fc1.setFiles).not.toHaveBeenCalled(); + await vi.waitFor(() => { + expect(fc2.setFiles).toHaveBeenCalledWith([secondCanonicalPath]); + }); + } finally { + await Promise.all([fs.rm(firstPath, { force: true }), fs.rm(secondPath, { force: true })]); } - (resolve1 as (value: unknown) => void)(fc1); - (resolve2 as (value: unknown) => void)(fc2); - await Promise.resolve(); - - expect(fc1.setFiles).not.toHaveBeenCalled(); - expect(fc2.setFiles).toHaveBeenCalledWith(["/tmp/2"]); }); it("arms the next dialog and accepts/dismisses (default timeout)", async () => { const accept = vi.fn(async () => {}); diff --git a/src/browser/pw-tools-core.screenshots-element-selector.test.ts b/src/browser/pw-tools-core.screenshots-element-selector.test.ts index 843d07050fb..1894d65912f 100644 --- a/src/browser/pw-tools-core.screenshots-element-selector.test.ts +++ b/src/browser/pw-tools-core.screenshots-element-selector.test.ts @@ -1,4 +1,8 @@ +import crypto from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; import { describe, expect, it, vi } from "vitest"; +import { DEFAULT_UPLOAD_DIR } from "./paths.js"; import { getPwToolsCoreSessionMocks, installPwToolsCoreTestHooks, @@ -81,6 +85,10 @@ describe("pw-tools-core", () => { ).rejects.toThrow(/fullPage is not supported/i); }); it("arms the next file chooser and sets files (default timeout)", async () => { + const uploadPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-upload-${crypto.randomUUID()}.txt`); + await fs.mkdir(path.dirname(uploadPath), { recursive: true }); + await fs.writeFile(uploadPath, "fixture", "utf8"); + const canonicalUploadPath = await fs.realpath(uploadPath); const fileChooser = { setFiles: vi.fn(async () => {}) }; const waitForEvent = vi.fn(async (_event: string, _opts: unknown) => fileChooser); setPwToolsCoreCurrentPage({ @@ -88,19 +96,47 @@ describe("pw-tools-core", () => { keyboard: { press: vi.fn(async () => {}) }, }); + try { + await mod.armFileUploadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + paths: [uploadPath], + }); + + // waitForEvent is awaited immediately; handler continues async. + await Promise.resolve(); + + expect(waitForEvent).toHaveBeenCalledWith("filechooser", { + timeout: 120_000, + }); + await vi.waitFor(() => { + expect(fileChooser.setFiles).toHaveBeenCalledWith([canonicalUploadPath]); + }); + } finally { + await fs.rm(uploadPath, { force: true }); + } + }); + it("revalidates file-chooser paths at use-time and cancels missing files", async () => { + const missingPath = path.join(DEFAULT_UPLOAD_DIR, `vitest-missing-${crypto.randomUUID()}.txt`); + const fileChooser = { setFiles: vi.fn(async () => {}) }; + const press = vi.fn(async () => {}); + const waitForEvent = vi.fn(async () => fileChooser); + setPwToolsCoreCurrentPage({ + waitForEvent, + keyboard: { press }, + }); + await mod.armFileUploadViaPlaywright({ cdpUrl: "http://127.0.0.1:18792", targetId: "T1", - paths: ["/tmp/a.txt"], + paths: [missingPath], }); - - // waitForEvent is awaited immediately; handler continues async. await Promise.resolve(); - expect(waitForEvent).toHaveBeenCalledWith("filechooser", { - timeout: 120_000, + await vi.waitFor(() => { + expect(press).toHaveBeenCalledWith("Escape"); }); - expect(fileChooser.setFiles).toHaveBeenCalledWith(["/tmp/a.txt"]); + expect(fileChooser.setFiles).not.toHaveBeenCalled(); }); it("arms the next file chooser and escapes if no paths provided", async () => { const fileChooser = { setFiles: vi.fn(async () => {}) };