From 906c1211c49004a3dc44a2b29b6a88ed8fc9eeb7 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Mon, 31 Aug 2026 10:51:24 +0200 Subject: [PATCH 1/2] fix(sdk): mask model output in the durable MessageEvent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only tool output was masked on the standard Agent path: bash output in the terminal executor and MCP tool output. The model's own response text was never masked, so a secret the model echoed back was persisted verbatim into the MessageEvent — the copy that is stored, replayed to the LLM, rendered, exported and POSTed to webhooks. Mask at _emit_message_event, the single point where both _handle_content_response and _handle_no_content_response build that event, shared by the sync and async paths. reasoning_content is masked as well since it is the only text a reasoning-only response carries. Signed provider payloads (thinking_blocks, responses_reasoning_item) are left untouched so their signatures stay valid. An empty registry short-circuits, so the non-secret path copies nothing. Fixes #4678 --- .../openhands/sdk/agent/response_dispatch.py | 43 +++++++++- tests/sdk/agent/test_response_dispatch.py | 78 +++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/agent/response_dispatch.py b/openhands-sdk/openhands/sdk/agent/response_dispatch.py index 77caf109b7e..28114149c06 100644 --- a/openhands-sdk/openhands/sdk/agent/response_dispatch.py +++ b/openhands-sdk/openhands/sdk/agent/response_dispatch.py @@ -281,7 +281,7 @@ def _emit_message_event( """Create and emit a MessageEvent, running critic if configured.""" msg_event = MessageEvent( source="agent", - llm_message=message, + llm_message=self._mask_secrets(message, conversation), llm_response_id=llm_response.id, ) if self.critic is not None and self.critic.mode == "finish_and_message": @@ -293,6 +293,47 @@ def _emit_message_event( on_event(msg_event) return msg_event + @staticmethod + def _mask_secrets(message: Message, conversation: LocalConversation) -> Message: + """Return ``message`` with registered secret values masked in its text. + + Tool output is masked by its executor, but nothing masked the model's + own words: a secret the model echoed back was persisted verbatim into + the durable ``MessageEvent`` — the copy that is stored, replayed to the + LLM, rendered, exported and POSTed to webhooks. + + Masking here rather than per response type covers every non-tool-call + response, because both ``_handle_content_response`` and + ``_handle_no_content_response`` build their event through + ``_emit_message_event``, on the sync and async paths alike. + ``reasoning_content`` is masked too — it is the only text a + reasoning-only response carries, so leaving it out would make this a + no-op for exactly one of the two callers. + + ``thinking_blocks`` and ``responses_reasoning_item`` are deliberately + left untouched: they are signed provider payloads, and rewriting them + invalidates the signature the next request replays. + """ + registry = conversation.state.secret_registry + if not registry.secret_sources: + return message + mask = registry.mask_secrets_in_output + return message.model_copy( + update={ + "content": [ + part.model_copy(update={"text": mask(part.text)}) + if isinstance(part, TextContent) + else part + for part in message.content + ], + "reasoning_content": ( + mask(message.reasoning_content) + if message.reasoning_content + else message.reasoning_content + ), + } + ) + def _send_corrective_nudge(self, on_event: ConversationCallbackType) -> None: """Inject corrective feedback when no tool call and no content. diff --git a/tests/sdk/agent/test_response_dispatch.py b/tests/sdk/agent/test_response_dispatch.py index 3a5fef872c0..b6f39ae4b5b 100644 --- a/tests/sdk/agent/test_response_dispatch.py +++ b/tests/sdk/agent/test_response_dispatch.py @@ -200,6 +200,7 @@ def _run_single_step( *, seed_events: list[Event] | None = None, record_emitted_events_in_state: bool = False, + secrets: dict[str, str] | None = None, ) -> tuple[list[Event], LocalConversation]: """Run one agent step with a canned LLM response.""" from pydantic import PrivateAttr @@ -220,6 +221,8 @@ def completion( # type: ignore[override] agent = Agent(llm=llm, tools=[]) conversation = Conversation(agent=agent) conversation._ensure_agent_ready() + if secrets is not None: + conversation.update_secrets(secrets) if seed_events is not None: for event in seed_events: conversation.state.append_event(event) @@ -397,3 +400,78 @@ def test_batch_action_events_are_emitted_consecutively(): assert [a.tool_call_id for a in action_events] == ["tc-1", "tc-2"] # Only the first event of the batch carries the turn's thought. assert action_events[0].thought and not action_events[1].thought + + +# --------------------------------------------------------------------------- +# Secret masking on the model's own output (issue #4678) +# --------------------------------------------------------------------------- + + +def _agent_message_text(events: list[Event]) -> str: + """Text of the single agent MessageEvent emitted by a step.""" + msg_event = next( + e for e in events if isinstance(e, MessageEvent) and e.source == "agent" + ) + part = msg_event.llm_message.content[0] + assert isinstance(part, TextContent) + return part.text + + +def test_content_response_masks_registered_secret(): + """The model's own words are masked in the durable MessageEvent. + + Only tool output was masked before, so a secret the model echoed back was + persisted verbatim into the copy that is stored, replayed to the LLM, + rendered, exported and POSTed to webhooks. + """ + msg = Message( + role="assistant", content=[TextContent(text="the value is supersecret now")] + ) + events, _ = _run_single_step( + _make_llm_response(msg), secrets={"TOKEN": "supersecret"} + ) + + text = _agent_message_text(events) + assert "supersecret" not in text + assert text == "the value is now" + + +def test_content_response_masks_secret_joined_from_stream_chunks(): + """A secret straddling two deltas is masked in the durable message. + + The ``Agent`` path accumulates deltas before it builds the ``Message``, so + neither chunk matches on its own — only the reassembled text does, and that + is what the durable copy is built from. + """ + chunks = ["the value is super", "secret now"] + msg = Message(role="assistant", content=[TextContent(text="".join(chunks))]) + events, _ = _run_single_step( + _make_llm_response(msg), secrets={"TOKEN": "supersecret"} + ) + + assert _agent_message_text(events) == "the value is now" + + +def test_content_response_without_secrets_is_left_alone(): + """An empty registry short-circuits: the message is passed through as-is.""" + msg = Message(role="assistant", content=[TextContent(text="nothing to hide")]) + events, _ = _run_single_step(_make_llm_response(msg)) + + assert _agent_message_text(events) == "nothing to hide" + + +def test_reasoning_only_response_masks_registered_secret(): + """The nudge path routes through the same chokepoint. + + A reasoning-only message carries no ``content``, so masking that alone + would be a no-op here — ``reasoning_content`` is the text that leaks. + """ + msg = Message(role="assistant", reasoning_content="the value is supersecret now") + events, _ = _run_single_step( + _make_llm_response(msg), secrets={"TOKEN": "supersecret"} + ) + + msg_event = next( + e for e in events if isinstance(e, MessageEvent) and e.source == "agent" + ) + assert msg_event.llm_message.reasoning_content == "the value is now" From 37341bdf1003a42128577ccf8ae8e0087c3b9a3c Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Mon, 31 Aug 2026 11:05:37 +0200 Subject: [PATCH 2/2] fix(sdk): mask from tracked values, not the registry source list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: trim the docstring to the one non-obvious constraint and drop the section banner in the tests. Also removes an empty-registry short-circuit that was wrong. Masking reads _exported_values, which is a separate field from secret_sources: EventService.activate_credential_binding pops a name from secret_sources via model_copy while the resolved value stays tracked, so the guard skipped masking a live credential that mask_secrets_in_output would still have masked. Calling it unconditionally is both correct and cheap — an empty _exported_values makes it a no-op. Covered by test_content_response_masks_value_dropped_from_secret_sources. --- .../openhands/sdk/agent/response_dispatch.py | 31 +++----------- tests/sdk/agent/test_response_dispatch.py | 40 ++++++++++++++++--- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/response_dispatch.py b/openhands-sdk/openhands/sdk/agent/response_dispatch.py index 28114149c06..aa8abdfad3b 100644 --- a/openhands-sdk/openhands/sdk/agent/response_dispatch.py +++ b/openhands-sdk/openhands/sdk/agent/response_dispatch.py @@ -297,27 +297,12 @@ def _emit_message_event( def _mask_secrets(message: Message, conversation: LocalConversation) -> Message: """Return ``message`` with registered secret values masked in its text. - Tool output is masked by its executor, but nothing masked the model's - own words: a secret the model echoed back was persisted verbatim into - the durable ``MessageEvent`` — the copy that is stored, replayed to the - LLM, rendered, exported and POSTed to webhooks. - - Masking here rather than per response type covers every non-tool-call - response, because both ``_handle_content_response`` and - ``_handle_no_content_response`` build their event through - ``_emit_message_event``, on the sync and async paths alike. - ``reasoning_content`` is masked too — it is the only text a - reasoning-only response carries, so leaving it out would make this a - no-op for exactly one of the two callers. - - ``thinking_blocks`` and ``responses_reasoning_item`` are deliberately - left untouched: they are signed provider payloads, and rewriting them - invalidates the signature the next request replays. + ``thinking_blocks`` and ``responses_reasoning_item`` are left alone: + they are signed provider payloads, and rewriting them invalidates the + signature replayed on the next request. """ - registry = conversation.state.secret_registry - if not registry.secret_sources: - return message - mask = registry.mask_secrets_in_output + mask = conversation.state.secret_registry.mask_secrets_in_output + reasoning = message.reasoning_content return message.model_copy( update={ "content": [ @@ -326,11 +311,7 @@ def _mask_secrets(message: Message, conversation: LocalConversation) -> Message: else part for part in message.content ], - "reasoning_content": ( - mask(message.reasoning_content) - if message.reasoning_content - else message.reasoning_content - ), + "reasoning_content": mask(reasoning) if reasoning else reasoning, } ) diff --git a/tests/sdk/agent/test_response_dispatch.py b/tests/sdk/agent/test_response_dispatch.py index b6f39ae4b5b..dd5a3493199 100644 --- a/tests/sdk/agent/test_response_dispatch.py +++ b/tests/sdk/agent/test_response_dispatch.py @@ -1,5 +1,6 @@ """Unit tests for LLM response classification and dispatch.""" +from collections.abc import Callable from unittest.mock import MagicMock import pytest @@ -201,6 +202,7 @@ def _run_single_step( seed_events: list[Event] | None = None, record_emitted_events_in_state: bool = False, secrets: dict[str, str] | None = None, + prepare: Callable[[LocalConversation], None] | None = None, ) -> tuple[list[Event], LocalConversation]: """Run one agent step with a canned LLM response.""" from pydantic import PrivateAttr @@ -226,6 +228,8 @@ def completion( # type: ignore[override] if seed_events is not None: for event in seed_events: conversation.state.append_event(event) + if prepare is not None: + prepare(conversation) events: list[Event] = [] @@ -402,11 +406,6 @@ def test_batch_action_events_are_emitted_consecutively(): assert action_events[0].thought and not action_events[1].thought -# --------------------------------------------------------------------------- -# Secret masking on the model's own output (issue #4678) -# --------------------------------------------------------------------------- - - def _agent_message_text(events: list[Event]) -> str: """Text of the single agent MessageEvent emitted by a step.""" msg_event = next( @@ -453,7 +452,7 @@ def test_content_response_masks_secret_joined_from_stream_chunks(): def test_content_response_without_secrets_is_left_alone(): - """An empty registry short-circuits: the message is passed through as-is.""" + """With nothing registered, the message text is passed through unchanged.""" msg = Message(role="assistant", content=[TextContent(text="nothing to hide")]) events, _ = _run_single_step(_make_llm_response(msg)) @@ -475,3 +474,32 @@ def test_reasoning_only_response_masks_registered_secret(): e for e in events if isinstance(e, MessageEvent) and e.source == "agent" ) assert msg_event.llm_message.reasoning_content == "the value is now" + + +def test_content_response_masks_value_dropped_from_secret_sources(): + """Masking follows the tracked values, not the registry's source list. + + ``EventService.activate_credential_binding`` pops a name from + ``secret_sources`` while the resolved value stays tracked for masking, so + keying off ``secret_sources`` would leak a live credential here. + """ + + def drop_source(convo: LocalConversation) -> None: + registry = convo.state.secret_registry + registry.get_secret_value("TOKEN") # resolve -> tracked for masking + with convo.state: + convo.state.secret_registry = registry.model_copy( + update={"secret_sources": {}} + ) + + msg = Message( + role="assistant", content=[TextContent(text="the value is supersecret now")] + ) + events, convo = _run_single_step( + _make_llm_response(msg), + secrets={"TOKEN": "supersecret"}, + prepare=drop_source, + ) + + assert not convo.state.secret_registry.secret_sources + assert _agent_message_text(events) == "the value is now"