Skip to content

⚠️ SECURITY-#6: Fix shell injection risk and missing timeout in hasCli()/checkCliVersion() - #16

Merged
FernandoCelmer merged 1 commit into
masterfrom
feature/6
Aug 15, 2026
Merged

⚠️ SECURITY-#6: Fix shell injection risk and missing timeout in hasCli()/checkCliVersion()#16
FernandoCelmer merged 1 commit into
masterfrom
feature/6

Conversation

@FernandoCelmer

@FernandoCelmer FernandoCelmer commented Aug 15, 2026

Copy link
Copy Markdown
Member

Description

  • src/features/chat/chat.controller.ts: replaced cp.exec() calls that interpolated the user-controlled pycodeloop.command setting into a shell string with cp.execFile() calls using an explicit argument array, plus a 5 s timeout.

Motivation and Context

hasCli() and checkCliVersion() passed pycodeloop.command — a user-editable VS Code setting — directly into a shell string via cp.exec(). A value containing " or $() could break out of the quoted shell word and execute arbitrary commands. Additionally, neither call had a timeout, so a hung binary would stall extension startup indefinitely.

installCli() and updateCli() were not changed: their commands come from buildInstallCommand/buildUpdateCommand (fully hardcoded, not user-controlled) and already carry a 180 s timeout.

Closes #6

Types of changes

  • Bug fix (change that fixes an issue)
  • New feature (change which adds functionality)
  • Documentation

Checklist

  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the CHANGELOG
  • I have updated the documentation accordingly

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code issues found: 0

The fix is correct and well-scoped. Switching cp.exec() to cp.execFile() for both hasCli() and checkCliVersion() eliminates the shell injection surface introduced by interpolating pycodeloop.command into a shell string, and the 5 s timeout prevents indefinite startup stalls.

A few observations worth noting for the record:

  • installCli() / updateCli(): correctly left untouched. Their commands are constructed from buildInstallCommand / buildUpdateCommand, which are fully hardcoded — no user input reaches them. They already carry a 180 s timeout.
  • addUserScriptsDirToPath(): also uses cp.exec with a Python one-liner, but the string is built exclusively from process.platform — not user-controlled. No change needed.
  • SIGTERM vs SIGKILL: execFile sends SIGTERM on timeout by default. A process that ignores SIGTERM could outlive the 5 s window, but this is an acceptable tradeoff for a developer-controlled binary. Adding killSignal: "SIGKILL" would make it more robust, though it is not a blocker.

Clean fix. Ready to merge.

@FernandoCelmer FernandoCelmer added the bug Something isn't working label Aug 15, 2026
@FernandoCelmer
FernandoCelmer merged commit 85baa65 into master Aug 15, 2026
2 checks passed
@FernandoCelmer
FernandoCelmer deleted the feature/6 branch August 15, 2026 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shell injection risk and missing timeout in hasCli() / installCli() / updateCli()

1 participant