fix: prevent command injection in pip install #4274
Conversation
Replace os.system() with subprocess.run() using a list of arguments to prevent shell injection via malicious library names.
There was a problem hiding this comment.
Pull request overview
This PR fixes a command injection vulnerability in the CodeInterpreterTool by replacing os.system() with subprocess.run() using a list of arguments instead of shell string interpolation.
Changes:
- Replaced
os.system(f"pip install {library}")withsubprocess.run(["pip", "install", library], check=False)to prevent shell injection attacks via malicious library names
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Install libraries on the host machine | ||
| for library in libraries_used: | ||
| os.system(f"pip install {library}") # noqa: S605 | ||
| subprocess.run(["pip", "install", library], check=False) # noqa: S603, S607 |
There was a problem hiding this comment.
Consider using check=True instead of check=False to handle pip installation failures. With check=False, if a library installation fails, the code will continue execution and likely fail later with an import error when trying to use the library. Using check=True would raise a CalledProcessError immediately, which could be caught and returned with a clearer error message to the user. This pattern is used consistently elsewhere in the codebase (e.g., mcp_adapter.py:103, browserbase_load_tool.py:62, and many other tools).
2 participants
Replace os.system() with subprocess.run() using a list of arguments to prevent shell injection via malicious library names.