diff --git a/src/browser/cdp.test.ts b/src/browser/cdp.test.ts index 1acd0004e5a..e8e2b9f6d6a 100644 --- a/src/browser/cdp.test.ts +++ b/src/browser/cdp.test.ts @@ -4,6 +4,7 @@ import { type WebSocket, WebSocketServer } from "ws"; import { SsrFBlockedError } from "../infra/net/ssrf.js"; import { rawDataToString } from "../infra/ws.js"; import { createTargetViaCdp, evaluateJavaScript, normalizeCdpWsUrl, snapshotAria } from "./cdp.js"; +import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; describe("cdp", () => { let httpServer: ReturnType | null = null; @@ -109,6 +110,21 @@ describe("cdp", () => { } }); + it("blocks unsupported non-network navigation URLs", async () => { + const fetchSpy = vi.spyOn(globalThis, "fetch"); + try { + await expect( + createTargetViaCdp({ + cdpUrl: "http://127.0.0.1:9222", + url: "file:///etc/passwd", + }), + ).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + fetchSpy.mockRestore(); + } + }); + it("allows private navigation targets when explicitly configured", async () => { const wsPort = await startWsServerWithMessages((msg, socket) => { if (msg.method !== "Target.createTarget") { diff --git a/src/browser/cdp.ts b/src/browser/cdp.ts index 577ac37a49a..20686b76fed 100644 --- a/src/browser/cdp.ts +++ b/src/browser/cdp.ts @@ -88,14 +88,11 @@ export async function createTargetViaCdp(opts: { cdpUrl: string; url: string; ssrfPolicy?: SsrFPolicy; - navigationChecked?: boolean; }): Promise<{ targetId: string }> { - if (!opts.navigationChecked) { - await assertBrowserNavigationAllowed({ - url: opts.url, - ...withBrowserNavigationPolicy(opts.ssrfPolicy), - }); - } + await assertBrowserNavigationAllowed({ + url: opts.url, + ...withBrowserNavigationPolicy(opts.ssrfPolicy), + }); const version = await fetchJson<{ webSocketDebuggerUrl?: string }>( appendCdpPath(opts.cdpUrl, "/json/version"), diff --git a/src/browser/navigation-guard.ts b/src/browser/navigation-guard.ts index 5567642e3b0..f9b9fe2268b 100644 --- a/src/browser/navigation-guard.ts +++ b/src/browser/navigation-guard.ts @@ -7,6 +7,11 @@ import { const NETWORK_NAVIGATION_PROTOCOLS = new Set(["http:", "https:"]); const SAFE_NON_NETWORK_URLS = new Set(["about:blank"]); +function isAllowedNonNetworkNavigationUrl(parsed: URL): boolean { + // Keep non-network navigation explicit; about:blank is the only allowed bootstrap URL. + return SAFE_NON_NETWORK_URLS.has(parsed.href); +} + export class InvalidBrowserNavigationUrlError extends Error { constructor(message: string) { super(message); @@ -43,7 +48,7 @@ export async function assertBrowserNavigationAllowed( } if (!NETWORK_NAVIGATION_PROTOCOLS.has(parsed.protocol)) { - if (SAFE_NON_NETWORK_URLS.has(parsed.href)) { + if (isAllowedNonNetworkNavigationUrl(parsed)) { return; } throw new InvalidBrowserNavigationUrlError( diff --git a/src/browser/pw-session.create-page.navigation-guard.test.ts b/src/browser/pw-session.create-page.navigation-guard.test.ts new file mode 100644 index 00000000000..088cbeaa721 --- /dev/null +++ b/src/browser/pw-session.create-page.navigation-guard.test.ts @@ -0,0 +1,95 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; +import { closePlaywrightBrowserConnection, createPageViaPlaywright } from "./pw-session.js"; + +const connectOverCdpMock = vi.fn(); +const getChromeWebSocketUrlMock = vi.fn(); + +vi.mock("playwright-core", () => ({ + chromium: { + connectOverCDP: (...args: unknown[]) => connectOverCdpMock(...args), + }, +})); + +vi.mock("./chrome.js", () => ({ + getChromeWebSocketUrl: (...args: unknown[]) => getChromeWebSocketUrlMock(...args), +})); + +function installBrowserMocks() { + const pageOn = vi.fn(); + const pageGoto = vi.fn(async () => {}); + const pageTitle = vi.fn(async () => ""); + const pageUrl = vi.fn(() => "about:blank"); + const contextOn = vi.fn(); + const browserOn = vi.fn(); + const browserClose = vi.fn(async () => {}); + const sessionSend = vi.fn(async (method: string) => { + if (method === "Target.getTargetInfo") { + return { targetInfo: { targetId: "TARGET_1" } }; + } + return {}; + }); + const sessionDetach = vi.fn(async () => {}); + + const context = { + pages: () => [], + on: contextOn, + newPage: vi.fn(async () => page), + newCDPSession: vi.fn(async () => ({ + send: sessionSend, + detach: sessionDetach, + })), + } as unknown as import("playwright-core").BrowserContext; + + const page = { + on: pageOn, + context: () => context, + goto: pageGoto, + title: pageTitle, + url: pageUrl, + } as unknown as import("playwright-core").Page; + + const browser = { + contexts: () => [context], + on: browserOn, + close: browserClose, + } as unknown as import("playwright-core").Browser; + + connectOverCdpMock.mockResolvedValue(browser); + getChromeWebSocketUrlMock.mockResolvedValue(null); + + return { pageGoto, browserClose }; +} + +afterEach(async () => { + connectOverCdpMock.mockReset(); + getChromeWebSocketUrlMock.mockReset(); + await closePlaywrightBrowserConnection().catch(() => {}); +}); + +describe("pw-session createPageViaPlaywright navigation guard", () => { + it("blocks unsupported non-network URLs", async () => { + const { pageGoto } = installBrowserMocks(); + + await expect( + createPageViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "file:///etc/passwd", + }), + ).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError); + + expect(pageGoto).not.toHaveBeenCalled(); + }); + + it("allows about:blank without network navigation", async () => { + const { pageGoto } = installBrowserMocks(); + + const created = await createPageViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "about:blank", + }); + + expect(created.targetId).toBe("TARGET_1"); + expect(pageGoto).not.toHaveBeenCalled(); + }); +}); diff --git a/src/browser/pw-session.ts b/src/browser/pw-session.ts index a8ff3c3f30d..b1a704b124a 100644 --- a/src/browser/pw-session.ts +++ b/src/browser/pw-session.ts @@ -7,8 +7,8 @@ import type { Response, } from "playwright-core"; import { chromium } from "playwright-core"; -import { formatErrorMessage } from "../infra/errors.js"; import type { SsrFPolicy } from "../infra/net/ssrf.js"; +import { formatErrorMessage } from "../infra/errors.js"; import { appendCdpPath, fetchJson, getHeadersWithAuth, withCdpSocket } from "./cdp.helpers.js"; import { normalizeCdpWsUrl } from "./cdp.js"; import { getChromeWebSocketUrl } from "./chrome.js"; @@ -722,7 +722,6 @@ export async function createPageViaPlaywright(opts: { cdpUrl: string; url: string; ssrfPolicy?: SsrFPolicy; - navigationChecked?: boolean; }): Promise<{ targetId: string; title: string; @@ -739,12 +738,10 @@ export async function createPageViaPlaywright(opts: { // Navigate to the URL const targetUrl = opts.url.trim() || "about:blank"; if (targetUrl !== "about:blank") { - if (!opts.navigationChecked) { - await assertBrowserNavigationAllowed({ - url: targetUrl, - ...withBrowserNavigationPolicy(opts.ssrfPolicy), - }); - } + await assertBrowserNavigationAllowed({ + url: targetUrl, + ...withBrowserNavigationPolicy(opts.ssrfPolicy), + }); await page.goto(targetUrl, { timeout: 30_000 }).catch(() => { // Navigation might fail for some URLs, but page is still created }); diff --git a/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts b/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts new file mode 100644 index 00000000000..07c2aa19f3c --- /dev/null +++ b/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it, vi } from "vitest"; +import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; +import { + getPwToolsCoreSessionMocks, + installPwToolsCoreTestHooks, + setPwToolsCoreCurrentPage, +} from "./pw-tools-core.test-harness.js"; + +installPwToolsCoreTestHooks(); +const mod = await import("./pw-tools-core.snapshot.js"); + +describe("pw-tools-core.snapshot navigate guard", () => { + it("blocks unsupported non-network URLs before page lookup", async () => { + const goto = vi.fn(async () => {}); + setPwToolsCoreCurrentPage({ + goto, + url: vi.fn(() => "about:blank"), + }); + + await expect( + mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "file:///etc/passwd", + }), + ).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError); + + expect(getPwToolsCoreSessionMocks().getPageForTargetId).not.toHaveBeenCalled(); + expect(goto).not.toHaveBeenCalled(); + }); + + it("navigates valid network URLs with clamped timeout", async () => { + const goto = vi.fn(async () => {}); + setPwToolsCoreCurrentPage({ + goto, + url: vi.fn(() => "https://example.com"), + }); + + const result = await mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "https://example.com", + timeoutMs: 10, + }); + + expect(goto).toHaveBeenCalledWith("https://example.com", { timeout: 1000 }); + expect(result.url).toBe("https://example.com"); + }); +}); diff --git a/src/browser/server-context.remote-tab-ops.test.ts b/src/browser/server-context.remote-tab-ops.test.ts index 05b6a515348..70d4ea844c5 100644 --- a/src/browser/server-context.remote-tab-ops.test.ts +++ b/src/browser/server-context.remote-tab-ops.test.ts @@ -1,8 +1,9 @@ import { afterEach, describe, expect, it, vi } from "vitest"; +import type { BrowserServerState } from "./server-context.js"; import { withFetchPreconnect } from "../test-utils/fetch-mock.js"; import * as cdpModule from "./cdp.js"; +import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; import * as pwAiModule from "./pw-ai-module.js"; -import type { BrowserServerState } from "./server-context.js"; import "./server-context.chrome-test-harness.js"; import { createBrowserRouteContext } from "./server-context.js"; @@ -94,7 +95,6 @@ describe("browser server-context remote profile tab operations", () => { cdpUrl: "https://browserless.example/chrome?token=abc", url: "http://127.0.0.1:3000", ssrfPolicy: { allowPrivateNetwork: true }, - navigationChecked: true, }); await remote.closeTab("T1"); @@ -256,7 +256,22 @@ describe("browser server-context tab selection state", () => { cdpUrl: "http://127.0.0.1:18800", url: "http://127.0.0.1:8080", ssrfPolicy: { allowPrivateNetwork: true }, - navigationChecked: true, }); }); + + it("blocks unsupported non-network URLs before any HTTP tab-open fallback", async () => { + const fetchMock = vi.fn(async () => { + throw new Error("unexpected fetch"); + }); + + global.fetch = withFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + const ctx = createBrowserRouteContext({ getState: () => state }); + const openclaw = ctx.forProfile("openclaw"); + + await expect(openclaw.openTab("file:///etc/passwd")).rejects.toBeInstanceOf( + InvalidBrowserNavigationUrlError, + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/browser/server-context.ts b/src/browser/server-context.ts index 7c7e27f34e8..93c90b1ef0f 100644 --- a/src/browser/server-context.ts +++ b/src/browser/server-context.ts @@ -1,4 +1,15 @@ import fs from "node:fs"; +import type { ResolvedBrowserProfile } from "./config.js"; +import type { PwAiModule } from "./pw-ai-module.js"; +import type { + BrowserServerState, + BrowserRouteContext, + BrowserTab, + ContextOptions, + ProfileContext, + ProfileRuntimeState, + ProfileStatus, +} from "./server-context.types.js"; import { SsrFBlockedError } from "../infra/net/ssrf.js"; import { fetchJson, fetchOk } from "./cdp.helpers.js"; import { appendCdpPath, createTargetViaCdp, normalizeCdpWsUrl } from "./cdp.js"; @@ -9,7 +20,6 @@ import { resolveOpenClawUserDataDir, stopOpenClawChrome, } from "./chrome.js"; -import type { ResolvedBrowserProfile } from "./config.js"; import { resolveProfile } from "./config.js"; import { ensureChromeExtensionRelayServer, @@ -20,21 +30,11 @@ import { InvalidBrowserNavigationUrlError, withBrowserNavigationPolicy, } from "./navigation-guard.js"; -import type { PwAiModule } from "./pw-ai-module.js"; import { getPwAiModule } from "./pw-ai-module.js"; import { refreshResolvedBrowserConfigFromDisk, resolveBrowserProfileWithHotReload, } from "./resolved-config-refresh.js"; -import type { - BrowserServerState, - BrowserRouteContext, - BrowserTab, - ContextOptions, - ProfileContext, - ProfileRuntimeState, - ProfileStatus, -} from "./server-context.types.js"; import { resolveTargetIdFromTabs } from "./target-id.js"; import { movePathToTrash } from "./trash.js"; @@ -137,7 +137,6 @@ function createProfileContext( const openTab = async (url: string): Promise => { const ssrfPolicyOpts = withBrowserNavigationPolicy(state().resolved.ssrfPolicy); - await assertBrowserNavigationAllowed({ url, ...ssrfPolicyOpts }); // For remote profiles, use Playwright's persistent connection to create tabs // This ensures the tab persists beyond a single request @@ -149,7 +148,6 @@ function createProfileContext( cdpUrl: profile.cdpUrl, url, ...ssrfPolicyOpts, - navigationChecked: true, }); const profileState = getProfileState(); profileState.lastTargetId = page.targetId; @@ -166,7 +164,6 @@ function createProfileContext( cdpUrl: profile.cdpUrl, url, ...ssrfPolicyOpts, - navigationChecked: true, }) .then((r) => r.targetId) .catch(() => null); @@ -196,6 +193,7 @@ function createProfileContext( }; const endpointUrl = new URL(appendCdpPath(profile.cdpUrl, "/json/new")); + await assertBrowserNavigationAllowed({ url, ...ssrfPolicyOpts }); const endpoint = endpointUrl.search ? (() => { endpointUrl.searchParams.set("url", url);