Skip to content

Commit

Permalink
Fix handling special chars in script paths
Browse files Browse the repository at this point in the history
This commit improves the handling of paths with spaces or special
characters during script execution in the desktop application.

Key improvements:

- Paths are now quoted for macOS/Linux, addressing issues with
  whitespace or single quotes.
- Windows paths are enclosed in double quotes to handle special
  characters.

Other supporting changes:

- Add more documentation for terminal execution commands.
- Refactor terminal script file execution into a dedicated file for
  improved separation of concerns.
- Refactor naming of `RuntimeEnvironment` to align with naming
  conventions (no interface with I prefix) and for clarity.
- Refactor `TemporaryFileCodeRunner` to simplify it by removing the `os`
  parameter and handling OS-specific logic within the filename generator
  instead.
- Refactor `fileName` to `filename` for consistency.
  • Loading branch information
undergroundwires committed Jan 2, 2024
1 parent fac72ed commit 40f5eb8
Show file tree
Hide file tree
Showing 27 changed files with 573 additions and 316 deletions.
3 changes: 0 additions & 3 deletions src/application/CodeRunner.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { OperatingSystem } from '@/domain/OperatingSystem';

export interface CodeRunner {
runCode(
code: string,
tempScriptFolderName: string,
os: OperatingSystem,
): Promise<void>;
}
4 changes: 2 additions & 2 deletions src/application/Context/ApplicationContextFactory.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { IApplicationContext } from '@/application/Context/IApplicationContext';
import { OperatingSystem } from '@/domain/OperatingSystem';
import { IApplication } from '@/domain/IApplication';
import { RuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironment';
import { HostRuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/HostRuntimeEnvironment';
import { IApplicationFactory } from '../IApplicationFactory';
import { ApplicationFactory } from '../ApplicationFactory';
import { ApplicationContext } from './ApplicationContext';

export async function buildContext(
factory: IApplicationFactory = ApplicationFactory.Current,
environment = RuntimeEnvironment.CurrentEnvironment,
environment = HostRuntimeEnvironment.CurrentEnvironment,
): Promise<IApplicationContext> {
const app = await factory.getApp();
const os = getInitialOs(app, environment.os);
Expand Down
3 changes: 3 additions & 0 deletions src/infrastructure/CodeRunner/Execution/ScriptFileExecutor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface ScriptFileExecutor {
executeScriptFile(filePath: string): Promise<void>;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { OperatingSystem } from '@/domain/OperatingSystem';
import { CommandOps, SystemOperations } from '@/infrastructure/CodeRunner/SystemOperations/SystemOperations';
import { Logger } from '@/application/Common/Log/Logger';
import { ElectronLogger } from '@/infrastructure/Log/ElectronLogger';
import { RuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironment';
import { HostRuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/HostRuntimeEnvironment';
import { createNodeSystemOperations } from '@/infrastructure/CodeRunner/SystemOperations/NodeSystemOperations';
import { ScriptFileExecutor } from './ScriptFileExecutor';

export class VisibleTerminalScriptExecutor implements ScriptFileExecutor {
constructor(
private readonly system: SystemOperations = createNodeSystemOperations(),
private readonly logger: Logger = ElectronLogger,
private readonly environment: RuntimeEnvironment = HostRuntimeEnvironment.CurrentEnvironment,
) { }

public async executeScriptFile(filePath: string): Promise<void> {
const { os } = this.environment;
if (os === undefined) {
throw new Error('Unknown operating system');
}
await this.setFileExecutablePermissions(filePath);
await this.runFileWithRunner(filePath, os);
}

private async setFileExecutablePermissions(filePath: string): Promise<void> {
this.logger.info(`Setting execution permissions for file at ${filePath}`);
await this.system.fileSystem.setFilePermissions(filePath, '755');
this.logger.info(`Execution permissions set successfully for ${filePath}`);
}

private async runFileWithRunner(filePath: string, os: OperatingSystem): Promise<void> {
this.logger.info(`Executing script file: ${filePath} on ${OperatingSystem[os]}.`);
const runner = TerminalRunners[os];
if (!runner) {
throw new Error(`Unsupported operating system: ${OperatingSystem[os]}`);
}
const context: TerminalExecutionContext = {
scriptFilePath: filePath,
commandOps: this.system.command,
logger: this.logger,
};
await runner(context);
this.logger.info('Command script file successfully.');
}
}

interface TerminalExecutionContext {
readonly scriptFilePath: string;
readonly commandOps: CommandOps;
readonly logger: Logger;
}

type TerminalRunner = (context: TerminalExecutionContext) => Promise<void>;

const TerminalRunners: Partial<Record<OperatingSystem, TerminalRunner>> = {
[OperatingSystem.Windows]: async (context) => {
/*
Options:
"path":
✅ Launches the script within `cmd.exe`.
✅ Uses user-friendly GUI sudo prompt.
*/
const command = cmdShellPathArgumentEscape(context.scriptFilePath);
await runCommand(command, context);
},
[OperatingSystem.Linux]: async (context) => {
const command = `x-terminal-emulator -e ${posixShellPathArgumentEscape(context.scriptFilePath)}`;
/*
Options:
`x-terminal-emulator -e`:
✅ Launches the script within the default terminal emulator.
❌ Requires terminal-based (not GUI) sudo prompt, which may not be very user friendly.
*/
await runCommand(command, context);
},
[OperatingSystem.macOS]: async (context) => {
const command = `open -a Terminal.app ${posixShellPathArgumentEscape(context.scriptFilePath)}`;
/*
Options:
`open -a Terminal.app`:
✅ Launches the script within Terminal app, that exists natively in all modern macOS
versions.
❌ Requires terminal-based (not GUI) sudo prompt, which may not be very user friendly.
❌ Terminal app requires many privileges to execute the script, this would prompt user
to grant privileges to the Terminal app.
`osascript -e "do shell script \\"${scriptPath}\\" with administrator privileges"`:
✅ Uses user-friendly GUI sudo prompt.
❌ Executes the script in the background, which does not provide the user with immediate
visual feedback or allow interaction with the script as it runs.
*/
await runCommand(command, context);
},
} as const;

async function runCommand(command: string, context: TerminalExecutionContext): Promise<void> {
context.logger.info(`Executing command:\n${command}`);
await context.commandOps.exec(command);
context.logger.info('Executed command successfully.');
}

function posixShellPathArgumentEscape(pathArgument: string): string {
// - Wraps the path in single quotes, which is a standard practice in POSIX shells
// (like bash and zsh) found on macOS/Linux to ensure that characters like spaces, '*', and
// '?' are treated as literals, not as special characters.
// - Escapes any single quotes within the path itself. This allows paths containing single
// quotes to be correctly interpreted in POSIX-compliant systems, such as Linux and macOS.
return `'${pathArgument.replaceAll('\'', '\'\\\'\'')}'`;
}

function cmdShellPathArgumentEscape(pathArgument: string): string {
// - Encloses the path in double quotes, which is necessary for Windows command line (cmd.exe)
// to correctly handle paths containing spaces.
// - Paths in Windows cannot include double quotes `"` themselves, so these are not escaped.
return `"${pathArgument}"`;
}
3 changes: 3 additions & 0 deletions src/infrastructure/CodeRunner/Filename/FilenameGenerator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface FilenameGenerator {
generateFilename(): string;
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { OperatingSystem } from '@/domain/OperatingSystem';
import { RuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironment';
import { HostRuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/HostRuntimeEnvironment';
import { FilenameGenerator } from './FilenameGenerator';

/**
* Generates a timestamped filename specific to the given operating system.
Expand All @@ -7,13 +10,22 @@ import { OperatingSystem } from '@/domain/OperatingSystem';
* - A timestamp for uniqueness and easier auditability.
* - File extension based on the operating system.
*/
export function generateOsTimestampedFileName(
currentOs: OperatingSystem,
date = new Date(),
): string {
const baseFileName = `run-${createTimeStampForFile(date)}`;
const extension = FileExtensions[currentOs];
return extension ? `${baseFileName}.${extension}` : baseFileName;
export class OsTimestampedFilenameGenerator implements FilenameGenerator {
private readonly currentOs?: OperatingSystem;

constructor(
environment: RuntimeEnvironment = HostRuntimeEnvironment.CurrentEnvironment,
) {
this.currentOs = environment.os;
}

public generateFilename(
date = new Date(),
): string {
const baseFileName = `run-${createTimeStampForFile(date)}`;
const extension = this.currentOs === undefined ? undefined : FileExtensions[this.currentOs];
return extension ? `${baseFileName}.${extension}` : baseFileName;
}
}

const FileExtensions: Partial<Record<OperatingSystem, string>> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function createNodeSystemOperations(): SystemOperations {
) => writeFile(filePath, data),
},
command: {
execute: (command) => new Promise((resolve, reject) => {
exec: (command) => new Promise((resolve, reject) => {
exec(command, (error) => {
if (error) {
reject(error);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface LocationOps {
}

export interface CommandOps {
execute(command: string): Promise<void>;
exec(command: string): Promise<void>;
}

export interface FileSystemOps {
Expand Down
61 changes: 12 additions & 49 deletions src/infrastructure/CodeRunner/TemporaryFileCodeRunner.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
import { OperatingSystem } from '@/domain/OperatingSystem';
import { CodeRunner } from '@/application/CodeRunner';
import { Logger } from '@/application/Common/Log/Logger';
import { ElectronLogger } from '../Log/ElectronLogger';
import { SystemOperations } from './SystemOperations/SystemOperations';
import { createNodeSystemOperations } from './SystemOperations/NodeSystemOperations';
import { generateOsTimestampedFileName } from './FileNameGenerator';

export type FileNameGenerator = (os: OperatingSystem) => string;
import { FilenameGenerator } from './Filename/FilenameGenerator';
import { ScriptFileExecutor } from './Execution/ScriptFileExecutor';
import { VisibleTerminalScriptExecutor } from './Execution/VisibleTerminalScriptFileExecutor';
import { OsTimestampedFilenameGenerator } from './Filename/OsTimestampedFilenameGenerator';

export class TemporaryFileCodeRunner implements CodeRunner {
constructor(
private readonly system: SystemOperations = createNodeSystemOperations(),
private readonly fileNameGenerator: FileNameGenerator = generateOsTimestampedFileName,
private readonly filenameGenerator: FilenameGenerator = new OsTimestampedFilenameGenerator(),
private readonly logger: Logger = ElectronLogger,
private readonly scriptFileExecutor: ScriptFileExecutor = new VisibleTerminalScriptExecutor(),
) { }

public async runCode(
code: string,
tempScriptFolderName: string,
os: OperatingSystem,
): Promise<void> {
this.logger.info(`Starting running code for OS: ${OperatingSystem[os]}`);
this.logger.info('Starting running code.');
try {
const fileName = this.fileNameGenerator(os);
const filePath = await this.createTemporaryFile(fileName, tempScriptFolderName, code);
await this.executeFile(filePath, os);
const filename = this.filenameGenerator.generateFilename();
const filePath = await this.createTemporaryFile(filename, tempScriptFolderName, code);
await this.scriptFileExecutor.executeScriptFile(filePath);
this.logger.info(`Successfully executed script at ${filePath}`);
} catch (error) {
this.logger.error(`Error executing script: ${error.message}`, error);
Expand All @@ -33,7 +33,7 @@ export class TemporaryFileCodeRunner implements CodeRunner {
}

private async createTemporaryFile(
fileName: string,
filename: string,
tempScriptFolderName: string,
contents: string,
): Promise<string> {
Expand All @@ -42,7 +42,7 @@ export class TemporaryFileCodeRunner implements CodeRunner {
tempScriptFolderName,
);
await this.createDirectoryIfNotExists(directoryPath);
const filePath = this.system.location.combinePaths(directoryPath, fileName);
const filePath = this.system.location.combinePaths(directoryPath, filename);
await this.createFile(filePath, contents);
return filePath;
}
Expand All @@ -58,41 +58,4 @@ export class TemporaryFileCodeRunner implements CodeRunner {
await this.system.fileSystem.createDirectory(directoryPath, true);
this.logger.info(`Directory confirmed at: ${directoryPath}`);
}

private async executeFile(filePath: string, os: OperatingSystem): Promise<void> {
await this.setFileExecutablePermissions(filePath);
const command = getExecuteCommand(filePath, os);
await this.executeCommand(command);
}

private async setFileExecutablePermissions(filePath: string): Promise<void> {
this.logger.info(`Setting execution permissions for file at ${filePath}`);
await this.system.fileSystem.setFilePermissions(filePath, '755');
this.logger.info(`Execution permissions set successfully for ${filePath}`);
}

private async executeCommand(command: string): Promise<void> {
this.logger.info(`Executing command: ${command}`);
await this.system.command.execute(command);
this.logger.info('Command executed successfully.');
}
}

function getExecuteCommand(
scriptPath: string,
os: OperatingSystem,
): string {
switch (os) {
case OperatingSystem.Linux:
return `x-terminal-emulator -e '${scriptPath}'`;
case OperatingSystem.macOS:
return `open -a Terminal.app ${scriptPath}`;
// Another option with graphical sudo would be
// `osascript -e "do shell script \\"${scriptPath}\\" with administrator privileges"`
// However it runs in background
case OperatingSystem.Windows:
return scriptPath;
default:
throw Error(`unsupported os: ${OperatingSystem[os]}`);
}
}
51 changes: 51 additions & 0 deletions src/infrastructure/RuntimeEnvironment/HostRuntimeEnvironment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { OperatingSystem } from '@/domain/OperatingSystem';
import { WindowVariables } from '@/infrastructure/WindowVariables/WindowVariables';
import { IEnvironmentVariables } from '@/infrastructure/EnvironmentVariables/IEnvironmentVariables';
import { EnvironmentVariablesFactory } from '@/infrastructure/EnvironmentVariables/EnvironmentVariablesFactory';
import { ConditionBasedOsDetector } from './BrowserOs/ConditionBasedOsDetector';
import { BrowserEnvironment, BrowserOsDetector } from './BrowserOs/BrowserOsDetector';
import { RuntimeEnvironment } from './RuntimeEnvironment';
import { isTouchEnabledDevice } from './TouchSupportDetection';

export class HostRuntimeEnvironment implements RuntimeEnvironment {
public static readonly CurrentEnvironment
: RuntimeEnvironment = new HostRuntimeEnvironment(window);

public readonly isDesktop: boolean;

public readonly os: OperatingSystem | undefined;

public readonly isNonProduction: boolean;

protected constructor(
window: Partial<Window>,
environmentVariables: IEnvironmentVariables = EnvironmentVariablesFactory.Current.instance,
browserOsDetector: BrowserOsDetector = new ConditionBasedOsDetector(),
touchDetector = isTouchEnabledDevice,
) {
if (!window) { throw new Error('missing window'); } // do not trust strictNullChecks for global objects
this.isNonProduction = environmentVariables.isNonProduction;
this.isDesktop = isDesktop(window);
if (this.isDesktop) {
this.os = window?.os;
} else {
this.os = undefined;
const userAgent = getUserAgent(window);
if (userAgent) {
const browserEnvironment: BrowserEnvironment = {
userAgent,
isTouchSupported: touchDetector(),
};
this.os = browserOsDetector.detect(browserEnvironment);
}
}
}
}

function getUserAgent(window: Partial<Window>): string | undefined {
return window?.navigator?.userAgent;
}

function isDesktop(window: Partial<WindowVariables>): boolean {
return window?.isDesktop === true;
}
7 changes: 0 additions & 7 deletions src/infrastructure/RuntimeEnvironment/IRuntimeEnvironment.ts

This file was deleted.

0 comments on commit 40f5eb8

Please sign in to comment.