fix: replace shell=True subprocess with argument list in modal CLI#3487
fix: replace shell=True subprocess with argument list in modal CLI#3487Hadar01 wants to merge 5 commits intoaxolotl-ai-cloud:mainfrom
Conversation
Using shell=True with a formatted string containing docker_image (a user-controlled value) is a command injection risk (Bandit B602). Replace with an argument list, which passes args directly to the process without shell interpretation, removing the nosec annotation.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors docker image hash retrieval in the modal CLI module by replacing shell-invoked subprocess command with direct argument list, eliminating shell injection risk while preserving error handling behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Removing shell=True (B602) surfaces B603 (subprocess without shell) and B607 (partial executable path for 'docker'). Use bare # nosec to suppress both, consistent with other nosec usages in the codebase.
Description
Replace
shell=Truesubprocess call with an argument list in the Modal CLI cloud integration.Motivation and Context
subprocess.check_output()was called withshell=Trueand a formatted string containingdocker_image, which is derived from user config (self.config.docker_tag). Passing user-controlled values to a shell string is a command injection risk (Bandit B602). Using an argument list passes args directly to the process without shell interpretation, eliminating the risk and allowing the# nosecannotation to be removed.How has this been tested?
pre-commit run --all-files— Bandit no longer flags this lineAI Usage Disclaimer
No
Types of changes
Summary by CodeRabbit