-
-
Notifications
You must be signed in to change notification settings - Fork 172
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix script cancellation with new dialog on Linux
This commit improves the management of script execution process by enhancing the way terminal commands are handled, paving the way for easier future modifications and providing clearer feedback to users when scripts are cancelled. Previously, the UI displayed a generic error message which could lead to confusion if the user intentionally cancelled the script execution. Now, a specific error dialog will appear, improving the user experience by accurately reflecting the action taken by the user. This change affects code execution on Linux where closing GNOME terminal returns exit code `137` which is then treated by script cancellation by privacy.sexy to show the accurate error dialog. It does not affect macOS and Windows as curret commands result in success (`0`) exit code on cancellation. Additionally, this update encapsulates OS-specific logic into dedicated classes, promoting better separation of concerns and increasing the modularity of the codebase. This makes it simpler to maintain and extend the application. Key changes: - Display a specific error message for script cancellations. - Refactor command execution into dedicated classes. - Improve file permission setting flexibility and avoid setting file permissions on Windows as it's not required to execute files. - Introduce more granular error types for script execution. - Increase logging for shell commands to aid in debugging. - Expand test coverage to ensure reliability. - Fix error dialogs not showing the error messages due to incorrect propagation of errors. Other supported changes: - Update `SECURITY.md` with details on script readback and verification. - Fix a typo in `IpcRegistration.spec.ts`. - Document antivirus scans in `desktop-vs-web-features.md`.
- Loading branch information
1 parent
694bf1a
commit 8c17396
Showing
49 changed files
with
2,095 additions
and
604 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 changes: 5 additions & 0 deletions
5
src/infrastructure/CodeRunner/Execution/CommandDefinition/CommandDefinition.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export interface CommandDefinition { | ||
buildShellCommand(filePath: string): string; | ||
isExecutionTerminatedExternally(exitCode: number): boolean; | ||
isExecutablePermissionsRequiredOnFile(): boolean; | ||
} |
61 changes: 61 additions & 0 deletions
61
...astructure/CodeRunner/Execution/CommandDefinition/Commands/LinuxVisibleTerminalCommand.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import { PosixShellArgumentEscaper } from './ShellArgument/PosixShellArgumentEscaper'; | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
import type { ShellArgumentEscaper } from './ShellArgument/ShellArgumentEscaper'; | ||
|
||
export const LinuxTerminalEmulator = 'x-terminal-emulator'; | ||
|
||
export class LinuxVisibleTerminalCommand implements CommandDefinition { | ||
constructor( | ||
private readonly escaper: ShellArgumentEscaper = new PosixShellArgumentEscaper(), | ||
) { } | ||
|
||
public buildShellCommand(filePath: string): string { | ||
return `${LinuxTerminalEmulator} -e ${this.escaper.escapePathArgument(filePath)}`; | ||
/* | ||
🤔 Potential improvements: | ||
Use user-friendly GUI sudo prompt (not terminal-based). | ||
If `pkexec` exists, we could do `x-terminal-emulator -e pkexec 'path'`, which always | ||
prompts with user-friendly GUI sudo prompt. | ||
📝 Options: | ||
`x-terminal-emulator -e 'path'`: | ||
✅ Visible terminal window | ||
❌ Terminal-based (not GUI) sudo prompt. | ||
`x-terminal-emulator -e pkexec 'path' | ||
✅ Visible terminal window | ||
✅ Always prompts with user-friendly GUI sudo prompt. | ||
🤔 Not using `pkexec` as it is not in all Linux distributions. It should have smarter | ||
logic to handle if it does not exist. | ||
`electron.shell.openPath`: | ||
❌ Opens the script in the default text editor, verified on | ||
Debian/Ubuntu-based distributions. | ||
`child_process.execFile()`: | ||
❌ Script execution in the background without a visible terminal. | ||
*/ | ||
} | ||
|
||
public isExecutionTerminatedExternally(exitCode: number): boolean { | ||
return exitCode === 137; | ||
/* | ||
`x-terminal-emulator` may return exit code `137` under specific circumstances like when the | ||
user closes the terminal (observed with `gnome-terminal` on Pop!_OS). This exit code (128 + | ||
Unix signal 9) indicates the process was terminated by a SIGKILL signal, which can occur due | ||
to user action (cancelling the progress) or the system (e.g., due to memory shortages). | ||
Additional exit codes noted for future consideration (currently not handled as they have not | ||
been reproduced): | ||
- 130 (130 = 128 + Unix signal 2): Indicates the script was terminated by the user | ||
(Control-C), corresponding to a SIGINT signal. | ||
- 143 (128 + Unix signal 15): Indicates termination by a SIGTERM signal, suggesting a request | ||
to gracefully terminate the process. | ||
*/ | ||
} | ||
|
||
public isExecutablePermissionsRequiredOnFile(): boolean { | ||
/* | ||
On Linux, a script file without executable permissions cannot be run directly by its path | ||
without specifying a shell explicitly. | ||
*/ | ||
return true; | ||
} | ||
} |
46 changes: 46 additions & 0 deletions
46
...astructure/CodeRunner/Execution/CommandDefinition/Commands/MacOsVisibleTerminalCommand.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import { PosixShellArgumentEscaper } from './ShellArgument/PosixShellArgumentEscaper'; | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
import type { ShellArgumentEscaper } from './ShellArgument/ShellArgumentEscaper'; | ||
|
||
export class MacOsVisibleTerminalCommand implements CommandDefinition { | ||
constructor( | ||
private readonly escaper: ShellArgumentEscaper = new PosixShellArgumentEscaper(), | ||
) { } | ||
|
||
public buildShellCommand(filePath: string): string { | ||
return `open -a Terminal.app ${this.escaper.escapePathArgument(filePath)}`; | ||
/* | ||
📝 Options: | ||
`child_process.execFile()` | ||
"path", `cmd.exe /c "path"` | ||
❌ Script execution in the background without a visible terminal. | ||
This occurs only when the user runs the application as administrator, as seen | ||
in Windows Pro VMs on Azure. | ||
`PowerShell Start -Verb RunAs "path"` | ||
✅ Visible terminal window | ||
✅ GUI sudo prompt (through `RunAs` option) | ||
`PowerShell Start "path"` | ||
`explorer.exe "path"` | ||
`electron.shell.openPath` | ||
`start cmd.exe /c "$path"` | ||
✅ Visible terminal window | ||
✅ GUI sudo prompt (through `RunAs` option) | ||
👍 Among all options `start` command is the most explicit one, being the most resilient | ||
against the potential changes in Windows or Electron framework (e.g. https://github.com/electron/electron/issues/36765). | ||
`%COMSPEC%` environment variable should be checked before defaulting to `cmd.exe. | ||
Related docs: https://web.archive.org/web/20240106002357/https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | ||
*/ | ||
} | ||
|
||
public isExecutionTerminatedExternally(): boolean { | ||
return false; | ||
} | ||
|
||
public isExecutablePermissionsRequiredOnFile(): boolean { | ||
/* | ||
On macOS, a script file without executable permissions cannot be run directly by its path | ||
without specifying a shell explicitly. | ||
*/ | ||
return true; | ||
} | ||
} |
14 changes: 14 additions & 0 deletions
14
.../CodeRunner/Execution/CommandDefinition/Commands/ShellArgument/CmdShellArgumentEscaper.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import type { ShellArgumentEscaper } from './ShellArgumentEscaper'; | ||
|
||
export class CmdShellArgumentEscaper implements ShellArgumentEscaper { | ||
public escapePathArgument(pathArgument: string): string { | ||
return cmdShellPathArgumentEscape(pathArgument); | ||
} | ||
} | ||
|
||
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}"`; | ||
} |
18 changes: 18 additions & 0 deletions
18
...odeRunner/Execution/CommandDefinition/Commands/ShellArgument/PosixShellArgumentEscaper.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import type { ShellArgumentEscaper } from './ShellArgumentEscaper'; | ||
|
||
export class PosixShellArgumentEscaper implements ShellArgumentEscaper { | ||
public escapePathArgument(pathArgument: string): string { | ||
return posixShellPathArgumentEscape(pathArgument); | ||
} | ||
} | ||
|
||
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('\'', '\'\\\'\'')}'`; | ||
} |
3 changes: 3 additions & 0 deletions
3
...ure/CodeRunner/Execution/CommandDefinition/Commands/ShellArgument/ShellArgumentEscaper.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export interface ShellArgumentEscaper { | ||
escapePathArgument(pathArgument: string): string; | ||
} |
52 changes: 52 additions & 0 deletions
52
...tructure/CodeRunner/Execution/CommandDefinition/Commands/WindowsVisibleTerminalCommand.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
import { CmdShellArgumentEscaper } from './ShellArgument/CmdShellArgumentEscaper'; | ||
import type { ShellArgumentEscaper } from './ShellArgument/ShellArgumentEscaper'; | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
|
||
export class WindowsVisibleTerminalCommand implements CommandDefinition { | ||
constructor( | ||
private readonly escaper: ShellArgumentEscaper = new CmdShellArgumentEscaper(), | ||
) { } | ||
|
||
public buildShellCommand(filePath: string): string { | ||
const command = [ | ||
'PowerShell', | ||
'Start-Process', | ||
'-Verb RunAs', // Run as administrator with GUI sudo prompt | ||
`-FilePath ${this.escaper.escapePathArgument(filePath)}`, | ||
].join(' '); | ||
return command; | ||
/* | ||
📝 Options: | ||
`child_process.execFile()` | ||
"path", `cmd.exe /c "path"` | ||
❌ Script execution in the background without a visible terminal. | ||
This occurs only when the user runs the application as administrator, as seen | ||
in Windows Pro VMs on Azure. | ||
`PowerShell Start -Verb RunAs "path"` | ||
✅ Visible terminal window | ||
✅ GUI sudo prompt (through `RunAs` option) | ||
`PowerShell Start "path"` | ||
`explorer.exe "path"` | ||
`electron.shell.openPath` | ||
`start cmd.exe /c "$path"` | ||
✅ Visible terminal window | ||
✅ GUI sudo prompt (through `RunAs` option) | ||
👍 Among all options `start` command is the most explicit one, being the most resilient | ||
against the potential changes in Windows or Electron framework (e.g. https://github.com/electron/electron/issues/36765). | ||
`%COMSPEC%` environment variable should be checked before defaulting to `cmd.exe. | ||
Related docs: https://web.archive.org/web/20240106002357/https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | ||
*/ | ||
} | ||
|
||
public isExecutionTerminatedExternally(): boolean { | ||
return false; | ||
} | ||
|
||
public isExecutablePermissionsRequiredOnFile(): boolean { | ||
/* | ||
In Windows, whether a file can be executed is determined by its file extension | ||
(.exe, .bat, .cmd, etc.) rather than executable permissions set on the file. | ||
*/ | ||
return false; | ||
} | ||
} |
5 changes: 5 additions & 0 deletions
5
...infrastructure/CodeRunner/Execution/CommandDefinition/Factory/CommandDefinitionFactory.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
|
||
export interface CommandDefinitionFactory { | ||
provideCommandDefinition(): CommandDefinition; | ||
} |
40 changes: 40 additions & 0 deletions
40
.../CodeRunner/Execution/CommandDefinition/Factory/OsSpecificTerminalLaunchCommandFactory.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import { OperatingSystem } from '@/domain/OperatingSystem'; | ||
import { CurrentEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironmentFactory'; | ||
import type { RuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironment'; | ||
import { WindowsVisibleTerminalCommand } from '../Commands/WindowsVisibleTerminalCommand'; | ||
import { LinuxVisibleTerminalCommand } from '../Commands/LinuxVisibleTerminalCommand'; | ||
import { MacOsVisibleTerminalCommand } from '../Commands/MacOsVisibleTerminalCommand'; | ||
import type { CommandDefinitionFactory } from './CommandDefinitionFactory'; | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
|
||
export class OsSpecificTerminalLaunchCommandFactory implements CommandDefinitionFactory { | ||
constructor( | ||
private readonly environment: RuntimeEnvironment = CurrentEnvironment, | ||
) { } | ||
|
||
public provideCommandDefinition(): CommandDefinition { | ||
const { os } = this.environment; | ||
if (os === undefined) { | ||
throw new Error('Operating system could not be identified from environment.'); | ||
} | ||
return getOperatingSystemCommandDefinition(os); | ||
} | ||
} | ||
|
||
function getOperatingSystemCommandDefinition( | ||
operatingSystem: OperatingSystem, | ||
): CommandDefinition { | ||
const definition = SupportedDesktopCommandDefinitions[operatingSystem]; | ||
if (!definition) { | ||
throw new Error(`Unsupported operating system: ${OperatingSystem[operatingSystem]}`); | ||
} | ||
return definition; | ||
} | ||
|
||
const SupportedDesktopCommandDefinitions: Readonly<Partial<Record< | ||
OperatingSystem, | ||
CommandDefinition>>> = { | ||
[OperatingSystem.Windows]: new WindowsVisibleTerminalCommand(), | ||
[OperatingSystem.Linux]: new LinuxVisibleTerminalCommand(), | ||
[OperatingSystem.macOS]: new MacOsVisibleTerminalCommand(), | ||
} as const; |
9 changes: 9 additions & 0 deletions
9
src/infrastructure/CodeRunner/Execution/CommandDefinition/Runner/CommandDefinitionRunner.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import type { ScriptFileExecutionOutcome } from '../../ScriptFileExecutor'; | ||
import type { CommandDefinition } from '../CommandDefinition'; | ||
|
||
export interface CommandDefinitionRunner { | ||
runCommandDefinition( | ||
commandDefinition: CommandDefinition, | ||
filePath: string, | ||
): Promise<ScriptFileExecutionOutcome>; | ||
} |
Oops, something went wrong.