refactoring(check_pr): Move script in github_scripts dir#138
refactoring(check_pr): Move script in github_scripts dir#138
Conversation
|
Comment this PR with update_code to update |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const execSync = require('child_process').execSync; | ||
| for (const file of IGNORED_FILES.trim().split(',')) { | ||
|
|
||
| const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'}) |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, the fix is to stop passing untrusted data (like environment variables) into shell commands via string concatenation. Instead, use an API that does not invoke a shell and accepts the command and its arguments as an array (e.g., child_process.execFileSync or spawnSync), or, if a shell is unavoidable, properly escape any untrusted values before interpolation.
For this specific file, the problematic parts are lines 14–15, where execSync is called with a shell command string containing BRANCH_NAME and a pipeline to grep/cut. We can replace the use of execSync with execFileSync and pass the arguments as an array so that BRANCH_NAME is not interpreted by a shell. To preserve existing behavior (which uses grep and cut to process the git diff --numstat output), we can instead: (1) call git directly via execFileSync with arguments ['--no-pager','diff','--numstat',\origin/main..origin/${BRANCH_NAME}`], (2) parse the resulting text in JavaScript to extract additions and deletions for each ignored file, and (3) compute the totals as before. This avoids any shell, gets rid of grep/cut, and keeps functionality equivalent. Concretely, inside .github/workflows/github_scripts/check_size.js, we will: add a require('child_process').execFileSyncalongside (or replacing)execSync, call it once to obtain the diff output, then for each fileinignored_files`, parse that shared diff output and sum the additions/deletions for matching lines. Lines 11–22 will be updated accordingly, with no changes to surrounding logic.
| @@ -8,19 +8,19 @@ | ||
| const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0); | ||
| if (ignored_files.length > 0) { | ||
| var ignored = 0 | ||
| const execSync = require('child_process').execSync; | ||
| for (const file of IGNORED_FILES.trim().split(',')) { | ||
| const execFileSync = require('child_process').execFileSync; | ||
| const diffOutput = execFileSync('git', ['--no-pager', 'diff', '--numstat', 'origin/main..origin/' + BRANCH_NAME], {encoding: 'utf-8'}); | ||
| for (const file of ignored_files) { | ||
| const lines = diffOutput.split('\n').filter(l => l.includes(file)); | ||
| const ignored_additions = lines | ||
| .map(l => l.trim().split('\t')[0]) | ||
| .map(val => parseInt(val || 0)) | ||
| .reduce((accumulator, currentValue) => accumulator + currentValue, 0); | ||
| const ignored_deletions = lines | ||
| .map(l => l.trim().split('\t')[1]) | ||
| .map(val => parseInt(val || 0)) | ||
| .reduce((accumulator, currentValue) => accumulator + currentValue, 0); | ||
|
|
||
| const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'}) | ||
| const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'}) | ||
|
|
||
| const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce( | ||
| (accumulator, currentValue) => accumulator + currentValue, | ||
| 0); | ||
| const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce( | ||
| (accumulator, currentValue) => accumulator + currentValue, | ||
| 0); | ||
|
|
||
| ignored += ignored_additions + ignored_deletions; | ||
| } | ||
| changes -= ignored |
| for (const file of IGNORED_FILES.trim().split(',')) { | ||
|
|
||
| const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'}) | ||
| const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'}) |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, the problem should be fixed by avoiding shell command construction with string concatenation for untrusted data, and instead using APIs that accept an argument array and do not invoke a shell (child_process.execFileSync or spawnSync with shell: false). This prevents shell metacharacters in BRANCH_NAME (or in file) from being interpreted by a shell.
The best fix here is to (1) switch from execSync to execFileSync and (2) replace the grep/cut pipeline with logic implemented in JavaScript: run git diff --numstat as git with arguments, capture its stdout, then filter lines and extract columns in Node. This removes all shell parsing and still preserves behavior.
Concretely, in .github/workflows/github_scripts/check_size.js:
- At line 11, change the import from
execSynctoexecFileSync. - Replace the two
execSync('git --no-pager diff ... | grep ... | cut -f N', ...)calls at lines 14–15 with a singleexecFileSync('git', [...])call that gets the full--numstatoutput once per loop iteration (or once total; but to avoid broader refactoring, keep it per loop), then:- Split the output into lines.
- For each line that includes
file, split on whitespace to get additions, deletions, and filename. - Accumulate additions and deletions into
ignored_additions_strandignored_deletions_str-equivalents (or directly into numbers).
- Maintain the rest of the logic (parsing, reducing) so functionality stays the same.
No extra external libraries are needed; we only use the standard child_process module and string processing.
| @@ -8,19 +8,38 @@ | ||
| const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0); | ||
| if (ignored_files.length > 0) { | ||
| var ignored = 0 | ||
| const execSync = require('child_process').execSync; | ||
| const execFileSync = require('child_process').execFileSync; | ||
| for (const file of IGNORED_FILES.trim().split(',')) { | ||
|
|
||
| const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'}) | ||
| const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'}) | ||
| const numstatOutput = execFileSync( | ||
| 'git', | ||
| ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`], | ||
| {encoding: 'utf-8'} | ||
| ); | ||
|
|
||
| const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce( | ||
| (accumulator, currentValue) => accumulator + currentValue, | ||
| 0); | ||
| const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce( | ||
| (accumulator, currentValue) => accumulator + currentValue, | ||
| 0); | ||
| const matchingLines = numstatOutput | ||
| .split('\n') | ||
| .filter(line => line && line.includes(file)); | ||
|
|
||
| const additionsList = []; | ||
| const deletionsList = []; | ||
|
|
||
| for (const line of matchingLines) { | ||
| const parts = line.trim().split(/\s+/); | ||
| if (parts.length >= 3) { | ||
| additionsList.push(parts[0]); | ||
| deletionsList.push(parts[1]); | ||
| } | ||
| } | ||
|
|
||
| const ignored_additions = additionsList | ||
| .map(elem => parseInt(elem || 0)) | ||
| .reduce((accumulator, currentValue) => accumulator + currentValue, 0); | ||
|
|
||
| const ignored_deletions = deletionsList | ||
| .map(elem => parseInt(elem || 0)) | ||
| .reduce((accumulator, currentValue) => accumulator + currentValue, 0); | ||
|
|
||
| ignored += ignored_additions + ignored_deletions; | ||
| } | ||
| changes -= ignored |
|
1 participant
List of Changes
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: