🛡️ Sentinel: [CRITICAL] Fix shell=True vulnerability on Windows#83
🛡️ Sentinel: [CRITICAL] Fix shell=True vulnerability on Windows#83haseeb-heaven wants to merge 1 commit into
Conversation
🚨 Severity: CRITICAL 💡 Vulnerability: Command injection risk due to `shell=True` usage in `subprocess` calls on Windows. 🎯 Impact: Execution of arbitrary commands. 🔧 Fix: - In `libs/package_manager.py`, replaced `shell=True` with `shutil.which` to safely resolve absolute paths of `.cmd` and `.bat` extensions, allowing execution with `shell=False`. - Updated `safe_pattern` in `libs/package_manager.py` to allow path separators and drive letters. - In `libs/utility_manager.py`, replaced `subprocess.call` with `os.startfile` to avoid shell execution. ✅ Verification: Ran `pytest` suite locally successfully.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
📝 WalkthroughWalkthroughThis PR hardens Windows subprocess execution by eliminating ChangesWindows Subprocess Safety Hardening
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_interpreter.py (1)
1783-1786: ⚡ Quick winMake the Windows safety test assert actual executable replacement.
Using
return_value="pip"won’t fail if the assignment at Line 33 is removed. Mock a distinct absolute path and assert that path is passed tocheck_call.Suggested patch
`@patch`("libs.package_manager.os.name", "nt") def test_windows_safe_args_pass_validation(self): with patch("subprocess.check_call", return_value=0) as mock_call: - with patch("shutil.which", return_value="pip"): + with patch("shutil.which", return_value=r"C:\Python\Scripts\pip.exe"): result = self.pm._run_command(["pip", "install", "requests"]) # On Windows, args are passed directly with shell=False - mock_call.assert_called_once_with(["pip", "install", "requests"], shell=False) + mock_call.assert_called_once_with([r"C:\Python\Scripts\pip.exe", "install", "requests"], shell=False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_interpreter.py` around lines 1783 - 1786, Replace the shutil.which return_value used in the Windows-safety test with a distinct absolute executable path (e.g. "C:\\Python\\Scripts\\pip.exe" or "/usr/bin/pip3") when patching shutil.which; then update the assertion for pm._run_command (the mocked check_call/mock_call) to assert it was called with that absolute path as the first element (e.g. assert_called_once_with([absolute_path, "install", "requests"], shell=False)) so the test verifies actual executable replacement instead of accepting "pip".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/package_manager.py`:
- Around line 30-31: Guard against an empty args before accessing args[0]: check
that args is truthy and len(args) > 0 prior to assigning cmd = args[0]; if
empty, raise a clear ValidationError/ValueError or return an explicit error
instead of letting an IndexError bubble up. After the check, keep the existing
resolved_cmd = shutil.which(cmd) call and ensure any error path logs or returns
consistent error information so callers can handle missing command arguments
reliably.
- Around line 23-35: The current argument validation (safe_pattern) still allows
cmd.exe metacharacters which are dangerous when args[0] resolves to a .cmd/.bat;
update the logic in the block around safe_pattern, args, resolved_cmd and
subprocess.check_call to perform an additional deny-check when running on
Windows or when resolved_cmd endswith '.cmd' or '.bat' — scan all arguments (or
at least subsequent args) for cmd metacharacters (e.g. & | < > ^ % ! ( ) ) and
raise ValueError if any are present before calling subprocess.check_call; keep
the existing safe_pattern validation but add this explicit deny-list branch
referencing resolved_cmd and args to prevent cmd.exe argument parsing exploits.
---
Nitpick comments:
In `@tests/test_interpreter.py`:
- Around line 1783-1786: Replace the shutil.which return_value used in the
Windows-safety test with a distinct absolute executable path (e.g.
"C:\\Python\\Scripts\\pip.exe" or "/usr/bin/pip3") when patching shutil.which;
then update the assertion for pm._run_command (the mocked check_call/mock_call)
to assert it was called with that absolute path as the first element (e.g.
assert_called_once_with([absolute_path, "install", "requests"], shell=False)) so
the test verifies actual executable replacement instead of accepting "pip".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e94f8bd8-c615-4853-a2d4-cb69e62ffdaf
📒 Files selected for processing (4)
.jules/sentinel.mdlibs/package_manager.pylibs/utility_manager.pytests/test_interpreter.py
| safe_pattern = re.compile(r'^[a-zA-Z0-9._\-\[\]=<>!,\/\\@:]+$') | ||
| for arg in args: | ||
| if not isinstance(arg, str) or not safe_pattern.match(arg): | ||
| raise ValueError(f"Unsafe command argument: {arg}") | ||
| # Convert args list to a single command string for shell=True | ||
| command_string = subprocess.list2cmdline(args) | ||
| return subprocess.check_call(command_string, shell=True) | ||
|
|
||
| # Use shutil.which to resolve .cmd/.bat extensions without shell=True | ||
| import shutil | ||
| cmd = args[0] | ||
| resolved_cmd = shutil.which(cmd) | ||
| if resolved_cmd: | ||
| args[0] = resolved_cmd | ||
|
|
||
| return subprocess.check_call(args, shell=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python's subprocess module on Windows, when shell=False and the executable is a .cmd/.bat file, are arguments still parsed by cmd.exe rules? Which metacharacters remain unsafe?
💡 Result:
When you launch a.bat or.cmd file in Python's subprocess module on Windows with shell=False, the file is not executed by cmd.exe directly [1][2]. Instead, Windows recognizes these as batch files and automatically spawns the system command processor (cmd.exe) to interpret them [1][2]. Because this happens at the operating system level, arguments are passed to this automatically invoked shell without the standard escaping/quoting that Python performs when it manually manages shell=True [1][2]. Consequently, arguments are parsed according to cmd.exe rules, making them vulnerable to command injection if they contain special metacharacters [1][2]. Unsafe metacharacters include any character that has special meaning to cmd.exe [3]. These include: - & (command chaining) - | (piping) - < or > (redirection) - ^ (escape character) - % (environment variable expansion) - ( and) (parentheses for grouping) If you must pass untrusted input to a batch file, it is generally safer to use shell=True and handle the command construction yourself, which allows Python to apply appropriate escaping, or strictly sanitize/validate the input [1][2].
Citations:
- 1: https://docs.python.org/3/library/subprocess.html
- 2: https://github.com/python/cpython/blob/0274d830/Doc/library/subprocess.rst
- 3: https://stackoverflow.com/questions/43990219/how-does-subprocess-call-work-with-shell-false
Harden .cmd/.bat argument handling: deny cmd.exe metacharacters before execution
On Windows, when args[0] resolves to a .cmd/.bat, cmd.exe parses the provided arguments even with shell=False. The current safe_pattern allows <, >, and !, which are cmd.exe metacharacters; add a deny-check for batch-target arguments (e.g., &|<>^%!()) before subprocess.check_call.
Suggested patch
# Use shutil.which to resolve .cmd/.bat extensions without shell=True
import shutil
cmd = args[0]
resolved_cmd = shutil.which(cmd)
if resolved_cmd:
args[0] = resolved_cmd
+
+ # Additional hardening for batch targets on Windows.
+ if args[0].lower().endswith((".cmd", ".bat")):
+ batch_meta = re.compile(r'[&|<>^%!()]')
+ for arg in args[1:]:
+ if batch_meta.search(arg):
+ raise ValueError(f"Unsafe batch argument: {arg}")
return subprocess.check_call(args, shell=False)🧰 Tools
🪛 Ruff (0.15.14)
[error] 35-35: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/package_manager.py` around lines 23 - 35, The current argument
validation (safe_pattern) still allows cmd.exe metacharacters which are
dangerous when args[0] resolves to a .cmd/.bat; update the logic in the block
around safe_pattern, args, resolved_cmd and subprocess.check_call to perform an
additional deny-check when running on Windows or when resolved_cmd endswith
'.cmd' or '.bat' — scan all arguments (or at least subsequent args) for cmd
metacharacters (e.g. & | < > ^ % ! ( ) ) and raise ValueError if any are present
before calling subprocess.check_call; keep the existing safe_pattern validation
but add this explicit deny-list branch referencing resolved_cmd and args to
prevent cmd.exe argument parsing exploits.
| cmd = args[0] | ||
| resolved_cmd = shutil.which(cmd) |
There was a problem hiding this comment.
Guard empty command arguments before Line 30 indexing.
args[0] is accessed unconditionally at Line 30. An empty iterable currently throws IndexError instead of a controlled validation error.
Suggested patch
if os.name == 'nt':
args = list(args)
+ if not args:
+ raise ValueError("Command arguments cannot be empty")
safe_pattern = re.compile(r'^[a-zA-Z0-9._\-\[\]=<>!,\/\\@:]+$')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/package_manager.py` around lines 30 - 31, Guard against an empty args
before accessing args[0]: check that args is truthy and len(args) > 0 prior to
assigning cmd = args[0]; if empty, raise a clear ValidationError/ValueError or
return an explicit error instead of letting an IndexError bubble up. After the
check, keep the existing resolved_cmd = shutil.which(cmd) call and ensure any
error path logs or returns consistent error information so callers can handle
missing command arguments reliably.
1 participant
This PR addresses a critical command injection vulnerability caused by the use of
shell=Trueinsubprocesscalls on Windows.Changes:
libs/package_manager.py:shell=Truefromsubprocess.check_call.shutil.whichto correctly resolve executable paths (e.g.,npm.cmd,pip.exe) without needing the shell.safe_patternregex to allow path separators (/,\) and drive letters (:), sinceshutil.whichreturns absolute paths.argsis a mutable list before modifyingargs[0].libs/utility_manager.py:subprocess.call(['start', filename], shell=True)withos.startfile(filename)on Windows, completely bypassing shell invocation for file opening.tests/test_interpreter.py:shell=Falseand mockedshutil.whichwhere appropriate.Verification:
python3 -m pytest tests/).PR created automatically by Jules for task 10982852139776001829 started by @haseeb-heaven
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests