Python: Enforce approval_mode in FunctionTool.invoke() - #5494
Python: Enforce approval_mode in FunctionTool.invoke()#5494chetantoshniwal wants to merge 1 commit into
Conversation
FunctionTool.invoke() previously ignored approval_mode='always_require', allowing direct callers (e.g., Claude/Copilot integrations) to bypass the human approval gate that only existed in _try_execute_function_calls(). Changes: - Add ToolApprovalRequiredException to exceptions.py - Add approval_mode check at the top of FunctionTool.invoke() - Add _approved parameter (default False) to invoke() signature and overloads - Pass _approved=True from _auto_invoke_function() call sites (both direct and middleware pipeline paths) - Export ToolApprovalRequiredException from agent_framework package - Add comprehensive tests for the new guard Based on upstream microsoft/agent-framework main branch. Fixes MSRC Case 109288 (VULN-176822) Co-authored-by: Azure SRE Agent <noreply@microsoft.com>
github-actions
Bot
changed the title
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✗ Correctness
The core approval-gate logic is correct:
invoke()properly checksapproval_mode == 'always_require'before the_approvedflag, both auto-invocation paths pass_approved=True, and the new exception inherits fromToolException. Two issues found: (1)ToolApprovalRequiredExceptionis inserted out of alphabetical order in__all__, breaking the clearly maintained sorted convention; (2) a test usesapproval_mode="auto"which is not a validApprovalModevalue (Literal["always_require", "never_require"]per line 93 of_tools.py), testing behavior on an undeclared mode.
✓ Security Reliability
This PR adds a defense-in-depth approval gate to FunctionTool.invoke() that raises ToolApprovalRequiredException when a tool with approval_mode='always_require' is called without the _approved flag. The real approval gate lives in _try_execute_function_calls (lines 1641-1672), which intercepts approval-required calls before execution and returns approval requests to the user. The _approved flag on invoke() is a secondary guard against direct calers bypassing the pipeline. The implementation is sound: the auto-invoke pipeline correctly passes _approved=True in both the direct (line 1522 in diff) and middleware (line 1554 in diff) paths, and _try_execute_function_calls already handles the approval flow upstream. No security or reliability issues found.
✓ Test Coverage
The new approval gate mechanism on FunctionTool.invoke() is well-tested at the unit level, with tests covering the blocked case, the _approved=True bypass, default/auto modes, and the call escape hatch. The pipeline integration path (_auto_invoke_function passing _approved=True) is already covered by existing tests in test_function_invocation_logic.py (e.g., test_approved_function_call_successful_execution). No bugs or missing critical coverage found. One minor suggestion for improving test robustness.
✓ Design Approach
The change does not enforce approval at the real tool execution boundary. It adds a new
_approvedboolean to the publicinvoke()API, but that flag is entirely caller-controlled and the underlyingFunctionTool.__call__path still executesapproval_mode="always_require"tools without any check. Because the framework already has a first-class approval flow based onfunction_approval_request/function_approval_responsecontent, this patch looks like a symptom-level guard one call path rather than a robust approval model.
Suggestions
- Enforce the approval policy in the shared execution path (
_tools.py:511-538) rather than only ininvoke(). CurrentlyFunctionTool.__call__executesapproval_mode='always_require'tools without any check, so alternative integrations can accidentally skip the approval contract.
Automated review by chetantoshniwal's agents
|
I would prefer that we fix this in Claude and Copilot, because this makes it seem like a fix, that is not a actual fix, this line in the docstring gives away the game, so this adds a new parameter which can be spoofed with ease once you have access to code: since there is nothing preventing you from calling with |
Evan Mattson (moonbox3)
changed the title
Tools declared with approval_mode="always_require" were bypassed by the ClaudeAgent and GitHubCopilotAgent because their SDK-managed tool-calling loops invoke FunctionTool.invoke() directly via package-supplied handlers, skipping the standard _try_execute_function_calls approval gate. Per discussion on microsoft#5494, the fix lives in the agents (not in FunctionTool): any flag added to the tool itself can be spoofed by code with the same level of access, so the security boundary is the agent that owns the tool-calling loop. - Add on_function_approval option to ClaudeAgentOptions and GitHubCopilotOptions. Callback receives a FunctionCallContent describing the pending call and returns bool (sync or async). - Gate FunctionTool.invoke() inside each agent's existing tool-handler closure when approval_mode == "always_require". Default policy is deny; callbacks that raise also deny safely. - Deny path returns a tool-error to the model (Claude: text content; Copilot: ToolResult(result_type="failure", error="approval_denied")) so the LLM can react gracefully instead of silently failing. - Tests for both agents covering: deny by default, sync False, sync True, async True, callback-raises -> deny, no-op for never_require tools. - Samples demonstrating sync, async, and deny-by-default flows for both agents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…crosoft#5562) * Python: Enforce approval_mode in Claude and GitHub Copilot agents Tools declared with approval_mode="always_require" were bypassed by the ClaudeAgent and GitHubCopilotAgent because their SDK-managed tool-calling loops invoke FunctionTool.invoke() directly via package-supplied handlers, skipping the standard _try_execute_function_calls approval gate. Per discussion on microsoft#5494, the fix lives in the agents (not in FunctionTool): any flag added to the tool itself can be spoofed by code with the same level of access, so the security boundary is the agent that owns the tool-calling loop. - Add on_function_approval option to ClaudeAgentOptions and GitHubCopilotOptions. Callback receives a FunctionCallContent describing the pending call and returns bool (sync or async). - Gate FunctionTool.invoke() inside each agent's existing tool-handler closure when approval_mode == "always_require". Default policy is deny; callbacks that raise also deny safely. - Deny path returns a tool-error to the model (Claude: text content; Copilot: ToolResult(result_type="failure", error="approval_denied")) so the LLM can react gracefully instead of silently failing. - Tests for both agents covering: deny by default, sync False, sync True, async True, callback-raises -> deny, no-op for never_require tools. - Samples demonstrating sync, async, and deny-by-default flows for both agents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review: preserve empty arg dicts, reject runtime approval override - _resolve_function_approval no longer collapses {} into None when building the FunctionCallContent passed to the callback (Claude + Copilot). - Claude _apply_runtime_options and Copilot _run_impl/_stream_updates now raise ValueError if on_function_approval is supplied via per-run options, instead of silently ignoring it. Approval policy must be set at agent construction time. - Drop unnecessary # type: ignore[attr-defined] on Content.name/.arguments in samples (Content is a unified class with both attributes defined). - Add regression tests for the new runtime-options validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * warning when non callback handler and approval needed --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 participants
Motivation and Context
FunctionTool.invoke() previously ignored approval_mode='always_require', allowing direct callers (e.g., Claude/Copilot integrations) to bypass the human approval gate that only existed in _try_execute_function_calls().
Description
Contribution Checklist