feat: implement tool call approval mechanism with dynamic code strategy#5398
feat: implement tool call approval mechanism with dynamic code strategy#5398
Conversation
Summary of ChangesHello @Soulter, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism for approving tool calls made by the agent. This feature provides an additional layer of security and user control by requiring explicit confirmation before potentially sensitive or impactful agent actions are executed. The primary implementation uses a dynamic code strategy, prompting the user to enter a generated code to proceed with a tool call, and includes comprehensive configuration options to tailor its behavior. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a tool call approval mechanism with a dynamic code strategy, which aims to enhance security by requiring user confirmation before executing certain tool calls. However, a critical security flaw has been identified in the implementation of the message handler within group chat scenarios. This flaw allows any user in a group to either approve a tool call (Broken Access Control) or accidentally/intentionally cancel it by sending any message (Denial of Service). It is strongly recommended to restrict the approval process to the original initiator of the request to mitigate these vulnerabilities.
| async def _handler( | ||
| controller: SessionController, incoming: AstrMessageEvent | ||
| ) -> None: | ||
| raw_input = (incoming.message_str or "").strip() | ||
| if _is_code_match( | ||
| expected=expected_code, | ||
| actual=raw_input, | ||
| case_sensitive=case_sensitive, | ||
| ): | ||
| if not controller.future.done(): | ||
| controller.future.set_result( | ||
| ToolCallApprovalResult(approved=True, reason="approved"), | ||
| ) | ||
| else: | ||
| if not controller.future.done(): | ||
| controller.future.set_result( | ||
| ToolCallApprovalResult( | ||
| approved=False, | ||
| reason="rejected", | ||
| detail=raw_input, | ||
| ) | ||
| ) | ||
| controller.stop() |
There was a problem hiding this comment.
The tool call approval handler is vulnerable to Denial of Service (DoS) and Broken Access Control in group chat environments.
- Denial of Service: The handler automatically rejects the tool call if ANY message is received in the session that does not match the expected code. In a group chat, any message from any user will trigger this rejection, causing the tool call to be cancelled. This allows any member of the group to prevent the bot from executing tools.
- Broken Access Control: The mechanism does not verify the identity of the user providing the approval code. Since the code is sent to the group, any member can see it and send it back to approve the tool call, bypassing the intent of 'user approval'.
Remediation: Modify the _handler to verify that the sender of the message is the same user who initiated the tool call request. Messages from other users should be ignored.
async def _handler(
controller: SessionController, incoming: AstrMessageEvent
) -> None:
if incoming.get_sender_id() != event.get_sender_id():
return
raw_input = (incoming.message_str or "").strip()
if _is_code_match(
expected=expected_code,
actual=raw_input,
case_sensitive=case_sensitive,
):
if not controller.future.done():
controller.future.set_result(
ToolCallApprovalResult(approved=True, reason="approved"),
)
else:
if not controller.future.done():
controller.future.set_result(
ToolCallApprovalResult(
approved=False,
reason="rejected",
detail=raw_input,
)
)
controller.stop()| timeout=timeout_seconds, | ||
| case_sensitive=case_sensitive, | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 |
There was a problem hiding this comment.
The except Exception as exc: # noqa: BLE001 is too broad. It's generally better to catch more specific exceptions to avoid masking unexpected errors. Consider catching more specific exceptions that _wait_for_code_input might raise, or at least asyncio.TimeoutError if that's the primary concern for register_wait.
| if parsed < minimum: | ||
| return minimum | ||
| return parsed | ||
| except Exception: # noqa: BLE001 |
There was a problem hiding this comment.
Similar to the previous comment, catching a broad Exception can hide bugs. It's better to catch ValueError or TypeError if int() conversion is the only expected failure, or specify (ValueError, TypeError) if both are possible. If other exceptions are possible, they should be handled explicitly.
1 participant
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
在智能体流水线中添加可配置的工具调用审批机制，使用动态代码确认策略。
New Features:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Add configurable tool call approval to the agent pipeline using a dynamic code confirmation strategy.
New Features:
Enhancements:
Tests: