-
Notifications
You must be signed in to change notification settings - Fork 5
Fn call approval roll up #58
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
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
… to require approval Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
…LLM context Add tracking for user-edited tool arguments during approval process. When arguments are modified, prepend a notification to the tool result content to inform the LLM of the changes, ensuring responses reflect the user's true intent. Also, update websocket handling to track active chat tasks for better approval response management during ongoing chat processing. This improves accuracy and user control in tool executions.
- Add REQUIRE_TOOL_APPROVAL_BY_DEFAULT env var to .env.example - Update tool_utils.py to track display_args for accurate edit detection using JSON comparison - Extend MCPServerConfig with require_approval and allow_edit lists - Add require_tool_approval_by_default to AppSettings and update config loading to merge mcp.json with env vars This enables per-tool approval requirements and argument editing capabilities, improving user control over agent tool executions.
- Introduce a toggle button to enable/disable auto-approve for non-admin tool calls directly from the message UI - Conditionally hide approval/rejection controls when auto-approve is enabled and admin approval is not required - Update badge styling to reflect auto-approved status - Enhance user control over tool approvals without navigating to settings
- Add test_multiple_concurrent_approvals to verify handling of multiple simultaneous requests - Add test_approval_with_no_arguments_change to ensure arguments are preserved when unchanged - Add test_double_response_handling to prevent issues from duplicate responses - Add test_rejection_with_empty_reason to handle rejections without specified reasons - Fix import path by removing redundant 'backend.' prefix for consistency These tests enhance coverage for concurrent operations and error handling in the approval workflow.
Add input sanitization using `sanitize_for_logging` to prevent potential log injection attacks and ensure sensitive data is safely logged in approval-related messages across approval_manager.py and main.py. This improves security by wrapping variables like tool_call_id, tool_name, and message_type in logging statements.
- Removed unused imports (e.g., pytest, json, tempfile) from test files to reduce dependencies and clean up code. - Eliminated unused 'active_chat_task' variable in main.py websocket handler for simplicity. - Adjusted indentation and comments in tool_utils.py, and fixed trailing newline in files manifest. - Updated frontend test imports by removing unused 'waitFor' to streamline testing setup. These changes improve code maintainability and reduce clutter without altering functionality.
Add a new FORCE_TOOL_APPROVAL_GLOBALLY setting that allows admins to enforce approval for every tool call, overriding per-tool and default approval configurations. This enhances security by providing a blanket policy control, useful for strict environments where all tool usage must be reviewed. Updated .env.example, tool_utils.py logic, and config_manager.py to support this feature.
| Returns: | ||
| True if request was found and handled, False otherwise | ||
| """ | ||
| logger.info(f"handle_approval_response called: tool_call_id={sanitize_for_logging(tool_call_id)}, approved={approved}") |
Check failure
Code scanning / CodeQL
Log Injection High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To fix the issue, any user-provided input logged by the application must be sanitized to prevent log injection. In the affected code, the handle_approval_response method logs two fields: tool_call_id (already safely sanitized), and approved, which can (in practice) be any type due to lack of prior validation. We should ensure that approved is passed through the same sanitize_for_logging function before logging, converting it to a string if needed. This change should be implemented in backend/application/chat/approval_manager.py, specifically on line 112, replacing direct use of approved with sanitize_for_logging(approved).
The fix simply updates the log statement, so no new imports or methods are needed. The change should preserve original code behavior.
-
Copy modified line R112
| @@ -109,7 +109,7 @@ | ||
| Returns: | ||
| True if request was found and handled, False otherwise | ||
| """ | ||
| logger.info(f"handle_approval_response called: tool_call_id={sanitize_for_logging(tool_call_id)}, approved={approved}") | ||
| logger.info(f"handle_approval_response called: tool_call_id={sanitize_for_logging(tool_call_id)}, approved={sanitize_for_logging(approved)}") | ||
| logger.info(f"Pending requests: {list(self._pending_requests.keys())}") | ||
|
|
||
| request = self._pending_requests.get(tool_call_id) |
|
|
||
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To robustly prevent log injection, ensure the utility function sanitize_for_logging comprehensively strips all characters that could inject unwanted newlines into log entries. In addition to the current ASCII control characters stripped by the regex, you should explicitly remove Unicode newline characters (such as \u2028 and \u2029), since the current regular expression does not match these.
The best fix is to modify sanitize_for_logging in backend/core/utils.py so it additionally removes Unicode line separator characters (\u2028, \u2029) and standard newlines and carriage returns (which are already covered by the regex but for clarity/robustness can be handled explicitly as well).
Only the utility function needs changing: all log invocations already sanitize their parameters via this function, so strengthening the sanitization will fix all downstream uses, including in backend/application/chat/approval_manager.py.
-
Copy modified line R13 -
Copy modified line R16 -
Copy modified lines R18-R21 -
Copy modified line R28 -
Copy modified lines R31-R32 -
Copy modified lines R31-R33
| @@ -10,26 +10,26 @@ | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| _CONTROL_CHARS_RE = re.compile(r'[\x00-\x1f\x7f-\x9f]') | ||
|
|
||
| _LOG_FORGERY_CHARS_RE = re.compile(r'[\u2028\u2029]') | ||
| def sanitize_for_logging(value: Any) -> str: | ||
| """ | ||
| Sanitize a value for safe logging by removing control characters. | ||
| Sanitize a value for safe logging by removing control characters and Unicode line breaks. | ||
|
|
||
| Removes ASCII control characters (C0 and C1 ranges) to prevent log injection | ||
| attacks and log corruption. This includes characters like newlines, tabs, | ||
| escape sequences, and other non-printable characters that could be used to | ||
| manipulate log output or inject fake log entries. | ||
| Removes ASCII control characters (C0 and C1 ranges) and Unicode line separators | ||
| to prevent log injection attacks and log corruption. This includes characters like | ||
| newlines, tabs, escape sequences, U+2028 (Line Separator), U+2029 (Paragraph Separator), and other | ||
| non-printable characters that could be used to manipulate log output or inject fake log entries. | ||
|
|
||
| Args: | ||
| value: Any value to sanitize. If not a string, it will be converted | ||
| to string representation first. | ||
|
|
||
| Returns: | ||
| str: Sanitized string with all control characters removed. | ||
| str: Sanitized string with all control characters and log-forgery relevant characters removed. | ||
|
|
||
| Examples: | ||
| >>> sanitize_for_logging("Hello\\nWorld") | ||
| 'HelloWorld' | ||
| >>> sanitize_for_logging("Hello\\nWorld\\u2028Test") | ||
| 'HelloWorldTest' | ||
| >>> sanitize_for_logging("Test\\x1b[31mRed\\x1b[0m") | ||
| 'TestRed' | ||
| >>> sanitize_for_logging(123) | ||
| @@ -39,10 +28,11 @@ | ||
| return '' | ||
| if not isinstance(value, str): | ||
| value = str(value) | ||
| return _CONTROL_CHARS_RE.sub('', value) | ||
| value = _CONTROL_CHARS_RE.sub('', value) | ||
| value = _LOG_FORGERY_CHARS_RE.sub('', value) | ||
| return value | ||
|
|
||
|
|
||
|
|
||
| async def get_current_user(request: Request) -> str: | ||
| """Get current user from request state (set by middleware).""" | ||
| return getattr(request.state, 'user_email', '[email protected]') |
| 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
Copilot Autofix
AI 21 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.
| 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 21 days ago
The problem arises because the approved parameter, intended to be a boolean, is logged without sanitization. Due to dynamic typing and the nature of the WebSocket payload, a user could craft a value (for example, a string containing newlines) that results in log injection—potentially creating forged or misleading log entries.
General fix:
All user-provided (or indirectly user-controllable) data written to log entries must be sanitized to remove or escape log-forging characters (newline, carriage return, etc.).
Detailed fix:
- Wrap the
approvedvalue with the same or equivalent sanitization method used for other fields (sanitize_for_logging()). - Change the relevant log statement on line 125 of
backend/application/chat/approval_manager.pyfrom:to: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)}")
- No additional imports or helper methods are needed, since
sanitize_for_loggingis already imported and used.
No changes are needed in backend/main.py because logging is already wrapped with sanitize_for_logging there.
-
Copy modified line R125
| @@ -122,7 +122,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): |
|
|
||
|
|
||
| # Debug: Log ALL incoming messages | ||
| logger.info(f"WS RECEIVED message_type={message_type}, data keys={list(data.keys())}") |
Check failure
Code scanning / CodeQL
Log Injection High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To fix the log injection vulnerability, all user-controlled data interpolated into log messages must be sanitized to remove log control characters that can cause log forging—most importantly, line breaks (\n and \r). The simplest approach here is to replace or remove these characters from any input field that originates from the WebSocket before using them in a log message.
The fix should be applied in backend/main.py at the point where message_type is logged (line 228). To ensure safety and maintain code clarity, create a small helper function named sanitize_for_logging elsewhere in the file, which replaces \n/\r with empty strings or another safe character, and call this function on message_type when logging. All other functionality remains unchanged.
Implementation steps:
- Locate the log statement on line 228 and wrap
message_typewith a sanitization function. - Define the sanitization function in backend/main.py.
- No new dependencies are required; use built-in string methods.
- Ensure sufficient context is provided for the log message.
-
Copy modified line R228
| @@ -225,7 +225,7 @@ | ||
| message_type = data.get("type") | ||
|
|
||
| # Debug: Log ALL incoming messages | ||
| logger.info(f"WS RECEIVED message_type={message_type}, data keys={list(data.keys())}") | ||
| logger.info(f"WS RECEIVED message_type={sanitize_for_logging(message_type)}, data keys={list(data.keys())}") | ||
|
|
||
| if message_type == "chat": | ||
| # Handle chat message in background so we can still receive approval responses |
| assert response["approved"] is True | ||
| assert response["arguments"] == original_args | ||
|
|
||
| await approval_task |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
Copilot Autofix
AI 21 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.
| assert response["approved"] is False | ||
| assert response.get("reason") is None or response.get("reason") == "" | ||
|
|
||
| await approval_task |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To address the detected "statement has no effect" error at line 306 (await approval_task), the statement should be updated to assign its (discarded) result to a dummy variable, commonly named _. This makes the intention clear that any return value or exception from the task is being captured (or purposely ignored), and the statement no longer appears to have no effect. Specifically, in backend/tests/test_approval_manager.py, line 306 should be changed to _ = await approval_task.
-
Copy modified line R306
| @@ -303,7 +303,7 @@ | ||
| assert response["approved"] is False | ||
| assert response.get("reason") is None or response.get("reason") == "" | ||
|
|
||
| await approval_task | ||
| _ = await approval_task | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_allow_edit_false(self): |
| assert response["arguments"]["nested"]["level1"]["level2"][1] == "modified_item" | ||
| assert len(response["arguments"]["numbers"]) == 6 | ||
|
|
||
| await approval_task |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
Copilot Autofix
AI 21 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.
| 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 21 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 21 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.
No description provided.