diff --git a/src/browser/cdp.ts b/src/browser/cdp.ts index 58616fc272..577ac37a49 100644 --- a/src/browser/cdp.ts +++ b/src/browser/cdp.ts @@ -1,6 +1,6 @@ import type { SsrFPolicy } from "../infra/net/ssrf.js"; import { appendCdpPath, fetchJson, isLoopbackHost, withCdpSocket } from "./cdp.helpers.js"; -import { assertBrowserNavigationAllowed } from "./navigation-guard.js"; +import { assertBrowserNavigationAllowed, withBrowserNavigationPolicy } from "./navigation-guard.js"; export { appendCdpPath, fetchJson, fetchOk, getHeadersWithAuth } from "./cdp.helpers.js"; @@ -88,11 +88,14 @@ export async function createTargetViaCdp(opts: { cdpUrl: string; url: string; ssrfPolicy?: SsrFPolicy; + navigationChecked?: boolean; }): Promise<{ targetId: string }> { - await assertBrowserNavigationAllowed({ - url: opts.url, - ssrfPolicy: opts.ssrfPolicy, - }); + if (!opts.navigationChecked) { + 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.test.ts b/src/browser/navigation-guard.test.ts index ce97e47754..efa07be639 100644 --- a/src/browser/navigation-guard.test.ts +++ b/src/browser/navigation-guard.test.ts @@ -1,6 +1,14 @@ -import { describe, expect, it } from "vitest"; -import { SsrFBlockedError } from "../infra/net/ssrf.js"; -import { assertBrowserNavigationAllowed } from "./navigation-guard.js"; +import { describe, expect, it, vi } from "vitest"; +import { SsrFBlockedError, type LookupFn } from "../infra/net/ssrf.js"; +import { + assertBrowserNavigationAllowed, + InvalidBrowserNavigationUrlError, +} from "./navigation-guard.js"; + +function createLookupFn(address: string): LookupFn { + const family = address.includes(":") ? 6 : 4; + return vi.fn(async () => [{ address, family }]) as unknown as LookupFn; +} describe("browser navigation guard", () => { it("blocks private loopback URLs by default", async () => { @@ -19,15 +27,39 @@ describe("browser navigation guard", () => { ).resolves.toBeUndefined(); }); - it("allows localhost when explicitly allowed", async () => { + it("allows blocked hostnames when explicitly allowed", async () => { + const lookupFn = createLookupFn("127.0.0.1"); await expect( assertBrowserNavigationAllowed({ - url: "http://localhost:3000", + url: "http://agent.internal:3000", ssrfPolicy: { - allowedHostnames: ["localhost"], + allowedHostnames: ["agent.internal"], }, + lookupFn, }), ).resolves.toBeUndefined(); + expect(lookupFn).toHaveBeenCalledWith("agent.internal", { all: true }); + }); + + it("blocks hostnames that resolve to private addresses by default", async () => { + const lookupFn = createLookupFn("127.0.0.1"); + await expect( + assertBrowserNavigationAllowed({ + url: "https://example.com", + lookupFn, + }), + ).rejects.toBeInstanceOf(SsrFBlockedError); + }); + + it("allows hostnames that resolve to public addresses", async () => { + const lookupFn = createLookupFn("93.184.216.34"); + await expect( + assertBrowserNavigationAllowed({ + url: "https://example.com", + lookupFn, + }), + ).resolves.toBeUndefined(); + expect(lookupFn).toHaveBeenCalledWith("example.com", { all: true }); }); it("rejects invalid URLs", async () => { @@ -35,6 +67,6 @@ describe("browser navigation guard", () => { assertBrowserNavigationAllowed({ url: "not a url", }), - ).rejects.toThrow(/Invalid URL/); + ).rejects.toBeInstanceOf(InvalidBrowserNavigationUrlError); }); }); diff --git a/src/browser/navigation-guard.ts b/src/browser/navigation-guard.ts index 6bcca27598..f5c4048476 100644 --- a/src/browser/navigation-guard.ts +++ b/src/browser/navigation-guard.ts @@ -1,21 +1,44 @@ -import { resolvePinnedHostnameWithPolicy, type SsrFPolicy } from "../infra/net/ssrf.js"; +import { + resolvePinnedHostnameWithPolicy, + type LookupFn, + type SsrFPolicy, +} from "../infra/net/ssrf.js"; const NETWORK_NAVIGATION_PROTOCOLS = new Set(["http:", "https:"]); -export async function assertBrowserNavigationAllowed(opts: { - url: string; +export class InvalidBrowserNavigationUrlError extends Error { + constructor(message: string) { + super(message); + this.name = "InvalidBrowserNavigationUrlError"; + } +} + +export type BrowserNavigationPolicyOptions = { ssrfPolicy?: SsrFPolicy; -}): Promise { +}; + +export function withBrowserNavigationPolicy( + ssrfPolicy?: SsrFPolicy, +): BrowserNavigationPolicyOptions { + return ssrfPolicy ? { ssrfPolicy } : {}; +} + +export async function assertBrowserNavigationAllowed( + opts: { + url: string; + lookupFn?: LookupFn; + } & BrowserNavigationPolicyOptions, +): Promise { const rawUrl = String(opts.url ?? "").trim(); if (!rawUrl) { - throw new Error("url is required"); + throw new InvalidBrowserNavigationUrlError("url is required"); } let parsed: URL; try { parsed = new URL(rawUrl); } catch { - throw new Error(`Invalid URL: ${rawUrl}`); + throw new InvalidBrowserNavigationUrlError(`Invalid URL: ${rawUrl}`); } if (!NETWORK_NAVIGATION_PROTOCOLS.has(parsed.protocol)) { @@ -23,6 +46,7 @@ export async function assertBrowserNavigationAllowed(opts: { } await resolvePinnedHostnameWithPolicy(parsed.hostname, { + lookupFn: opts.lookupFn, policy: opts.ssrfPolicy, }); } diff --git a/src/browser/pw-session.ts b/src/browser/pw-session.ts index b8022a6bee..a8ff3c3f30 100644 --- a/src/browser/pw-session.ts +++ b/src/browser/pw-session.ts @@ -12,7 +12,7 @@ import type { SsrFPolicy } from "../infra/net/ssrf.js"; import { appendCdpPath, fetchJson, getHeadersWithAuth, withCdpSocket } from "./cdp.helpers.js"; import { normalizeCdpWsUrl } from "./cdp.js"; import { getChromeWebSocketUrl } from "./chrome.js"; -import { assertBrowserNavigationAllowed } from "./navigation-guard.js"; +import { assertBrowserNavigationAllowed, withBrowserNavigationPolicy } from "./navigation-guard.js"; export type BrowserConsoleMessage = { type: string; @@ -722,6 +722,7 @@ export async function createPageViaPlaywright(opts: { cdpUrl: string; url: string; ssrfPolicy?: SsrFPolicy; + navigationChecked?: boolean; }): Promise<{ targetId: string; title: string; @@ -738,10 +739,12 @@ export async function createPageViaPlaywright(opts: { // Navigate to the URL const targetUrl = opts.url.trim() || "about:blank"; if (targetUrl !== "about:blank") { - await assertBrowserNavigationAllowed({ - url: targetUrl, - ssrfPolicy: opts.ssrfPolicy, - }); + if (!opts.navigationChecked) { + 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.ts b/src/browser/pw-tools-core.snapshot.ts index a1a59399fc..076a33a114 100644 --- a/src/browser/pw-tools-core.snapshot.ts +++ b/src/browser/pw-tools-core.snapshot.ts @@ -1,6 +1,6 @@ import type { SsrFPolicy } from "../infra/net/ssrf.js"; import { type AriaSnapshotNode, formatAriaSnapshot, type RawAXNode } from "./cdp.js"; -import { assertBrowserNavigationAllowed } from "./navigation-guard.js"; +import { assertBrowserNavigationAllowed, withBrowserNavigationPolicy } from "./navigation-guard.js"; import { buildRoleSnapshotFromAiSnapshot, buildRoleSnapshotFromAriaSnapshot, @@ -168,7 +168,7 @@ export async function navigateViaPlaywright(opts: { } await assertBrowserNavigationAllowed({ url, - ssrfPolicy: opts.ssrfPolicy, + ...withBrowserNavigationPolicy(opts.ssrfPolicy), }); const page = await getPageForTargetId(opts); ensurePageState(page); diff --git a/src/browser/routes/agent.snapshot.ts b/src/browser/routes/agent.snapshot.ts index 45b367f483..3fb076bc06 100644 --- a/src/browser/routes/agent.snapshot.ts +++ b/src/browser/routes/agent.snapshot.ts @@ -6,6 +6,7 @@ import { DEFAULT_AI_SNAPSHOT_EFFICIENT_MAX_CHARS, DEFAULT_AI_SNAPSHOT_MAX_CHARS, } from "../constants.js"; +import { withBrowserNavigationPolicy } from "../navigation-guard.js"; import { DEFAULT_BROWSER_SCREENSHOT_MAX_BYTES, DEFAULT_BROWSER_SCREENSHOT_MAX_SIDE, @@ -65,12 +66,11 @@ export function registerBrowserAgentSnapshotRoutes( targetId, feature: "navigate", run: async ({ cdpUrl, tab, pw }) => { - const ssrfPolicy = ctx.state().resolved.ssrfPolicy; const result = await pw.navigateViaPlaywright({ cdpUrl, targetId: tab.targetId, url, - ...(ssrfPolicy ? { ssrfPolicy } : {}), + ...withBrowserNavigationPolicy(ctx.state().resolved.ssrfPolicy), }); res.json({ ok: true, targetId: tab.targetId, ...result }); }, diff --git a/src/browser/routes/tabs.ts b/src/browser/routes/tabs.ts index c0644a97d1..89531b22f9 100644 --- a/src/browser/routes/tabs.ts +++ b/src/browser/routes/tabs.ts @@ -121,6 +121,7 @@ export function registerBrowserTabRoutes(app: BrowserRouteRegistrar, ctx: Browse req, res, ctx, + mapTabError: true, run: async (profileCtx) => { await profileCtx.ensureBrowserAvailable(); const tab = await profileCtx.openTab(url); diff --git a/src/browser/server-context.remote-tab-ops.test.ts b/src/browser/server-context.remote-tab-ops.test.ts index f2a49b400c..05b6a51534 100644 --- a/src/browser/server-context.remote-tab-ops.test.ts +++ b/src/browser/server-context.remote-tab-ops.test.ts @@ -90,6 +90,12 @@ describe("browser server-context remote profile tab operations", () => { const opened = await remote.openTab("http://127.0.0.1:3000"); expect(opened.targetId).toBe("T2"); expect(state.profiles.get("remote")?.lastTargetId).toBe("T2"); + expect(createPageViaPlaywright).toHaveBeenCalledWith({ + cdpUrl: "https://browserless.example/chrome?token=abc", + url: "http://127.0.0.1:3000", + ssrfPolicy: { allowPrivateNetwork: true }, + navigationChecked: true, + }); await remote.closeTab("T1"); expect(closePageByTargetIdViaPlaywright).toHaveBeenCalledWith({ @@ -214,7 +220,9 @@ describe("browser server-context remote profile tab operations", () => { describe("browser server-context tab selection state", () => { it("updates lastTargetId when openTab is created via CDP", async () => { - vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "CREATED" }); + const createTargetViaCdp = vi + .spyOn(cdpModule, "createTargetViaCdp") + .mockResolvedValue({ targetId: "CREATED" }); const fetchMock = vi.fn(async (url: unknown) => { const u = String(url); @@ -244,5 +252,11 @@ describe("browser server-context tab selection state", () => { const opened = await openclaw.openTab("http://127.0.0.1:8080"); expect(opened.targetId).toBe("CREATED"); expect(state.profiles.get("openclaw")?.lastTargetId).toBe("CREATED"); + expect(createTargetViaCdp).toHaveBeenCalledWith({ + cdpUrl: "http://127.0.0.1:18800", + url: "http://127.0.0.1:8080", + ssrfPolicy: { allowPrivateNetwork: true }, + navigationChecked: true, + }); }); }); diff --git a/src/browser/server-context.ts b/src/browser/server-context.ts index e0f45c6f78..7c7e27f34e 100644 --- a/src/browser/server-context.ts +++ b/src/browser/server-context.ts @@ -15,7 +15,11 @@ import { ensureChromeExtensionRelayServer, stopChromeExtensionRelayServer, } from "./extension-relay.js"; -import { assertBrowserNavigationAllowed } from "./navigation-guard.js"; +import { + assertBrowserNavigationAllowed, + InvalidBrowserNavigationUrlError, + withBrowserNavigationPolicy, +} from "./navigation-guard.js"; import type { PwAiModule } from "./pw-ai-module.js"; import { getPwAiModule } from "./pw-ai-module.js"; import { @@ -132,8 +136,8 @@ function createProfileContext( }; const openTab = async (url: string): Promise => { - const ssrfPolicy = state().resolved.ssrfPolicy; - await assertBrowserNavigationAllowed({ url, ssrfPolicy }); + 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 @@ -144,7 +148,8 @@ function createProfileContext( const page = await createPageViaPlaywright({ cdpUrl: profile.cdpUrl, url, - ...(ssrfPolicy ? { ssrfPolicy } : {}), + ...ssrfPolicyOpts, + navigationChecked: true, }); const profileState = getProfileState(); profileState.lastTargetId = page.targetId; @@ -160,7 +165,8 @@ function createProfileContext( const createdViaCdp = await createTargetViaCdp({ cdpUrl: profile.cdpUrl, url, - ...(ssrfPolicy ? { ssrfPolicy } : {}), + ...ssrfPolicyOpts, + navigationChecked: true, }) .then((r) => r.targetId) .catch(() => null); @@ -645,10 +651,10 @@ export function createBrowserRouteContext(opts: ContextOptions): BrowserRouteCon if (err instanceof SsrFBlockedError) { return { status: 400, message: err.message }; } - const msg = String(err); - if (msg.includes("Invalid URL:")) { - return { status: 400, message: msg }; + if (err instanceof InvalidBrowserNavigationUrlError) { + return { status: 400, message: err.message }; } + const msg = String(err); if (msg.includes("ambiguous target id prefix")) { return { status: 409, message: "ambiguous target id prefix" }; } diff --git a/src/browser/server.post-tabs-open-profile-unknown-returns-404.test.ts b/src/browser/server.post-tabs-open-profile-unknown-returns-404.test.ts index c240e58efb..3fd00cdc1b 100644 --- a/src/browser/server.post-tabs-open-profile-unknown-returns-404.test.ts +++ b/src/browser/server.post-tabs-open-profile-unknown-returns-404.test.ts @@ -32,6 +32,20 @@ describe("browser control server", () => { const body = (await result.json()) as { error: string }; expect(body.error).toContain("not found"); }); + + it("POST /tabs/open returns 400 for invalid URLs", async () => { + await startBrowserControlServerFromConfig(); + const base = getBrowserControlServerBaseUrl(); + + const result = await realFetch(`${base}/tabs/open`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ url: "not a url" }), + }); + expect(result.status).toBe(400); + const body = (await result.json()) as { error: string }; + expect(body.error).toContain("Invalid URL:"); + }); }); describe("profile CRUD endpoints", () => {