From e482fdf156fb55854e159c2dd6502c9208961217 Mon Sep 17 00:00:00 2001 From: StillKnotKnown <192589389+StillKnotKnown@users.noreply.github.com> Date: Mon, 19 Jan 2026 14:12:57 +0200 Subject: [PATCH] fix: add shell: true and argument sanitization for Windows packaging (#1340) On Windows, spawnSync cannot execute .cmd files directly without a shell context. This adds shell: isWindows() to the spawnSync options in runCommand() to properly execute electron-vite.cmd and electron-builder.cmd during packaging. Additionally, adds argument validation to prevent potential command injection via shell metacharacters when shell: true is used on Windows. When using shell: true, cmd.exe interprets certain characters (& | > < ^ % ; $ $`) as special operators, which could enable command injection if present in user- controlled arguments. The validateArgs() function checks for these metacharacters on Windows and throws an error if any are found, following the same security pattern used in apps/frontend/src/main/ipc-handlers/mcp-handlers.ts. This follows the existing pattern used throughout the codebase for Windows .cmd file execution (env-utils.ts, mcp-handlers.ts). Fixes ACS-365 Co-authored-by: StillKnotKnown --- apps/frontend/scripts/package-with-python.cjs | 69 +++++- .../frontend/scripts/package-with-python.d.ts | 5 + .../__tests__/package-with-python.test.ts | 218 ++++++++++++++++++ 3 files changed, 288 insertions(+), 4 deletions(-) create mode 100644 apps/frontend/scripts/package-with-python.d.ts create mode 100644 apps/frontend/src/main/__tests__/package-with-python.test.ts diff --git a/apps/frontend/scripts/package-with-python.cjs b/apps/frontend/scripts/package-with-python.cjs index 10883dbe..bc57d072 100644 --- a/apps/frontend/scripts/package-with-python.cjs +++ b/apps/frontend/scripts/package-with-python.cjs @@ -13,6 +13,55 @@ const path = require('path'); const { isWindows, getCurrentPlatform, toNodePlatform } = require('../src/shared/platform.cjs'); const { downloadPython } = require('./download-python.cjs'); +/** + * Shell metacharacters that could enable command injection when shell: true is used on Windows. + * These characters have special meaning in cmd.exe and could be used to inject arbitrary commands. + * + * Includes: + * - Standard operators: & | > < ^ % + * - Command separators: ; + * - Variable expansion: $ % + * - Command grouping: ( ) [ ] { } + * - Delayed expansion: ! + * - Command substitution: ` + * - Quotes: " + * - Line breaks: \n \r + * + * Note: Single quote (') is not included as cmd.exe does not treat it as a shell metacharacter. + */ +const SHELL_METACHARACTERS = Object.freeze(['&', '|', '>', '<', '^', '%', ';', '$', '(', ')', '[', ']', '{', '}', '!', '`', '"', '\n', '\r']); + +/** + * Validate that arguments don't contain shell metacharacters on Windows. + * When shell: true is used, cmd.exe interprets metacharacters which could lead to command injection. + * + * @param {string[]} commandArgs - Arguments to validate + * @throws {Error} If any argument contains dangerous shell metacharacters on Windows + * @throws {TypeError} If any argument is not a string + */ +function validateArgs(commandArgs) { + if (!isWindows()) return; // Only validate on Windows where shell: true is used + + for (const arg of commandArgs) { + // Defensive check: skip non-string arguments to prevent TypeError + if (typeof arg !== 'string') { + throw new TypeError( + `Security: Argument must be a string, got ${typeof arg}. ` + + `This may indicate incorrect argument passing.` + ); + } + + for (const char of SHELL_METACHARACTERS) { + if (arg.includes(char)) { + throw new Error( + `Security: Argument contains shell metacharacter '${char}' which could enable command injection. ` + + `Argument: "${arg}"` + ); + } + } + } +} + const args = process.argv.slice(2); const PLATFORM_FLAGS = new Map([ @@ -92,11 +141,17 @@ function buildEnv(frontendDir) { } function runCommand(command, commandArgs, cwd, env) { + // Validate arguments to prevent command injection via shell metacharacters. + // Note: validateArgs only validates on Windows because shell: true is only used on Windows. + // On non-Windows platforms, .cmd files are not used and shell: false, so no injection risk. + validateArgs(commandArgs); + const bin = isWindows() ? `${command}.cmd` : command; const result = spawnSync(bin, commandArgs, { cwd, env, stdio: 'inherit', + shell: isWindows(), }); if (result.error) { @@ -219,7 +274,13 @@ async function main() { runCommand('electron-builder', builderArgs, frontendDir, env); } -main().catch((err) => { - console.error(`[package] Error: ${err.message}`); - process.exitCode = 1; -}); +// Run main() only when this file is executed directly (not when imported for testing) +if (require.main === module) { + main().catch((err) => { + console.error(`[package] Error: ${err.message}`); + process.exitCode = 1; + }); +} + +// Export for testing +module.exports = { validateArgs, SHELL_METACHARACTERS }; diff --git a/apps/frontend/scripts/package-with-python.d.ts b/apps/frontend/scripts/package-with-python.d.ts new file mode 100644 index 00000000..7bf561d5 --- /dev/null +++ b/apps/frontend/scripts/package-with-python.d.ts @@ -0,0 +1,5 @@ +/** + * Type declarations for package-with-python.cjs + */ +export declare const SHELL_METACHARACTERS: readonly string[]; +export declare function validateArgs(commandArgs: string[]): void; diff --git a/apps/frontend/src/main/__tests__/package-with-python.test.ts b/apps/frontend/src/main/__tests__/package-with-python.test.ts new file mode 100644 index 00000000..45849dcc --- /dev/null +++ b/apps/frontend/src/main/__tests__/package-with-python.test.ts @@ -0,0 +1,218 @@ +/** + * Unit tests for package-with-python.cjs security validation + * + * Tests the validateArgs function which prevents command injection via + * shell metacharacters when shell: true is used on Windows. + */ + +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; +import path from 'node:path'; +// Import from the scripts directory (relative to src/main/__tests__) +// @ts-expect-error - TypeScript doesn't auto-resolve .d.ts for .cjs imports (types exist in package-with-python.d.ts) +import { validateArgs, SHELL_METACHARACTERS } from '../../../scripts/package-with-python.cjs'; + +// Mock the isWindows function from platform.cjs +const originalPlatform = process.platform; + +describe('validateArgs', () => { + // We need to mock the isWindows function by modifying process.platform + // since the platform.cjs module uses process.platform === 'win32' to check + + afterEach(() => { + // Restore original platform after each test + Object.defineProperty(process, 'platform', { + value: originalPlatform, + writable: true, + configurable: true, + }); + }); + + describe('on Windows (shell injection risk)', () => { + beforeEach(() => { + Object.defineProperty(process, 'platform', { + value: 'win32', + writable: true, + configurable: true, + }); + }); + + describe('should throw for shell metacharacters', () => { + // Test each metacharacter individually + it.each([ + ['&', 'command & malicious'], + ['|', 'command | malicious'], + ['>', 'output > file.txt'], + ['<', 'command < input.txt'], + ['^', 'escape ^ character'], + ['%', '%PATH%'], + [';', 'command ; malicious'], + ['$', '$variable'], + ['(', 'command (group)'], + [')', 'command)after'], + ['[', 'array[index]'], + [']', 'command]after'], + ['{', '{block}'], + ['}', 'command}after'], + ['!', '!delayed!'], + ['"', '"quoted"'], + ['`', 'command `subshell`'], + ['\n', 'command\nnext'], + ['\r', 'command\rnext'], + ])('should throw for metacharacter "%s"', (char, arg) => { + expect(() => validateArgs([arg])).toThrowError(/shell metacharacter/); + expect(() => validateArgs([arg])).toThrowError(new RegExp(`\\${char}`)); + }); + + // Test metacharacters in different positions + it('should throw when metacharacter is at the start', () => { + expect(() => validateArgs(['& malicious'])).toThrow(); + }); + + it('should throw when metacharacter is in the middle', () => { + expect(() => validateArgs(['config&malicious'])).toThrow(); + }); + + it('should throw when metacharacter is at the end', () => { + expect(() => validateArgs(['config&'])).toThrow(); + }); + + // Test multiple metacharacters + it('should throw for multiple metacharacters in one argument', () => { + expect(() => validateArgs(['& | >'])).toThrow(); + }); + + // Test metacharacters across multiple arguments + it('should throw for metacharacters in different arguments', () => { + expect(() => validateArgs(['--flag', 'value&', 'other'])).toThrow(); + }); + + // Test error message includes the offending argument + it('should include offending argument in error message', () => { + expect(() => validateArgs(['file&evil.exe'])) + .toThrowError(/Argument: "file&evil\.exe"/); + }); + }); + + describe('should throw for non-string arguments', () => { + it('should throw TypeError for null argument', () => { + expect(() => validateArgs([null])).toThrowError(TypeError); + expect(() => validateArgs([null])).toThrowError(/must be a string/); + }); + + it('should throw TypeError for undefined argument', () => { + expect(() => validateArgs([undefined])).toThrowError(TypeError); + expect(() => validateArgs([undefined])).toThrowError(/must be a string/); + }); + + it('should throw TypeError for number argument', () => { + expect(() => validateArgs([123])).toThrowError(TypeError); + expect(() => validateArgs([123])).toThrowError(/got number/); + }); + + it('should throw TypeError for object argument', () => { + expect(() => validateArgs([{ key: 'value' }])).toThrowError(TypeError); + expect(() => validateArgs([{ key: 'value' }])).toThrowError(/got object/); + }); + + it('should throw TypeError for mixed valid and invalid arguments', () => { + expect(() => validateArgs(['--flag', null])).toThrowError(TypeError); + }); + }); + + describe('should NOT throw for safe inputs', () => { + it('should allow empty array', () => { + expect(() => validateArgs([])).not.toThrow(); + }); + + it('should allow alphanumeric arguments', () => { + expect(() => validateArgs(['build', 'test', 'production'])).not.toThrow(); + }); + + it('should allow flag arguments', () => { + expect(() => validateArgs(['--win', '--x64', '--publish=never'])).not.toThrow(); + }); + + it('should allow paths with forward slashes', () => { + expect(() => validateArgs(['../config/file.txt'])).not.toThrow(); + }); + + it('should allow paths with backslashes', () => { + // Use path.win32.join to construct a Windows-style path without hardcoding system locations + const windowsPath = path.win32.join('C:', 'Apps', 'App', 'config.txt'); + expect(() => validateArgs([windowsPath])).not.toThrow(); + }); + + it('should allow dots and hyphens', () => { + expect(() => validateArgs(['--config.file', 'my-config.json'])).not.toThrow(); + }); + + it('should allow underscores', () => { + expect(() => validateArgs(['my_config_file', '--output_dir'])).not.toThrow(); + }); + + it('should allow @ symbol', () => { + expect(() => validateArgs(['@lydell/node-pty'])).not.toThrow(); + }); + + it('should allow equals sign', () => { + expect(() => validateArgs(['--publish=never'])).not.toThrow(); + }); + + it('should allow common electron-builder arguments', () => { + expect(() => validateArgs([ + '--win', + '--x64', + '--publish', + 'never', + '--config', + 'config.yml' + ])).not.toThrow(); + }); + }); + }); + + describe('on non-Windows platforms', () => { + it('should return immediately on macOS without throwing', () => { + Object.defineProperty(process, 'platform', { + value: 'darwin', + writable: true, + configurable: true, + }); + + // Even with metacharacters, should not throw on non-Windows + expect(() => validateArgs(['command & malicious'])).not.toThrow(); + }); + + it('should return immediately on Linux without throwing', () => { + Object.defineProperty(process, 'platform', { + value: 'linux', + writable: true, + configurable: true, + }); + + // Even with metacharacters, should not throw on non-Windows + expect(() => validateArgs(['command & malicious'])).not.toThrow(); + }); + + it('should allow empty array on macOS', () => { + Object.defineProperty(process, 'platform', { + value: 'darwin', + writable: true, + configurable: true, + }); + + expect(() => validateArgs([])).not.toThrow(); + }); + }); +}); + +describe('SHELL_METACHARACTERS constant', () => { + it('should contain all expected dangerous characters', () => { + const expectedChars = [ + '&', '|', '>', '<', '^', '%', ';', '$', + '(', ')', '[', ']', '{', '}', + '!', '"', '`', '\n', '\r' + ]; + expect(SHELL_METACHARACTERS).toEqual(expect.arrayContaining(expectedChars)); + }); +});