Skip to content

Potential fix for code scanning alert no. 3: Uncontrolled command line #8

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
Kinin-Code-Offical merged 1 commit into from
Dec 21, 2025
Merged

Potential fix for code scanning alert no. 3: Uncontrolled command line #8

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
34 changes: 23 additions & 11 deletions src/core/selfUpdate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,30 +170,42 @@ export function pickAsset(release: ReleaseInfo, mode: 'auto' | 'installer' | 'ex
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(' ')
};

Comment on lines +181 to +183
Copy link

@sourcery-ai sourcery-ai bot Dec 21, 2025

Choose a reason for hiding this comment

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

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

const basePsCommand = `
$p = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_PATH')
$a = [System.Environment]::GetEnvironmentVariable('PS_INSTALLER_ARGS')
Comment on lines +181 to +186
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
'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
}

Copilot uses AI. Check for mistakes.
`.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 }
});
Comment on lines +189 to +208
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
}
Comment on lines 170 to 210
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

Expand Down
Loading