fix(browser): scope the CDP binary-payload exemption to typed fields (#94138)

Architecture-review follow-up: the method-scoped binary_payload flag skipped redaction for every string anywhere in the result of the two listed methods, and the same corruption stayed reachable through Network.getResponseBody / Fetch.getResponseBody / IO.read / Network.streamResourceContent. Make the exemption field-scoped instead: an explicit schema exempts exactly Page.captureScreenshot.result.data and Page.printToPDF.result.data (carriers with no flag of their own), and any dict whose base64Encoded sibling is exactly True exempts its body/data/bufferedData string (the protocol's own discriminator — text bodies with base64Encoded: false stay redacted). Every other string in every result keeps full secret redaction.
2026-08-25 03:55:05 +08:00
parent a56885495e
commit b2a17bfe82
2 changed files with 131 additions and 21 deletions
@@ -233,8 +233,8 @@ def test_print_to_pdf_base64_passes_through_unredacted(cdp_server):


 def test_binary_payload_flag_keeps_secret_redaction_off_method_list(cdp_server):
-    """Fail-closed pin: only the two binary-payload methods skip redaction;
-    any other method's text output keeps full secret redaction."""
+    """Fail-closed pin: methods without a binary payload field keep full
+    secret redaction; the listed methods pass their payload through."""
    fake_key = "sk-" + "CDPSECRETSTILLREDACTED1234567890"
    cdp_server.on(
        "Runtime.evaluate",
@@ -254,6 +254,90 @@ def test_binary_payload_flag_keeps_secret_redaction_off_method_list(cdp_server):
    assert shot_result["result"]["data"] == "gAAAA" + "B" * 60


+def test_binary_payload_field_sibling_string_still_redacted(cdp_server):
+    """Path-scoped exemption: on a binary-bearing result only the payload
+    field skips redaction; a sibling string keeps full secret redaction,
+    proving the exemption cannot widen to the whole result object."""
+    fake_key = "sk-" + "CDPSECRETSIBLING1234567890"
+    shot_b64 = "iVBORw0KGgoAAAANSUhEUg+" + "gAAAA" + "B" * 60 + "=="
+    cdp_server.on(
+        "Page.captureScreenshot",
+        lambda params, sid: {"data": shot_b64, "note": fake_key},
+    )
+
+    result = json.loads(browser_cdp_tool.browser_cdp(method="Page.captureScreenshot"))
+
+    assert result["success"] is True
+    assert result["result"]["data"] == shot_b64
+    assert "CDPSECRETSIBLING" not in json.dumps(result)
+    assert result["result"]["note"].startswith("sk-")
+
+
+def test_get_response_body_base64_discriminator_passes_through(cdp_server):
+    """Network.getResponseBody with base64Encoded: true — the body is opaque
+    base64 bytes and must remain byte-identical (#94138 review on #94142)."""
+    body_b64 = "q9Z7" + "gAAAA" + "B" * 60 + "=="
+    cdp_server.on(
+        "Network.getResponseBody",
+        lambda params, sid: {"body": body_b64, "base64Encoded": True},
+    )
+
+    result = json.loads(
+        browser_cdp_tool.browser_cdp(method="Network.getResponseBody")
+    )
+
+    assert result["success"] is True
+    assert result["result"]["body"] == body_b64
+
+
+def test_get_response_body_text_discriminator_still_redacts(cdp_server):
+    """Same method with base64Encoded: false — the body is text and a real
+    secret in it must still be redacted."""
+    fake_key = "sk-" + "CDPSECRETBODY1234567890"
+    cdp_server.on(
+        "Network.getResponseBody",
+        lambda params, sid: {"body": f"leak {fake_key} here", "base64Encoded": False},
+    )
+
+    result = json.loads(
+        browser_cdp_tool.browser_cdp(method="Network.getResponseBody")
+    )
+
+    assert result["success"] is True
+    assert "CDPSECRETBODY" not in json.dumps(result)
+
+
+def test_io_read_base64_discriminator_passes_through(cdp_server):
+    """IO.read honors the same discriminator contract for its data field."""
+    chunk_b64 = "AAA" + "gAAAA" + "C" * 60 + "="
+    cdp_server.on(
+        "IO.read",
+        lambda params, sid: {"data": chunk_b64, "base64Encoded": True, "eof": True},
+    )
+
+    result = json.loads(browser_cdp_tool.browser_cdp(method="IO.read"))
+
+    assert result["success"] is True
+    assert result["result"]["data"] == chunk_b64
+    assert result["result"]["eof"] is True
+
+
+def test_fetch_get_response_body_base64_discriminator_passes_through(cdp_server):
+    """Fetch.getResponseBody pins the same body/base64Encoded contract."""
+    body_b64 = "zz7+" + "gAAAA" + "D" * 60 + "=="
+    cdp_server.on(
+        "Fetch.getResponseBody",
+        lambda params, sid: {"body": body_b64, "base64Encoded": True},
+    )
+
+    result = json.loads(
+        browser_cdp_tool.browser_cdp(method="Fetch.getResponseBody")
+    )
+
+    assert result["success"] is True
+    assert result["result"]["body"] == body_b64
+
+
 # ---------------------------------------------------------------------------
 # Happy-path: target-attached call
 # ---------------------------------------------------------------------------
@@ -43,37 +43,62 @@ _CDP_PRIVATE_PAGE_ALLOWED_METHODS = {
 }


-_BINARY_PAYLOAD_CDP_METHODS = {
-    # Methods whose result is dominated by a base64-encoded binary payload.
-    # redact_sensitive_text's Fernet pattern ("gAAAA" + base64 alphabet) can
-    # match arbitrary spans inside such payloads — collapsing them to
-    # "first6...last4" and corrupting the decoded screenshot/PDF (#94138).
-    # The payload is binary, not free text the model reads, so redaction has
-    # no secret to protect there; every other method keeps full redaction.
-    "Page.captureScreenshot",
-    "Page.printToPDF",
+_CDP_BINARY_RESULT_FIELDS: Dict[str, frozenset] = {
+    # Protocol carriers whose result.<field> is an opaque base64 payload with
+    # no discriminator flag of their own. redact_sensitive_text's Fernet
+    # pattern ("gAAAA" + base64 alphabet) can match arbitrary spans inside
+    # such payloads — collapsing them to "first6...last4" and corrupting the
+    # decoded bytes (#94138). The payload is binary, not free text the model
+    # reads, so redaction has no secret to protect there. The generic
+    # body/read methods (Network.getResponseBody, Fetch.getResponseBody,
+    # IO.read, Network.streamResourceContent) are NOT here: they carry a
+    # base64Encoded sibling and go through the discriminator below.
+    "Page.captureScreenshot": frozenset({"data"}),
+    "Page.printToPDF": frozenset({"data"}),
 }

+# Generic body/read methods signal "this string is opaque base64 bytes" via a
+# sibling boolean. Honor the protocol discriminator anywhere it appears: the
+# flag is type information, so it is safe to trust even for methods not
+# listed above (#94138 review on #94142).
+_BASE64_DISCRIMINATED_FIELDS = frozenset({"body", "data", "bufferedData"})

-def _redact_cdp_output(value: Any, *, binary_payload: bool = False) -> Any:
+
+def _redact_cdp_output(value: Any, *, exempt_fields: frozenset = frozenset()) -> Any:
    """Redact browser-originated CDP result data before returning it.

-    When *binary_payload* is set (the calling method's result is a base64
-    binary payload, see ``_BINARY_PAYLOAD_CDP_METHODS``), strings pass
-    through unchanged so the payload stays byte-identical (#94138).
+    Policy: semantic text is redacted; opaque bytes stay byte-identical
+    (#94138). *exempt_fields* names the calling method's top-level binary
+    payload fields (``_CDP_BINARY_RESULT_FIELDS``), so only those exact
+    ``method.result.<field>`` slots skip redaction — sibling fields keep
+    full redaction. Any dict whose ``base64Encoded`` sibling is exactly
+    ``True`` exempts its ``body``/``data``/``bufferedData`` string the same
+    way; ``base64Encoded: false`` or absent means the value is text and is
+    redacted.
    """
    from agent.redact import redact_sensitive_text

    if isinstance(value, str):
-        if binary_payload:
-            return value
        return redact_sensitive_text(value, force=True)
    if isinstance(value, list):
-        return [_redact_cdp_output(item, binary_payload=binary_payload) for item in value]
+        return [_redact_cdp_output(item) for item in value]
    if isinstance(value, tuple):
-        return tuple(_redact_cdp_output(item, binary_payload=binary_payload) for item in value)
+        return tuple(_redact_cdp_output(item) for item in value)
    if isinstance(value, dict):
-        return {key: _redact_cdp_output(item, binary_payload=binary_payload) for key, item in value.items()}
+        base64_flagged = value.get("base64Encoded") is True
+        redacted: Dict[str, Any] = {}
+        for key, item in value.items():
+            if (
+                isinstance(item, str)
+                and (
+                    key in exempt_fields
+                    or (base64_flagged and key in _BASE64_DISCRIMINATED_FIELDS)
+                )
+            ):
+                redacted[key] = item
+            else:
+                redacted[key] = _redact_cdp_output(item)
+        return redacted
    return value

 # ``websockets`` is a direct hermes-agent dependency because the browser CDP
@@ -545,7 +570,8 @@ def browser_cdp(
        "success": True,
        "method": method,
        "result": _redact_cdp_output(
-            result, binary_payload=method in _BINARY_PAYLOAD_CDP_METHODS
+            result,
+            exempt_fields=_CDP_BINARY_RESULT_FIELDS.get(method, frozenset())
        ),
    }
    if target_id: