fix: wait for a quiet frame tree before keeping a secret-masked screenshot

openadapt_flow/backends/playwright_backend.py

@@ -12,6 +12,7 @@
 import hmac
 import math
 import re
+import time
 import uuid
 from dataclasses import dataclass, field
 from datetime import datetime, timezone
@@ -33,7 +34,11 @@
 )
 
 VIEWPORT: tuple[int, int] = (1280, 800)
-_MASKED_SCREENSHOT_ATTEMPTS = 3
+# Secret-masked screenshots must not keep bytes from a changing frame tree.
+# Login pages often attach and detach iframes for a few seconds after a click.
+# Retry until the tree is quiet. Still refuse if it never settles.
+_MASKED_SCREENSHOT_TIMEOUT_S = 8.0
+_MASKED_SCREENSHOT_RETRY_SLEEP_S = 0.05
 
 _MODIFIER_ALIASES = {
     "meta": "Meta",
@@ -2300,10 +2305,14 @@ def screenshot(self) -> bytes:
         if not self._screenshot_mask_selectors:
             return self.page.screenshot(type="png", full_page=False, **base_options)
 
-        for _attempt in range(_MASKED_SCREENSHOT_ATTEMPTS):
+        deadline = time.monotonic() + _MASKED_SCREENSHOT_TIMEOUT_S
+        while True:
             generation = self._screenshot_frame_generation
             frames = tuple(self.page.frames)
             if generation != self._screenshot_frame_generation:
+                if time.monotonic() >= deadline:
+                    break
+                time.sleep(_MASKED_SCREENSHOT_RETRY_SLEEP_S)
                 continue
             options = dict(base_options)
             options["mask"] = [
@@ -2319,6 +2328,9 @@ def screenshot(self) -> bytes:
                 self.page.evaluate("() => null")
             except Exception:
                 if generation != self._screenshot_frame_generation:
+                    if time.monotonic() >= deadline:
+                        break
+                    time.sleep(_MASKED_SCREENSHOT_RETRY_SLEEP_S)
                     continue
                 raise
             current_frames = tuple(self.page.frames)
@@ -2328,6 +2340,9 @@ def screenshot(self) -> bytes:
                 return png
             # ``png`` is intentionally discarded here. It never reaches the
             # recorder, disk, or a compiled bundle.
+            if time.monotonic() >= deadline:
+                break
+            time.sleep(_MASKED_SCREENSHOT_RETRY_SLEEP_S)
         raise ScreenshotMaskStabilityError(
             "the browser frame tree changed during every secret-masked "
             "screenshot attempt; recording was refused"

tests/test_browser_attach.py

@@ -268,6 +268,122 @@ def screenshot(self, **kwargs):
     assert not any(page.listeners.values())
 
 
+def test_masked_screenshot_survives_three_iframe_storms_then_settles(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Reproduce the ServiceNow Sign-in refuse with a fake page.
+
+    After Sign in, a login SPA attaches and detaches iframes. The old loop
+    tried three captures with no wait and then raised
+    ScreenshotMaskStabilityError. This page storms for those three captures
+    and then goes quiet. The record must keep a masked frame, not stop.
+    """
+
+    class Frame:
+        def __init__(self, name: str) -> None:
+            self.name = name
+
+        def locator(self, selector):
+            return f"locator:{self.name}:{selector}"
+
+    class Page:
+        def __init__(self) -> None:
+            self.frames = [Frame("main")]
+            self.listeners: dict[str, list] = {}
+            self.capture_count = 0
+
+        def on(self, event, listener):
+            self.listeners.setdefault(event, []).append(listener)
+
+        def remove_listener(self, event, listener):
+            self.listeners[event].remove(listener)
+
+        def evaluate(self, _script):
+            return None
+
+        def screenshot(self, **kwargs):
+            self.capture_count += 1
+            if self.capture_count <= 3:
+                frame = Frame(f"storm-{self.capture_count}")
+                self.frames.append(frame)
+                for listener in self.listeners.get("frameattached", []):
+                    listener(frame)
+                self.frames.pop()
+                for listener in self.listeners.get("framedetached", []):
+                    listener(frame)
+            return b"png"
+
+    monkeypatch.setattr(
+        "openadapt_flow.backends.playwright_backend._MASKED_SCREENSHOT_RETRY_SLEEP_S",
+        0.0,
+    )
+    page = Page()
+    backend = PlaywrightBackend(  # type: ignore[arg-type]
+        page,
+        screenshot_mask_selectors=("input[type='password']",),
+    )
+    assert backend.screenshot() == b"png"
+    assert page.capture_count >= 4
+    backend.stop_screenshot_mask_tracking()
+
+
+def test_masked_screenshot_still_refuses_a_frame_tree_that_never_settles(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A tree that never goes quiet must still refuse. Do not keep those bytes."""
+
+    class Frame:
+        def __init__(self, name: str) -> None:
+            self.name = name
+
+        def locator(self, selector):
+            return f"locator:{self.name}:{selector}"
+
+    class Page:
+        def __init__(self) -> None:
+            self.frames = [Frame("main")]
+            self.listeners: dict[str, list] = {}
+            self.capture_count = 0
+
+        def on(self, event, listener):
+            self.listeners.setdefault(event, []).append(listener)
+
+        def remove_listener(self, event, listener):
+            self.listeners[event].remove(listener)
+
+        def evaluate(self, _script):
+            return None
+
+        def screenshot(self, **kwargs):
+            self.capture_count += 1
+            frame = Frame(f"churn-{self.capture_count}")
+            self.frames.append(frame)
+            for listener in self.listeners.get("frameattached", []):
+                listener(frame)
+            self.frames.pop()
+            for listener in self.listeners.get("framedetached", []):
+                listener(frame)
+            return b"png"
+
+    monkeypatch.setattr(
+        "openadapt_flow.backends.playwright_backend._MASKED_SCREENSHOT_TIMEOUT_S",
+        0.0,
+    )
+    monkeypatch.setattr(
+        "openadapt_flow.backends.playwright_backend._MASKED_SCREENSHOT_RETRY_SLEEP_S",
+        0.0,
+    )
+    page = Page()
+    backend = PlaywrightBackend(  # type: ignore[arg-type]
+        page,
+        screenshot_mask_selectors=("input[type='password']",),
+    )
+    with pytest.raises(ScreenshotMaskStabilityError, match="frame tree"):
+        backend.screenshot()
+    assert page.capture_count >= 1
+    backend.stop_screenshot_mask_tracking()
+
+
 def test_declared_secret_selectors_use_css_string_escaping() -> None:
     selectors = _secret_screenshot_selectors({"päss", 'quote"\\line\nend'})
 
@@ -2284,9 +2400,45 @@ def attach_and_detach_during_every_capture(**kwargs):
                     "screenshot",
                     attach_and_detach_during_every_capture,
                 )
+                patch_context.setattr(
+                    "openadapt_flow.backends.playwright_backend._MASKED_SCREENSHOT_TIMEOUT_S",
+                    0.0,
+                )
+                patch_context.setattr(
+                    "openadapt_flow.backends.playwright_backend._MASKED_SCREENSHOT_RETRY_SLEEP_S",
+                    0.0,
+                )
                 with pytest.raises(ScreenshotMaskStabilityError, match="frame tree"):
                     race_backend.screenshot()
-                assert churn_attempts == 3
+                assert churn_attempts >= 1
+
+            settle_attempts = 0
+
+            def churn_twice_then_keep(**kwargs):
+                nonlocal settle_attempts
+                settle_attempts += 1
+                if settle_attempts <= 2:
+                    race_page.evaluate(
+                        """attempt => {
+                          const frame = document.createElement('iframe');
+                          frame.id = `settle-${attempt}`;
+                          frame.srcdoc = '<input type="password" value="tmp">';
+                          document.body.appendChild(frame);
+                          frame.remove();
+                        }""",
+                        settle_attempts,
+                    )
+                return original_screenshot(**kwargs)
+
+            with monkeypatch.context() as patch_context:
+                patch_context.setattr(
+                    race_page,
+                    "screenshot",
+                    churn_twice_then_keep,
+                )
+                png = race_backend.screenshot()
+            assert png
+            assert settle_attempts >= 3
         finally:
             if frame_race_session.page is not None:
                 frame_race_session.page.evaluate(