fix(security): prevent command injection in GenericWindowHandler.launch()#1131
fix(security): prevent command injection in GenericWindowHandler.launch()#1131rookweb1 wants to merge 1 commit intotrycua:mainfrom
Conversation
…ch() Use shlex.split() to safely parse command strings instead of passing directly to subprocess with shell=True. This prevents command injection attacks where attackers could inject shell commands through the app parameter. Fixes issue trycua#1097 Co-authored-by: Rook <tydenweb@gmail.com>
|
@rookweb1 is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis change modifies the command execution logic in the generic window handler to improve security. When no explicit arguments are provided, the command is now tokenized using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/python/computer-server/computer_server/handlers/generic.py (1)
116-117: Use platform-specificshlexparsing to handle Windows and POSIX command strings correctly.The default
posix=Truemode treats backslashes as escape characters, which corrupts Windows paths (e.g.,C:\Program Files). On Windows, switch toposix=False; on POSIX systems, keep the default. Also guard against empty command splits.Suggested adjustment
- cmd_parts = shlex.split(app) - proc = subprocess.Popen(cmd_parts) + cmd_parts = shlex.split(app, posix=(platform.system().lower() != "windows")) + if not cmd_parts: + return {"success": False, "error": "Empty command"} + proc = subprocess.Popen(cmd_parts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/computer-server/computer_server/handlers/generic.py` around lines 116 - 117, The current parsing uses shlex.split(app) which uses posix=True by default and breaks Windows paths; update the parsing in the block around cmd_parts and subprocess.Popen to choose shlex.split(app, posix=False) when running on Windows (e.g., sys.platform.startswith("win") or os.name == "nt") and posix=True otherwise, then guard the result so if cmd_parts is empty raise/log a clear error instead of calling subprocess.Popen([]); use the existing variables cmd_parts, app and the subprocess.Popen call to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/python/computer-server/computer_server/handlers/generic.py`:
- Around line 116-117: The current parsing uses shlex.split(app) which uses
posix=True by default and breaks Windows paths; update the parsing in the block
around cmd_parts and subprocess.Popen to choose shlex.split(app, posix=False)
when running on Windows (e.g., sys.platform.startswith("win") or os.name ==
"nt") and posix=True otherwise, then guard the result so if cmd_parts is empty
raise/log a clear error instead of calling subprocess.Popen([]); use the
existing variables cmd_parts, app and the subprocess.Popen call to locate and
change the code.
|
Hi! This PR fixes a security vulnerability (command injection) reported in issue #1097. The fix uses shlex.split() instead of shell=True to prevent RCE. Let me know if you need any adjustments! |
1 participant
Summary
Fixes command injection vulnerability reported in issue #1097.
Problem
The
GenericWindowHandler.launch()method was passing theappparameter directly tosubprocess.Popen()withshell=True, allowing arbitrary shell command execution.Solution
Use
shlex.split()to safely parse the command string:Impact
AI-assisted contribution
Summary by CodeRabbit