Quality: Insecure subprocess usage in get_system_info.py#1977
Conversation
The script uses shell=True with subprocess.check_output for multiple commands. While the commands are hardcoded, using shell=True is a security anti-pattern and could be exploited if any command input becomes dynamic. Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
📝 WalkthroughWalkthroughSystem information collection now avoids shell execution for NVIDIA GPU and CUDA queries and reads Linux OS metadata directly from ChangesSystem information collection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 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 @.github/ISSUE_TEMPLATE/get_system_info.py:
- Around line 67-71: Update the PRETTY_NAME parsing in the system information
collection flow to use an os-release-aware parser that correctly handles both
single- and double-quoted values, rather than manually stripping only double
quotes. Preserve the existing behavior of selecting PRETTY_NAME and stopping
after it is found.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d11b60a-029c-4a9c-aadb-dd0c5d4cad41
📒 Files selected for processing (1)
.github/ISSUE_TEMPLATE/get_system_info.py
| with open("/etc/os-release", "r") as f: | ||
| for line in f: | ||
| if line.startswith("PRETTY_NAME="): | ||
| os_info = line.split("=", 1)[1].strip().strip('"') | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' .github/ISSUE_TEMPLATE/get_system_info.pyRepository: NVIDIA/Model-Optimizer
Length of output: 3724
🌐 Web query:
os-release PRETTY_NAME single quotes double quotes systemd man page
💡 Result:
According to the os-release manual page, variable assignment values must be enclosed in either double or single quotes if they contain spaces, semicolons, or any special characters outside of the range A–Z, a–z, and 0–9 [1][2]. If the value does not include these special characters, the use of quotes is optional but still permitted [1][3]. When using quotes, shell special characters (such as $, quotes, backslashes, or backticks) must be escaped with backslashes following standard shell style [1][2]. Concatenation of multiple individually quoted strings is not supported [1][2]. Because PRETTY_NAME typically includes spaces (e.g., "Fedora 32 (Workstation Edition)"), it is essentially always quoted in practice [1][4]. The specification ensures the file remains compatible with Bourne shell scripts by using environment-like variable assignments, although full shell evaluation is not supported (e.g., variable expansion is explicitly disallowed) [1][5].
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/os-release
- 2: https://man.archlinux.org/man/os-release.5.en
- 3: https://man.archlinux.org/man/os-release.5
- 4: https://freedesktop.org/software/systemd/man/latest/os-release.html
- 5: https://manpages.ubuntu.com/manpages/kinetic/man5/os-release.5.html
Parse PRETTY_NAME using os-release quoting rules.
/etc/os-release allows single- or double-quoted values, but this only strips double quotes. A single-quoted PRETTY_NAME will show up in the issue output with literal quotes. Use an os-release-aware parser instead of trimming one quote character.
🤖 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 @.github/ISSUE_TEMPLATE/get_system_info.py around lines 67 - 71, Update the
PRETTY_NAME parsing in the system information collection flow to use an
os-release-aware parser that correctly handles both single- and double-quoted
values, rather than manually stripping only double quotes. Preserve the existing
behavior of selecting PRETTY_NAME and stopping after it is found.
Source: MCP tools
There was a problem hiding this comment.
can you address this as well?
There was a problem hiding this comment.
@kevalmorabia97 Sure, here's the fix to handle both single- and double-quoted PRETTY_NAME values per the os-release spec:
- Update the
PRETTY_NAMEparsing logic in.github/ISSUE_TEMPLATE/get_system_info.pyto strip matching single or double quotes instead of only double quotes.
</plan_code_changes]
Failed to handle agent chat message. Please try again.
2 participants
Summary
Quality: Insecure subprocess usage in get_system_info.py
Problem
Severity:
Medium| File:.github/ISSUE_TEMPLATE/get_system_info.py:L10The script uses shell=True with subprocess.check_output for multiple commands. While the commands are hardcoded, using shell=True is a security anti-pattern and could be exploited if any command input becomes dynamic.
Solution
Replace shell=True with explicit command lists (e.g., ['nvidia-smi', '--query-gpu=...']). Use shlex.split() if complex parsing is needed.
Changes
.github/ISSUE_TEMPLATE/get_system_info.py(modified)Summary by CodeRabbit