fix(security): extend action type coverage in security check#12870
Conversation
FileWriteAction, BrowseURLAction, and MCPAction are not passed to the SecurityAnalyzer when confirmation_mode is enabled. This means these action types bypass security analysis entirely, allowing potentially malicious file writes, URL navigation, and MCP tool calls to execute without review. Add the missing action types to the confirmation_mode security check so they are evaluated by the configured SecurityAnalyzer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
all-hands-bot
left a comment
There was a problem hiding this comment.
Good security fix! The PR correctly identifies and addresses the gap in security coverage. However, this type-checking approach is verbose and error-prone.
| or type(action) is FileReadAction | ||
| or type(action) is FileWriteAction | ||
| or type(action) is MCPAction | ||
| ): |
There was a problem hiding this comment.
🟡 Suggestion: This approach is verbose and error-prone. Consider refactoring to check for the security_risk attribute instead of listing specific types. This would be more maintainable and automatically include any future actions with security_risk attributes without requiring code changes here.
| ): | |
| if self.state.confirmation_mode and hasattr(action, "security_risk"): |
There was a problem hiding this comment.
Good suggestion — the hasattr approach is cleaner and would automatically cover future action types. I noticed this file is marked Legacy V0 (scheduled for removal April 2026), so the maintainability benefit may be limited here. Happy to adopt the hasattr pattern if you'd prefer, or keep the explicit approach to minimize diff in legacy code. Your call.
all-hands-bot
left a comment
There was a problem hiding this comment.
Additional feedback on testing coverage.
| or type(action) is FileEditAction | ||
| or type(action) is FileReadAction | ||
| or type(action) is FileWriteAction | ||
| or type(action) is MCPAction |
There was a problem hiding this comment.
🟠 Important: FileWriteAction is particularly critical from a security perspective (it can overwrite sensitive files like SSH keys, config files, etc.). Consider adding test cases in tests/unit/security/test_security.py to verify that FileWriteAction, BrowseURLAction, and MCPAction all trigger security analysis when confirmation_mode is enabled. This would prevent regression if this code is refactored in the future.
There was a problem hiding this comment.
Agreed that test coverage here would be valuable. I noticed there aren't currently any confirmation_mode tests in test_security.py, and this code path is marked Legacy V0 (removal April 2026). Would you like me to add tests covering all action types in the confirmation_mode check, or would you prefer to keep the scope minimal given the V0 timeline? Happy to go either direction.
|
LGTM! This is a critical security fix for GHSA-rx6-2ghm-27gv. The three missing action types (FileWriteAction, BrowseURLAction, MCPAction) are now properly included in the confirmation_mode security check, ensuring they go through the SecurityAnalyzer like other actions. The change is minimal and focused, exactly what a security fix should be. +1 for merge. |
enyst
left a comment
There was a problem hiding this comment.
Thank you for looking into this! To be fair, while we really don't intend to change behavior of these files since they are legacy, this looks minimal and safe.
…ds#12870) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Engel Nyst <engel.nyst@gmail.com>
Summary of PR
FileWriteAction,BrowseURLAction, andMCPActionare not included in theconfirmation_modesecurity check inagent_controller.py. When confirmation mode is enabled, these action types bypass theSecurityAnalyzerentirely.Adds the three missing action types to the security check so they are evaluated by the configured
SecurityAnalyzer.Changes
openhands/controller/agent_controller.py: Import and addFileWriteAction,BrowseURLAction,MCPActionto the confirmation_mode type checkIdentified via GHSA-rxg6-2ghm-27gv.
Demo Screenshots/Videos
N/A — backend security logic, no UI impact.
Change Type
Checklist
Fixes
Related to GHSA-rxg6-2ghm-27gv
Release Notes