From ef0dbcf49d858763a73d2ce5996dc541a45f78e0 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Mon, 4 May 2026 13:07:17 -0700 Subject: [PATCH] Guard current browser tab exports (#75731) * fix(browser): guard current tab exports * fix(browser): expand tab guard coverage * fix(browser): guard tab reads * fix(browser): guard screenshot route * changelog: PR #75731 --------- Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + .../browser/src/browser/routes/agent.act.ts | 1 + .../browser/src/browser/routes/agent.debug.ts | 5 + .../src/browser/routes/agent.shared.test.ts | 70 +++++++++++++- .../src/browser/routes/agent.shared.ts | 26 +++++ .../src/browser/routes/agent.snapshot.ts | 9 +- .../src/browser/routes/agent.storage.ts | 8 ++ ...-contract-form-layout-act-commands.test.ts | 96 +++++++++++++++++++ .../server.control-server.test-harness.ts | 24 ++++- 9 files changed, 230 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8812b358f8..259fb809ed5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -212,6 +212,7 @@ Docs: https://docs.openclaw.ai - Security/Windows: pin Windows registry-probe `reg.exe` resolution to the canonical Windows install root in install-root probing, so `SystemRoot`/`WINDIR` env overrides cannot redirect registry queries during Windows host detection. (#74454) Thanks @mmaps. - QQBot: preserve the framework command authorization decision when converting framework command contexts into engine slash command contexts, so downstream slash handlers see `commandAuthorized` matching the channel's resolved `isAuthorizedSender` instead of a hardcoded `true`. (#77453) Thanks @drobison00. - Security/Windows: block `LOCALAPPDATA` from workspace `.env` and resolve Windows update-flow portable Git path prepends from the trusted process-local `LOCALAPPDATA` only, so workspace-supplied values cannot redirect `git` discovery during `openclaw update`. (#77470) Thanks @drobison00. +- Browser/SSRF: enforce the existing current-tab URL navigation policy before tab-scoped debug, export, and read routes (console, page errors, network requests, trace start/stop, response body, screenshot, snapshot, storage, etc.) collect from an already-selected tab, so blocked tabs return a policy error instead of being read first and redacted only at response time. (#75731) Thanks @eleqtrizit. ## 2026.5.3-1 diff --git a/extensions/browser/src/browser/routes/agent.act.ts b/extensions/browser/src/browser/routes/agent.act.ts index ab33f4fb07f..9820ebbb8a8 100644 --- a/extensions/browser/src/browser/routes/agent.act.ts +++ b/extensions/browser/src/browser/routes/agent.act.ts @@ -695,6 +695,7 @@ export function registerBrowserAgentActRoutes( res, ctx, targetId, + enforceCurrentUrlAllowed: true, run: async ({ profileCtx, cdpUrl, tab, resolveTabUrl }) => { if (getBrowserProfileCapabilities(profileCtx.profile).usesChromeMcp) { return jsonError(res, 501, EXISTING_SESSION_LIMITS.responseBody); diff --git a/extensions/browser/src/browser/routes/agent.debug.ts b/extensions/browser/src/browser/routes/agent.debug.ts index 948f1f03be4..81ba1f1b4af 100644 --- a/extensions/browser/src/browser/routes/agent.debug.ts +++ b/extensions/browser/src/browser/routes/agent.debug.ts @@ -29,6 +29,7 @@ export function registerBrowserAgentDebugRoutes( ctx, targetId, feature: "console messages", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw, resolveTabUrl }) => { const messages = await pw.getConsoleMessagesViaPlaywright({ cdpUrl, @@ -54,6 +55,7 @@ export function registerBrowserAgentDebugRoutes( ctx, targetId, feature: "page errors", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw, resolveTabUrl }) => { const result = await pw.getPageErrorsViaPlaywright({ cdpUrl, @@ -80,6 +82,7 @@ export function registerBrowserAgentDebugRoutes( ctx, targetId, feature: "network requests", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw, resolveTabUrl }) => { const result = await pw.getNetworkRequestsViaPlaywright({ cdpUrl, @@ -109,6 +112,7 @@ export function registerBrowserAgentDebugRoutes( ctx, targetId, feature: "trace start", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw, resolveTabUrl }) => { await pw.traceStartViaPlaywright({ cdpUrl, @@ -137,6 +141,7 @@ export function registerBrowserAgentDebugRoutes( ctx, targetId, feature: "trace stop", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw, resolveTabUrl }) => { const id = crypto.randomUUID(); const tracePath = await resolveWritableOutputPathOrRespond({ diff --git a/extensions/browser/src/browser/routes/agent.shared.test.ts b/extensions/browser/src/browser/routes/agent.shared.test.ts index a6247d35724..1de49298c01 100644 --- a/extensions/browser/src/browser/routes/agent.shared.test.ts +++ b/extensions/browser/src/browser/routes/agent.shared.test.ts @@ -1,10 +1,13 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; +import type { BrowserRouteContext, ProfileContext } from "../server-context.js"; import { readBody, resolveSafeRouteTabUrl, resolveTargetIdFromBody, resolveTargetIdFromQuery, + withRouteTabContext, } from "./agent.shared.js"; +import { createBrowserRouteResponse } from "./test-helpers.js"; import type { BrowserRequest } from "./types.js"; function requestWithBody(body: unknown): BrowserRequest { @@ -36,6 +39,31 @@ function profileContext(tabs: Array<{ targetId: string; url: string }>) { }; } +function routeContextForTab(url: string): BrowserRouteContext { + const profileCtx = { + profile: { + cdpUrl: "http://127.0.0.1:9222", + name: "default", + }, + ensureTabAvailable: vi.fn(async () => ({ + targetId: "tab-1", + title: "Tab", + url, + type: "page", + })), + } as unknown as ProfileContext; + + return { + forProfile: () => profileCtx, + state: () => ({ + resolved: { + ssrfPolicy: {}, + }, + }), + mapTabError: () => null, + } as unknown as BrowserRouteContext; +} + describe("browser route shared helpers", () => { describe("readBody", () => { it("returns object bodies", () => { @@ -100,4 +128,44 @@ describe("browser route shared helpers", () => { ).resolves.toBeUndefined(); }); }); + + describe("withRouteTabContext", () => { + it("does not enforce current-tab URL policy unless requested", async () => { + const response = createBrowserRouteResponse(); + const run = vi.fn(async () => { + response.res.json({ ok: true }); + }); + + await withRouteTabContext({ + req: requestWithBody({}), + res: response.res, + ctx: routeContextForTab("http://127.0.0.1:8080/admin"), + run, + }); + + expect(run).toHaveBeenCalledOnce(); + expect(response.body).toEqual({ ok: true }); + }); + + it("blocks guarded routes before running on a disallowed current tab", async () => { + const response = createBrowserRouteResponse(); + const run = vi.fn(async () => { + response.res.json({ ok: true }); + }); + + await withRouteTabContext({ + req: requestWithBody({}), + res: response.res, + ctx: routeContextForTab("http://127.0.0.1:8080/admin"), + enforceCurrentUrlAllowed: true, + run, + }); + + expect(run).not.toHaveBeenCalled(); + expect(response.statusCode).toBe(400); + expect(response.body).toMatchObject({ error: expect.any(String) }); + const body = response.body as { error?: unknown }; + expect(body.error).not.toBe(""); + }); + }); }); diff --git a/extensions/browser/src/browser/routes/agent.shared.ts b/extensions/browser/src/browser/routes/agent.shared.ts index e3c1af2acf4..d95e3f13216 100644 --- a/extensions/browser/src/browser/routes/agent.shared.ts +++ b/extensions/browser/src/browser/routes/agent.shared.ts @@ -107,6 +107,11 @@ type RouteWithTabParams = { res: BrowserResponse; ctx: BrowserRouteContext; targetId?: string; + /** + * Set for routes that read from or return data scoped to the selected tab. + * Leave false only for routes that navigate, activate, close, or otherwise manage the tab. + */ + enforceCurrentUrlAllowed?: boolean; run: (ctx: RouteTabContext) => Promise; }; @@ -119,6 +124,17 @@ export async function withRouteTabContext( } try { const tab = await profileCtx.ensureTabAvailable(params.targetId); + if (params.enforceCurrentUrlAllowed) { + await assertBrowserNavigationResultAllowed({ + url: tab.url, + ...withBrowserNavigationPolicy(params.ctx.state().resolved.ssrfPolicy, { + browserProxyMode: resolveBrowserNavigationProxyMode({ + resolved: params.ctx.state().resolved, + profile: profileCtx.profile, + }), + }), + }); + } return await params.run({ profileCtx, tab, @@ -137,6 +153,10 @@ export async function withRouteTabContext( } } +/** + * Response-only URL redaction. This swallows policy failures and must not be used as + * an execution gate; use enforceCurrentUrlAllowed on the route helper instead. + */ export async function resolveSafeRouteTabUrl(params: { ctx: BrowserRouteContext; profileCtx: ProfileContext; @@ -171,6 +191,11 @@ type RouteWithPwParams = { ctx: BrowserRouteContext; targetId?: string; feature: string; + /** + * Set for routes that read from or return data scoped to the selected tab. + * Leave false only for routes that navigate, activate, close, or otherwise manage the tab. + */ + enforceCurrentUrlAllowed?: boolean; run: (ctx: RouteTabPwContext) => Promise; }; @@ -182,6 +207,7 @@ export async function withPlaywrightRouteContext( res: params.res, ctx: params.ctx, targetId: params.targetId, + enforceCurrentUrlAllowed: params.enforceCurrentUrlAllowed, run: async ({ profileCtx, tab, cdpUrl, resolveTabUrl }) => { const pw = await requirePwAi(params.res, params.feature); if (!pw) { diff --git a/extensions/browser/src/browser/routes/agent.snapshot.ts b/extensions/browser/src/browser/routes/agent.snapshot.ts index 36d6d3039ee..37446db2267 100644 --- a/extensions/browser/src/browser/routes/agent.snapshot.ts +++ b/extensions/browser/src/browser/routes/agent.snapshot.ts @@ -318,6 +318,7 @@ export function registerBrowserAgentSnapshotRoutes( ctx, targetId, feature: "pdf", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw }) => { const pdf = await pw.pdfViaPlaywright({ cdpUrl, @@ -361,18 +362,12 @@ export function registerBrowserAgentSnapshotRoutes( res, ctx, targetId, + enforceCurrentUrlAllowed: true, run: async ({ profileCtx, tab, cdpUrl }) => { if (getBrowserProfileCapabilities(profileCtx.profile).usesChromeMcp) { - const ssrfPolicyOpts = browserNavigationPolicyForProfile(ctx, profileCtx); if (element) { return jsonError(res, 400, EXISTING_SESSION_LIMITS.snapshot.screenshotElement); } - if (ssrfPolicyOpts.ssrfPolicy) { - await assertBrowserNavigationResultAllowed({ - url: tab.url, - ...ssrfPolicyOpts, - }); - } if (labels) { const snapshot = await takeChromeMcpSnapshot({ profileName: profileCtx.profile.name, diff --git a/extensions/browser/src/browser/routes/agent.storage.ts b/extensions/browser/src/browser/routes/agent.storage.ts index 0ec039f14b6..cc78dbd1f03 100644 --- a/extensions/browser/src/browser/routes/agent.storage.ts +++ b/extensions/browser/src/browser/routes/agent.storage.ts @@ -85,6 +85,7 @@ export function registerBrowserAgentStorageRoutes( ctx, targetId, feature: "cookies", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw }) => { const result = await pw.cookiesGetViaPlaywright({ cdpUrl, @@ -109,6 +110,7 @@ export function registerBrowserAgentStorageRoutes( return jsonError(res, 400, "cookie is required"); } + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, @@ -148,6 +150,7 @@ export function registerBrowserAgentStorageRoutes( const body = readBody(req); const targetId = resolveTargetIdFromBody(body); + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, @@ -181,6 +184,7 @@ export function registerBrowserAgentStorageRoutes( ctx, targetId, feature: "storage get", + enforceCurrentUrlAllowed: true, run: async ({ cdpUrl, tab, pw }) => { const result = await pw.storageGetViaPlaywright({ cdpUrl, @@ -207,6 +211,7 @@ export function registerBrowserAgentStorageRoutes( } const value = typeof mutation.body.value === "string" ? mutation.body.value : ""; + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, @@ -235,6 +240,7 @@ export function registerBrowserAgentStorageRoutes( return; } + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, @@ -263,6 +269,7 @@ export function registerBrowserAgentStorageRoutes( return jsonError(res, 400, "offline is required"); } + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, @@ -301,6 +308,7 @@ export function registerBrowserAgentStorageRoutes( } } + // Intentional: mutation routes are outside the tab-scoped read/export guard scope. await withPlaywrightRouteContext({ req, res, diff --git a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts index 97a958cbbe2..dc5947309d8 100644 --- a/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -11,6 +11,8 @@ import { import { getBrowserControlServerTestState, getPwMocks, + setBrowserControlServerSsrFPolicy, + setBrowserControlServerTabUrl, } from "./server.control-server.test-harness.js"; import { getBrowserTestFetch, type BrowserTestFetch } from "./test-support/fetch.js"; @@ -18,6 +20,81 @@ const state = getBrowserControlServerTestState(); const pwMocks = getPwMocks(); const realFetch: BrowserTestFetch = (input, init) => getBrowserTestFetch()(input, init); +type GuardedCurrentTabRouteCase = { + method: "GET" | "POST"; + path: string; + body?: Record; + mockName: + | "cookiesGetViaPlaywright" + | "pdfViaPlaywright" + | "getConsoleMessagesViaPlaywright" + | "getPageErrorsViaPlaywright" + | "getNetworkRequestsViaPlaywright" + | "responseBodyViaPlaywright" + | "storageGetViaPlaywright" + | "takeScreenshotViaPlaywright" + | "traceStartViaPlaywright" + | "traceStopViaPlaywright"; +}; + +const guardedCurrentTabRouteCases: readonly GuardedCurrentTabRouteCase[] = [ + { + method: "GET", + path: "/console?targetId=abcd1234", + mockName: "getConsoleMessagesViaPlaywright", + }, + { + method: "GET", + path: "/errors?targetId=abcd1234", + mockName: "getPageErrorsViaPlaywright", + }, + { + method: "GET", + path: "/requests?targetId=abcd1234", + mockName: "getNetworkRequestsViaPlaywright", + }, + { + method: "POST", + path: "/pdf", + body: { targetId: "abcd1234" }, + mockName: "pdfViaPlaywright", + }, + { + method: "POST", + path: "/screenshot", + body: { targetId: "abcd1234" }, + mockName: "takeScreenshotViaPlaywright", + }, + { + method: "POST", + path: "/response/body", + body: { targetId: "abcd1234", url: "**/api/data" }, + mockName: "responseBodyViaPlaywright", + }, + { + method: "GET", + path: "/cookies?targetId=abcd1234", + mockName: "cookiesGetViaPlaywright", + }, + { + method: "GET", + path: "/storage/local?targetId=abcd1234", + mockName: "storageGetViaPlaywright", + }, + { + method: "POST", + path: "/trace/start", + body: { targetId: "abcd1234" }, + mockName: "traceStartViaPlaywright", + }, + { + method: "POST", + path: "/trace/stop", + body: { targetId: "abcd1234" }, + mockName: "traceStopViaPlaywright", + }, +] as const; + async function withSymlinkPathEscape(params: { rootDir: string; run: (relativePath: string) => Promise; @@ -439,6 +516,25 @@ describe("browser control server", () => { ); }); + it.each(guardedCurrentTabRouteCases)( + "blocks $method $path on disallowed current tab URLs", + async (routeCase) => { + setBrowserControlServerSsrFPolicy({ allowPrivateNetwork: false }); + setBrowserControlServerTabUrl("http://127.0.0.1:8080/admin"); + const base = await startServerAndBase(); + + const res = await realFetch(`${base}${routeCase.path}`, { + method: routeCase.method, + headers: routeCase.body ? { "Content-Type": "application/json" } : undefined, + body: routeCase.body ? JSON.stringify(routeCase.body) : undefined, + }); + expect(res.status).toBe(400); + const body = (await res.json()) as { error?: unknown }; + expect(body.error).toEqual(expect.stringMatching(/(blocked|denied|not allowed|policy)/i)); + expect(pwMocks[routeCase.mockName]).not.toHaveBeenCalled(); + }, + ); + it("wait/download rejects traversal path outside downloads dir", async () => { const base = await startServerAndBase(); const waitRes = await postJson<{ error?: string }>(`${base}/wait/download`, { diff --git a/extensions/browser/src/browser/server.control-server.test-harness.ts b/extensions/browser/src/browser/server.control-server.test-harness.ts index fe7a525aee4..bc5d16eb00c 100644 --- a/extensions/browser/src/browser/server.control-server.test-harness.ts +++ b/extensions/browser/src/browser/server.control-server.test-harness.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, vi } from "vitest"; import { deriveDefaultBrowserCdpPortRange } from "../config/port-defaults.js"; +import type { SsrFPolicy } from "../infra/net/ssrf.js"; import type { MockFn } from "../test-utils/vitest-mock-fn.js"; import { installChromeUserDataDirHooks } from "./chrome-user-data-dir.test-harness.js"; import { getFreePort } from "./test-port.js"; @@ -10,6 +11,7 @@ type HarnessState = { reachable: boolean; cfgAttachOnly: boolean; cfgEvaluateEnabled: boolean; + cfgSsrfPolicy: SsrFPolicy | undefined; cfgDefaultProfile: string; cfgProfiles: Record< string, @@ -21,6 +23,7 @@ type HarnessState = { attachOnly?: boolean; } >; + tabUrl: string; prevGatewayPort: string | undefined; prevGatewayToken: string | undefined; prevGatewayPassword: string | undefined; @@ -32,8 +35,10 @@ const state: HarnessState = { reachable: false, cfgAttachOnly: false, cfgEvaluateEnabled: true, + cfgSsrfPolicy: undefined, cfgDefaultProfile: "openclaw", cfgProfiles: {}, + tabUrl: "https://example.com", prevGatewayPort: undefined, prevGatewayToken: undefined, prevGatewayPassword: undefined, @@ -59,10 +64,18 @@ export function setBrowserControlServerEvaluateEnabled(enabled: boolean): void { state.cfgEvaluateEnabled = enabled; } +export function setBrowserControlServerSsrFPolicy(policy: SsrFPolicy | undefined): void { + state.cfgSsrfPolicy = policy; +} + export function setBrowserControlServerReachable(reachable: boolean): void { state.reachable = reachable; } +export function setBrowserControlServerTabUrl(url: string): void { + state.tabUrl = url; +} + export function setBrowserControlServerProfiles( profiles: HarnessState["cfgProfiles"], defaultProfile = Object.keys(profiles)[0] ?? "openclaw", @@ -152,6 +165,7 @@ const pwMocks = vi.hoisted(() => ({ clickViaPlaywright: vi.fn(async (_opts?: unknown) => {}), closePageViaPlaywright: vi.fn(async (_opts?: unknown) => {}), closePlaywrightBrowserConnection: vi.fn(async () => {}), + cookiesGetViaPlaywright: vi.fn(async () => ({ cookies: [] })), downloadViaPlaywright: vi.fn(async () => ({ url: "https://example.com/report.pdf", suggestedFilename: "report.pdf", @@ -161,6 +175,8 @@ const pwMocks = vi.hoisted(() => ({ evaluateViaPlaywright: vi.fn(async (_opts?: unknown) => "ok"), fillFormViaPlaywright: vi.fn(async (_opts?: unknown) => {}), getConsoleMessagesViaPlaywright: vi.fn(async () => []), + getNetworkRequestsViaPlaywright: vi.fn(async () => ({ requests: [] })), + getPageErrorsViaPlaywright: vi.fn(async () => ({ errors: [] })), hoverViaPlaywright: vi.fn(async (_opts?: unknown) => {}), scrollIntoViewViaPlaywright: vi.fn(async (_opts?: unknown) => {}), navigateViaPlaywright: vi.fn(async () => ({ url: "https://example.com" })), @@ -181,7 +197,9 @@ const pwMocks = vi.hoisted(() => ({ refs: { e1: { role: "button", name: "Role" } }, stats: { lines: 1, chars: 24, refs: 1, interactive: 1 }, })), + storageGetViaPlaywright: vi.fn(async () => ({ values: {} })), storeAriaSnapshotRefsViaPlaywright: vi.fn(async () => {}), + traceStartViaPlaywright: vi.fn(async () => {}), traceStopViaPlaywright: vi.fn(async () => {}), takeScreenshotViaPlaywright: vi.fn(async () => ({ buffer: Buffer.from("png"), @@ -393,13 +411,13 @@ vi.mock("../config/config.js", async () => { evaluateEnabled: state.cfgEvaluateEnabled, color: "#FF4500", attachOnly: state.cfgAttachOnly, + ssrfPolicy: state.cfgSsrfPolicy ?? { dangerouslyAllowPrivateNetwork: true }, headless: true, defaultProfile: state.cfgDefaultProfile, profiles: Object.keys(state.cfgProfiles).length > 0 ? state.cfgProfiles : defaultProfilesForState(state.testPort), - ssrfPolicy: { dangerouslyAllowPrivateNetwork: true }, }, }; }; @@ -513,8 +531,10 @@ export async function resetBrowserControlServerTestContext(): Promise { state.reachable = false; state.cfgAttachOnly = false; state.cfgEvaluateEnabled = true; + state.cfgSsrfPolicy = undefined; state.cfgDefaultProfile = "openclaw"; state.cfgProfiles = defaultProfilesForState(state.testPort); + state.tabUrl = "https://example.com"; mockClearAll(pwMocks); mockClearAll(cdpMocks); @@ -580,7 +600,7 @@ export function installBrowserControlServerHooks() { { id: "abcd1234", title: "Tab", - url: "https://example.com", + url: state.tabUrl, webSocketDebuggerUrl: "ws://127.0.0.1/devtools/page/abcd1234", type: "page", },