mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-27 19:58:50 +00:00
fix(browser): validate current tab before snapshots (#78526)
* fix(browser): validate current tab before snapshots
* fix(browser): reject snapshot selector before SSRF guard
* fix(test): stabilize plugin activation normalization
* fix(ci): fetch opengrep base history
* fix(snapshot): enforce snapshot ssrf policy
* docs(changelog): add unreleased entry for snapshot SSRF fix
* Revert "docs(changelog): add unreleased entry for snapshot SSRF fix"
This reverts commit 4f3031ff65.
* fix(changelog): record snapshot ssrf entry
This commit is contained in:
2
.github/workflows/opengrep-precise.yml
vendored
2
.github/workflows/opengrep-precise.yml
vendored
@@ -44,7 +44,7 @@ jobs:
|
||||
uses: actions/checkout@v6
|
||||
with:
|
||||
ref: ${{ github.sha }}
|
||||
fetch-depth: 1
|
||||
fetch-depth: 0
|
||||
fetch-tags: false
|
||||
persist-credentials: false
|
||||
submodules: false
|
||||
|
||||
@@ -8,6 +8,8 @@ Docs: https://docs.openclaw.ai
|
||||
|
||||
### Fixes
|
||||
|
||||
- Browser/snapshot: validate current tab URLs against the configured SSRF policy before ChromeMCP or direct CDP snapshot reads, closing the local-managed CDP bypass from GHSA-2x93-h3hg-2xfp while preserving existing-session coverage; the PR also rejects existing-session selectors before URL checks, adds focused route coverage, fetches full opengrep CI history, and stabilizes plugin activation normalization tests. Thanks @zsxsoft.
|
||||
|
||||
- Crabbox: bootstrap raw AWS macOS JavaScript commands launched through `/usr/bin/env` so native mac runners without preinstalled Node, Corepack, or pnpm can still run wrapped Node and pnpm proof.
|
||||
- macOS: let app packaging fall back to `corepack pnpm` when a fresh native runner has Node/Corepack but no pnpm shim on `PATH`.
|
||||
- E2E: keep package/onboarding/plugin smoke commands bounded on macOS shells that have Node but no GNU `timeout` or `gtimeout` binary.
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { EXISTING_SESSION_LIMITS } from "./existing-session-limits.js";
|
||||
import {
|
||||
createExistingSessionAgentSharedModule,
|
||||
existingSessionRouteState,
|
||||
@@ -215,13 +216,72 @@ describe("existing-session browser routes", () => {
|
||||
it("checks existing-session snapshot URL when SSRF policy is configured", async () => {
|
||||
const handler = getSnapshotGetHandler({ allowPrivateNetwork: false });
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "ai" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(navigationGuardMocks.assertBrowserNavigationAllowed).not.toHaveBeenCalled();
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).toHaveBeenCalledWith({
|
||||
url: "https://example.com",
|
||||
ssrfPolicy: { allowPrivateNetwork: false },
|
||||
});
|
||||
expect(chromeMcpMocks.takeChromeMcpSnapshot).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows existing-session snapshots under the default SSRF policy object", async () => {
|
||||
const handler = getSnapshotGetHandler({});
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "ai" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(navigationGuardMocks.assertBrowserNavigationAllowed).not.toHaveBeenCalled();
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).toHaveBeenCalledWith({
|
||||
url: "https://example.com",
|
||||
ssrfPolicy: {},
|
||||
});
|
||||
expect(chromeMcpMocks.takeChromeMcpSnapshot).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("blocks existing-session snapshots when the current URL violates browser navigation policy", async () => {
|
||||
routeState.profileCtx.ensureTabAvailable.mockResolvedValueOnce({
|
||||
targetId: "7",
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
});
|
||||
navigationGuardMocks.assertBrowserNavigationResultAllowed.mockRejectedValueOnce(
|
||||
new Error("browser navigation blocked by policy"),
|
||||
);
|
||||
const handler = getSnapshotGetHandler({ allowPrivateNetwork: false });
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "ai" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.body).toEqual({ error: "browser navigation blocked by policy" });
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).toHaveBeenCalledWith({
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
ssrfPolicy: { allowPrivateNetwork: false },
|
||||
});
|
||||
expect(chromeMcpMocks.takeChromeMcpSnapshot).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects existing-session snapshot selectors before checking the current URL", async () => {
|
||||
routeState.profileCtx.ensureTabAvailable.mockResolvedValueOnce({
|
||||
targetId: "7",
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
});
|
||||
const handler = getSnapshotGetHandler({ allowPrivateNetwork: false });
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "ai", selector: "#admin" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.body).toEqual({
|
||||
error: EXISTING_SESSION_LIMITS.snapshot.snapshotSelector,
|
||||
});
|
||||
expect(navigationGuardMocks.assertBrowserNavigationAllowed).not.toHaveBeenCalled();
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).not.toHaveBeenCalled();
|
||||
expect(chromeMcpMocks.takeChromeMcpSnapshot).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("checks existing-session screenshot URL when SSRF policy is configured", async () => {
|
||||
|
||||
@@ -0,0 +1,148 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { createBrowserRouteApp, createBrowserRouteResponse } from "./test-helpers.js";
|
||||
import type { BrowserRequest } from "./types.js";
|
||||
|
||||
const routeState = vi.hoisted(() => ({
|
||||
profileCtx: {
|
||||
profile: {
|
||||
driver: "openclaw" as const,
|
||||
name: "openclaw",
|
||||
cdpUrl: "http://127.0.0.1:18800",
|
||||
cdpIsLoopback: true,
|
||||
},
|
||||
ensureTabAvailable: vi.fn(async () => ({
|
||||
targetId: "7",
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
wsUrl: "ws://127.0.0.1/devtools/page/7",
|
||||
})),
|
||||
},
|
||||
}));
|
||||
|
||||
const cdpMocks = vi.hoisted(() => ({
|
||||
snapshotAria: vi.fn(async () => ({
|
||||
nodes: [{ ref: "1", role: "link", name: "private", depth: 0 }],
|
||||
})),
|
||||
snapshotRoleViaCdp: vi.fn(async () => ({
|
||||
snapshot: '- link "private" [ref=e1]',
|
||||
refs: { e1: { role: "link", name: "private" } },
|
||||
stats: { lines: 1, chars: 25, refs: 1, interactive: 1 },
|
||||
})),
|
||||
}));
|
||||
|
||||
const navigationGuardMocks = vi.hoisted(() => ({
|
||||
assertBrowserNavigationAllowed: vi.fn(async () => {}),
|
||||
assertBrowserNavigationResultAllowed: vi.fn(async () => {
|
||||
throw new Error("browser navigation blocked by policy");
|
||||
}),
|
||||
withBrowserNavigationPolicy: vi.fn((ssrfPolicy?: unknown) => (ssrfPolicy ? { ssrfPolicy } : {})),
|
||||
}));
|
||||
|
||||
vi.mock("../cdp.js", () => ({
|
||||
captureScreenshot: vi.fn(),
|
||||
snapshotAria: cdpMocks.snapshotAria,
|
||||
snapshotRoleViaCdp: cdpMocks.snapshotRoleViaCdp,
|
||||
}));
|
||||
|
||||
vi.mock("../chrome-mcp.js", () => ({
|
||||
evaluateChromeMcpScript: vi.fn(),
|
||||
navigateChromeMcpPage: vi.fn(),
|
||||
takeChromeMcpScreenshot: vi.fn(),
|
||||
takeChromeMcpSnapshot: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("../navigation-guard.js", () => ({
|
||||
assertBrowserNavigationAllowed: navigationGuardMocks.assertBrowserNavigationAllowed,
|
||||
assertBrowserNavigationResultAllowed: navigationGuardMocks.assertBrowserNavigationResultAllowed,
|
||||
withBrowserNavigationPolicy: navigationGuardMocks.withBrowserNavigationPolicy,
|
||||
}));
|
||||
|
||||
vi.mock("../screenshot.js", () => ({
|
||||
DEFAULT_BROWSER_SCREENSHOT_MAX_BYTES: 128,
|
||||
DEFAULT_BROWSER_SCREENSHOT_MAX_SIDE: 64,
|
||||
normalizeBrowserScreenshot: vi.fn(async (buffer: Buffer) => ({
|
||||
buffer,
|
||||
contentType: "image/png",
|
||||
})),
|
||||
}));
|
||||
|
||||
vi.mock("../../media/store.js", () => ({
|
||||
ensureMediaDir: vi.fn(async () => {}),
|
||||
saveMediaBuffer: vi.fn(async () => ({ path: "/tmp/fake.png" })),
|
||||
}));
|
||||
|
||||
vi.mock("./agent.shared.js", () => ({
|
||||
getPwAiModule: vi.fn(async () => null),
|
||||
handleRouteError: vi.fn(
|
||||
(
|
||||
_ctx: unknown,
|
||||
res: { status: (code: number) => unknown; json: (body: unknown) => void },
|
||||
err: unknown,
|
||||
) => {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
res.status(400);
|
||||
res.json({ error: message });
|
||||
},
|
||||
),
|
||||
readBody: vi.fn((req: BrowserRequest) => req.body ?? {}),
|
||||
requirePwAi: vi.fn(async () => null),
|
||||
resolveProfileContext: vi.fn(() => routeState.profileCtx),
|
||||
withPlaywrightRouteContext: vi.fn(),
|
||||
withRouteTabContext: vi.fn(),
|
||||
}));
|
||||
|
||||
const { registerBrowserAgentSnapshotRoutes } = await import("./agent.snapshot.js");
|
||||
|
||||
function getSnapshotGetHandler() {
|
||||
const { app, getHandlers } = createBrowserRouteApp();
|
||||
registerBrowserAgentSnapshotRoutes(app, {
|
||||
state: () => ({
|
||||
resolved: {
|
||||
extraArgs: [],
|
||||
ssrfPolicy: { dangerouslyAllowPrivateNetwork: false },
|
||||
},
|
||||
}),
|
||||
} as never);
|
||||
const handler = getHandlers.get("/snapshot");
|
||||
expect(handler).toBeTypeOf("function");
|
||||
return handler;
|
||||
}
|
||||
|
||||
describe("local-managed browser snapshot routes", () => {
|
||||
beforeEach(() => {
|
||||
routeState.profileCtx.ensureTabAvailable.mockClear();
|
||||
cdpMocks.snapshotAria.mockClear();
|
||||
cdpMocks.snapshotRoleViaCdp.mockClear();
|
||||
navigationGuardMocks.assertBrowserNavigationResultAllowed.mockClear();
|
||||
navigationGuardMocks.withBrowserNavigationPolicy.mockClear();
|
||||
});
|
||||
|
||||
it("blocks ARIA CDP snapshots when the current tab violates browser navigation policy", async () => {
|
||||
const handler = getSnapshotGetHandler();
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "aria" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.body).toEqual({ error: "browser navigation blocked by policy" });
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).toHaveBeenCalledWith({
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
ssrfPolicy: { dangerouslyAllowPrivateNetwork: false },
|
||||
});
|
||||
expect(cdpMocks.snapshotAria).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("blocks AI CDP role snapshots when the current tab violates browser navigation policy", async () => {
|
||||
const handler = getSnapshotGetHandler();
|
||||
const response = createBrowserRouteResponse();
|
||||
|
||||
await handler?.({ params: {}, query: { format: "ai", interactive: "true" } }, response.res);
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.body).toEqual({ error: "browser navigation blocked by policy" });
|
||||
expect(navigationGuardMocks.assertBrowserNavigationResultAllowed).toHaveBeenCalledWith({
|
||||
url: "http://127.0.0.1:8080/admin",
|
||||
ssrfPolicy: { dangerouslyAllowPrivateNetwork: false },
|
||||
});
|
||||
expect(cdpMocks.snapshotRoleViaCdp).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -546,12 +546,20 @@ export function registerBrowserAgentSnapshotRoutes(
|
||||
const tab = await profileCtx.ensureTabAvailable(targetId || undefined);
|
||||
const usesChromeMcp = getBrowserProfileCapabilities(profileCtx.profile).usesChromeMcp;
|
||||
const ssrfPolicyOpts = browserNavigationPolicyForProfile(ctx, profileCtx);
|
||||
let observedBrowserState: unknown;
|
||||
if (!usesChromeMcp && pwModule) {
|
||||
if ((plan.labels || plan.mode === "efficient") && plan.format === "aria") {
|
||||
return jsonError(res, 400, "labels/mode=efficient require format=ai");
|
||||
}
|
||||
if (usesChromeMcp && (plan.selectorValue || plan.frameSelectorValue)) {
|
||||
return jsonError(res, 400, EXISTING_SESSION_LIMITS.snapshot.snapshotSelector);
|
||||
}
|
||||
if (ssrfPolicyOpts.ssrfPolicy) {
|
||||
await assertBrowserNavigationResultAllowed({
|
||||
url: tab.url,
|
||||
...ssrfPolicyOpts,
|
||||
});
|
||||
}
|
||||
let observedBrowserState: unknown;
|
||||
if (!usesChromeMcp && pwModule) {
|
||||
observedBrowserState = await pwModule
|
||||
.getObservedBrowserStateViaPlaywright({
|
||||
cdpUrl: profileCtx.profile.cdpUrl,
|
||||
@@ -560,19 +568,7 @@ export function registerBrowserAgentSnapshotRoutes(
|
||||
})
|
||||
.catch(() => undefined);
|
||||
}
|
||||
if ((plan.labels || plan.mode === "efficient") && plan.format === "aria") {
|
||||
return jsonError(res, 400, "labels/mode=efficient require format=ai");
|
||||
}
|
||||
if (usesChromeMcp) {
|
||||
if (plan.selectorValue || plan.frameSelectorValue) {
|
||||
return jsonError(res, 400, EXISTING_SESSION_LIMITS.snapshot.snapshotSelector);
|
||||
}
|
||||
if (ssrfPolicyOpts.ssrfPolicy) {
|
||||
await assertBrowserNavigationResultAllowed({
|
||||
url: tab.url,
|
||||
...ssrfPolicyOpts,
|
||||
});
|
||||
}
|
||||
const snapshot = await takeChromeMcpSnapshot({
|
||||
profileName: profileCtx.profile.name,
|
||||
profile: profileCtx.profile,
|
||||
|
||||
@@ -4,7 +4,7 @@ import {
|
||||
withBrowserNavigationPolicy,
|
||||
} from "../navigation-guard.js";
|
||||
import type { BrowserRouteContext } from "../server-context.js";
|
||||
import type { BrowserRequest } from "./types.js";
|
||||
import type { BrowserRequest, BrowserResponse } from "./types.js";
|
||||
|
||||
export const existingSessionRouteState = {
|
||||
profileCtx: {
|
||||
@@ -32,7 +32,11 @@ export const existingSessionRouteState = {
|
||||
export function createExistingSessionAgentSharedModule() {
|
||||
return {
|
||||
getPwAiModule: vi.fn(async () => null),
|
||||
handleRouteError: vi.fn(),
|
||||
handleRouteError: vi.fn((_ctx: BrowserRouteContext, res: BrowserResponse, err: unknown) => {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
res.status(400);
|
||||
res.json({ error: message });
|
||||
}),
|
||||
readBody: vi.fn((req: BrowserRequest) => req.body ?? {}),
|
||||
requirePwAi: vi.fn(async () => {
|
||||
throw new Error("Playwright should not be used for existing-session tests");
|
||||
|
||||
@@ -3,6 +3,22 @@ import { normalizeModelRef } from "./agents/model-selection-normalize.js";
|
||||
import { isStaticallyChannelConfigured } from "./config/channel-configured-shared.js";
|
||||
import { parseBrowserMajorVersion } from "./plugin-sdk/browser-host-inspection.js";
|
||||
|
||||
const testModelIdNormalization = {
|
||||
providers: {
|
||||
google: {
|
||||
aliases: {
|
||||
"gemini-3.1-pro": "gemini-3.1-pro-preview",
|
||||
"gemini-3-pro-preview": "gemini-3.1-pro-preview",
|
||||
},
|
||||
},
|
||||
xai: {
|
||||
aliases: {
|
||||
"grok-4-fast-reasoning": "grok-4-fast",
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const loadBundledPluginPublicSurfaceModuleSync = vi.hoisted(() =>
|
||||
vi.fn((params: { artifactBasename: string }) => {
|
||||
if (params.artifactBasename === "browser-host-inspection.js") {
|
||||
@@ -34,21 +50,7 @@ const loadPluginManifestRegistryForPluginRegistry = vi.hoisted(() =>
|
||||
slack: ["SLACK_BOT_TOKEN"],
|
||||
telegram: ["TELEGRAM_BOT_TOKEN"],
|
||||
},
|
||||
modelIdNormalization: {
|
||||
providers: {
|
||||
google: {
|
||||
aliases: {
|
||||
"gemini-3.1-pro": "gemini-3.1-pro-preview",
|
||||
"gemini-3-pro-preview": "gemini-3.1-pro-preview",
|
||||
},
|
||||
},
|
||||
xai: {
|
||||
aliases: {
|
||||
"grok-4-fast-reasoning": "grok-4-fast",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
modelIdNormalization: testModelIdNormalization,
|
||||
skills: [],
|
||||
hooks: [],
|
||||
origin: "bundled",
|
||||
@@ -137,25 +139,7 @@ describe("plugin activation boundary", () => {
|
||||
expect(isStaticallyChannelConfigured({}, "whatsapp", {})).toBe(false);
|
||||
const staticNormalize = {
|
||||
allowPluginNormalization: false,
|
||||
manifestPlugins: [
|
||||
{
|
||||
modelIdNormalization: {
|
||||
providers: {
|
||||
google: {
|
||||
aliases: {
|
||||
"gemini-3.1-pro": "gemini-3.1-pro-preview",
|
||||
"gemini-3-pro-preview": "gemini-3.1-pro-preview",
|
||||
},
|
||||
},
|
||||
xai: {
|
||||
aliases: {
|
||||
"grok-4-fast-reasoning": "grok-4-fast",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
manifestPlugins: [{ modelIdNormalization: testModelIdNormalization }],
|
||||
};
|
||||
expect(normalizeModelRef("google", "gemini-3.1-pro", staticNormalize)).toEqual({
|
||||
provider: "google",
|
||||
|
||||
Reference in New Issue
Block a user