fix: removed unsafe shell when executing process

Ref: PNX-3671

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

Author: Akos Kitta

arduino-ide-extension/package.json

@@ -110,11 +110,9 @@
   "devDependencies": {
     "@octokit/rest": "^18.12.0",
     "@types/chai": "^4.2.7",
-    "@types/chai-string": "^1.4.2",
     "@types/mocha": "^5.2.7",
     "@types/react-window": "^1.8.5",
     "chai": "^4.2.0",
-    "chai-string": "^1.5.0",
     "decompress": "^4.2.0",
     "decompress-tarbz2": "^4.1.1",
     "decompress-targz": "^4.1.1",

arduino-ide-extension/src/common/protocol/executable-service.ts

@@ -5,6 +5,5 @@ export interface ExecutableService {
     clangdUri: string;
     cliUri: string;
     lsUri: string;
-    fwuploaderUri: string;
   }>;
 }

arduino-ide-extension/src/node/arduino-daemon-impl.ts

@@ -44,7 +44,6 @@ export class ArduinoDaemonImpl
 
   private _running = false;
   private _port = new Deferred<string>();
-  private _execPath: string | undefined;
 
   // Backend application lifecycle.
 
@@ -68,7 +67,7 @@ export class ArduinoDaemonImpl
   async start(): Promise<string> {
     try {
       this.toDispose.dispose(); // This will `kill` the previously started daemon process, if any.
-      const cliPath = await this.getExecPath();
+      const cliPath = this.getExecPath();
       this.onData(`Starting daemon from ${cliPath}...`);
       const { daemon, port } = await this.spawnDaemonProcess();
       // Watchdog process for terminating the daemon process when the backend app terminates.
@@ -132,12 +131,8 @@ export class ArduinoDaemonImpl
     return this.onDaemonStoppedEmitter.event;
   }
 
-  async getExecPath(): Promise<string> {
-    if (this._execPath) {
-      return this._execPath;
-    }
-    this._execPath = await getExecPath('arduino-cli', this.onError.bind(this));
-    return this._execPath;
+  getExecPath(): string {
+    return getExecPath('arduino-cli');
   }
 
   protected async getSpawnArgs(): Promise<string[]> {
@@ -151,7 +146,7 @@ export class ArduinoDaemonImpl
       '--port',
       '0',
       '--config-file',
-      `"${cliConfigPath}"`,
+      cliConfigPath,
       '-v',
     ];
     if (debug) {
@@ -173,10 +168,8 @@ export class ArduinoDaemonImpl
     daemon: ChildProcess;
     port: string;
   }> {
-    const [cliPath, args] = await Promise.all([
-      this.getExecPath(),
-      this.getSpawnArgs(),
-    ]);
+    const args = await this.getSpawnArgs();
+    const cliPath = this.getExecPath();
     const ready = new Deferred<{ daemon: ChildProcess; port: string }>();
     const options = { shell: true };
     const daemon = spawn(`"${cliPath}"`, args, options);

arduino-ide-extension/src/node/arduino-firmware-uploader-impl.ts

@@ -1,45 +1,22 @@
+import { ILogger } from '@theia/core/lib/common/logger';
+import { inject, injectable, named } from '@theia/core/shared/inversify';
+import type { Port } from '../common/protocol';
 import {
   ArduinoFirmwareUploader,
   FirmwareInfo,
 } from '../common/protocol/arduino-firmware-uploader';
-import { injectable, inject, named } from '@theia/core/shared/inversify';
-import { ExecutableService, Port } from '../common/protocol';
 import { getExecPath, spawnCommand } from './exec-util';
-import { ILogger } from '@theia/core/lib/common/logger';
 import { MonitorManager } from './monitor-manager';
 
 @injectable()
 export class ArduinoFirmwareUploaderImpl implements ArduinoFirmwareUploader {
-  @inject(ExecutableService)
-  protected executableService: ExecutableService;
-
-  protected _execPath: string | undefined;
-
   @inject(ILogger)
   @named('fwuploader')
-  protected readonly logger: ILogger;
-
+  private readonly logger: ILogger;
   @inject(MonitorManager)
-  protected readonly monitorManager: MonitorManager;
-
-  protected onError(error: any): void {
-    this.logger.error(error);
-  }
-
-  async getExecPath(): Promise<string> {
-    if (this._execPath) {
-      return this._execPath;
-    }
-    this._execPath = await getExecPath('arduino-fwuploader');
-    return this._execPath;
-  }
-
-  async runCommand(args: string[]): Promise<any> {
-    const execPath = await this.getExecPath();
-    return await spawnCommand(`"${execPath}"`, args, this.onError.bind(this));
-  }
+  private readonly monitorManager: MonitorManager;
 
-  async uploadCertificates(command: string): Promise<any> {
+  async uploadCertificates(command: string): Promise<string> {
     return await this.runCommand(['certificates', 'flash', command]);
   }
 
@@ -70,14 +47,13 @@ export class ArduinoFirmwareUploaderImpl implements ArduinoFirmwareUploader {
   }
 
   async flash(firmware: FirmwareInfo, port: Port): Promise<string> {
-    let output;
     const board = {
       name: firmware.board_name,
       fqbn: firmware.board_fqbn,
     };
     try {
       await this.monitorManager.notifyUploadStarted(board.fqbn, port);
-      output = await this.runCommand([
+      const output = await this.runCommand([
         'firmware',
         'flash',
         '--fqbn',
@@ -87,11 +63,18 @@ export class ArduinoFirmwareUploaderImpl implements ArduinoFirmwareUploader {
         '--module',
         `${firmware.module}@${firmware.firmware_version}`,
       ]);
-    } catch (e) {
-      throw e;
+      return output;
     } finally {
       await this.monitorManager.notifyUploadFinished(board.fqbn, port);
-      return output;
     }
   }
+
+  private onError(error: Error): void {
+    this.logger.error(error);
+  }
+
+  private async runCommand(args: string[]): Promise<string> {
+    const execPath = getExecPath('arduino-fwuploader');
+    return await spawnCommand(execPath, args, this.onError.bind(this));
+  }
 }

arduino-ide-extension/src/node/clang-formatter.ts

@@ -1,4 +1,3 @@
-import * as os from 'node:os';
 import { EnvVariablesServer } from '@theia/core/lib/common/env-variables';
 import { MaybePromise } from '@theia/core/lib/common/types';
 import { FileUri } from '@theia/core/lib/node/file-uri';
@@ -15,7 +14,7 @@ export class ClangFormatter implements Formatter {
   private readonly configService: ConfigService;
 
   @inject(EnvVariablesServer)
-  private readonly envVariableServer: EnvVariablesServer;
+  private readonly envVariablesServer: EnvVariablesServer;
 
   async format({
     content,
@@ -26,26 +25,19 @@ export class ClangFormatter implements Formatter {
     formatterConfigFolderUris: string[];
     options?: FormatterOptions;
   }): Promise<string> {
-    const [execPath, style] = await Promise.all([
-      this.execPath(),
-      this.style(formatterConfigFolderUris, options),
-    ]);
+    const execPath = this.execPath();
+    const args = await this.styleArgs(formatterConfigFolderUris, options);
     const formatted = await spawnCommand(
-      `"${execPath}"`,
-      [style],
+      execPath,
+      args,
       console.error,
       content
     );
     return formatted;
   }
 
-  private _execPath: string | undefined;
-  private async execPath(): Promise<string> {
-    if (this._execPath) {
-      return this._execPath;
-    }
-    this._execPath = await getExecPath('clang-format');
-    return this._execPath;
+  private execPath(): string {
+    return getExecPath('clang-format');
   }
 
   /**
@@ -60,10 +52,10 @@ export class ClangFormatter implements Formatter {
    *
    * See: https://github.com/arduino/arduino-ide/issues/566
    */
-  private async style(
+  private async styleArgs(
     formatterConfigFolderUris: string[],
     options?: FormatterOptions
-  ): Promise<string> {
+  ): Promise<string[]> {
     const clangFormatPaths = await Promise.all([
       ...formatterConfigFolderUris.map((uri) => this.clangConfigPath(uri)),
       this.clangConfigPath(this.configDirPath()),
@@ -74,9 +66,9 @@ export class ClangFormatter implements Formatter {
       console.debug(
         `Using ${ClangFormatFile} style configuration from '${first}'.`
       );
-      return `-style=file:"${first}"`;
+      return ['-style', `file:${first}`];
     }
-    return `-style="${style(toClangOptions(options))}"`;
+    return ['-style', style(toClangOptions(options))];
   }
 
   private async dataDirPath(): Promise<string | undefined> {
@@ -88,7 +80,7 @@ export class ClangFormatter implements Formatter {
   }
 
   private async configDirPath(): Promise<string> {
-    const configDirUri = await this.envVariableServer.getConfigDirUri();
+    const configDirUri = await this.envVariablesServer.getConfigDirUri();
     return FileUri.fsPath(configDirUri);
   }
 
@@ -129,30 +121,14 @@ function toClangOptions(
   return { UseTab: 'Never', TabWidth: 2 };
 }
 
-export function style({ TabWidth, UseTab }: ClangFormatOptions): string {
-  let styleArgument = JSON.stringify(styleJson({ TabWidth, UseTab })).replace(
-    /[\\"]/g,
-    '\\$&'
-  );
-  if (os.platform() === 'win32') {
-    // Windows command interpreter does not use backslash escapes. This causes the argument to have alternate quoted and
-    // unquoted sections.
-    // Special characters in the unquoted sections must be caret escaped.
-    const styleArgumentSplit = styleArgument.split('"');
-    for (let i = 1; i < styleArgumentSplit.length; i += 2) {
-      styleArgumentSplit[i] = styleArgumentSplit[i].replace(/[<>^|]/g, '^$&');
-    }
-
-    styleArgument = styleArgumentSplit.join('"');
-  }
-
-  return styleArgument;
+function style({ TabWidth, UseTab }: ClangFormatOptions): string {
+  return stringifyConfiguration(styleJson({ TabWidth, UseTab }));
 }
 
 function styleJson({
   TabWidth,
   UseTab,
-}: ClangFormatOptions): Record<string, unknown> {
+}: ClangFormatOptions): ClangConfiguration {
   // Source: https://github.com/arduino/tooling-project-assets/tree/main/other/clang-format-configuration
   const defaultConfig = require('../../src/node/default-formatter-config.json');
   return {
@@ -161,3 +137,26 @@ function styleJson({
     UseTab,
   };
 }
+
+export type ClangStyleValue =
+  | string
+  | number
+  | boolean
+  | ClangStyleValue[]
+  | { [key: string]: ClangStyleValue };
+export type ClangConfiguration = Record<string, ClangStyleValue>;
+
+export function stringifyConfiguration(
+  value: ClangStyleValue | ClangConfiguration
+): string {
+  if (Array.isArray(value)) {
+    return `[${value.map((item) => stringifyConfiguration(item)).join(', ')}]`;
+  } else if (typeof value === 'object') {
+    return `{${Object.entries(value)
+      .map(([key, v]) => `${key}: ${stringifyConfiguration(v)}`)
+      .join(', ')}}`;
+  } else if (typeof value === 'string') {
+    return `'${value}'`;
+  }
+  return String(value);
+}

arduino-ide-extension/src/node/config-service-impl.ts

@@ -222,8 +222,8 @@ export class ConfigServiceImpl
   }
 
   private async getFallbackCliConfig(): Promise<DefaultCliConfig> {
-    const cliPath = await this.daemon.getExecPath();
-    const rawJson = await spawnCommand(`"${cliPath}"`, [
+    const cliPath = this.daemon.getExecPath();
+    const rawJson = await spawnCommand(cliPath, [
       'config',
       'dump',
       'format',
@@ -233,13 +233,8 @@ export class ConfigServiceImpl
   }
 
   private async initCliConfigTo(fsPathToDir: string): Promise<void> {
-    const cliPath = await this.daemon.getExecPath();
-    await spawnCommand(`"${cliPath}"`, [
-      'config',
-      'init',
-      '--dest-dir',
-      `"${fsPathToDir}"`,
-    ]);
+    const cliPath = this.daemon.getExecPath();
+    await spawnCommand(cliPath, ['config', 'init', '--dest-dir', fsPathToDir]);
   }
 
   private async mapCliConfigToAppConfig(