fix(browser): avoid unsafe recovery replay

Author: MagMueller

packages/bcode-browser/src/cdp/session.ts

@@ -83,6 +83,11 @@ export class Session implements Transport {
    * and we connect directly to the supplied endpoint.
    */
   async connect(opts: ConnectOptions = {}): Promise<void> {
+    // No-argument connect is an ensure-connected operation. Reopening the
+    // same configured endpoint would discard the active target session and
+    // make the next page command run against the browser-level socket.
+    if (!opts.wsUrl && !opts.profileDir && this.isConnected()) return;
+
     const timeoutMs = opts.timeoutMs ?? 5_000;
     if (opts.wsUrl || opts.profileDir) {
       const wsUrl = await resolveWsUrl(opts, timeoutMs);
@@ -294,16 +299,33 @@ export class Session implements Transport {
   }
 
   private async attachPage(staleSessionId: string, staleTargetId?: string): Promise<void> {
+    if (!staleTargetId) {
+      if (this.activeSessionId === staleSessionId) {
+        this.activeSessionId = undefined;
+        this.activeTargetId = undefined;
+        this.enabledDomains.delete(staleSessionId);
+      }
+      throw new Error(
+        'CDP target session was lost and its target is unknown; command was not retried on another page.',
+      );
+    }
     const domainsToRestore = [...(this.enabledDomains.get(staleSessionId)?.entries() ?? [])];
     const { targetInfos } = await this.domains.Target.getTargets({});
     const pages = targetInfos as PageTarget[];
-    const exactTarget = staleTargetId
-      ? pages.find(target => target.type === 'page' && target.targetId === staleTargetId)
-      : undefined;
-    const targetId = exactTarget?.targetId
-      ?? (!staleTargetId ? pages.find(isUsablePageTarget)?.targetId : undefined)
-      ?? (await this.domains.Target.createTarget({ url: 'about:blank' })).targetId;
-    const sessionId = await this.use(targetId);
+    const exactTarget = pages.find(
+      target => target.type === 'page' && target.targetId === staleTargetId,
+    );
+    if (!exactTarget) {
+      if (this.activeSessionId === staleSessionId) {
+        this.activeSessionId = undefined;
+        this.activeTargetId = undefined;
+        this.enabledDomains.delete(staleSessionId);
+      }
+      throw new Error(
+        `CDP target ${staleTargetId} was closed; command was not retried on another page.`,
+      );
+    }
+    const sessionId = await this.use(exactTarget.targetId);
     await Promise.all(
       domainsToRestore.map(
         ([method, params]) => this.send(method, params, sessionId),
@@ -440,15 +462,6 @@ export async function listPageTargets(session: Session): Promise<PageTarget[]> {
   );
 }
 
-function isUsablePageTarget(target: PageTarget): boolean {
-  return target.type === 'page'
-    && !target.url.startsWith('chrome://')
-    && !target.url.startsWith('chrome-untrusted://')
-    && !target.url.startsWith('devtools://')
-    && !target.url.startsWith('chrome-extension://')
-    && (!target.url.startsWith('about:') || target.url === 'about:blank');
-}
-
 /**
  * Scan OS-specific user-data directories for Chromium-based browsers that
  * currently have remote debugging enabled (a `DevToolsActivePort` file exists

packages/bcode-browser/test/cdp-recovery.test.ts

@@ -235,9 +235,10 @@ test("reattach reuses an existing about:blank target", async () => {
   }
 })
 
-test("reattach creates a blank page instead of switching to another live target", async () => {
+test("a missing original target is reported without replaying on another page", async () => {
   let attachCount = 0
-  let createdTarget: unknown
+  let createCount = 0
+  let commandCount = 0
   const attachedTargets: string[] = []
   const server = Bun.serve({
     port: 0,
@@ -266,15 +267,16 @@ test("reattach creates a blank page instead of switching to another live target"
           return
         }
         if (message.method === "Target.createTarget") {
-          createdTarget = message.params
-          socket.send(JSON.stringify({ id: message.id, result: { targetId: "fresh-page" } }))
+          createCount++
+          socket.send(JSON.stringify({ id: message.id, result: { targetId: "unexpected-page" } }))
           return
         }
         if (["Page.enable", "DOM.enable", "Runtime.enable", "Network.enable"].includes(message.method)) {
           socket.send(JSON.stringify({ id: message.id, result: {} }))
           return
         }
         if (message.method === "Runtime.evaluate") {
+          commandCount++
           if (message.sessionId === "session-1") {
             socket.send(JSON.stringify({
               id: message.id,
@@ -293,12 +295,57 @@ test("reattach creates a blank page instead of switching to another live target"
   try {
     await session.connect({ wsUrl: wsUrl(server) })
     await session.use("old-page")
+    await expect(session.domains.Runtime.evaluate({ expression: "submit()" }))
+      .rejects.toThrow("CDP target old-page was closed")
+
+    expect(commandCount).toBe(1)
+    expect(createCount).toBe(0)
+    expect(attachCount).toBe(1)
+    expect(attachedTargets).toEqual(["old-page"])
+  } finally {
+    session.close()
+    server.stop(true)
+  }
+})
+
+test("no-argument connect preserves a healthy socket and active target", async () => {
+  let connectionCount = 0
+  const commandSessions: string[] = []
+  const server = Bun.serve({
+    port: 0,
+    fetch(req, bunServer) {
+      if (!bunServer.upgrade(req)) return new Response("nope", { status: 400 })
+      connectionCount++
+      return undefined
+    },
+    websocket: {
+      message(socket, raw) {
+        const message = JSON.parse(String(raw))
+        if (message.method === "Target.attachToTarget") {
+          socket.send(JSON.stringify({ id: message.id, result: { sessionId: "session-1" } }))
+          return
+        }
+        if (message.method !== "Runtime.evaluate") return
+        commandSessions.push(message.sessionId)
+        socket.send(JSON.stringify({
+          id: message.id,
+          result: { result: { type: "boolean", value: true } },
+        }))
+      },
+      close() {},
+    },
+  })
+  const session = new Session()
+
+  try {
+    await session.connect({ wsUrl: wsUrl(server) })
+    await session.use("page-1")
+    await session.connect()
     const result = await session.domains.Runtime.evaluate({ expression: "true" })
 
     expect(result.result.value).toBe(true)
-    expect(createdTarget).toEqual({ url: "about:blank" })
-    expect(attachCount).toBe(2)
-    expect(attachedTargets).toEqual(["old-page", "fresh-page"])
+    expect(connectionCount).toBe(1)
+    expect(commandSessions).toEqual(["session-1"])
   } finally {
     session.close()
     server.stop(true)