Skip to content

fix(cli): mitigate shell injection in quick_commands exec via shlex.split() - #48571

Open
baolingao wants to merge 1 commit into
NousResearch:mainfrom
baolingao:fix/quick-commands-shell-injection
Open

fix(cli): mitigate shell injection in quick_commands exec via shlex.split()#48571
baolingao wants to merge 1 commit into
NousResearch:mainfrom
baolingao:fix/quick-commands-shell-injection

Conversation

@baolingao

Copy link
Copy Markdown
Contributor

Summary

The quick_commands exec handler in cli.py runs user-defined commands with
shell=True unconditionally (CWE-78). While these commands are configured in
config.yaml by the user and are not agent-controlled, a compromised config
file could inject arbitrary shell commands through metacharacters.

Change

Prefer shlex.split() + shell=False for simple commands, falling back to
shell=True only when the command contains shell operators (|, ;, &&,
||, >, <, >>, &, $()) that cannot be expressed as an argv list, or
when shlex.split() raises ValueError.

This preserves backward compatibility — existing configurations that use shell
features continue to work unchanged. Simple commands like git status or
python script.py now run without a shell.

Tested

  • Simple command: echo helloshell=False, works
  • Shell operator: echo hello | wc -cshell=True fallback, works
  • Invalid syntax: echo "unclosedshlex.split() ValueError → shell=True fallback
  • Platform: Windows

Related

  • CWE-78: OS Command Injection

@baolingao
baolingao requested a review from a team June 18, 2026 18:10
@alt-glitch alt-glitch added invalid This doesn't seem right P3 Low — cosmetic, nice to have labels Jun 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This PR is filed against an unclean base: it shows 5,140 changed files and ~1.84M deletions (CONFLICTING), effectively a whole-fork/whole-repo push rather than the focused cli.py shlex.split() change the description promises. The diff exceeds GitHub's 300-file limit and cannot be reviewed or merged as-is.

Marking invalid — the same whole-fork push was already filed and closed as #48528. Please rebase onto a clean main and re-open a PR containing ONLY the cli.py quick_commands change.

Note: the underlying shell=True in the quick_commands exec handler (cli.py) is a legitimate concern and already has open fixes in flight (#33495, #35545) — a clean single-file PR is welcome.

@baolingao
baolingao force-pushed the fix/quick-commands-shell-injection branch from de42564 to 3900d92 Compare June 18, 2026 19:06
…plit()

Prefer shlex.split() + shell=False for user-defined quick commands,
falling back to shell=True only when the command contains shell
operators (pipes, redirects, chaining) that cannot be expressed as an
argv list.

This reduces the attack surface if config.yaml is compromised
(CWE-78) while preserving backward compatibility for existing
configurations that rely on shell features.

Tested:
- echo hello → shell=False, runs
- echo hello | wc -c → shell=True fallback, runs
- echo "unclosed → shlex.split() ValueError → shell=True fallback
@baolingao
baolingao force-pushed the fix/quick-commands-shell-injection branch from 3900d92 to 61dcbb6 Compare June 19, 2026 14:42
@teknium1

Copy link
Copy Markdown
Contributor

Thank you for narrowing this to the quick-command handler. The current-main implementation still uses a shell, but this change needs a clarified execution contract before it can be salvaged.

Problems

  • cli.py:8934-8943 deliberately supports config-defined shell snippets; the PR's operator list does not cover shell-dependent forms such as $VAR, backticks, globs, or ~, so shlex.split() would silently change behavior.
  • Current main added child-env sanitization and output redaction in commit c8e5f999c (cli.py:8936-8948). The PR's replacement calls omit those protections and currently conflicts with main.
  • Equivalent exec paths remain shell-based in gateway/run.py:10024-10030 and tui_gateway/server.py:11865-11872, while the docs describe quick commands as cross-surface (website/docs/user-guide/configuration.md:1696-1702).

Suggested changes

  • Define whether quick-command config is intentionally trusted shell input or an argv-only interface, then preserve that contract consistently across CLI, gateway, and TUI.
  • Retain the sanitization/redaction introduced by c8e5f999c and add execution-mode and compatibility tests.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants