fix(security): 2 improvements across 2 files#1691
Conversation
- Security: Command Injection via shell=True in exec_command - Security: Insecure Loading of PyTorch Checkpoints Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Command Injection via shell=True in exec_command - Security: Insecure Loading of PyTorch Checkpoints Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors exec_command in mechanical_refactor_verify_utils.py to accept a list of command arguments and execute subprocesses with shell=False for improved security. Additionally, it updates torch.load in ci_utils.py to use weights_only=True. The review feedback points out a potential issue where running git diff with check=False could silently ignore errors (such as an invalid target commit) and falsely report a successful verification; it is recommended to remove check=False or set it to True to ensure failures are caught.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| print(f"[4/4] Diffing against {target_commit[:8]}...") | ||
| diff = exec_command( | ||
| f"git diff {target_commit} -- .", | ||
| ["git", "diff", target_commit, "--", "."], |
There was a problem hiding this comment.
By default, git diff exits with 0 on success (regardless of whether differences exist or not) and only returns a non-zero exit code on actual errors (such as an invalid target_commit).
Using check=False here is dangerous because if git diff fails (e.g., due to a bad revision or repository error), the error is silently ignored, exec_command returns an empty string, and the script incorrectly reports a successful verification (PASS: transform reproduces the commit exactly.).
Please remove check=False (or set check=True) so that any actual git errors are caught and cause the script to fail loudly.
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:.claude/skills/mechanical-refactor-verify/mechanical_refactor_verify_utils.py:L14The
exec_commandfunction usessubprocess.runwithshell=Trueand passes the command string directly. If any part of the command string is derived from user input or external data (e.g., commit messages, file paths, branch names), it can lead to arbitrary command execution.Solution
Avoid using
shell=True. Pass the command as a list of arguments usingshlex.split()or useshlex.quote()for individual arguments if a shell is strictly necessary.Changes
.claude/skills/mechanical-refactor-verify/mechanical_refactor_verify_utils.py(modified)miles/backends/training_utils/ci_utils.py(modified)