Fix shell injection in local folder packing#503
Conversation
📝 WalkthroughWalkthroughThis PR introduces a shell-safe command execution utility to prevent shell injection vulnerabilities. A new ChangesShell-safe command execution for npm pack
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
lib/download/local.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. lib/utils.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. test/installLocal.test.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by replacing shell-based command execution with a new utils.execFile utility (using execa with shell: false) when packing local folders. It also introduces a security test case to verify that shell metacharacters in file paths do not lead to command injection. Feedback was provided to improve the robustness of the execFile implementation by ensuring the shell option cannot be overridden by caller options and by simplifying the asynchronous return.
| exports.execFile = async (file, args, options) => { | ||
| return await execa(file, args, { | ||
| shell: false, | ||
| ...options, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
To ensure the security fix is robust, shell: false should be enforced by placing it after ...options. This prevents any caller from accidentally re-enabling the shell. Additionally, the await keyword is redundant when returning a promise directly, and providing default values for args and options improves the function's resilience.
| exports.execFile = async (file, args, options) => { | |
| return await execa(file, args, { | |
| shell: false, | |
| ...options, | |
| }); | |
| }; | |
| exports.execFile = (file, args = [], options = {}) => { | |
| return execa(file, args, { | |
| ...options, | |
| shell: false, | |
| }); | |
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@lib/utils.js`:
- Around line 668-673: The execFile helper (exports.execFile) currently spreads
caller options after setting shell: false, allowing callers to override it and
enable a shell; fix by ensuring shell: false cannot be overridden — either
spread options first and then set shell: false (so the literal wins), or
explicitly set shell: false after merging options (e.g., finalOptions = {
...options, shell: false }) before passing to execa; update exports.execFile to
enforce shell: false regardless of provided options.
🪄 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: cc214f92-98ba-43fb-96b3-3b744dcfeb68
📒 Files selected for processing (3)
lib/download/local.jslib/utils.jstest/installLocal.test.js
| exports.execFile = async (file, args, options) => { | ||
| return await execa(file, args, { | ||
| shell: false, | ||
| ...options, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Critical: Spread operator order allows shell override.
The spread operator ...options comes after shell: false, which means callers can override the security setting by passing { shell: true } in options. This defeats the purpose of execFile as a shell-safe execution helper.
🔒 Proposed fix to prevent override
exports.execFile = async (file, args, options) => {
return await execa(file, args, {
- shell: false,
...options,
+ shell: false,
});
};This ensures shell: false always takes precedence, even if a caller mistakenly passes shell: true.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exports.execFile = async (file, args, options) => { | |
| return await execa(file, args, { | |
| shell: false, | |
| ...options, | |
| }); | |
| }; | |
| exports.execFile = async (file, args, options) => { | |
| return await execa(file, args, { | |
| ...options, | |
| 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 `@lib/utils.js` around lines 668 - 673, The execFile helper (exports.execFile)
currently spreads caller options after setting shell: false, allowing callers to
override it and enable a shell; fix by ensuring shell: false cannot be
overridden — either spread options first and then set shell: false (so the
literal wins), or explicitly set shell: false after merging options (e.g.,
finalOptions = { ...options, shell: false }) before passing to execa; update
exports.execFile to enforce shell: false regardless of provided options.
1 participant
This change is
Summary by CodeRabbit
Bug Fixes
Tests