Fix shell injection, AppleScript injection, and symlink escape in skill installer

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"
   }
 }

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<Buffer>)
+  })
+
+  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')
+  })
+})

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 '<result>'`, 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('`')
+  })
+})

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
+  })
+})

src/main/index.ts

@@ -1,13 +1,14 @@
 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'
 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-<uuid>.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
   }
 })

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, "'\\''") + "'"
+}