-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(launch-app): Add auto-install dependencies with full UI feedback #1313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
youngmrz
wants to merge
18
commits into
AndyMik90:develop
Choose a base branch
from
youngmrz:feat/launch-app-auto-install
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+5,349
−1,002
Draft
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
5fcb13c
feat(#1119): Add Launch App button for Human Review tasks
youngmrz d304fb4
fix(types): add worktreeLaunchApp to ElectronAPI interface
youngmrz cfaafc9
fix(mock): add worktreeLaunchApp to workspace mock
youngmrz 3b97476
fix(terminal): verify terminal exists before spawn on Linux
youngmrz c50eeed
fix(security): prevent command injection in worktreeLaunchApp
youngmrz 6a166d8
fix: address automated review issues in launch-app handler
youngmrz 447bf92
test: add comprehensive tests for escapePathForShell security function
youngmrz 55624ea
fix(launch-app): address PR #1120 review issues
youngmrz 3a5053c
fix(types): resolve TypeScript errors in worktree-handlers and Worksp…
youngmrz 6483660
fix(security): improve shell escaping for terminal launch commands
youngmrz cb75b8b
fix(terminal): use --working-directory for xfce4-terminal path handling
youngmrz e97a8f7
fix(terminal): address PR #1120 bot review comments
youngmrz bbf87c6
fix: address remaining bot review feedback
youngmrz 918be7b
fix(launch-app): fix Windows command quoting for Launch App button
youngmrz 84640a3
Merge branch 'feat/launch-app-button' into feat/launch-app-auto-install
youngmrz 8c888f2
feat(launch-app): Add auto-install dependencies with UI feedback
youngmrz 7b27c8d
fix: Address bot feedback on launch app dependency handling
youngmrz 04edc76
fix(launch-app): Improve process cleanup and dependency detection
youngmrz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
270 changes: 270 additions & 0 deletions
270
apps/frontend/src/main/ipc-handlers/task/shell-escape.test.ts
This file contains hidden or 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,270 @@ | ||
| /** | ||
| * Unit tests for shell-escape utilities. | ||
| * Tests command injection prevention via path escaping. | ||
| */ | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { escapePathForShell, escapePathForAppleScript } from './shell-escape'; | ||
|
|
||
| describe('escapePathForShell', () => { | ||
| describe('null byte injection prevention', () => { | ||
| it('should reject paths containing null bytes', () => { | ||
| expect(escapePathForShell('/path/with\0null', 'linux')).toBeNull(); | ||
| expect(escapePathForShell('C:\\path\\with\0null', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject null byte at start of path', () => { | ||
| expect(escapePathForShell('\0/path', 'darwin')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject null byte at end of path', () => { | ||
| expect(escapePathForShell('/path\0', 'linux')).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('newline injection prevention', () => { | ||
| it('should reject paths containing LF newlines', () => { | ||
| expect(escapePathForShell('/path/with\nnewline', 'linux')).toBeNull(); | ||
| expect(escapePathForShell('C:\\path\\with\nnewline', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths containing CR newlines', () => { | ||
| expect(escapePathForShell('/path/with\rnewline', 'darwin')).toBeNull(); | ||
| expect(escapePathForShell('C:\\path\\with\rnewline', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths containing CRLF', () => { | ||
| expect(escapePathForShell('/path/with\r\nnewline', 'linux')).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Windows platform (win32)', () => { | ||
| it('should reject paths with < character', () => { | ||
| expect(escapePathForShell('C:\\path<file', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with > character', () => { | ||
| expect(escapePathForShell('C:\\path>file', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with | pipe character', () => { | ||
| expect(escapePathForShell('C:\\path|command', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with & ampersand', () => { | ||
| expect(escapePathForShell('C:\\path&command', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with ^ caret', () => { | ||
| expect(escapePathForShell('C:\\path^file', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with % percent', () => { | ||
| expect(escapePathForShell('C:\\path%VAR%', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with ! exclamation', () => { | ||
| expect(escapePathForShell('C:\\path!file', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths with ` backtick', () => { | ||
| expect(escapePathForShell('C:\\path`command`', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should escape double quotes with double-double quotes', () => { | ||
| expect(escapePathForShell('C:\\path\\"file"', 'win32')).toBe('C:\\path\\""file""'); | ||
| }); | ||
|
|
||
| it('should pass through safe Windows paths unchanged', () => { | ||
| expect(escapePathForShell('C:\\Users\\name\\project', 'win32')).toBe('C:\\Users\\name\\project'); | ||
| }); | ||
|
|
||
| it('should allow paths with spaces', () => { | ||
| expect(escapePathForShell('C:\\Program Files\\app', 'win32')).toBe('C:\\Program Files\\app'); | ||
| }); | ||
|
|
||
| it('should allow paths with parentheses', () => { | ||
| expect(escapePathForShell('C:\\Program Files (x86)\\app', 'win32')).toBe('C:\\Program Files (x86)\\app'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Unix platforms (linux, darwin)', () => { | ||
| it('should escape single quotes with quote-escape-quote pattern', () => { | ||
| const result = escapePathForShell("/path/with'quote", 'linux'); | ||
| expect(result).toBe("/path/with'\\''quote"); | ||
| }); | ||
|
|
||
| it('should handle multiple single quotes', () => { | ||
| const result = escapePathForShell("it's a 'test'", 'darwin'); | ||
| expect(result).toBe("it'\\''s a '\\''test'\\''"); | ||
| }); | ||
|
|
||
| it('should pass through safe Unix paths unchanged', () => { | ||
| expect(escapePathForShell('/usr/local/bin', 'linux')).toBe('/usr/local/bin'); | ||
| expect(escapePathForShell('/Users/name/project', 'darwin')).toBe('/Users/name/project'); | ||
| }); | ||
|
|
||
| it('should allow paths with spaces', () => { | ||
| expect(escapePathForShell('/path/with spaces/file', 'linux')).toBe('/path/with spaces/file'); | ||
| }); | ||
|
|
||
| it('should allow paths with special characters (not injection vectors)', () => { | ||
| // These are safe when single-quoted in bash | ||
| expect(escapePathForShell('/path/$var', 'linux')).toBe('/path/$var'); | ||
| expect(escapePathForShell('/path/`cmd`', 'darwin')).toBe('/path/`cmd`'); | ||
| expect(escapePathForShell('/path/!history', 'linux')).toBe('/path/!history'); | ||
| }); | ||
|
|
||
| it('should allow paths with glob characters', () => { | ||
| // Safe when single-quoted | ||
| expect(escapePathForShell('/path/*.txt', 'linux')).toBe('/path/*.txt'); | ||
| expect(escapePathForShell('/path/[abc]', 'darwin')).toBe('/path/[abc]'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('edge cases', () => { | ||
| it('should handle empty string', () => { | ||
| expect(escapePathForShell('', 'linux')).toBe(''); | ||
| expect(escapePathForShell('', 'win32')).toBe(''); | ||
| }); | ||
|
|
||
| it('should handle path with only quotes', () => { | ||
| expect(escapePathForShell("'''", 'linux')).toBe("'\\'''\\'''\\''"); | ||
| expect(escapePathForShell('"""', 'win32')).toBe('""""""'); | ||
| }); | ||
|
|
||
| it('should handle very long paths', () => { | ||
| const longPath = '/a'.repeat(1000); | ||
| expect(escapePathForShell(longPath, 'linux')).toBe(longPath); | ||
| }); | ||
|
|
||
| it('should handle Unicode characters', () => { | ||
| expect(escapePathForShell('/path/日本語/файл', 'linux')).toBe('/path/日本語/файл'); | ||
| expect(escapePathForShell('C:\\путь\\文件', 'win32')).toBe('C:\\путь\\文件'); | ||
| }); | ||
|
|
||
| it('should handle emoji in paths', () => { | ||
| expect(escapePathForShell('/path/📁/file', 'darwin')).toBe('/path/📁/file'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('command injection attack patterns', () => { | ||
| it('should block semicolon command chaining on Windows via newline', () => { | ||
| // Attacker might try: path; malicious_command | ||
| // But semicolons are actually safe on Windows with proper quoting | ||
| // The danger comes from newlines which we block | ||
| expect(escapePathForShell('path\n; malicious', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should block pipe injection on Windows', () => { | ||
| expect(escapePathForShell('path | evil-command', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should block command substitution on Windows', () => { | ||
| expect(escapePathForShell('path & cmd /c evil', 'win32')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should escape quote escapes on Unix', () => { | ||
| // Attacker might try to break out with: ' ; evil ' | ||
| const result = escapePathForShell("' ; evil '", 'linux'); | ||
| expect(result).toBe("'\\'' ; evil '\\''"); | ||
| }); | ||
|
|
||
| it('should block null byte truncation attacks', () => { | ||
| // Attacker might try: /safe/path\0/../../etc/passwd | ||
| expect(escapePathForShell('/safe/path\0/../../etc/passwd', 'linux')).toBeNull(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('escapePathForAppleScript', () => { | ||
| describe('null byte injection prevention', () => { | ||
| it('should reject paths containing null bytes', () => { | ||
| expect(escapePathForAppleScript('/path/with\0null')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject null byte at start of path', () => { | ||
| expect(escapePathForAppleScript('\0/path')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject null byte at end of path', () => { | ||
| expect(escapePathForAppleScript('/path\0')).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('newline injection prevention', () => { | ||
| it('should reject paths containing LF newlines', () => { | ||
| expect(escapePathForAppleScript('/path/with\nnewline')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths containing CR newlines', () => { | ||
| expect(escapePathForAppleScript('/path/with\rnewline')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should reject paths containing CRLF', () => { | ||
| expect(escapePathForAppleScript('/path/with\r\nnewline')).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('AppleScript escaping', () => { | ||
| it('should escape backslashes', () => { | ||
| expect(escapePathForAppleScript('/path\\with\\backslashes')).toBe('/path\\\\with\\\\backslashes'); | ||
| }); | ||
|
|
||
| it('should escape double quotes', () => { | ||
| expect(escapePathForAppleScript('/path/with"quotes"')).toBe('/path/with\\"quotes\\"'); | ||
| }); | ||
|
|
||
| it('should escape both backslashes and double quotes', () => { | ||
| expect(escapePathForAppleScript('/path\\"complex"')).toBe('/path\\\\\\"complex\\"'); | ||
| }); | ||
|
|
||
| it('should pass through safe paths unchanged', () => { | ||
| expect(escapePathForAppleScript('/Users/name/project')).toBe('/Users/name/project'); | ||
| }); | ||
|
|
||
| it('should allow paths with spaces', () => { | ||
| expect(escapePathForAppleScript('/path/with spaces/file')).toBe('/path/with spaces/file'); | ||
| }); | ||
|
|
||
| it('should allow single quotes (safe in AppleScript double-quoted strings)', () => { | ||
| expect(escapePathForAppleScript("/path/with'quote")).toBe("/path/with'quote"); | ||
| }); | ||
| }); | ||
|
|
||
| describe('edge cases', () => { | ||
| it('should handle empty string', () => { | ||
| expect(escapePathForAppleScript('')).toBe(''); | ||
| }); | ||
|
|
||
| it('should handle path with only double quotes', () => { | ||
| expect(escapePathForAppleScript('"""')).toBe('\\"\\"\\"'); | ||
| }); | ||
|
|
||
| it('should handle path with only backslashes', () => { | ||
| expect(escapePathForAppleScript('\\\\\\')).toBe('\\\\\\\\\\\\'); | ||
| }); | ||
|
|
||
| it('should handle Unicode characters', () => { | ||
| expect(escapePathForAppleScript('/path/日本語/файл')).toBe('/path/日本語/файл'); | ||
| }); | ||
|
|
||
| it('should handle emoji in paths', () => { | ||
| expect(escapePathForAppleScript('/path/📁/file')).toBe('/path/📁/file'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('command injection prevention', () => { | ||
| it('should escape double quotes used in injection attempts', () => { | ||
| // Attacker might try to break out of the AppleScript string | ||
| const result = escapePathForAppleScript('/path" & do shell script "evil"'); | ||
| expect(result).toBe('/path\\" & do shell script \\"evil\\"'); | ||
| }); | ||
|
|
||
| it('should escape backslash escape attempts', () => { | ||
| // Attacker might try to use backslash to escape the escaping | ||
| const result = escapePathForAppleScript('/path\\"'); | ||
| expect(result).toBe('/path\\\\\\"'); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or 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,68 @@ | ||
| /** | ||
| * Shell path escaping utilities for secure command execution. | ||
| * | ||
| * These functions prevent command injection by properly escaping | ||
| * or rejecting dangerous characters in file paths. | ||
| */ | ||
|
|
||
| /** | ||
| * Escape a path for use in shell commands. | ||
| * Prevents command injection by escaping or rejecting dangerous characters. | ||
| * | ||
| * @param filePath - The path to escape | ||
| * @param platform - Target platform ('win32', 'darwin', 'linux') | ||
| * @returns Escaped path string safe for shell use, or null if path is invalid | ||
| */ | ||
| export function escapePathForShell(filePath: string, platform: NodeJS.Platform): string | null { | ||
| // Reject paths with null bytes (always dangerous) | ||
| if (filePath.includes('\0')) { | ||
| return null; | ||
| } | ||
|
|
||
| // Reject paths with newlines (can break command structure) | ||
| if (filePath.includes('\n') || filePath.includes('\r')) { | ||
| return null; | ||
| } | ||
|
|
||
| if (platform === 'win32') { | ||
| // Windows: Reject paths with characters that could escape cmd.exe quoting | ||
| // These characters can break out of double-quoted strings in cmd | ||
| const dangerousWinChars = /[<>|&^%!`]/; | ||
| if (dangerousWinChars.test(filePath)) { | ||
| return null; | ||
| } | ||
| // Double-quote the path (already done in caller, but escape any internal quotes) | ||
| return filePath.replace(/"/g, '""'); | ||
| } else { | ||
| // Unix (macOS/Linux): Use single quotes and escape any internal single quotes | ||
| // Single-quoted strings in bash treat everything literally except single quotes | ||
| // Escape ' as '\'' (end quote, escaped quote, start quote) | ||
| return filePath.replace(/'/g, "'\\''"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Escape a path for use in AppleScript double-quoted strings. | ||
| * AppleScript uses different escaping rules than POSIX shells. | ||
| * | ||
| * @param filePath - The path to escape (should already be validated) | ||
| * @returns Escaped path string safe for AppleScript double-quoted context | ||
| */ | ||
| export function escapePathForAppleScript(filePath: string): string | null { | ||
| // Reject paths with null bytes (always dangerous) | ||
| if (filePath.includes('\0')) { | ||
| return null; | ||
| } | ||
|
|
||
| // Reject paths with newlines (can break command structure) | ||
| if (filePath.includes('\n') || filePath.includes('\r')) { | ||
| return null; | ||
| } | ||
|
|
||
| // AppleScript double-quoted strings require escaping: | ||
| // 1. Backslashes must be escaped as \\ | ||
| // 2. Double quotes must be escaped as \" | ||
| return filePath | ||
| .replace(/\\/g, '\\\\') // Escape backslashes first | ||
| .replace(/"/g, '\\"'); // Then escape double quotes | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some code duplication here for validating the
filePath. The same checks for null bytes and newlines are also present inescapePathForShell(lines 17-25). To improve maintainability and reduce redundancy, consider extracting this validation logic into a private helper function within this module. This function could then be called at the beginning of bothescapePathForShellandescapePathForAppleScript.