Skip to content

Commit

Permalink
fix: removed unsafe shell when executing process
Browse files Browse the repository at this point in the history 
Ref: PNX-3671

Signed-off-by: Akos Kitta <[email protected]>
  • Loading branch information
Akos Kitta committed May 16, 2023
1 parent e47fb2e commit d3dc5b8
Show file tree
Hide file tree
Showing 12 changed files with 407 additions and 221 deletions.
5 changes: 1 addition & 4 deletions arduino-ide-extension/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,14 @@
"temp": "^0.9.1",
"temp-dir": "^2.0.0",
"tree-kill": "^1.2.1",
"util": "^0.12.5",
"which": "^1.3.1"
"util": "^0.12.5"
},
"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",
Expand Down
1 change: 0 additions & 1 deletion arduino-ide-extension/src/common/protocol/executable-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,5 @@ export interface ExecutableService {
clangdUri: string;
cliUri: string;
lsUri: string;
fwuploaderUri: string;
}>;
}
19 changes: 6 additions & 13 deletions arduino-ide-extension/src/node/arduino-daemon-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ export class ArduinoDaemonImpl

private _running = false;
private _port = new Deferred<string>();
private _execPath: string | undefined;

// Backend application lifecycle.

Expand All @@ -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.
Expand Down Expand Up @@ -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[]> {
Expand All @@ -151,7 +146,7 @@ export class ArduinoDaemonImpl
'--port',
'0',
'--config-file',
`"${cliConfigPath}"`,
cliConfigPath,
'-v',
];
if (debug) {
Expand All @@ -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);
Expand Down
51 changes: 17 additions & 34 deletions arduino-ide-extension/src/node/arduino-firmware-uploader-impl.ts
View file
Original file line number Diff line number Diff line change
@@ -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]);
}

Expand Down Expand Up @@ -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',
Expand All @@ -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));
}
}
83 changes: 41 additions & 42 deletions arduino-ide-extension/src/node/clang-formatter.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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,
Expand All @@ -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');
}

/**
Expand All @@ -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()),
Expand All @@ -72,11 +64,11 @@ export class ClangFormatter implements Formatter {
const first = clangFormatPaths.filter(Boolean).shift();
if (first) {
console.debug(
`Using ${ClangFormatFile} style configuration from '${first}'.`
`Using ${clangFormatFilename} 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> {
Expand All @@ -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);
}

Expand All @@ -100,7 +92,7 @@ export class ClangFormatter implements Formatter {
return undefined;
}
const folderPath = FileUri.fsPath(uri);
const clangFormatPath = join(folderPath, ClangFormatFile);
const clangFormatPath = join(folderPath, clangFormatFilename);
try {
await fs.access(clangFormatPath, constants.R_OK);
return clangFormatPath;
Expand All @@ -115,7 +107,7 @@ interface ClangFormatOptions {
readonly TabWidth: number;
}

const ClangFormatFile = '.clang-format';
export const clangFormatFilename = '.clang-format';

function toClangOptions(
options?: FormatterOptions | undefined
Expand All @@ -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 {
Expand All @@ -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);
}
13 changes: 4 additions & 9 deletions arduino-ide-extension/src/node/config-service-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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(
Expand Down
Loading

0 comments on commit d3dc5b8

Please sign in to comment.