Conversation
Co-authored-by: Simon Brebeck <mail+github@simonbrebeck.de>
|
|
||
| function execCommand(command, options = {}) { | ||
| try { | ||
| execSync(command, { stdio: 'inherit', ...options }); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, the safest fix is to avoid constructing full shell command strings and instead invoke commands directly with argument arrays, so the shell never interprets the dynamic values. For Node, that means replacing execSync(commandString) with execFileSync(binary, argsArray, options) (or spawnSync). Each dynamic path or branch name then becomes an element of the args array, and execFileSync does not go through a shell by default.
For this file, the best targeted change is:
- Replace the generic
execCommand(command, options)helper with a helper that accepts a command name and an array of arguments and callsexecFileSync. - Update all call sites:
-
git clone ${targetRepository} ${tempDir}→execCommand('git', ['clone', targetRepository, tempDir]) -
git checkout ${targetBranch}→execCommand('git', ['checkout', targetBranch]) -
npm install→execCommand('npm', ['install']) -
npm run build→execCommand('npm', ['run', 'build']) -
rm -rf ${modulePath}→execCommand('rm', ['-rf', modulePath]) -
mkdir -p ${modulePath}→execCommand('mkdir', ['-p', modulePath]) -
cp -R ${tempDir}${artifactPath} ${modulePath}→execCommand('cp', ['-R',${tempDir}${artifactPath}, modulePath]) -
rm -rf ${tempDir}→execCommand('rm', ['-rf', tempDir])
-
- Adjust logging to reflect the new argument style, but keep functionality the same.
- Import
execFileSyncfromchild_processin addition to (or instead of)execSync. We can retainexecSyncif needed elsewhere, but in this snippet we only use the new helper.
All edits are confined to scripts/install-peer-dependency.js: updating the import on line 9, replacing the execCommand definition at the bottom, and updating each execCommand(...) call between lines 54 and 79.
| @@ -6,7 +6,7 @@ | ||
| // "postinstall": "node ./scripts/install-peer-dependency.js collection-hooks:property-filter-token-groups" | ||
| // where "collection-hooks" is the package to fetch and "property-filter-token-groups" is the branch name in GitHub. | ||
|
|
||
| import { execSync } from 'child_process'; | ||
| import { execFileSync } from 'child_process'; | ||
| import process from 'node:process'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| @@ -51,14 +51,14 @@ | ||
|
|
||
| // Clone the repository and checkout the branch | ||
| console.log(`Cloning ${packageName}:${targetBranch}...`); | ||
| execCommand(`git clone ${targetRepository} ${tempDir}`); | ||
| execCommand('git', ['clone', targetRepository, tempDir]); | ||
| process.chdir(tempDir); | ||
| execCommand(`git checkout ${targetBranch}`); | ||
| execCommand('git', ['checkout', targetBranch]); | ||
|
|
||
| // Install dependencies and build | ||
| console.log(`Installing dependencies and building ${packageName}...`); | ||
| execCommand('npm install'); | ||
| execCommand('npm run build'); | ||
| execCommand('npm', ['install']); | ||
| execCommand('npm', ['run', 'build']); | ||
|
|
||
| // Remove existing peer dependency in node_modules | ||
| for (const moduleName of getModules(packageName)) { | ||
| @@ -66,25 +62,25 @@ | ||
| const artifactPath = getArtifactPath(moduleName); | ||
|
|
||
| console.log(`Removing existing ${moduleName} from node_modules...`, modulePath); | ||
| execCommand(`rm -rf ${modulePath}`); | ||
| execCommand('rm', ['-rf', modulePath]); | ||
|
|
||
| // Copy built peer dependency to node_modules | ||
| console.log(`Copying built ${moduleName} to node_modules...`, modulePath, `${tempDir}${artifactPath}`); | ||
| execCommand(`mkdir -p ${modulePath}`); | ||
| execCommand(`cp -R ${tempDir}${artifactPath} ${modulePath}`); | ||
| execCommand('mkdir', ['-p', modulePath]); | ||
| execCommand('cp', ['-R', `${tempDir}${artifactPath}`, modulePath]); | ||
| } | ||
|
|
||
| // Clean up | ||
| console.log('Cleaning up...'); | ||
| execCommand(`rm -rf ${tempDir}`); | ||
| execCommand('rm', ['-rf', tempDir]); | ||
|
|
||
| console.log(`${packageName} has been successfully installed from branch ${targetBranch}!`); | ||
|
|
||
| function execCommand(command, options = {}) { | ||
| function execCommand(command, args = [], options = {}) { | ||
| try { | ||
| execSync(command, { stdio: 'inherit', ...options }); | ||
| execFileSync(command, args, { stdio: 'inherit', ...options }); | ||
| } catch (error) { | ||
| console.error(`Error executing command: ${command}`); | ||
| console.error(`Error executing command: ${command} ${args.join(' ')}`); | ||
| console.error(`Error message: ${error.message}`); | ||
| console.error(`Stdout: ${error.stdout && error.stdout.toString()}`); | ||
| console.error(`Stderr: ${error.stderr && error.stderr.toString()}`); |
1af2027 to
302a0ac
Compare
302a0ac to
b84bb22
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
===========================================
+ Coverage 98.59% 100.00% +1.40%
===========================================
Files 16 13 -3
Lines 498 418 -80
Branches 171 156 -15
===========================================
- Hits 491 418 -73
+ Misses 5 0 -5
+ Partials 2 0 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Same as #126, but with selection tree and track-by utils coming from the toolkit.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.