diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a7453d389e..118789581b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -268,6 +268,7 @@ Docs: https://docs.openclaw.ai - Discord/groups: instruct group-chat agents to stay silent when a message is addressed to someone else, replying only when invited or correcting key facts. (#78615) - Discord/groups: tell Discord-channel agents to wrap bare URLs as `` so link previews do not expand into uninvited embeds. (#78614) - Agents/fallback: fail fast on session write-lock timeouts instead of trying fallback models for local file contention. Fixes #66646. Thanks @sallyom. +- Browser/SSRF: stop closing user-owned Chrome tabs when a read-only operation (snapshot/screenshot/interactions) is rejected by the SSRF guard — only OpenClaw-initiated navigations now close on policy denial. Thanks @scotthuang. - Telegram/Codex: generate DM topic labels with Codex-compatible simple-completion requests so auto-created private topics can be renamed instead of staying `New Chat`. - Plugins/runtime fetch: drop third-party symbol metadata from plain request header dictionaries before passing them into native `fetch` or `Headers`, so SDK and guarded/proxy fetch paths do not reject otherwise valid plugin requests. Fixes #77846. Thanks @shakkernerd. - Web fetch: bound guarded dispatcher cleanup after request timeouts so timed-out fetches return tool errors instead of leaving Gateway tool lanes active. (#78439) Thanks @obviyus. diff --git a/extensions/browser/src/browser/pw-session.assert-navigation-safety.test.ts b/extensions/browser/src/browser/pw-session.assert-navigation-safety.test.ts new file mode 100644 index 00000000000..c132ddf1ac7 --- /dev/null +++ b/extensions/browser/src/browser/pw-session.assert-navigation-safety.test.ts @@ -0,0 +1,124 @@ +import type { Page } from "playwright-core"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { SsrFBlockedError } from "../infra/net/ssrf.js"; +import { + assertBrowserNavigationRedirectChainAllowed, + assertBrowserNavigationResultAllowed, +} from "./navigation-guard.js"; +import { assertPageNavigationCompletedSafely } from "./pw-session.js"; + +vi.mock("./navigation-guard.js", async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + assertBrowserNavigationRedirectChainAllowed: vi.fn(async () => {}), + assertBrowserNavigationResultAllowed: vi.fn(async () => {}), + }; +}); + +const mockedRedirectChain = vi.mocked(assertBrowserNavigationRedirectChainAllowed); +const mockedResultAllowed = vi.mocked(assertBrowserNavigationResultAllowed); + +afterEach(() => { + mockedRedirectChain.mockReset(); + mockedRedirectChain.mockImplementation(async () => {}); + mockedResultAllowed.mockReset(); + mockedResultAllowed.mockImplementation(async () => {}); +}); + +function fakePage(url = "https://blocked.example/admin"): { + page: Page; + close: ReturnType; +} { + const close = vi.fn(async () => {}); + const page = { + url: vi.fn(() => url), + close, + } as unknown as Page; + return { page, close }; +} + +describe("assertPageNavigationCompletedSafely", () => { + it("does not close the tab when a read-only caller hits an SSRF-blocked URL (response: null)", async () => { + // A read-only caller (snapshot/screenshot/interactions) passes response: null + // and must never lose the user's tab when the policy guard rejects. + mockedResultAllowed.mockRejectedValueOnce(new SsrFBlockedError("blocked by policy")); + + const { page, close } = fakePage(); + + await expect( + assertPageNavigationCompletedSafely({ + cdpUrl: "http://127.0.0.1:18792", + page, + response: null, + ssrfPolicy: { allowPrivateNetwork: false }, + targetId: "tab-1", + }), + ).rejects.toBeInstanceOf(SsrFBlockedError); + + expect(close).not.toHaveBeenCalled(); + }); + + it("does not close the tab when a navigate caller hits an SSRF-blocked URL (response: non-null)", async () => { + // Even when the helper is invoked with a real Response (i.e. on the + // navigate path), the close decision now belongs to the caller. The + // helper must only quarantine + rethrow; the caller's try/catch is + // responsible for closing if it owns the navigation lifecycle. + mockedResultAllowed.mockRejectedValueOnce(new SsrFBlockedError("blocked by policy")); + + const { page, close } = fakePage(); + const response = { request: () => undefined } as unknown as Parameters< + typeof assertPageNavigationCompletedSafely + >[0]["response"]; + + await expect( + assertPageNavigationCompletedSafely({ + cdpUrl: "http://127.0.0.1:18792", + page, + response, + ssrfPolicy: { allowPrivateNetwork: false }, + targetId: "tab-1", + }), + ).rejects.toBeInstanceOf(SsrFBlockedError); + + expect(close).not.toHaveBeenCalled(); + }); + + it("rethrows non-policy errors without touching the tab", async () => { + const boom = new Error("transient playwright error"); + mockedResultAllowed.mockRejectedValueOnce(boom); + + const { page, close } = fakePage(); + + await expect( + assertPageNavigationCompletedSafely({ + cdpUrl: "http://127.0.0.1:18792", + page, + response: null, + ssrfPolicy: { allowPrivateNetwork: false }, + targetId: "tab-1", + }), + ).rejects.toBe(boom); + + expect(close).not.toHaveBeenCalled(); + }); + + it("returns silently when both guards pass", async () => { + const { page, close } = fakePage("https://allowed.example/"); + + await expect( + assertPageNavigationCompletedSafely({ + cdpUrl: "http://127.0.0.1:18792", + page, + response: null, + ssrfPolicy: { allowPrivateNetwork: false }, + targetId: "tab-1", + }), + ).resolves.toBeUndefined(); + + expect(close).not.toHaveBeenCalled(); + expect(mockedResultAllowed).toHaveBeenCalledWith( + expect.objectContaining({ url: "https://allowed.example/" }), + ); + }); +}); diff --git a/extensions/browser/src/browser/pw-session.ts b/extensions/browser/src/browser/pw-session.ts index 31241ed2d7d..ea18eb71b18 100644 --- a/extensions/browser/src/browser/pw-session.ts +++ b/extensions/browser/src/browser/pw-session.ts @@ -854,16 +854,20 @@ function isSubframeDocumentNavigationRequest(page: Page, request: Request): bool } } -function isPolicyDenyNavigationError(err: unknown): boolean { +export function isPolicyDenyNavigationError(err: unknown): boolean { return err instanceof SsrFBlockedError || err instanceof InvalidBrowserNavigationUrlError; } -async function closeBlockedNavigationTarget(opts: { +// Mark a page (and its CDP target id when resolvable) as blocked so subsequent +// OpenClaw operations short-circuit instead of re-running the SSRF check on a +// page we have already proven is non-compliant. This is a pure bookkeeping +// step; it does NOT close the tab. Read-only paths can call this safely on a +// user-owned tab without losing the user's content. +async function quarantineBlockedTarget(opts: { cdpUrl: string; page: Page; targetId?: string; }): Promise { - // Quarantine the concrete page first; then persist by target id when available. markPageRefBlocked(opts.cdpUrl, opts.page); const resolvedTargetId = await pageTargetId(opts.page).catch(() => null); const fallbackTargetId = normalizeOptionalString(opts.targetId) ?? ""; @@ -871,9 +875,24 @@ async function closeBlockedNavigationTarget(opts: { if (targetIdToBlock) { markTargetBlocked(opts.cdpUrl, targetIdToBlock); } +} + +// Quarantine and close a tab that OpenClaw itself navigated to a blocked URL. +// Only callers that own the navigation lifecycle (gotoPageWithNavigationGuard +// and the navigate-style entry points that wrap it) may invoke this — closing +// a tab is a destructive action that must not happen on user-owned tabs from +// read-only operations like snapshot/screenshot/interactions. +export async function closeBlockedNavigationTarget(opts: { + cdpUrl: string; + page: Page; + targetId?: string; +}): Promise { + await quarantineBlockedTarget(opts); await opts.page.close().catch(() => {}); } +// On policy denial: quarantines and rethrows (never closes). +// Navigate-style callers catch the rethrow and close via closeBlockedNavigationTarget. export async function assertPageNavigationCompletedSafely( opts: { cdpUrl: string; @@ -896,7 +915,7 @@ export async function assertPageNavigationCompletedSafely( }); } catch (err) { if (isPolicyDenyNavigationError(err)) { - await closeBlockedNavigationTarget({ + await quarantineBlockedTarget({ cdpUrl: opts.cdpUrl, page: opts.page, targetId: opts.targetId, @@ -1340,14 +1359,27 @@ export async function createPageViaPlaywright( throw err; } } - await assertPageNavigationCompletedSafely({ - cdpUrl: opts.cdpUrl, - page, - response, - ssrfPolicy: opts.ssrfPolicy, - browserProxyMode: opts.browserProxyMode, - targetId: createdTargetId ?? undefined, - }); + // OpenClaw owns this newly-created tab: if the post-navigation safety + // check trips, close the tab we just spawned. + try { + await assertPageNavigationCompletedSafely({ + cdpUrl: opts.cdpUrl, + page, + response, + ssrfPolicy: opts.ssrfPolicy, + browserProxyMode: opts.browserProxyMode, + targetId: createdTargetId ?? undefined, + }); + } catch (err) { + if (isPolicyDenyNavigationError(err)) { + await closeBlockedNavigationTarget({ + cdpUrl: opts.cdpUrl, + page, + targetId: createdTargetId ?? undefined, + }); + } + throw err; + } } // Get the targetId for this page diff --git a/extensions/browser/src/browser/pw-tools-core.browser-ssrf-guard.test.ts b/extensions/browser/src/browser/pw-tools-core.browser-ssrf-guard.test.ts index c9086f440c7..464643147aa 100644 --- a/extensions/browser/src/browser/pw-tools-core.browser-ssrf-guard.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.browser-ssrf-guard.test.ts @@ -7,6 +7,7 @@ const pageState = vi.hoisted(() => ({ const sessionMocks = vi.hoisted(() => ({ assertPageNavigationCompletedSafely: vi.fn(async () => {}), + closeBlockedNavigationTarget: vi.fn(async () => {}), ensurePageState: vi.fn(() => ({})), forceDisconnectPlaywrightForTarget: vi.fn(async () => {}), getPageForTargetId: vi.fn(async () => { @@ -16,6 +17,7 @@ const sessionMocks = vi.hoisted(() => ({ return pageState.page; }), gotoPageWithNavigationGuard: vi.fn(async () => null), + isPolicyDenyNavigationError: vi.fn(() => false), refLocator: vi.fn(() => { if (!pageState.locator) { throw new Error("missing locator"); diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts index 546e459ad23..0a4ee02f00e 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts @@ -144,5 +144,38 @@ describe("pw-tools-core.snapshot navigate guard", () => { expect(getPwToolsCoreSessionMocks().assertPageNavigationCompletedSafely).toHaveBeenCalledTimes( 1, ); + // Navigate-style entry points OWN the navigation lifecycle, so when the + // post-navigation safety check rejects with an SSRF policy error the + // caller is responsible for closing the tab it just navigated. This is + // the counterpart to the read-only paths (snapshot/screenshot/ + // interactions), which must NOT close the tab on the same error. + expect(getPwToolsCoreSessionMocks().closeBlockedNavigationTarget).toHaveBeenCalledTimes(1); + expect(getPwToolsCoreSessionMocks().closeBlockedNavigationTarget).toHaveBeenCalledWith({ + cdpUrl: "http://127.0.0.1:18792", + page: expect.anything(), + targetId: undefined, + }); + }); + + it("does not close the tab when post-navigation rejection is not a policy deny", async () => { + // Non-policy errors (e.g. transient playwright failures) must not be + // treated as "we navigated to a blocked URL" — the tab stays open. + const goto = vi.fn(async () => ({ request: () => undefined })); + setPwToolsCoreCurrentPage({ + goto, + url: vi.fn(() => "https://example.com/final"), + }); + getPwToolsCoreSessionMocks().assertPageNavigationCompletedSafely.mockRejectedValueOnce( + new Error("transient playwright error"), + ); + + await expect( + mod.navigateViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + url: "https://example.com/final", + }), + ).rejects.toThrow("transient playwright error"); + + expect(getPwToolsCoreSessionMocks().closeBlockedNavigationTarget).not.toHaveBeenCalled(); }); }); diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts index 97aebadf2e7..dab89e8052f 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.test.ts @@ -9,10 +9,12 @@ const formatAriaSnapshot = vi.fn(); vi.mock("./pw-session.js", () => ({ assertPageNavigationCompletedSafely: vi.fn(), + closeBlockedNavigationTarget: vi.fn(), ensurePageState, forceDisconnectPlaywrightForTarget: vi.fn(), getPageForTargetId, gotoPageWithNavigationGuard: vi.fn(), + isPolicyDenyNavigationError: vi.fn(() => false), storeRoleRefsForTarget, })); diff --git a/extensions/browser/src/browser/pw-tools-core.snapshot.ts b/extensions/browser/src/browser/pw-tools-core.snapshot.ts index 084fa9ec059..d57e814f217 100644 --- a/extensions/browser/src/browser/pw-tools-core.snapshot.ts +++ b/extensions/browser/src/browser/pw-tools-core.snapshot.ts @@ -19,10 +19,12 @@ import { } from "./pw-role-snapshot.js"; import { assertPageNavigationCompletedSafely, + closeBlockedNavigationTarget, ensurePageState, forceDisconnectPlaywrightForTarget, getPageForTargetId, gotoPageWithNavigationGuard, + isPolicyDenyNavigationError, storeRoleRefsForTarget, } from "./pw-session.js"; import { markBackendDomRefsOnPage, withPageScopedCdpClient } from "./pw-session.page-cdp.js"; @@ -378,14 +380,25 @@ export async function navigateViaPlaywright(opts: { ensurePageState(page); response = await navigate(); } - await assertPageNavigationCompletedSafely({ - cdpUrl: opts.cdpUrl, - page, - response, - ssrfPolicy: opts.ssrfPolicy, - browserProxyMode: opts.browserProxyMode, - targetId: opts.targetId, - }); + try { + await assertPageNavigationCompletedSafely({ + cdpUrl: opts.cdpUrl, + page, + response, + ssrfPolicy: opts.ssrfPolicy, + browserProxyMode: opts.browserProxyMode, + targetId: opts.targetId, + }); + } catch (err) { + if (isPolicyDenyNavigationError(err)) { + await closeBlockedNavigationTarget({ + cdpUrl: opts.cdpUrl, + page, + targetId: opts.targetId, + }); + } + throw err; + } const finalUrl = page.url(); return { url: finalUrl }; } diff --git a/extensions/browser/src/browser/pw-tools-core.test-harness.ts b/extensions/browser/src/browser/pw-tools-core.test-harness.ts index 421ed5649e6..bcacd7d0f84 100644 --- a/extensions/browser/src/browser/pw-tools-core.test-harness.ts +++ b/extensions/browser/src/browser/pw-tools-core.test-harness.ts @@ -18,6 +18,7 @@ let pageState: { const sessionMocks = vi.hoisted(() => ({ assertPageNavigationCompletedSafely: vi.fn(async () => {}), + closeBlockedNavigationTarget: vi.fn(async () => {}), getPageForTargetId: vi.fn(async () => { if (!currentPage) { throw new Error("missing page"); @@ -33,6 +34,13 @@ const sessionMocks = vi.hoisted(() => ({ page: { goto: (url: string, init: { timeout: number }) => Promise }; }) => (await opts.page.goto(opts.url, { timeout: opts.timeoutMs })) ?? null, ), + // Match by name so mocked errors are recognized without importing real classes. + isPolicyDenyNavigationError: vi.fn((err: unknown) => { + if (!(err instanceof Error)) { + return false; + } + return err.name === "SsrFBlockedError" || err.name === "InvalidBrowserNavigationUrlError"; + }), restoreRoleRefsForTarget: vi.fn(() => {}), storeRoleRefsForTarget: vi.fn(() => {}), refLocator: vi.fn(() => {