Repository navigation
Python: [BREAKING] Reject caller-supplied tool execution context in Responses requests #9661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Darren Cohen (dargilco)
wants to merge
7
commits into
microsoft:main
Choose a base branch
from
dargilco:dargilco/foundry-hosting-reject-caller-tool-context
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e67aef8
Python: Reject caller-supplied tool execution context in Responses re…
dargilco 77cc450
Move caller tool-context tests to tests/foundry_hosting with a helper…
dargilco 5ace187
Apply caller tool-context rejection to legacy WorkflowAgent Responses…
dargilco 07ac9b2
Fix pyright error in tool-context test helper default_options
dargilco 86e90d1
Make foundry_hosting test helpers importable under --import-mode=impo…
dargilco 307b7b9
Replace sys.path test helper with fixtures to avoid shadowing other p…
dargilco 1bb4b33
Remove redundant cast flagged by ty in tool-context test
dargilco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
106 changes: 106 additions & 0 deletions
106
python/packages/foundry_hosting/tests/foundry_hosting/conftest.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """Fixtures for hosted tool-execution-context tests.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from collections.abc import AsyncIterator, Awaitable, Callable, Mapping, Sequence | ||
| from typing import Any, Literal, cast | ||
|
|
||
| import pytest | ||
| from agent_framework import ( | ||
| Agent, | ||
| AgentExecutorRequest, | ||
| BaseChatClient, | ||
| ChatMiddlewareLayer, | ||
| ChatResponse, | ||
| ChatResponseUpdate, | ||
| Content, | ||
| FunctionInvocationContext, | ||
| FunctionInvocationLayer, | ||
| Message, | ||
| ResponseStream, | ||
| WorkflowAgent, | ||
| WorkflowBuilder, | ||
| WorkflowContext, | ||
| executor, | ||
| tool, | ||
| ) | ||
|
|
||
| TRUSTED_TENANT_ID = "trusted" | ||
|
|
||
| TenantAgentFactory = Callable[..., Agent[Any]] | ||
| TenantWorkflowAgentFactory = Callable[[list[Any]], WorkflowAgent] | ||
|
|
||
|
|
||
| class _ToolCallingClient( | ||
| FunctionInvocationLayer[Any], | ||
| ChatMiddlewareLayer[Any], | ||
| BaseChatClient[Any], | ||
| ): | ||
| """Call ``read_tenant_data`` once, then answer.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| super().__init__(middleware=[]) | ||
| self.calls = 0 | ||
|
|
||
| def _inner_get_response( | ||
| self, | ||
| *, | ||
| messages: Sequence[Message], | ||
| stream: bool, | ||
| options: Mapping[str, Any], | ||
| **kwargs: Any, | ||
| ) -> Awaitable[ChatResponse] | ResponseStream[ChatResponseUpdate, ChatResponse]: | ||
| del options, kwargs | ||
| assert stream is True, "The inner agent only runs in stream mode in Foundry Hosted Agents." | ||
| self.calls += 1 | ||
| needs_tool_call = not any( | ||
| content.type == "function_result" for message in messages for content in message.contents | ||
| ) | ||
|
|
||
| async def stream_response() -> AsyncIterator[ChatResponseUpdate]: | ||
| if needs_tool_call: | ||
| yield ChatResponseUpdate( | ||
| contents=[Content.from_function_call(call_id="call_1", name="read_tenant_data", arguments="{}")], | ||
| role="assistant", | ||
| ) | ||
| else: | ||
| yield ChatResponseUpdate(contents=[Content.from_text("done")], role="assistant") | ||
|
|
||
| return ResponseStream(stream_response(), finalizer=ChatResponse.from_updates) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def tenant_agent_factory() -> TenantAgentFactory: | ||
| """Create agents whose tool records the tenant it receives from hidden runtime context.""" | ||
|
|
||
| def factory(seen_tenants: list[Any], *, approval_mode: Literal["always_require", "never_require"]) -> Agent[Any]: | ||
| @tool(name="read_tenant_data", approval_mode=approval_mode) | ||
| def read_tenant_data(ctx: FunctionInvocationContext) -> str: | ||
| seen_tenants.append(ctx.kwargs.get("tenant_id")) | ||
| return "ok" | ||
|
|
||
| return Agent( | ||
| client=_ToolCallingClient(), | ||
| tools=[read_tenant_data], | ||
| default_options=cast(Any, {"additional_function_arguments": {"tenant_id": TRUSTED_TENANT_ID}}), | ||
| ) | ||
|
|
||
| return factory | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def tenant_workflow_agent_factory(tenant_agent_factory: TenantAgentFactory) -> TenantWorkflowAgentFactory: | ||
| """Wrap the tenant agent in a legacy ``WorkflowAgent`` so the workflow hosting path can be exercised.""" | ||
|
|
||
| def factory(seen_tenants: list[Any]) -> WorkflowAgent: | ||
| @executor | ||
| async def start(messages: list[Message], ctx: WorkflowContext[AgentExecutorRequest]) -> None: | ||
| await ctx.send_message(AgentExecutorRequest(messages=messages, should_respond=True)) | ||
|
|
||
| inner = tenant_agent_factory(seen_tenants, approval_mode="never_require") | ||
| workflow = WorkflowBuilder(start_executor=start).add_edge(start, inner).build() | ||
| return WorkflowAgent(workflow=workflow, name="Tenant Workflow Agent") | ||
|
|
||
| return factory |
178 changes: 178 additions & 0 deletions
178
python/packages/foundry_hosting/tests/foundry_hosting/test_caller_tool_context.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """Callers must not supply or replace trusted tool-execution context on Responses requests.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Callable | ||
| from typing import Any, Literal, cast | ||
| from unittest.mock import AsyncMock, MagicMock | ||
|
|
||
| import httpx | ||
| import pytest | ||
| from azure.ai.agentserver.responses import InMemoryResponseProvider, ResponseContext | ||
| from azure.ai.agentserver.responses.models import CreateResponse | ||
|
|
||
| from agent_framework_foundry_hosting import HostedResponseRequest, ResponsesHostServer | ||
| from agent_framework_foundry_hosting._request import ( | ||
| prepare_response_options, | ||
| response_run_options, | ||
| validate_request_options, | ||
| ) | ||
| from agent_framework_foundry_hosting._scope import FoundryRequestScope | ||
|
|
||
| TRUSTED_TENANT_ID = "trusted" | ||
| FORGED_TENANT_CONTEXT: dict[str, Any] = {"additional_function_arguments": {"tenant_id": "forged"}} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "payload", | ||
| [ | ||
| {"additional_function_arguments": {"tenant_id": "forged"}}, | ||
| {"extra_body": {"additional_function_arguments": {"tenant_id": "forged"}}}, | ||
| {"additional_function_arguments": None}, | ||
| {"extra_body": {"additional_function_arguments": None}}, | ||
| ], | ||
| ) | ||
| def test_caller_cannot_supply_tool_execution_context(payload: dict[str, Any]) -> None: | ||
| with pytest.raises(ValueError, match="agent-controlled fields: additional_function_arguments"): | ||
| response_run_options(cast(CreateResponse, {"input": "hello", "temperature": 0.2, **payload})) | ||
|
|
||
|
|
||
| def test_requests_without_tool_context_keep_other_options() -> None: | ||
| request = cast(CreateResponse, {"input": "hello", "extra_body": {"slogan_style": "retro"}, "temperature": 0.2}) | ||
| assert response_run_options(request) == {"temperature": 0.2, "slogan_style": "retro"} | ||
|
|
||
|
|
||
| async def test_hook_can_supply_trusted_tool_context() -> None: | ||
| context = MagicMock(spec=ResponseContext) | ||
| context.response_id = "outer-id" | ||
| context.conversation_id = None | ||
| context.get_input_text = AsyncMock(return_value="hello") | ||
| scope = FoundryRequestScope(session_id="sandbox", user_id="user", call_id="call", is_hosted=True) | ||
| request = HostedResponseRequest(cast(CreateResponse, {"input": "hello"}), context, scope, {}) | ||
|
|
||
| def inject(view: HostedResponseRequest, options: dict[str, Any]) -> dict[str, Any]: | ||
| return {**options, "additional_function_arguments": {"tenant_id": view.scope.user_id}} | ||
|
|
||
| await prepare_response_options(request, inject) | ||
| validate_request_options(request.options) | ||
| assert request.options["additional_function_arguments"] == {"tenant_id": "user"} | ||
|
|
||
|
|
||
| async def _post_json(server: ResponsesHostServer, payload: dict[str, Any]) -> httpx.Response: | ||
| transport = httpx.ASGITransport(app=server) | ||
| async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: | ||
| return await client.post("/responses", json=payload) | ||
|
|
||
|
|
||
| def _make_server(agent: Any, **kwargs: Any) -> ResponsesHostServer: | ||
| return ResponsesHostServer(agent, response_store=InMemoryResponseProvider(), **kwargs) | ||
|
|
||
|
|
||
| @pytest.mark.xdist_group("session_isolation") | ||
| class TestCallerCannotOverrideToolContext: | ||
| async def test_baseline_uses_trusted_tool_context(self, tenant_agent_factory: Callable[..., Any]) -> None: | ||
| seen: list[Any] = [] | ||
| server = _make_server(tenant_agent_factory(seen, approval_mode="never_require")) | ||
|
|
||
| response = await _post_json(server, {"input": "go", "stream": False}) | ||
|
|
||
| assert response.json()["status"] == "completed" | ||
| assert seen == [TRUSTED_TENANT_ID] | ||
|
|
||
| @pytest.mark.parametrize("stream", [False, True]) | ||
| @pytest.mark.parametrize("location", ["top_level", "extra_body"]) | ||
| async def test_forged_tool_context_is_rejected_before_any_tool_runs( | ||
| self, tenant_agent_factory: Callable[..., Any], location: Literal["top_level", "extra_body"], stream: bool | ||
| ) -> None: | ||
| seen: list[Any] = [] | ||
| agent = tenant_agent_factory(seen, approval_mode="never_require") | ||
| server = _make_server(agent) | ||
| payload: dict[str, Any] = {"input": "go", "stream": stream} | ||
| if location == "top_level": | ||
| payload.update(FORGED_TENANT_CONTEXT) | ||
| else: | ||
| payload["extra_body"] = FORGED_TENANT_CONTEXT | ||
|
|
||
| response = await _post_json(server, payload) | ||
|
|
||
| assert "additional_function_arguments" in response.text | ||
| assert "failed" in response.text | ||
| assert seen == [] | ||
| assert agent.client.calls == 0 | ||
|
|
||
| @pytest.mark.filterwarnings("ignore:Hosting WorkflowAgent through agent=.*:DeprecationWarning") | ||
| @pytest.mark.parametrize("location", ["top_level", "extra_body"]) | ||
| async def test_forged_tool_context_is_rejected_for_legacy_workflow_agents( | ||
| self, tenant_workflow_agent_factory: Callable[..., Any], location: Literal["top_level", "extra_body"] | ||
| ) -> None: | ||
| seen: list[Any] = [] | ||
| server = _make_server(tenant_workflow_agent_factory(seen)) | ||
| payload: dict[str, Any] = {"input": "go", "stream": False} | ||
| if location == "top_level": | ||
| payload.update(FORGED_TENANT_CONTEXT) | ||
| else: | ||
| payload["extra_body"] = FORGED_TENANT_CONTEXT | ||
|
|
||
| response = await _post_json(server, payload) | ||
|
|
||
| assert "additional_function_arguments" in response.text | ||
| assert response.json()["status"] == "failed" | ||
| assert seen == [] | ||
|
|
||
| @pytest.mark.filterwarnings("ignore:Hosting WorkflowAgent through agent=.*:DeprecationWarning") | ||
| async def test_legacy_workflow_agent_without_forged_context_still_runs( | ||
| self, tenant_workflow_agent_factory: Callable[..., Any] | ||
| ) -> None: | ||
| seen: list[Any] = [] | ||
| server = _make_server(tenant_workflow_agent_factory(seen)) | ||
|
|
||
| response = await _post_json(server, {"input": "go", "stream": False}) | ||
|
|
||
| assert response.json()["status"] == "completed" | ||
| assert seen == [TRUSTED_TENANT_ID] | ||
|
|
||
| async def test_hook_can_still_supply_trusted_tool_context(self, tenant_agent_factory: Callable[..., Any]) -> None: | ||
| seen: list[Any] = [] | ||
| server = _make_server( | ||
| tenant_agent_factory(seen, approval_mode="never_require"), | ||
| prepare_options=lambda request, options: { | ||
| **options, | ||
| "additional_function_arguments": {"tenant_id": f"from-hook:{request.scope.user_id}"}, | ||
| }, | ||
| ) | ||
|
|
||
| response = await _post_json(server, {"input": "go", "stream": False}) | ||
|
|
||
| assert response.json()["status"] == "completed" | ||
| assert len(seen) == 1 | ||
| assert str(seen[0]).startswith("from-hook:") | ||
|
|
||
| async def test_forged_context_on_approval_resume_is_rejected_without_consuming_approval( | ||
| self, tenant_agent_factory: Callable[..., Any] | ||
| ) -> None: | ||
| seen: list[Any] = [] | ||
| server = _make_server(tenant_agent_factory(seen, approval_mode="always_require")) | ||
|
|
||
| first = await _post_json(server, {"input": "go", "stream": False, "store": True}) | ||
| first_body = first.json() | ||
| approval_id = next(item["id"] for item in first_body["output"] if item["type"] == "mcp_approval_request") | ||
| assert seen == [] | ||
|
|
||
| def resume(**extra: Any) -> dict[str, Any]: | ||
| return { | ||
| "input": [{"type": "mcp_approval_response", "approval_request_id": approval_id, "approve": True}], | ||
| "previous_response_id": first_body["id"], | ||
| "stream": False, | ||
| "store": True, | ||
| **extra, | ||
| } | ||
|
|
||
| forged = await _post_json(server, resume(**FORGED_TENANT_CONTEXT)) | ||
| assert "additional_function_arguments" in forged.text | ||
| assert seen == [] | ||
|
|
||
| legitimate = await _post_json(server, resume()) | ||
| assert legitimate.json()["status"] == "completed" | ||
| assert seen == [TRUSTED_TENANT_ID] |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I miss in this, is the broader discussion on if WE should decide which options get rejected or the creator of the server, I think it should be the latter (which is also how the hosting packages work), because there are good reasons to either allow this to be passed through and handled in your code (maybe the user adds a middleware that unpacks this so that non-openAI responses parameters can be passed through through this mechanism. and in general so that as the creator of the server you can enhance the behavior over the existing protocol without having to go fully into the native mode or skipping FHA altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment Eduard van Valkenburg (@eavanvalkenburg)! In the spirit of "secure by default", I'm wondering if
additional_function_argumentsshould be rejected by default, but a hosted agent library setting can allow it for advanced developers where the author "opts-in" by overriding this default. And we clearly document the risk associated with opting in. Let's discuss further.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think in general we can do something like that for all the options, there are also instances where it makes sense to reject, alter or allow through any of the other arguments defined by the OAI responses spec and the developer should have control, and I fully agree with a defensive posture as default. Darren Cohen (@dargilco)