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 <drobison@nvidia.com>
This commit is contained in:
Agustin Rivera
2026-05-04 13:07:17 -07:00
committed by GitHub
parent f2efe33afc
commit ef0dbcf49d
9 changed files with 230 additions and 10 deletions

View File

@@ -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

View File

@@ -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);

View File

@@ -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({

View File

@@ -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("");
});
});
});

View File

@@ -107,6 +107,11 @@ type RouteWithTabParams<T> = {
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<T>;
};
@@ -119,6 +124,17 @@ export async function withRouteTabContext<T>(
}
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<T>(
}
}
/**
* 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<T> = {
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<T>;
};
@@ -182,6 +207,7 @@ export async function withPlaywrightRouteContext<T>(
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) {

View File

@@ -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,

View File

@@ -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,

View File

@@ -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<string, unknown>;
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<T>(params: {
rootDir: string;
run: (relativePath: string) => Promise<T>;
@@ -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`, {

View File

@@ -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<void> {
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",
},