diff --git a/package.json b/package.json index 77a26ebc..f7f239f4 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,9 @@ "preview": "electron-vite preview", "dist": "electron-vite build --mode production && electron-builder --mac --dir", "doctor": "bash scripts/doctor.sh", - "postinstall": "electron-builder install-app-deps && bash scripts/patch-dev-icon.sh" + "postinstall": "electron-builder install-app-deps && bash scripts/patch-dev-icon.sh", + "test": "node node_modules/vitest/vitest.mjs run", + "test:watch": "node node_modules/vitest/vitest.mjs" }, "dependencies": { "@phosphor-icons/react": "^2.1.10", @@ -64,6 +66,7 @@ "react-dom": "^19.0.0", "tailwindcss": "^4.2.1", "typescript": "^5.7.0", - "vite": "^6.0.0" + "vite": "^6.0.0", + "vitest": "^2.0.0" } } diff --git a/src/main/__tests__/installer.test.ts b/src/main/__tests__/installer.test.ts new file mode 100644 index 00000000..0f2f25f4 --- /dev/null +++ b/src/main/__tests__/installer.test.ts @@ -0,0 +1,132 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { SpawnSyncReturns } from 'child_process' + +// ─── Mock child_process before importing installer ─────────────────────────── +// spawnSync is used to run curl and tar without shell interpretation. +// We mock it to verify argument arrays (not shell strings) are used. + +const spawnSyncMock = vi.fn() + +vi.mock('child_process', () => ({ + spawnSync: spawnSyncMock, +})) + +// ─── Mock fs and path operations so installer doesn't touch the real filesystem ─ + +vi.mock('fs', () => ({ + existsSync: vi.fn().mockReturnValue(false), + mkdirSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + renameSync: vi.fn(), + rmSync: vi.fn(), + cpSync: vi.fn(), +})) + +// Manifest mock — one github skill with a path that includes shell-special characters +vi.mock('../skills/manifest', () => ({ + SKILLS: [ + { + name: 'test-skill', + version: '1.0.0', + requiredFiles: [], + source: { + type: 'github', + repo: 'org/repo', + // A path containing characters that would be dangerous in a shell string + path: 'skills/test-skill', + commitSha: 'abc123', + }, + }, + ], +})) + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe('installGithubSkill — spawnSync argument safety', () => { + beforeEach(() => { + vi.clearAllMocks() + // Default: both curl and tar succeed + spawnSyncMock.mockReturnValue({ status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') } as SpawnSyncReturns) + }) + + it('calls curl with the tarball URL as a separate argument (not embedded in a shell string)', async () => { + const { ensureSkills } = await import('../skills/installer') + await ensureSkills(() => {}) + + const curlCall = spawnSyncMock.mock.calls[0] + const [curlBin, curlArgs] = curlCall + + expect(curlBin).toBe('/usr/bin/curl') + // URL must be a standalone argument, NOT embedded in a shell string + expect(Array.isArray(curlArgs)).toBe(true) + expect(curlArgs).toContain('-sL') + // The URL argument must be a plain string, not a shell command + const urlArg = curlArgs.find((a: string) => a.startsWith('https://')) + expect(urlArg).toBeDefined() + expect(urlArg).not.toContain('&&') + expect(urlArg).not.toContain(';') + expect(urlArg).not.toContain('|') + }) + + it('calls tar with tmpDir and path as separate arguments (no shell interpolation possible)', async () => { + const { ensureSkills } = await import('../skills/installer') + await ensureSkills(() => {}) + + const tarCall = spawnSyncMock.mock.calls[1] + const [tarBin, tarArgs] = tarCall + + expect(tarBin).toBe('/usr/bin/tar') + expect(Array.isArray(tarArgs)).toBe(true) + // --no-symlinks must be present to block symlink creation at extraction time + expect(tarArgs).toContain('--no-symlinks') + // -C must be a separate argument from the directory path + const cIndex = tarArgs.indexOf('-C') + expect(cIndex).toBeGreaterThan(-1) + expect(tarArgs[cIndex + 1]).toBeTruthy() // tmpDir follows -C as its own element + // No shell metacharacters in the argument list itself + const joinedArgs = tarArgs.join(' ') + expect(joinedArgs).not.toContain('$(') + expect(joinedArgs).not.toContain('`') + }) + + it('passes curl stdout directly as stdin to tar (no temp file, no shell pipe)', async () => { + const fakeBuffer = Buffer.from('fake tarball content') + spawnSyncMock + .mockReturnValueOnce({ status: 0, stdout: fakeBuffer, stderr: Buffer.from('') }) + .mockReturnValueOnce({ status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') }) + + const { ensureSkills } = await import('../skills/installer') + await ensureSkills(() => {}) + + const tarOptions = spawnSyncMock.mock.calls[1][2] + // The tar call must receive curl's stdout as its input buffer + expect(tarOptions.input).toBe(fakeBuffer) + }) + + it('reports failure when curl exits non-zero (no shell — no silent swallow)', async () => { + spawnSyncMock.mockReturnValueOnce({ + status: 22, + stdout: Buffer.from(''), + stderr: Buffer.from('404 Not Found'), + }) + + const statuses: string[] = [] + const { ensureSkills } = await import('../skills/installer') + await ensureSkills((s) => statuses.push(s.state)) + + expect(statuses).toContain('failed') + }) + + it('reports failure when tar exits non-zero', async () => { + spawnSyncMock + .mockReturnValueOnce({ status: 0, stdout: Buffer.from('data'), stderr: Buffer.from('') }) + .mockReturnValueOnce({ status: 1, stdout: Buffer.from(''), stderr: Buffer.from('invalid archive') }) + + const statuses: string[] = [] + const { ensureSkills } = await import('../skills/installer') + await ensureSkills((s) => statuses.push(s.state)) + + expect(statuses).toContain('failed') + }) +}) diff --git a/src/main/__tests__/shell-utils.test.ts b/src/main/__tests__/shell-utils.test.ts new file mode 100644 index 00000000..1c5bfd11 --- /dev/null +++ b/src/main/__tests__/shell-utils.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect } from 'vitest' +import { shellSingleQuote } from '../shell-utils' + +describe('shellSingleQuote', () => { + it('wraps a simple path in single quotes', () => { + expect(shellSingleQuote('/home/user/project')).toBe("'/home/user/project'") + }) + + it('escapes embedded single quotes using the close-escape-reopen pattern', () => { + // "it's" → 'it'\''s' + expect(shellSingleQuote("/tmp/it's here")).toBe("'/tmp/it'\\''s here'") + }) + + it('does NOT escape double quotes — they are inert inside single-quoted strings', () => { + const path = '/tmp/evil"; rm -rf /' + const result = shellSingleQuote(path) + // double-quote must appear verbatim inside the single-quoted string + expect(result).toBe("'/tmp/evil\"; rm -rf /'") + }) + + it('does NOT escape dollar signs — shell expansion is blocked by single quotes', () => { + const path = '/tmp/$HOME/foo' + expect(shellSingleQuote(path)).toBe("'/tmp/$HOME/foo'") + }) + + it('does NOT escape backticks — command substitution is blocked by single quotes', () => { + const path = '/tmp/`whoami`/foo' + expect(shellSingleQuote(path)).toBe("'/tmp/`whoami`/foo'") + }) + + it('does NOT escape backslashes — they are literal inside single-quoted strings', () => { + const path = '/tmp/foo\\bar' + expect(shellSingleQuote(path)).toBe("'/tmp/foo\\bar'") + }) + + it('handles multiple consecutive single quotes', () => { + const result = shellSingleQuote("a''b") + // Each ' is replaced by '\'' — two consecutive quotes become '\'''\'' + // The result must start/end with single quotes and round-trip correctly in shell. + expect(result.startsWith("'")).toBe(true) + expect(result.endsWith("'")).toBe(true) + // Every ' that was inside the original string is now escaped as '\'' + // so the escaped sequence '\'' must appear twice + expect(result.split("'\\''").length - 1).toBe(2) + }) + + it('handles empty string', () => { + expect(shellSingleQuote('')).toBe("''") + }) + + it('handles a classic AppleScript injection payload', () => { + // This was the original attack vector: a project path that could break + // out of an AppleScript `do script "..."` string. + const maliciousPath = '/tmp/test"; do shell script "rm -rf /"; echo "' + const result = shellSingleQuote(maliciousPath) + // The double quotes appear verbatim — inert inside single-quoted shell strings. + // The result, when used in a shell `cd ''`, is safe. + expect(result.startsWith("'")).toBe(true) + expect(result.endsWith("'")).toBe(true) + // No shell-significant characters can escape when the whole thing is single-quoted + expect(result).not.toContain('$(') + expect(result).not.toContain('`') + }) +}) diff --git a/src/main/__tests__/symlink-escape.test.ts b/src/main/__tests__/symlink-escape.test.ts new file mode 100644 index 00000000..3ad09242 --- /dev/null +++ b/src/main/__tests__/symlink-escape.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from 'fs' +import { join, sep } from 'path' +import { tmpdir } from 'os' +import { assertNoSymlinkEscape } from '../skills/installer' + +// Each test gets a fresh isolated temp directory +let base: string +let outside: string + +beforeEach(() => { + base = mkdtempSync(join(tmpdir(), 'clui-symtest-')) + outside = mkdtempSync(join(tmpdir(), 'clui-outside-')) +}) + +afterEach(() => { + rmSync(base, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +describe('assertNoSymlinkEscape', () => { + it('passes for a directory with no symlinks', () => { + writeFileSync(join(base, 'skill.md'), '# skill') + mkdirSync(join(base, 'sub')) + writeFileSync(join(base, 'sub', 'file.ts'), 'export {}') + + expect(() => assertNoSymlinkEscape(base)).not.toThrow() + }) + + it('passes for a symlink that points to a file inside the base directory', () => { + writeFileSync(join(base, 'real.md'), 'content') + symlinkSync(join(base, 'real.md'), join(base, 'link.md')) + + expect(() => assertNoSymlinkEscape(base)).not.toThrow() + }) + + it('throws when a top-level symlink points outside the base directory', () => { + writeFileSync(join(outside, 'secret'), 'sensitive data') + symlinkSync(join(outside, 'secret'), join(base, 'escape')) + + expect(() => assertNoSymlinkEscape(base)).toThrow('Symlink escape detected') + }) + + it('throws when a nested symlink points outside the base directory', () => { + mkdirSync(join(base, 'deep', 'nested'), { recursive: true }) + writeFileSync(join(outside, 'target'), 'exfiltrated') + symlinkSync(join(outside, 'target'), join(base, 'deep', 'nested', 'escape')) + + expect(() => assertNoSymlinkEscape(base)).toThrow('Symlink escape detected') + }) + + it('throws for a symlink to a sensitive path like ~/.ssh/id_rsa (simulated)', () => { + // Simulate the classic tarball attack: skill/config → /etc/passwd equivalent + writeFileSync(join(outside, 'passwd'), 'root:x:0:0') + symlinkSync(join(outside, 'passwd'), join(base, 'config')) + + expect(() => assertNoSymlinkEscape(base)).toThrow('Symlink escape detected') + }) + + it('throws for a symlink chain where the final target is outside', () => { + // Chain: base/link1 → base/link2 → outside/secret + // realpathSync must follow the full chain + writeFileSync(join(outside, 'secret'), 'exfil') + symlinkSync(join(outside, 'secret'), join(base, 'link2')) + symlinkSync(join(base, 'link2'), join(base, 'link1')) + + expect(() => assertNoSymlinkEscape(base)).toThrow('Symlink escape detected') + }) + + it('throws when the error message includes the offending symlink path', () => { + writeFileSync(join(outside, 'target'), 'data') + const escapeLink = join(base, 'evil-link') + symlinkSync(join(outside, 'target'), escapeLink) + + let msg = '' + try { assertNoSymlinkEscape(base) } catch (e) { msg = String(e) } + + // Error must identify the symlink path so operators can diagnose which file is malicious + expect(msg).toContain('evil-link') + expect(msg).toContain(sep) // contains a path separator — it's a real path, not a vague message + }) +}) diff --git a/src/main/index.ts b/src/main/index.ts index 2036c450..38c461e4 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1,6 +1,6 @@ import { app, BrowserWindow, ipcMain, dialog, screen, globalShortcut, Tray, Menu, nativeImage, nativeTheme, shell, systemPreferences } from 'electron' import { join } from 'path' -import { existsSync, readdirSync, statSync, createReadStream } from 'fs' +import { existsSync, readdirSync, statSync, createReadStream, writeFileSync, unlinkSync } from 'fs' import { createInterface } from 'readline' import { homedir } from 'os' import { ControlPlane } from './claude/control-plane' @@ -8,6 +8,7 @@ import { ensureSkills, type SkillStatus } from './skills/installer' import { fetchCatalog, listInstalled, installPlugin, uninstallPlugin } from './marketplace/catalog' import { log as _log, LOG_FILE, flushLogs } from './logger' import { getCliEnv } from './cli-env' +import { shellSingleQuote } from './shell-utils' import { IPC } from '../shared/types' import type { RunOptions, NormalizedEvent, EnrichedError } from '../shared/types' @@ -924,35 +925,38 @@ ipcMain.handle(IPC.OPEN_IN_TERMINAL, (_event, arg: string | null | { sessionId?: return false } - // Shell-safe single-quote escaping: replace ' with '\'' (end quote, escaped literal quote, reopen quote) - // Single quotes block all shell expansion ($, `, \, etc.) — unlike double quotes which allow $() and backticks - const shellSingleQuote = (s: string): string => "'" + s.replace(/'/g, "'\\''") + "'" - // AppleScript string escaping: backslashes doubled, double quotes escaped - const escapeAppleScript = (s: string): string => s.replace(/\\/g, '\\\\').replace(/"/g, '\\"') - - const safeDir = escapeAppleScript(shellSingleQuote(projectPath)) + // Write the shell command to a temp script file so no dynamic content is ever + // embedded in the AppleScript string. Terminal receives only the script file path + // (a safe constant-format string), eliminating all AppleScript injection surface. + const { randomUUID } = require('crypto') + const { tmpdir } = require('os') + const scriptPath = join(tmpdir(), `clui-term-${randomUUID()}.sh`) - let cmd: string + let shellCmd: string if (sessionId) { - // sessionId is UUID-validated above, safe to embed directly - cmd = `cd ${safeDir} && ${claudeBin} --resume ${sessionId}` + // sessionId is UUID-validated above — alphanumeric + hyphens, safe to embed directly + shellCmd = `cd ${shellSingleQuote(projectPath)} && ${claudeBin} --resume ${sessionId}` } else { - cmd = `cd ${safeDir} && ${claudeBin}` + shellCmd = `cd ${shellSingleQuote(projectPath)} && ${claudeBin}` } - const script = `tell application "Terminal" - activate - do script "${cmd}" -end tell` - try { - execFile('/usr/bin/osascript', ['-e', script], (err: Error | null) => { + writeFileSync(scriptPath, `#!/bin/sh\n${shellCmd}\n`, { mode: 0o700 }) + + // The AppleScript only contains the script file path (UUID-based, no user input). + // scriptPath is safe: it's /tmp/clui-term-.sh — no special characters possible. + const appleScript = `tell application "Terminal"\n activate\n do script ${shellSingleQuote(scriptPath)}\nend tell` + + execFile('/usr/bin/osascript', ['-e', appleScript], (err: Error | null) => { + // Clean up temp script after a short delay (Terminal needs time to read it) + setTimeout(() => { try { unlinkSync(scriptPath) } catch {} }, 5000) if (err) log(`Failed to open terminal: ${err.message}`) - else log(`Opened terminal with: ${cmd}`) + else log(`Opened terminal with: ${shellCmd}`) }) return true } catch (err: unknown) { log(`Failed to open terminal: ${err}`) + try { unlinkSync(scriptPath) } catch {} return false } }) diff --git a/src/main/shell-utils.ts b/src/main/shell-utils.ts new file mode 100644 index 00000000..935de6ce --- /dev/null +++ b/src/main/shell-utils.ts @@ -0,0 +1,17 @@ +/** + * Shell and script escaping utilities. + * Extracted as pure functions so they can be tested independently. + */ + +/** + * Wraps a string in POSIX single quotes, escaping any single-quote characters + * inside. Single-quoted strings block all shell expansion ($, `, \, globs). + * + * Examples: + * /home/user → '/home/user' + * /tmp/it's here → '/tmp/it'\''s here' + * /foo"; rm -rf / → '/foo"; rm -rf /' + */ +export function shellSingleQuote(s: string): string { + return "'" + s.replace(/'/g, "'\\''") + "'" +} diff --git a/src/main/skills/installer.ts b/src/main/skills/installer.ts index 0f8c13c1..407b81dd 100644 --- a/src/main/skills/installer.ts +++ b/src/main/skills/installer.ts @@ -8,10 +8,10 @@ * it was placed there by the user and we don't touch it. */ -import { existsSync, mkdirSync, readFileSync, writeFileSync, renameSync, rmSync, cpSync } from 'fs' -import { join, dirname } from 'path' +import { existsSync, mkdirSync, readFileSync, writeFileSync, renameSync, rmSync, cpSync, readdirSync, lstatSync, realpathSync } from 'fs' +import { join } from 'path' import { homedir } from 'os' -import { execSync } from 'child_process' +import { spawnSync } from 'child_process' import { randomUUID } from 'crypto' import { SKILLS, type SkillEntry } from './manifest' @@ -65,6 +65,32 @@ function writeVersionFile(skillDir: string, entry: SkillEntry): void { writeFileSync(join(skillDir, VERSION_FILE), JSON.stringify(meta, null, 2) + '\n') } +/** + * Recursively walks dir and throws if any symlink resolves to a path outside dir. + * Defends against malicious tarballs that include symlinks pointing to sensitive + * files on the host (e.g. ~/.ssh/id_rsa, /etc/passwd). + */ +export function assertNoSymlinkEscape(dir: string): void { + const base = realpathSync(dir) + + const scan = (current: string): void => { + for (const entry of readdirSync(current, { withFileTypes: true })) { + const full = join(current, entry.name) + if (entry.isSymbolicLink()) { + // realpathSync follows the chain to the final target + const target = realpathSync(full) + if (!target.startsWith(base + '/') && target !== base) { + throw new Error(`Symlink escape detected: ${full} → ${target}`) + } + } else if (entry.isDirectory()) { + scan(full) + } + } + } + + scan(base) +} + function validateSkill(dir: string, requiredFiles: string[]): string | null { for (const f of requiredFiles) { if (!existsSync(join(dir, f))) { @@ -94,14 +120,27 @@ async function installGithubSkill( const pathDepth = path.split('/').length + 1 // +1 for the github top-level dir const tarballUrl = `https://api.github.com/repos/${repo}/tarball/${commitSha}` - // Use curl + tar — both always available on macOS - const cmd = [ - `curl -sL "${tarballUrl}"`, - '|', - `tar -xz --strip-components=${pathDepth} -C "${tmpDir}" "*/${path}"`, - ].join(' ') + // Use curl piped to tar via spawn — no shell interpolation, arguments passed directly + // to each binary so special characters in tmpDir or path cannot escape into shell. + const curl = spawnSync('/usr/bin/curl', ['-sL', tarballUrl], { timeout: 60000, maxBuffer: 100 * 1024 * 1024 }) + if (curl.status !== 0) { + throw new Error(`curl failed (exit ${curl.status}): ${curl.stderr?.toString().trim()}`) + } + + const tar = spawnSync( + '/usr/bin/tar', + // --no-symlinks: refuse to create symlinks during extraction (first line of defence) + ['-xz', '--no-symlinks', `--strip-components=${pathDepth}`, '-C', tmpDir, `*/${path}`], + { input: curl.stdout, timeout: 30000 }, + ) + if (tar.status !== 0) { + throw new Error(`tar failed (exit ${tar.status}): ${tar.stderr?.toString().trim()}`) + } - execSync(cmd, { timeout: 60000, stdio: 'pipe' }) + // Second line of defence: scan the extracted tree and reject any symlink that + // resolves outside tmpDir (catches hardlinks-as-symlinks, old tar versions that + // ignore --no-symlinks, or any future extraction path we missed). + assertNoSymlinkEscape(tmpDir) // Validate extracted files onStatus({ name: entry.name, state: 'validating' })