security: harden scheduler/auth/mcp against shell injection (port upstream #32) - #41
Merged
JeanBaptisteRenard merged 1 commit intoJun 9, 2026
Conversation
Port of doctly#32 (by @joeytwiddle), manually adapted to the fork's diverged main.js (~1900 LOC vs upstream ~350). Vectors fixed: - schedule-runner.js: buildScheduleCommand now returns { claudeArgs } (a plain string[]) instead of a shell-interpolated string. Callers pass the argv to runScheduleCommand, which re-serialises safely via quoteArgvForShell() before handing to the user's login shell. Adds isSafeScalar/assertSafe guards that reject control characters and validates max-budget-usd is numeric. - main.js: runScheduleCommand signature changed to accept claudeArgv[], imports quoteArgvForShell from shell-profiles. - schedule-ipc.js: updated call site to use { claudeArgs } destructuring. - shell-profiles.js: adds quoteArgForShell() and quoteArgvForShell() with POSIX/PowerShell/cmd quoting, exported for use by main.js. - claude-auth.js: keychain read switched from execSync (shell) to execFileSync (argv), preventing $USER injection. - mcp-bridge.js: lockfile written with mode 0o600 (was 0o644); adds chmodSync for pre-existing files from older builds. open-terminal argv refactor is deliberately deferred to a separate PR to avoid scope collision with a parallel port in progress.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of doctly/switchboard#32 (author: @joeytwiddle), manually adapted — the fork's
main.jsis ~1900 LOC vs upstream's ~350, so this is a full hand-port, not a cherry-pick.Vectors fixed
1. Scheduler shell injection (high severity)
buildScheduleCommandpreviously built a shell string by interpolating YAML frontmatter fields (model,permission-mode,max-budget-usd,allowed-tools,append-system-prompt,add-dirs) directly into abash -ccommand. A maliciousschedule-*.mdin any indexed project could achieve RCE within 60 s of the next cron tick.Fix:
buildScheduleCommandnow returns{ claudeArgs: string[] }— a plain argv array.runScheduleCommandinmain.jsre-serialises it viaquoteArgvForShell()(new helper inshell-profiles.js) before passing to the user's login shell. Each token is POSIX/PowerShell/cmd-quoted so shell metacharacters inside frontmatter values are never interpreted. Input validation (isSafeScalar/assertSafe) additionally rejects control characters, andmax-budget-usdis validated as a number.2.
claude-auth.jskeychain readexecSyncwith a shell-interpolated string →execFileSyncwith an argv array. Prevents$USERor a craftedCLAUDE_CONFIG_DIRfrom being interpolated into a shell command.3.
mcp-bridge.jslockfile permissionsThe lockfile at
~/.claude/ide/<port>.lockcontained the MCP auth token and was previously created with default umask (world-readable on many systems). Now written withmode: 0o600. AchmodSynccall also tightens permissions on pre-existing files from older builds.Deliberately deferred
The
open-terminalargv refactor (ipcMain.handle('open-terminal', ...)) is a heavier change and is being ported in a separate PR to avoid collision with a parallel in-progress port.Tests
test/schedule-injection.test.js— 16 new tests covering:$()) survive as literal tokensmax-budget-usdnumeric validationquoteArgForShellcorrectness for bash/zsh/PowerShellAll 16 pass. Existing pure-logic tests (32 tests) continue to pass.