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 <stillknotknown@users.noreply.github.com>
2026-01-19 14:12:57 +02:00
parent 141f44f619
commit e482fdf156
3 changed files with 288 additions and 4 deletions
@@ -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 };
@@ -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;
@@ -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));
+  });
+});