Remove usage of execSync in scripts #4099
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: skoeva The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks. I want to hold off on this until after 0.37.0 |
a0bb3b9 to
f7903c7
Compare
f7903c7 to
62a1f08
Compare
62a1f08 to
c5b6877
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors shell command execution across the codebase by replacing execSync with execFileSync to improve security by avoiding shell interpretation and reducing the risk of command injection vulnerabilities. Additionally, it consolidates duplicate imports in one file.
- Replaced
execSyncwithexecFileSyncfor git, npm, and make commands across multiple files - Updated command invocations to pass arguments as arrays instead of shell strings
- Consolidated duplicate imports in
app/scripts/start.js
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/releaser/src/utils/git.ts | Converted all git commands from execSync to execFileSync with array arguments |
| tools/releaser/src/commands/start.ts | Changed npm install command to use execFileSync |
| plugins/headlamp-plugin/bin/headlamp-plugin.js | Updated npm commands to use execFileSync and removed an error message |
| frontend/make-env.js | Converted git command to use execFileSync |
| app/scripts/start.js | Consolidated duplicate child_process imports |
| app/scripts/build-backend.js | Changed make command to use execFileSync |
| app/electron/main.ts | Converted binary execution and Windows taskkill command to use execFileSync |
Comments suppressed due to low confidence (1)
app/scripts/build-backend.js:25
- On Windows,
makeis not typically available as a direct executable and may require shell interpretation or a specific make implementation (likemingw32-make). UsingexecFileSyncwithoutshell: truemay cause failures on Windows where make is a batch file or requires shell invocation. Consider addingshell: trueto the options or handling Windows separately.
execFileSync('make', ['backend'], {
env: {
...process.env, // needed otherwise important vars like PATH and GOROOT are not set
GOARCH: arch,
OS: osName,
},
cwd: '..',
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| execSync('npm install', { stdio: 'inherit', cwd: path.join(repoRoot, 'app') }); | ||
| execFileSync('npm', ['install'], { | ||
| stdio: 'inherit', | ||
| cwd: path.join(repoRoot, 'app'), |
There was a problem hiding this comment.
On Windows, npm is typically a .cmd file, not an executable. Using execFileSync('npm', ...) without shell: true will fail on Windows. Consider adding shell: true to the options or using a cross-platform solution like checking process.platform === 'win32' and adjusting the command accordingly (e.g., 'npm.cmd' on Windows).
| cwd: path.join(repoRoot, 'app'), | |
| cwd: path.join(repoRoot, 'app'), | |
| shell: true, |
There was a problem hiding this comment.
This is a real issue, I tested it in node on windows with cmd.
There was a problem hiding this comment.
this could probably be reverted, doing this now
illume
left a comment
There was a problem hiding this comment.
I think these require testing. The copilot found one issue which turned out to be a real one, so I'm worried that these changes will break other things.
It might be worth considering which if any of these is a security problem really to reduce the amount of testing that needs to be done.
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
These changes remove usage of
execSyncin the scripts and replace them withexecFileSync.