fix(system): spawn update commands without a shell (P2)

[wjc2821296948]

P2 — System update spawns commands through sh -c

Vulnerability description

runShellCommand in server/modules/system/system.module.ts invoked spawn('sh', ['-c', commandString], ...), parsing the entire update command as a single string passed to /bin/sh. The commandString was assembled in system.service.ts updateSystem() from a fixed template that included path arguments, but any future caller that interpolated an external value into the template would have a shell-injection sink: a single ; rm -rf / or backtick expansion is enough to execute arbitrary shell under the server's user.

Fix

Replace the shell-string path with an explicit argv array. Each system action is now a { command, args } pair; runShellCommand invokes spawn(command, args, { shell: false }) so no shell parses the args and no metacharacter (;, &&, |, backtick, $(), glob, $IFS) is interpreted. updateSystem() builds the argv pairs directly from the system state. The test suite that previously asserted the sh -c behaviour is updated to assert the new argv-based behaviour.

Files

• server/modules/system/system.module.ts
• server/modules/system/system.service.ts
• server/modules/system/tests/system.service.test.ts

Commits

• bbb350b — fix(system): spawn update commands without a shell

Code snippet

export function runShellCommand(command: string, args: string[]): ChildProcess {
  return spawn(command, args, { shell: false, stdio: ['ignore', 'pipe', 'pipe'] });
}

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved the reliability and security of system update commands by executing programs and their arguments separately.
  • Preserved existing update behavior, including output reporting, error handling, logging, and working-directory selection.
  • Updated Git, npm, and platform update workflows to use explicit command arguments.

[wjc2821296948]

Severity summary

P2 — System update spawns commands through sh -c, a latent shell-injection sink.

This PR is split out from the previous umbrella PR (#1106) per @blackmammoth's request that each finding be reviewed in isolation.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 01982719-de0c-4bf1-9ec7-ec769327f3ef

📥 Commits

Reviewing files that changed from the base of the PR and between f0dca2d and bbb350b.

📒 Files selected for processing (3)

• server/modules/system/system.module.ts
• server/modules/system/system.service.ts
• server/modules/system/tests/system.service.test.ts

---

📝 Walkthrough

Walkthrough

The system update workflow now passes executables and argument arrays separately. runShellCommand uses spawn with shell: false. Git chained commands still run through explicit sh -c arguments. Tests reflect the new call shape.

Changes

System update command execution

Layer / File(s)	Summary
Command runner contract and process execution server/modules/system/system.module.ts, server/modules/system/system.service.ts	runShellCommand accepts a separate argument array and starts the executable without a shell.
Update command plans and workflow validation server/modules/system/system.service.ts, server/modules/system/tests/system.service.test.ts	Platform, Git, and npm workflows use explicit command plans. Tests verify the separate executable and argument values.

Sequence Diagram(s)

sequenceDiagram
  participant SystemUpdateService
  participant runShellCommand
  participant Process
  SystemUpdateService->>runShellCommand: command and args
  runShellCommand->>Process: spawn with shell false
  Process-->>runShellCommand: output and exit code
  runShellCommand-->>SystemUpdateService: command result

Loading

Suggested reviewers: blackmammoth

Poem

A rabbit ships commands, neat and clear,
With args in rows and shells held near.
Git hops through sh -c by design,
While npm follows an explicit line.
Tests thump softly: “The calls align!”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: system update commands now spawn without a shell.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[wjc2821296948]

bbb350b — fix(system): spawn update commands without a shell

Severity

P2 — shell-injection sink.

Description

runShellCommand in server/modules/system/system.module.ts invoked spawn('sh', ['-c', commandString], ...), parsing the entire update command as a single string passed to /bin/sh. The commandString was assembled in system.service.ts updateSystem() from a fixed template that included path arguments, but any future caller that interpolated an external value into the template would have a shell-injection sink: a single ; rm -rf / or backtick expansion is enough to execute arbitrary shell under the server's user.

Fix

Replace the shell-string path with an explicit argv array. Each system action is now a { command, args } pair; runShellCommand invokes spawn(command, args, { shell: false }) so no shell parses the args and no metacharacter (;, &&, |, backtick, $(), glob, $IFS) is interpreted. updateSystem() builds the argv pairs directly from the system state. The test suite that previously asserted the sh -c behaviour is updated to assert the new argv-based behaviour.

Code snippet

export function runShellCommand(command: string, args: string[]): ChildProcess {
  return spawn(command, args, { shell: false, stdio: ['ignore', 'pipe', 'pipe'] });
}

🤖 Generated with Claude Code