Potential fix for code scanning alert no. 8: Unsafe shell command constructed from library input#610
Conversation
…structed from library input Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR attempts to address code-scanning alert #8 by removing Windows non-PowerShell command concatenation (&& chaining) and executing commands via execFile instead of exec.
Changes:
- Replaced the Windows non-
psbranch from building a single chained command string to executingchcp 65001and then each command individually. - Added a
childExecCmdWindowshelper that runscmd.exeviachildProcess.execFileand preserves existing logging/error behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function childExecCmdWindows (cmd, options = {}) { | ||
| return new Promise((resolve, reject) => { | ||
| const execOptions = { ...options } | ||
| delete execOptions.type | ||
| delete execOptions.printErrorLog | ||
|
|
||
| log.info('shell:', cmd) | ||
| childProcess.execFile('cmd.exe', ['/d', '/s', '/c', cmd], execOptions, (error, stdout, stderr) => { | ||
| if (error) { |
There was a problem hiding this comment.
The PR description says this avoids shell interpretation by using execFile with explicit argument arrays, but this helper still runs user-provided cmd through cmd.exe /c, which does interpret shell metacharacters (&, |, redirection, etc.). If cmd can contain untrusted input, this remains command-injection prone; the safer approach is to execFile the target executable directly with an argv array (or restrict/escape allowed commands/args before invoking cmd.exe).
| await childExecCmdWindows('chcp 65001', args) | ||
| let ret | ||
| for (const cmd of cmds) { | ||
| compose += ` && ${cmd}` | ||
| ret = await childExecCmdWindows(cmd, args) | ||
| } | ||
| // compose += '&& exit' | ||
| return await childExec(compose, args) | ||
| return ret |
There was a problem hiding this comment.
In the non-ps branch, ret will be undefined when cmds is an empty array, whereas previously the function would return the (post-processed) output from running chcp 65001. Consider returning an explicit empty string (or the chcp output) when there are no commands to preserve a consistent return type.
| await childExecCmdWindows('chcp 65001', args) | ||
| let ret | ||
| for (const cmd of cmds) { | ||
| compose += ` && ${cmd}` | ||
| ret = await childExecCmdWindows(cmd, args) | ||
| } |
There was a problem hiding this comment.
chcp 65001 is now executed in a separate cmd.exe process, so the code page change won’t persist for the subsequent command executions (each childExecCmdWindows call spawns a new process). This likely regresses the original intent of ensuring UTF-8 output for the actual commands; consider running the chcp 65001 step in the same invocation as each command (or otherwise ensuring the code page is set for the process that runs the command).
Potential fix for https://github.com/docmirror/dev-sidecar/security/code-scanning/8
Best fix: avoid building a shell command string from
cmdsin the Windows non-psbranch. Execute commands without shell interpretation by usingexecFileand explicit argument arrays.In this file, the safest minimal change is:
cmd.exesafely with/d /s /cand a single provided command string argument (no concatenation across user-provided commands).WindowsSystemShell.execnon-psbranch, runchcp 65001as a fixed command first, then execute eachcmdseparately through the helper, collecting the last output (matching current return behavior of last command output).psmode and other platforms unchanged.This removes the vulnerable concatenation at line 64 and avoids passing a composed shell string to
childProcess.exec.Suggested fixes powered by Copilot Autofix. Review carefully before merging.