From b2a17bfe823b5541666e3096378ffa9763145a2b Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Tue, 25 Aug 2026 03:55:05 +0800 Subject: [PATCH] fix(browser): scope the CDP binary-payload exemption to typed fields (#94138) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/tools/test_browser_cdp_tool.py | 88 +++++++++++++++++++++++++++- tools/browser_cdp_tool.py | 64 ++++++++++++++------ 2 files changed, 131 insertions(+), 21 deletions(-) diff --git a/tests/tools/test_browser_cdp_tool.py b/tests/tools/test_browser_cdp_tool.py index 493abc2a81..66fff615b7 100644 --- a/tests/tools/test_browser_cdp_tool.py +++ b/tests/tools/test_browser_cdp_tool.py @@ -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 # --------------------------------------------------------------------------- diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index fa961da0fe..e0f37ce3a0 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -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. 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.`` 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: