-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(task): add Launch App button for Human Review tasks #1120
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
base: develop
Are you sure you want to change the base?
Changes from all commits
5fcb13c
d304fb4
cfaafc9
3b97476
c50eeed
6a166d8
447bf92
55624ea
3a5053c
6483660
cb75b8b
e97a8f7
bbf87c6
918be7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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\\\\\\"'); | ||
| }); | ||
| }); | ||
| }); | ||
| 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, "'\\''"); | ||
| } | ||
|
Comment on lines
+27
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Use platform abstraction helpers instead of string checks. Line 27 hardcodes 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function intentionally takes
The calling code in worktree-handlers.ts already uses the platform helpers (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This approach doesn't violate the platform abstraction principles since the detection layer uses the helpers and this utility remains a reusable, side-effect-free building block. Great architectural separation of concerns! 🧠 Learnings used |
||
| } | ||
|
|
||
| /** | ||
| * 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 | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.