-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add comprehensive tool approval system with configurable controls #59
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
Conversation
Implements a complete tool approval system that allows users to control when and how LLM tool calls are executed, with support for argument editing, per-tool configuration, and auto-approval settings. Key Features: - Tool approval manager with configurable approval policies - Inline toggle for auto-approve in tool approval messages - User-level and tool-level auto-approval settings - Argument editing with tracked changes and LLM context - Global force approval override for all tools - WebSocket-based approval flow with request/response handling - Comprehensive test coverage for approval manager and config - CodeQL data extensions for sanitizers Backend Changes: - Add ToolApprovalManager for centralized approval logic - Update agent loops (ReAct, Think-Act, Act) to support approval flow - Add approval endpoints and WebSocket message handlers - Integrate approval checks into tools mode runner - Add configuration management for tool approval settings Frontend Changes: - Add tool approval UI in chat area - Implement approval dialog with argument editing - Add user-level auto-approval toggle in settings - Track argument changes and provide context to LLM Documentation: - Complete implementation guide - E2E test scenarios - System architecture documentation Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
|
||
| request = self._pending_requests.get(tool_call_id) | ||
| if request is None: | ||
| logger.warning(f"Received approval response for unknown tool call: {sanitize_for_logging(tool_call_id)}") |
Check failure
Code scanning / CodeQL
Log Injection High
user-provided value
Copilot Autofix
AI 19 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| logger.warning(f"Available pending requests: {list(self._pending_requests.keys())}") | ||
| return False | ||
|
|
||
| logger.info(f"Found pending request for {sanitize_for_logging(tool_call_id)}, setting response") |
Check failure
Code scanning / CodeQL
Log Injection High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
To fully address the possibility of log confusion or ambiguity—even after stripping control characters—we should clearly delineate user input within log messages by wrapping it in a consistent delimiter (such as single quotes or square brackets). This ensures even tricky printable (but weird) Unicode, leading/trailing spaces, or empty strings, cannot create confusion with adjacent log message text. No changes are needed to the control-character stripping logic, but the log message itself should be changed to wrap sanitize_for_logging(tool_call_id) in quotes.
The only necessary edit is in backend/application/chat/approval_manager.py, at the log message on line 120, to update:
logger.info(f"Found pending request for {sanitize_for_logging(tool_call_id)}, setting response")to
logger.info(f"Found pending request for '{sanitize_for_logging(tool_call_id)}', setting response")This follows the best practice of marking user input and cannot break existing functionality.
-
Copy modified line R120
| @@ -117,7 +117,7 @@ | ||
| logger.warning(f"Available pending requests: {list(self._pending_requests.keys())}") | ||
| return False | ||
|
|
||
| logger.info(f"Found pending request for {sanitize_for_logging(tool_call_id)}, setting response") | ||
| logger.info(f"Found pending request for '{sanitize_for_logging(tool_call_id)}', setting response") | ||
| request.set_response(approved, arguments, reason) | ||
| # Keep the request in the dict for a bit to avoid race conditions | ||
| # It will be cleaned up later |
| request.set_response(approved, arguments, reason) | ||
| # Keep the request in the dict for a bit to avoid race conditions | ||
| # It will be cleaned up later | ||
| logger.info(f"Approval response handled for tool {sanitize_for_logging(request.tool_name)}: approved={approved}") |
Check failure
Code scanning / CodeQL
Log Injection High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
The best way to fix this problem is to ensure that all user-controlled or potentially tainted values being logged are passed through an appropriate sanitization function prior to inclusion in the log message. In this codebase, the existing sanitize_for_logging function (imported from core.utils) should be used for consistency and effectiveness.
Specifically:
- In
backend/application/chat/approval_manager.py, on line 124, the log entry that referencesapprovedshould wrapapprovedinsanitize_for_logging, just as is done fortool_name. - No changes are required to the rest of the logging/statements, given that similar fields already use
sanitize_for_logging. - No new imports or helpers are needed, as
sanitize_for_loggingis already present.
-
Copy modified line R124
| @@ -121,7 +121,7 @@ | ||
| request.set_response(approved, arguments, reason) | ||
| # Keep the request in the dict for a bit to avoid race conditions | ||
| # It will be cleaned up later | ||
| logger.info(f"Approval response handled for tool {sanitize_for_logging(request.tool_name)}: approved={approved}") | ||
| logger.info(f"Approval response handled for tool {sanitize_for_logging(request.tool_name)}: approved={sanitize_for_logging(approved)}") | ||
| return True | ||
|
|
||
| def cleanup_request(self, tool_call_id: str): |
| task1 = asyncio.create_task(approve1()) | ||
| response1 = await request1.wait_for_response(timeout=1.0) | ||
| manager.cleanup_request("seq_1") | ||
| await task1 |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
Copilot Autofix
AI 19 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| task2 = asyncio.create_task(approve2()) | ||
| response2 = await request2.wait_for_response(timeout=1.0) | ||
| manager.cleanup_request("seq_2") | ||
| await task2 |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
Copilot Autofix
AI 19 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Fixed bug where REQUIRE_TOOL_APPROVAL_BY_DEFAULT was treated as admin-mandated instead of user-level, preventing the inline auto-approve toggle from appearing in the UI. Changes: - Updated requires_approval() to return admin_required=False for default approval requirement, allowing users to toggle via inline UI - Only FORCE_TOOL_APPROVAL_GLOBALLY and per-tool require_approval=true now set admin_required=True (user cannot override) - Updated test expectations to match corrected behavior - Enhanced docstring to clarify admin vs user-level approval logic This ensures the inline toggle appears for all tool approvals except those explicitly marked as admin-required. Fixes requirement #4 from fn-approvals.md: "The user can easily toggle auto approval via the UI settings panel OR when a tool approval is requested an an inline ui element that is adjacent to the approval requests." Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unused UUID import in approval_manager.py - Sanitize tool_call_id keys and approval results in logs to prevent data leakage - Clean up test code by removing unnecessary await approval_task statements - Ensure all logged data is sanitized using sanitize_for_logging utility
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
- Switch websocket logging from f-string to parameterized format for better performance and security - Remove unused approval_task variables in test_approval_manager.py to eliminate linter warnings and improve code clarity
- Applied sanitize_for_logging to user inputs, file paths, and config data in log messages across chat service, MCP tools, RAG client, and admin routes - This prevents potential exposure of sensitive information in application logs, enhancing security and compliance
- Updated `sanitize_for_logging` in `backend/core/utils.py` to remove Unicode LINE SEPARATOR (U+2028) and PARAGRAPH SEPARATOR (U+2029) in addition to ASCII control characters, preventing log injection attacks and corruption from these characters. - Modified logging format in `backend/main.py` to add brackets around message_type and keys for improved readability. - Added comprehensive tests in `backend/tests/test_core_utils.py` to verify removal of Unicode separators, including edge cases with mixed characters.
Implements a complete tool approval system that allows users to control when and how LLM tool calls are executed, with support for argument editing, per-tool configuration, and auto-approval settings.
Key Features:
Backend Changes:
Frontend Changes:
Documentation:
Generated with Claude Code