Skip to content

Potential fix for code scanning alert no. 8: Unsafe shell command constructed from library input #610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cute-omega merged 1 commit into from
Apr 25, 2026
Merged

Potential fix for code scanning alert no. 8: Unsafe shell command constructed from library input #610

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions packages/core/src/shell/shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,36 @@ class WindowsSystemShell extends SystemShell {
ps.dispose()
}
} else {
let compose = 'chcp 65001' // 'chcp 65001 '
await childExecCmdWindows('chcp 65001', args)
let ret
for (const cmd of cmds) {
compose += ` && ${cmd}`
ret = await childExecCmdWindows(cmd, args)
}
Comment on lines +62 to 66

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
// compose += '&& exit'
return await childExec(compose, args)
return ret
Comment on lines +62 to +67

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}
}

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) {
Comment on lines +72 to +80

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
if (options.printErrorLog !== false) {
log.error('cmd 命令执行错误：\n===>\ncommands:', cmd, '\n error:', error, '\n<===')
}
reject(new Error(stderr))
} else {
resolve(stdout.replace('Active code page: 65001\r\n', ''))
}
})
})
}

function childExec (composeCmds, options = {}) {
return new Promise((resolve, reject) => {
log.info('shell:', composeCmds)
Expand Down
Loading