fix(browser): keep user tabs open on SSRF-denied reads (#78874)

Summary:
- Split browser SSRF quarantine from tab closure so read-only browser operations do not close user-owned tabs on policy denial.
- Keep OpenClaw-initiated navigation/create paths closing blocked tabs, and add regression coverage for both contracts.
- Update changelog with contributor credit.

Verification:
- pnpm test extensions/browser/src/browser/pw-session.assert-navigation-safety.test.ts extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts
- pnpm test extensions/browser/src/browser/pw-tools-core.browser-ssrf-guard.test.ts extensions/browser/src/browser/pw-tools-core.snapshot.test.ts
- Exact-head CI success: 25535578610
- Exact-head Real behavior proof success: 25536652326

Thanks @scotthuang.

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 `<https://example.com>` 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.

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<Record<string, unknown>>();
+  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<typeof vi.fn>;
+} {
+  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/" }),
+    );
+  });
+});

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<void> {
-  // 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<void> {
+  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

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

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

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,
 }));
 

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 };
 }

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<unknown> };
     }) => (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(() => {