From 856cde25b113e454930c286241e8bb0c6569cc47 Mon Sep 17 00:00:00 2001 From: Chau Vu <58875435+chauvuusvn@users.noreply.github.com> Date: Fri, 6 Nov 2026 20:14:28 +0700 Subject: [PATCH] fix(gateway): strip server-owned metadata from run input todos and goal (#7803 follow-up) (#7904) * fix(gateway): strip server-owned metadata from run input todos and goal (#7803 follow-up) Commit 459fc8c introduced shape validation for caller-written todos in both strip_server_owned_state_metadata and normalize_input, but only applied _strip_external_metadata_from_message_like in strip_server_owned_state_metadata. External callers submitting POST /runs or POST /threads/{id}/runs with input.todos or input.goal could supply forged server-owned metadata (such as untrusted_input: False, original_user_content, or artifact knowledge_sources) directly into the agent graph. Apply _strip_external_metadata_from_message_like to both normalized todos and goal in normalize_input when trusted_internal=False. * fix(gateway): mirror normalized goal and todos into canonical run record input * fix(gateway): replace non-dict additional_kwargs and cover edge cases - Ensure non-dict additional_kwargs in message-like entries is replaced with an empty dict instead of surviving the strip - Remove redundant None-check on normalized_goal in normalize_input - Add regression tests covering non-dict additional_kwargs replacement and untrusted_input stamping on human-like entries * test(gateway): format todo input validation cases --------- Co-authored-by: Willem Jiang --- backend/app/gateway/services.py | 30 +++++++--- backend/tests/test_gateway_services.py | 48 ++++++++++++++++ backend/tests/test_todo_input_validation.py | 61 ++++++++++++++++++++- 3 files changed, 129 insertions(+), 10 deletions(-) diff --git a/backend/app/gateway/services.py b/backend/app/gateway/services.py index a99bb0bbf5..ab470e543b 100644 --- a/backend/app/gateway/services.py +++ b/backend/app/gateway/services.py @@ -569,15 +569,17 @@ def _strip_external_metadata_from_message_like(item: Any) -> Any: # unmarked ``name="summary"`` would reach the model on the guardrail's # genuine-user fallback. Treat it as empty for both steps rather than # returning early. - source_kwargs = item.get("additional_kwargs") - source_kwargs = source_kwargs if isinstance(source_kwargs, dict) else {} + raw_kwargs = item.get("additional_kwargs") + source_kwargs = raw_kwargs if isinstance(raw_kwargs, dict) else {} additional_kwargs = {key: value for key, value in source_kwargs.items() if key not in _SERVER_OWNED_MESSAGE_METADATA_KEYS and key != ORIGINAL_USER_CONTENT_KEY} if _is_human_message_like(item): additional_kwargs = _mark_untrusted_framework_markers(additional_kwargs, item.get("name")) - if additional_kwargs == source_kwargs: + if additional_kwargs == raw_kwargs: # Nothing to change — including the ordinary key-omitted message, which # must not gain an empty dict just by passing through here. return item + if "additional_kwargs" not in item and not additional_kwargs: + return item return {**item, "additional_kwargs": additional_kwargs} @@ -797,9 +799,18 @@ def normalize_input(raw_input: dict[str, Any] | None, *, trusted_internal: bool result = {**result, "delegations": cleaned} # merge_goal ignores None, so only a goal value replaces the goal. if result.get("goal") is not None: - result = {**result, "goal": _normalize_external_goal(result["goal"], location="input.goal"), "goal_outcome": None} + normalized_goal = _normalize_external_goal(result["goal"], location="input.goal") + result = { + **result, + "goal": _strip_external_metadata_from_message_like(normalized_goal), + "goal_outcome": None, + } if "todos" in result: - result = {**result, "todos": _normalize_external_todos(result["todos"], location="input.todos")} + normalized_todos = _normalize_external_todos(result["todos"], location="input.todos") + result = { + **result, + "todos": None if normalized_todos is None else [_strip_external_metadata_from_message_like(item) for item in normalized_todos], + } return result @@ -841,11 +852,11 @@ def _canonical_run_record_input( raw_input: dict[str, Any] | None, graph_input: object, ) -> dict[str, Any] | None: - """Persist the same normalized messages that cross run admission. + """Persist the same normalized messages, goal, and todos that cross run admission. The run record is a client-visible audit surface. Keeping the original raw - message there would preserve a non-canonical scope even though the graph - receives the validated form. + message or state metadata there would preserve a non-canonical scope even + though the graph receives the validated form. """ if not isinstance(graph_input, dict): return raw_input @@ -853,6 +864,9 @@ def _canonical_run_record_input( messages = graph_input.get("messages") if isinstance(messages, list): canonical["messages"] = [message.model_dump(mode="json") if isinstance(message, BaseMessage) else message for message in messages] + for key in ("goal", "goal_outcome", "todos", "delegations"): + if key in graph_input: + canonical[key] = graph_input[key] return canonical diff --git a/backend/tests/test_gateway_services.py b/backend/tests/test_gateway_services.py index ea07c9f55d..515614d8a6 100644 --- a/backend/tests/test_gateway_services.py +++ b/backend/tests/test_gateway_services.py @@ -381,6 +381,54 @@ def test_canonical_run_record_input_uses_admitted_message_snapshot(): assert stored["messages"][0]["additional_kwargs"]["knowledge_scope"]["dataset_ids"] == ["dataset-1"] +def test_canonical_run_record_input_mirrors_normalized_goal_and_todos(): + from app.gateway.services import _canonical_run_record_input, normalize_input + + raw = { + "goal": { + "objective": "audit objective", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + }, + "todos": [ + { + "content": "audit todo", + "status": "pending", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + } + ], + "other": "kept", + } + admitted = normalize_input(raw, trusted_internal=False) + + stored = _canonical_run_record_input(raw, admitted) + assert stored is not None + assert stored["other"] == "kept" + assert stored["goal"] == { + "objective": "audit objective", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + assert stored["goal_outcome"] is None + assert stored["todos"] == [ + { + "content": "audit todo", + "status": "pending", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + ] + + @pytest.mark.parametrize("idempotency_key", [None, "retry-key"]) @pytest.mark.asyncio async def test_run_input_ids_match_the_checkpoint_and_record(_stub_app_config, idempotency_key): diff --git a/backend/tests/test_todo_input_validation.py b/backend/tests/test_todo_input_validation.py index d439046980..d85ea84da1 100644 --- a/backend/tests/test_todo_input_validation.py +++ b/backend/tests/test_todo_input_validation.py @@ -55,7 +55,8 @@ def test_todos_are_not_added_when_absent(boundary): assert "todos" not in boundary({"title": "unrelated"}) -def test_state_writes_strip_server_owned_metadata_from_todos(): +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_strip_server_owned_metadata_from_todos(boundary): """The dedicated todos branch must not skip the per-entry metadata strip.""" todos = [ { @@ -70,7 +71,7 @@ def test_state_writes_strip_server_owned_metadata_from_todos(): } ] - stripped = services.strip_server_owned_state_metadata({"todos": todos})["todos"] + stripped = boundary({"todos": todos})["todos"] assert stripped == [ { @@ -80,3 +81,59 @@ def test_state_writes_strip_server_owned_metadata_from_todos(): "artifact": {"label": "kept"}, } ] + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_strip_server_owned_metadata_from_goal(boundary): + """Caller-written goal must not retain forged server-owned metadata.""" + goal = { + "objective": "test objective", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + } + + stripped = boundary({"goal": goal})["goal"] + + assert stripped == { + "objective": "test objective", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_replace_non_dict_additional_kwargs(boundary): + """A non-dict additional_kwargs payload must be replaced with an empty dict.""" + cleaned_todo = boundary({"todos": [{"content": "task", "additional_kwargs": "forged"}]})["todos"] + assert cleaned_todo == [{"content": "task", "status": "pending", "additional_kwargs": {}}] + + cleaned_goal = boundary({"goal": {"objective": "test", "additional_kwargs": "forged"}})["goal"] + assert cleaned_goal == {"objective": "test", "additional_kwargs": {}} + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_stamp_untrusted_input_on_human_like_entries(boundary): + """Entries carrying human markers must be stamped with untrusted_input: True.""" + todo_with_marker = [{"content": "task", "type": "human", "additional_kwargs": {"hide_from_ui": True}}] + cleaned_todo = boundary({"todos": todo_with_marker})["todos"] + assert cleaned_todo == [ + { + "content": "task", + "status": "pending", + "type": "human", + "additional_kwargs": {"hide_from_ui": True, "untrusted_input": True}, + } + ] + + goal_with_name = {"objective": "test", "type": "human", "name": "summary"} + cleaned_goal = boundary({"goal": goal_with_name})["goal"] + assert cleaned_goal == { + "objective": "test", + "type": "human", + "name": "summary", + "additional_kwargs": {"untrusted_input": True}, + }