Python: Enforce approval_mode in FunctionTool.invoke()#5494
Python: Enforce approval_mode in FunctionTool.invoke()#5494chetantoshniwal wants to merge 1 commit intomicrosoft:mainfrom
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>
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
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 |
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