From 3dbb897df732e2f8f1c0e7ab114b02a5078ca023 Mon Sep 17 00:00:00 2001 From: Richard Abrich Date: Thu, 3 Sep 2026 19:47:10 -0400 Subject: [PATCH 1/3] fix: wait for a quiet frame tree before keeping a secret-masked screenshot Three immediate retries lost to SPA login pages that attach and detach iframes after Sign in. Retry until the tree is still, up to eight seconds. Discard every PNG taken while frames moved. Still refuse if it never settles. Opened by an agent session, not the founder. --- openadapt_flow/backends/playwright_backend.py | 19 +++++++++- tests/test_browser_attach.py | 38 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/openadapt_flow/backends/playwright_backend.py b/openadapt_flow/backends/playwright_backend.py index 478cc1fd..cc1fc0cc 100644 --- a/openadapt_flow/backends/playwright_backend.py +++ b/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" diff --git a/tests/test_browser_attach.py b/tests/test_browser_attach.py index d23199b5..6d418d6e 100644 --- a/tests/test_browser_attach.py +++ b/tests/test_browser_attach.py @@ -2284,9 +2284,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 = ''; + 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( From 2528c58f6dafba97624ae6de494798177e6e048d Mon Sep 17 00:00:00 2001 From: Richard Abrich Date: Thu, 3 Sep 2026 19:49:37 -0400 Subject: [PATCH 2/3] test: reproduce secret-mask refuse after three iframe storms A fake login page attaches and detaches iframes for the old three-try budget, then goes quiet. That is the ServiceNow Sign-in failure. The record must keep a masked frame. A tree that never settles still refuses. Opened by an agent session, not the founder. --- tests/test_browser_attach.py | 116 +++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/tests/test_browser_attach.py b/tests/test_browser_attach.py index 6d418d6e..1c31a551 100644 --- a/tests/test_browser_attach.py +++ b/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'}) From 4a10acdb37a29b4748630bcedb83c807f1e65daa Mon Sep 17 00:00:00 2001 From: Richard Abrich Date: Tue, 8 Sep 2026 17:29:16 -0400 Subject: [PATCH 3/3] fix: recheck privacy boundaries across masked screenshot retries Re-run the privacy guard before each masked capture and after the browser response. Keep privacy refusals outside the frame-churn retry handler so a new closed shadow secret cannot be retained after the frame tree settles. The real Chromium regression covers a secret added between retries and during capture, with three trials per condition. All six cases fail on PR #465 head 2528c58f and pass with this patch. The focused group passes 11 tests. The existing integrated recorder test passes in 94.06 seconds. Ruff lint, formatting, and diff checks pass. Signed-off-by: Richard Abrich --- openadapt_flow/backends/playwright_backend.py | 12 ++- tests/test_browser_attach.py | 80 +++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/openadapt_flow/backends/playwright_backend.py b/openadapt_flow/backends/playwright_backend.py index cc1fc0cc..534fed86 100644 --- a/openadapt_flow/backends/playwright_backend.py +++ b/openadapt_flow/backends/playwright_backend.py @@ -2297,16 +2297,20 @@ def _same_frames(left: tuple[Any, ...], right: tuple[Any, ...]) -> bool: def screenshot(self) -> bytes: """Return a stable current full-viewport frame as PNG bytes.""" - if self._screenshot_guard is not None: - self._screenshot_guard() base_options: dict[str, Any] = {} if self._screenshot_scale == "css": base_options["scale"] = "css" if not self._screenshot_mask_selectors: + if self._screenshot_guard is not None: + self._screenshot_guard() return self.page.screenshot(type="png", full_page=False, **base_options) deadline = time.monotonic() + _MASKED_SCREENSHOT_TIMEOUT_S while True: + # A retry can observe a different secret boundary even after the + # frame tree settles. Rebind or refuse before every capture. + if self._screenshot_guard is not None: + self._screenshot_guard() generation = self._screenshot_frame_generation frames = tuple(self.page.frames) if generation != self._screenshot_frame_generation: @@ -2333,6 +2337,10 @@ def screenshot(self) -> bytes: time.sleep(_MASKED_SCREENSHOT_RETRY_SLEEP_S) continue raise + # A closed root can appear during capture without changing the + # frame tree. Its privacy refusal must escape the retry handler. + if self._screenshot_guard is not None: + self._screenshot_guard() current_frames = tuple(self.page.frames) if generation == self._screenshot_frame_generation and self._same_frames( frames, current_frames diff --git a/tests/test_browser_attach.py b/tests/test_browser_attach.py index 1c31a551..6c5b1adb 100644 --- a/tests/test_browser_attach.py +++ b/tests/test_browser_attach.py @@ -1140,6 +1140,86 @@ def test_launched_browser_refuses_static_unbound_closed_shadow_password( assert not output.exists() +@pytest.mark.timeout(60) +@pytest.mark.parametrize("injection_point", ("before_retry", "during_capture")) +@pytest.mark.parametrize("trial", range(3)) +def test_masked_screenshot_rechecks_new_closed_shadow_secret_boundaries( + attach_app_url: str, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + injection_point: str, + trial: int, +) -> None: + """No retry may retain a secret that appears after the first privacy scan.""" + + if _chromium_executable() is None: + pytest.skip("no Chromium executable is installed") + output = tmp_path / f"new-closed-boundary-{injection_point}-{trial}" + session = InteractiveRecorder( + attach_app_url, + output, + headless=True, + secret_fields=("retry-secret",), + ) + session.start() + try: + page, backend = session.page, session.backend + assert page is not None and backend is not None + original_screenshot = page.screenshot + original_sleep = time.sleep + captures = 0 + inserted = False + + def insert_unbound_secret() -> None: + nonlocal inserted + inserted = True + page.evaluate( + """() => { + const host = document.createElement('x-unbound-secret'); + document.body.appendChild(host); + const root = host.attachShadow({mode: 'closed'}); + const input = document.createElement('input'); + input.name = 'retry-secret'; + input.value = 'SYNTHETIC-RETRY-SECRET'; + root.appendChild(input); + }""" + ) + + def screenshot(**kwargs): + nonlocal captures + captures += 1 + if injection_point == "before_retry" and captures <= 3: + page.evaluate( + """() => { + const frame = document.createElement('iframe'); + document.body.appendChild(frame); + frame.remove(); + }""" + ) + elif injection_point == "during_capture" and captures == 1: + insert_unbound_secret() + return original_screenshot(**kwargs) + + def retry_sleep(seconds: float) -> None: + if injection_point == "before_retry" and captures == 3 and not inserted: + insert_unbound_secret() + original_sleep(seconds) + + with monkeypatch.context() as patch: + patch.setattr(page, "screenshot", screenshot) + patch.setattr( + "openadapt_flow.backends.playwright_backend.time.sleep", + retry_sleep, + ) + with pytest.raises(BrowserAttachError, match="closed shadow root"): + backend.screenshot() + assert inserted + assert captures == (3 if injection_point == "before_retry" else 1) + finally: + session.abort() + assert not output.exists() + + @pytest.mark.timeout(30) def test_page_closure_scrubs_replaced_prefilled_and_reflected_secrets() -> None: """Real Chromium proves the page-local guard before screenshot handling."""