Potential fix for code scanning alert no. 3: Uncontrolled command line #8
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Reviewer's GuideRefactors the Windows self-update installer launch path to always invoke a fixed PowerShell executable and pass the (potentially tainted) installer path and arguments via environment variables, eliminating uncontrolled command execution while preserving behavior for elevated and non-elevated runs. Sequence diagram for updated applyUpdateInstaller PowerShell invocationsequenceDiagram
actor User
participant CLI as cloudsqlctl
participant SelfUpdate as selfUpdate_applyUpdateInstaller
participant Execa as execa
participant PS as powershell
participant Installer
User->>CLI: run upgrade command
CLI->>SelfUpdate: applyUpdateInstaller(installerPath, silent, elevate)
SelfUpdate->>SelfUpdate: build args array (silent flags)
SelfUpdate->>SelfUpdate: set envVars PS_INSTALLER_PATH, PS_INSTALLER_ARGS
SelfUpdate->>SelfUpdate: build basePsCommand
alt elevate is true
SelfUpdate->>Execa: execa(powershell, [-NoProfile, -NonInteractive, -Command, psCommand], env)
Execa->>PS: start PowerShell process
PS->>PS: $p = GetEnvironmentVariable(PS_INSTALLER_PATH)
PS->>PS: $a = GetEnvironmentVariable(PS_INSTALLER_ARGS)
PS->>Installer: Start-Process -FilePath $p -ArgumentList $a -Verb RunAs -Wait
Installer-->>PS: exit code
PS-->>Execa: process exit
Execa-->>SelfUpdate: resolved or rejected promise
else elevate is false
SelfUpdate->>Execa: execa(powershell, [-NoProfile, -NonInteractive, -Command, psCommand], env)
Execa->>PS: start PowerShell process
PS->>PS: $p = GetEnvironmentVariable(PS_INSTALLER_PATH)
PS->>PS: $a = GetEnvironmentVariable(PS_INSTALLER_ARGS)
PS->>Installer: Start-Process -FilePath $p -ArgumentList $a -Wait
Installer-->>PS: exit code
PS-->>Execa: process exit
Execa-->>SelfUpdate: resolved or rejected promise
end
SelfUpdate-->>CLI: completion or error
CLI-->>User: report update result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/core/selfUpdate.ts:181-183` </location>
<code_context>
- if (elevate) {
- // Use PowerShell Start-Process -Verb RunAs
- // To prevent command injection, we pass arguments via environment variables.
- const envVars: Record<string, string> = {
- 'PS_INSTALLER_PATH': installerPath,
- 'PS_INSTALLER_ARGS': args.join(' ')
- };
+ // Use PowerShell Start-Process with environment variables for both elevated and non-elevated runs
</code_context>
<issue_to_address>
**issue (bug_risk):** Combining arguments into a single space-separated string risks incorrect parsing when arguments contain spaces or special characters.
Previously `execa(installerPath, args)` kept each argument separate, but now all args are collapsed into one string and passed via `PS_INSTALLER_ARGS` to `Start-Process -ArgumentList`. Any arg with spaces or special chars may be split or misparsed by PowerShell. To avoid this, either preserve the array structure (e.g., one env var per arg and rebuild an array in PowerShell) or encode the args in a format that can be safely reconstructed (e.g., JSON + `ConvertFrom-Json`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 'PS_INSTALLER_ARGS': args.join(' ') | ||
| }; | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Combining arguments into a single space-separated string risks incorrect parsing when arguments contain spaces or special characters.
Previously execa(installerPath, args) kept each argument separate, but now all args are collapsed into one string and passed via PS_INSTALLER_ARGS to Start-Process -ArgumentList. Any arg with spaces or special chars may be split or misparsed by PowerShell. To avoid this, either preserve the array structure (e.g., one env var per arg and rebuild an array in PowerShell) or encode the args in a format that can be safely reconstructed (e.g., JSON + ConvertFrom-Json).
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability (code scanning alert #3) related to uncontrolled command line execution in the self-update functionality. The fix prevents potential command injection by ensuring that the installer path from potentially tainted data is never directly executed as a process. Instead, both elevated and non-elevated code paths now invoke a fixed, trusted executable (PowerShell) and pass the installer path as data via environment variables.
Key Changes:
- Modified
applyUpdateInstallerto use PowerShell Start-Process for both elevated and non-elevated installer execution - Replaced direct
execa(installerPath, args)call with environment-variable-based PowerShell invocation - Unified the security approach across both execution paths
Comments suppressed due to low confidence (1)
src/core/selfUpdate.ts:109
- Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped.
response.data.pipe(writer);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'PS_INSTALLER_ARGS': args.join(' ') | ||
| }; | ||
|
|
||
| const basePsCommand = ` | ||
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | ||
| $a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS') |
There was a problem hiding this comment.
The PowerShell Start-Process -ArgumentList parameter expects an array, but a single joined string is being passed. When arguments are joined with spaces into a single string and passed to PowerShell's ArgumentList parameter, PowerShell will treat the entire string as a single argument rather than parsing it into multiple arguments.
For example, if args = ['/VERYSILENT', '/SUPPRESSMSGBOXES', '/NORESTART'], joining them with spaces produces '/VERYSILENT /SUPPRESSMSGBOXES /NORESTART', which PowerShell will pass as one argument to the installer instead of three separate arguments.
Consider passing the arguments as an array to PowerShell. One approach is to use PowerShell's comma-separated syntax or pass each argument individually through environment variables (e.g., PS_INSTALLER_ARG_0, PS_INSTALLER_ARG_1, etc.) and reconstruct the array in PowerShell, or use a JSON-encoded array that PowerShell can parse.
| 'PS_INSTALLER_ARGS': args.join(' ') | |
| }; | |
| const basePsCommand = ` | |
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | |
| $a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS') | |
| 'PS_INSTALLER_ARGS_JSON': JSON.stringify(args) | |
| }; | |
| const basePsCommand = ` | |
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | |
| $aJson = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS_JSON') | |
| if ([string]::IsNullOrWhiteSpace($aJson)) { | |
| $a = @() | |
| } else { | |
| $a = $aJson | ConvertFrom-Json | |
| } |
| if (elevate) { | ||
| // Elevated: use -Verb RunAs | ||
| const psCommand = ` | ||
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | ||
| $a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS') | ||
| ${basePsCommand} | ||
| Start-Process -FilePath $p -ArgumentList $a -Verb RunAs -Wait | ||
| `.trim(); | ||
|
|
||
| await execa('powershell', ['-NoProfile', '-NonInteractive', '-Command', psCommand], { | ||
| env: { ...process.env, ...envVars } | ||
| }); | ||
| } else { | ||
| await execa(installerPath, args); | ||
| // Non-elevated: run without -Verb RunAs | ||
| const psCommand = ` | ||
| ${basePsCommand} | ||
| Start-Process -FilePath $p -ArgumentList $a -Wait | ||
| `.trim(); | ||
|
|
||
| await execa('powershell', ['-NoProfile', '-NonInteractive', '-Command', psCommand], { | ||
| env: { ...process.env, ...envVars } | ||
| }); |
There was a problem hiding this comment.
There is code duplication between the elevated and non-elevated branches. Both branches construct nearly identical PowerShell commands and invoke execa with the same parameters, differing only by the presence of '-Verb RunAs' in the Start-Process call.
Consider extracting the common logic to reduce duplication. For example, construct the Start-Process line conditionally and use a single execa invocation.
| export async function applyUpdateInstaller(installerPath: string, silent: boolean, elevate: boolean) { | ||
| logger.info('Launching installer...'); | ||
|
|
||
| const args = []; | ||
| const args: string[] = []; | ||
| if (silent) { | ||
| args.push('/VERYSILENT', '/SUPPRESSMSGBOXES', '/NORESTART'); | ||
| } | ||
|
|
||
| if (elevate) { | ||
| // Use PowerShell Start-Process -Verb RunAs | ||
| // To prevent command injection, we pass arguments via environment variables. | ||
| const envVars: Record<string, string> = { | ||
| 'PS_INSTALLER_PATH': installerPath, | ||
| 'PS_INSTALLER_ARGS': args.join(' ') | ||
| }; | ||
| // Use PowerShell Start-Process with environment variables for both elevated and non-elevated runs | ||
| const envVars: Record<string, string> = { | ||
| 'PS_INSTALLER_PATH': installerPath, | ||
| 'PS_INSTALLER_ARGS': args.join(' ') | ||
| }; | ||
|
|
||
| const basePsCommand = ` | ||
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | ||
| $a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS') | ||
| `.trim(); | ||
|
|
||
| if (elevate) { | ||
| // Elevated: use -Verb RunAs | ||
| const psCommand = ` | ||
| $p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH') | ||
| $a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS') | ||
| ${basePsCommand} | ||
| Start-Process -FilePath $p -ArgumentList $a -Verb RunAs -Wait | ||
| `.trim(); | ||
|
|
||
| await execa('powershell', ['-NoProfile', '-NonInteractive', '-Command', psCommand], { | ||
| env: { ...process.env, ...envVars } | ||
| }); | ||
| } else { | ||
| await execa(installerPath, args); | ||
| // Non-elevated: run without -Verb RunAs | ||
| const psCommand = ` | ||
| ${basePsCommand} | ||
| Start-Process -FilePath $p -ArgumentList $a -Wait | ||
| `.trim(); | ||
|
|
||
| await execa('powershell', ['-NoProfile', '-NonInteractive', '-Command', psCommand], { | ||
| env: { ...process.env, ...envVars } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The applyUpdateInstaller function lacks test coverage. The existing test file tests/selfUpdate.test.ts tests checkForUpdates and pickAsset, but doesn't include tests for applyUpdateInstaller.
Given the security-sensitive nature of this function (handling installer execution) and that comprehensive automated testing exists for this module, consider adding tests to verify that the PowerShell command is constructed correctly for both elevated and non-elevated cases, and that arguments are properly passed.
Potential fix for https://github.com/Kinin-Code-Offical/cloudsqlctl/security/code-scanning/3
General approach: Ensure that any file path used as an executable for
execa(or passed into PowerShell) is strictly controlled and not directly derived from tainted data in a way that could lead to arbitrary command execution. In practice here, we should (1) avoid passing a taintedinstallerPathas the process to launch and instead rely on a known runtime (likepowershellorcmd) with a fixed command, or (2) validate and restrictinstallerPathso it cannot be abused, or (3) ensure that in all code paths the tainted string is only ever used as a data argument, not as an executable.Best fix with minimal behavior change: In the non-elevated branch of
applyUpdateInstaller, replaceexeca(installerPath, args)with a PowerShell invocation similar to the elevated path, whereinstallerPathand arguments are passed as data. This keeps the functional behavior (we still run the installer with the same arguments), but instead of treating the taintedinstallerPathas the executable directly, we always execute a fixed, trusted program (powershell) and let it run the installer path that we control (downloaded to our chosen directory and checksum-verified). To maintain the behavior of waiting for the installer to finish and propagating errors, we can pass-Waitand-PassThruin PowerShell and keep usingawait execa(...)to observe exit codes. The elevated and non-elevated branches will both use the same pattern: executepowershellwith a small inline script and environment variables holding the path and arguments.Concretely, in
src/core/selfUpdate.ts:applyUpdateInstaller, keep the current elevated branch but extend the non-elevated branch to use PowerShell instead of directly callingexeca(installerPath, args).PS_INSTALLER_PATH,PS_INSTALLER_ARGS), and use a simple PowerShell command that reads those variables and callsStart-Process -FilePath $p -ArgumentList $a -Wait(without-Verb RunAsfor non-elevated).powershell) a hard-coded, trusted string and treating installerPath as data.No changes are needed in
src/commands/upgrade.tsbecausedownloadPathremains a data argument passed through toapplyUpdateInstaller/applyUpdatePortableExe; hardening the sink inapplyUpdateInstalleris sufficient for this alert.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Enhancements: