fix: complete browser cleanup security boundaries

Author: chubes4

packages/runtime-playground/src/browser-preview-routing.ts

@@ -350,13 +350,25 @@ export async function drainBrowserPreviewRouteTracker(tracker: BrowserPreviewRou
   }
 }
 
-export async function closeBrowserAndDrainPreviewRoutes(browser: Pick<import("playwright").Browser, "close">, tracker: BrowserPreviewRouteTracker): Promise<Error[]> {
+export async function closeBrowserAndDrainPreviewRoutes(browser: Pick<import("playwright").Browser, "close">, tracker: BrowserPreviewRouteTracker, closeTimeoutMs = 1_000): Promise<Error[]> {
   const errors: Error[] = []
+  let closeTimer: ReturnType<typeof setTimeout> | undefined
   try {
-    await browser.close()
-  } catch (error) {
-    errors.push(browserPreviewLifecycleError("browser-close", error))
+    const closeResult = await Promise.race([
+      browser.close().then(() => ({ status: "closed" as const }), (error: unknown) => ({ status: "failed" as const, error })),
+      new Promise<{ status: "timeout" }>((resolve) => {
+        closeTimer = setTimeout(() => resolve({ status: "timeout" }), closeTimeoutMs)
+      }),
+    ])
+    if (closeResult.status === "failed") {
+      errors.push(browserPreviewLifecycleError("browser-close", closeResult.error))
+    } else if (closeResult.status === "timeout") {
+      errors.push(browserPreviewLifecycleError("browser-close-timeout", new Error(`Browser close exceeded ${closeTimeoutMs}ms`)))
+    }
   } finally {
+    if (closeTimer) {
+      clearTimeout(closeTimer)
+    }
     try {
       await drainBrowserPreviewRouteTracker(tracker)
     } catch (error) {
@@ -568,7 +580,7 @@ async function fetchBrowserPreviewRoutedHost(route: Route, requestUrl: URL, poli
         }
 
         if (resourceType !== "document" || !retryable) {
-          await route.abort("failed").catch(() => undefined)
+          await abortBrowserPreviewRoute(route, "abort-recoverable-fetch")
           return undefined
         }
         throw browserPreviewRouteFetchExhaustedError(route, currentUrl, attempt, error)
@@ -597,15 +609,15 @@ async function fetchBrowserPreviewRoutedHost(route: Route, requestUrl: URL, poli
       stat.external = true
       stat.blocked += 1
       policy.routedRedirectEscapes.push({ rawOrigin: redirectedUrl.origin, effectiveOrigin: origin.origin, reason: "redirect-host-not-routed-to-preview" })
-      await route.abort("blockedbyclient")
+      await abortBrowserPreviewRoute(route, "abort-redirect-escape", "blockedbyclient")
       return undefined
     }
 
     currentUrl = redirectedUrl
   }
 
   if (route.request().resourceType() !== "document") {
-    await route.abort("failed").catch(() => undefined)
+    await abortBrowserPreviewRoute(route, "abort-redirect-limit")
     return undefined
   }
 
@@ -664,6 +676,20 @@ function browserPreviewLifecycleError(operation: string, error: unknown): Error
   return diagnostic
 }
 
+async function abortBrowserPreviewRoute(route: Route, operation: string, errorCode: Parameters<Route["abort"]>[0] = "failed"): Promise<void> {
+  try {
+    await route.abort(errorCode)
+  } catch (error) {
+    if (isBrowserPreviewRouteClosedError(error)) {
+      return
+    }
+    const cause = sanitizeBrowserPreviewRouteError(error).message.replace(/[\r\n]+/g, " ")
+    const diagnostic = new Error(`wordpress.browser-probe route operation failed: operation=${operation} cause=${cause}`)
+    diagnostic.name = "BrowserPreviewRouteOperationError"
+    throw diagnostic
+  }
+}
+
 function browserPreviewRouteFetchExhaustedError(route: Route, requestUrl: URL, attempts: number, error: unknown): Error {
   const method = route.request().method()
   const resourceType = route.request().resourceType()

packages/runtime-playground/src/browser-probe-runner.ts

@@ -292,7 +292,6 @@ export async function runSingleBrowserProbeCommand({
     pendingError = pendingError ?? new Error("Browser command aborted during runtime cleanup")
     void page?.close().catch(() => undefined)
     void context?.close().catch(() => undefined)
-    void browser.close().catch(() => undefined)
   }
   abortSignal?.addEventListener("abort", abortHandler, { once: true })
 
@@ -451,26 +450,38 @@ export async function runSingleBrowserProbeCommand({
     errors.push(serializeBrowserError("probe-error", error))
   } finally {
     if (abortSignal?.aborted) {
-      await closeBrowserBestEffort(browser)
-      await drainBrowserPreviewRouteTracker(routeTracker).catch(() => undefined)
-      abortSignal.removeEventListener("abort", abortHandler)
-      throw pendingError ?? new Error("Browser command aborted during runtime cleanup")
+      pendingError ??= new Error("Browser command aborted during runtime cleanup")
+      progress.fail("probe-error", pendingError)
     }
     if (page) {
-      finalUrl = page.url()
+      try {
+        finalUrl = page.url()
+      } catch (error) {
+        errors.push(serializeBrowserError("probe-error", error))
+      }
       windowLocationOrigin = windowLocationOrigin ?? await page.evaluate(() => window.location.origin).catch(() => undefined)
       if (captureSelection.metrics) {
-        checkpoints.push(await browserProbeCheckpoint(page, "final"))
-        if (capture.has("memory")) {
-          memoryArtifact = browserProbeMemoryArtifact(checkpoints)
-        }
-        if (capture.has("performance")) {
-          performanceArtifact = browserProbePerformanceArtifact(checkpoints, { consoleMessages, errors, network, startedAt })
+        try {
+          checkpoints.push(await browserProbeCheckpoint(page, "final"))
+          if (capture.has("memory")) {
+            memoryArtifact = browserProbeMemoryArtifact(checkpoints)
+          }
+          if (capture.has("performance")) {
+            performanceArtifact = browserProbePerformanceArtifact(checkpoints, { consoleMessages, errors, network, startedAt })
+          }
+        } catch (error) {
+          pendingError ??= redactError(error, { redactAllUrlQueryValues: true, redactUrlHash: true, redactQueryAssignments: true })
+          errors.push(serializeBrowserError("probe-error", error))
         }
       }
-      const lifecycle = lifecycleSelectors.length > 0 ? await collectBrowserProbeLifecycle(page) : undefined
-      if (lifecycle) {
-        lifecycleArtifact = browserProbeLifecycleArtifact(lifecycle)
+      try {
+        const lifecycle = lifecycleSelectors.length > 0 ? await collectBrowserProbeLifecycle(page) : undefined
+        if (lifecycle) {
+          lifecycleArtifact = browserProbeLifecycleArtifact(lifecycle)
+        }
+      } catch (error) {
+        pendingError ??= redactError(error, { redactAllUrlQueryValues: true, redactUrlHash: true, redactQueryAssignments: true })
+        errors.push(serializeBrowserError("probe-error", error))
       }
 
       if (capture.has("html")) {
@@ -642,16 +653,6 @@ export async function runSingleBrowserProbeCommand({
   }
 }
 
-async function closeBrowserBestEffort(browser: import("playwright").Browser): Promise<void> {
-  await Promise.race([
-    browser.close().catch(() => undefined),
-    new Promise<void>((resolve) => {
-      const timeout = setTimeout(resolve, 1_000)
-      timeout.unref()
-    }),
-  ])
-}
-
 export type BoundedBrowserDiagnosticResult<T> = { ok: true; value: T } | { ok: false; error: Error }
 
 /**

packages/runtime-playground/src/browser-result-sanitization.ts

@@ -25,7 +25,13 @@ export function sanitizeBrowserResultValue<T>(value: T, key = ""): T {
   if (DIAGNOSTIC_KEY_PATTERN.test(key)) {
     return redactString(value, { redactAllUrlQueryValues: true, redactUrlHash: true, redactQueryAssignments: true }) as T
   }
+  return sanitizePersistedBrowserText(value) as T
+}
+
+function sanitizePersistedBrowserText(value: string): string {
   return value
+    .replace(/(?:https?|wss?):\/\/[^\s"'<>]+|\/[A-Za-z0-9._~!$&'()*+,;=:@%/-]*\?[^\s"'<>]+/gi, (url) => sanitizeBrowserResultUrl(url))
+    .replace(/\b((?:access[_-]?token|api[_-]?key|authorization|cookie|nonce|password|secret|session[_-]?token)\s*[=:]\s*)[^\s,;]+/gi, "$1[redacted]")
 }
 
 export function sanitizeBrowserArtifact(artifact: BrowserArtifact): BrowserArtifact {

packages/runtime-playground/src/editor-command-runners.ts

@@ -67,6 +67,7 @@ export async function runEditorCanvasProbeCommand({
   const startedAt = now()
   const startedAtMs = Date.now()
   const browser = await launchChromiumBrowser()
+  const routeTracker = createBrowserPreviewRouteTracker()
   const errors: BrowserProbeErrorRecord[] = []
   let artifact: BrowserArtifact | undefined
   let finalUrl = targetUrl
@@ -83,7 +84,7 @@ export async function runEditorCanvasProbeCommand({
 
     const context = browserPreviewNeedsContextRouting(networkPolicy) ? await browser.newContext(topology.contextOptions()) : null
     if (context) {
-      await routeBrowserPreviewContextNetwork(context, networkPolicy, topology.origins.localProxyOrigin)
+      await routeBrowserPreviewContextNetwork(context, networkPolicy, topology.origins.localProxyOrigin, routeTracker)
     }
     const page = context ? await context.newPage() : await browser.newPage()
     viewport = await browserProbeViewport(page)
@@ -232,9 +233,36 @@ export async function runEditorCanvasProbeCommand({
       })
     }
   } finally {
-    await browser.close()
+    for (const routeError of await closeBrowserAndDrainPreviewRoutes(browser, routeTracker)) {
+      errors.push(serializeBrowserError("probe-error", routeError))
+      pendingError ??= routeError
+    }
+    if (artifact) {
+      artifact.summary.errors = errors.length
+      await artifactSession.writeJson("summary", "editor-canvas-summary.json", {
+        schema: "wp-codebox/editor-canvas-probe/v1",
+        requestedUrl: targetUrl,
+        preview,
+        ...previewOrigins,
+        finalUrl,
+        ...(windowLocationOrigin ? { windowLocationOrigin } : {}),
+        startedAt,
+        finishedAt: now(),
+        timeoutMs,
+        files: artifact.files,
+        hashes: {
+          ...(screenshotSha256 ? { screenshot: { algorithm: "sha256", value: screenshotSha256 } } : {}),
+        },
+        viewport,
+        errors,
+        summary: artifact.summary.editorCanvas,
+      })
+    }
   }
 
+  if (!artifact) {
+    throw pendingError ?? new Error("wordpress.editor-canvas-probe did not produce an artifact")
+  }
   if (pendingError) {
     throw new BrowserCommandArtifactError(pendingError.message, artifact)
   }

tests/browser-preview-routing.test.ts

@@ -131,6 +131,15 @@ test("shared browser cleanup drains late registrations even when browser close f
   assertNoSentinels(lifecycleErrors[0]!.message, "cleanup diagnostic")
 })
 
+test("shared browser cleanup bounds a close operation that never settles", async () => {
+  const tracker = createBrowserPreviewRouteTracker()
+  const startedAt = Date.now()
+  const lifecycleErrors = await closeBrowserAndDrainPreviewRoutes({ close: () => new Promise<void>(() => {}) }, tracker, 10)
+  assert(Date.now() - startedAt < 250)
+  assert.equal(lifecycleErrors.length, 1)
+  assert.match(lifecycleErrors[0]!.message, /operation=browser-close-timeout/)
+})
+
 test("the complete route callback contains continue and policy abort failures", async () => {
   const cases = [
     { name: "invalid URL continue", options: { url: "not-a-url", continueErrors: [routeFetchError("continue failed")] }, operation: "continue-invalid-url" },
@@ -153,6 +162,26 @@ test("closed-context failures across the route callback are swallowed during cle
   assert.equal(fixture.tracker.errors.length, 0)
 })
 
+test("unexpected recoverable and redirect abort failures are operation-classified", async () => {
+  const cases = [
+    { outcomes: [routeFetchError("failed to decompress 'gzip' encoding")], operation: "abort-recoverable-fetch" },
+    { outcomes: [routedResponse(302, { location: "https://outside.test/escape" })], operation: "abort-redirect-escape" },
+    { outcomes: Array.from({ length: 10 }, () => routedResponse(302, { location: "http://routed.test/loop" })), operation: "abort-redirect-limit" },
+  ]
+  for (const item of cases) {
+    const fixture = await routedFixture("script", item.outcomes, undefined, { abortErrors: [routeFetchError("abort transport failed")] })
+    await assert.doesNotReject(fixture.run())
+    await assert.rejects(drainBrowserPreviewRouteTracker(fixture.tracker), new RegExp(`operation=${item.operation}`))
+    assertNoSentinels(JSON.stringify(fixture.tracker.errors), item.operation)
+  }
+
+  const closedAbort = await routedFixture("script", [routeFetchError("failed to decompress 'gzip' encoding")], undefined, {
+    abortErrors: [new Error("route.abort: Target page, context or browser has been closed")],
+  })
+  await assert.doesNotReject(closedAbort.run())
+  await assert.doesNotReject(drainBrowserPreviewRouteTracker(closedAbort.tracker))
+})
+
 test("disposed contexts and decompression failures abort without retrying or tracking errors", async () => {
   for (const message of ["Request context disposed.", "failed to decompress 'gzip' encoding"]) {
     const fixture = await routedFixture("document", [routeFetchError(message)])