Skip to content

fix(security): restrict terminal shell to platform allowlist (#5479)#6071

Open
CVE-Hunter-Leo wants to merge 1 commit into
nexu-io:mainfrom
CVE-Hunter-Leo:fix/shell-allowlist-5479
Open

fix(security): restrict terminal shell to platform allowlist (#5479)#6071
CVE-Hunter-Leo wants to merge 1 commit into
nexu-io:mainfrom
CVE-Hunter-Leo:fix/shell-allowlist-5479

Conversation

@CVE-Hunter-Leo

@CVE-Hunter-Leo CVE-Hunter-Leo commented Jul 25, 2026

Copy link
Copy Markdown

Summary

Adds a platform-aware shell allowlist to resolveShell() so client-supplied shell values on POST /api/projects/:id/terminals are validated before being passed to pty.spawn().

Before: any non-empty string from the client was passed directly to the PTY spawn, allowing arbitrary process execution as the daemon user (CWE-78).

After:

  • Posix: only /bin/bash, /bin/sh, /bin/zsh, /bin/fish, /bin/dash (and /usr/bin/ equivalents) are accepted
  • Win32: only powershell.exe, pwsh.exe, cmd.exe (case-insensitive)
  • Non-allowlisted values are silently dropped; falls through to env default
  • Path traversal / command injection blocked by exact string matching

Changes

  • apps/daemon/src/terminals.ts: add SHELL_ALLOWLIST_POSIX, SHELL_ALLOWLIST_WIN32, isAllowedShell() helper, enforce in resolveShell()
  • apps/daemon/tests/terminals-shell-allowlist.test.ts: 7 test cases

Surface area

This change affects the daemon terminal subsystem specifically:

  • Route: POST /api/projects/:id/terminals (the terminal create endpoint)
  • Function: resolveShell() in apps/daemon/src/terminals.ts
  • Impact: Any client that sends a shell field in the terminal create body
  • Desktop/Web: Both surfaces use the same daemon route, so both are protected
  • Non-affected: Terminal resize, write, kill, list, and SSE streaming are unchanged

Validation

  • Ran pnpm --filter @open-design/daemon test -- terminals-shell-allowlist — all 7 test cases pass
  • Manually verified:
    • Sending {"shell": "/bin/bash"} → terminal spawns with bash ✓
    • Sending {"shell": "/tmp/evil"} → terminal spawns with default shell (not evil) ✓
    • Sending {"shell": "/bin/bash; rm -rf /"} → terminal spawns with default shell (exact match blocks injection) ✓
    • Omitting shell field → resolves from SHELL env as before ✓
    • Win32: {"shell": "powershell.exe"} accepted, {"shell": "calc.exe"} dropped ✓

Closes #5479

…#5479)

Add shell allowlist to resolveShell() so client-supplied shell values
are validated against a platform-specific set of known binaries before
being passed to pty.spawn().

- Posix: /bin/bash, /bin/sh, /bin/zsh, /bin/fish, /bin/dash and
  /usr/bin/ equivalents
- Win32: powershell.exe, pwsh.exe, cmd.exe (case-insensitive)
- Non-allowlisted values are silently dropped; falls through to env/default
- Prevents arbitrary process execution via POST /api/projects/:id/terminals
- Adds regression tests covering allowlist acceptance, arbitrary command
  rejection, path traversal, and platform-specific behavior

Closes nexu-io#5479
@lefarcen

Copy link
Copy Markdown
Contributor

Thanks @CVE-Hunter-Leo — good call tightening the shell gate inside resolveShell(), and the scope here stays nicely focused on the terminal path plus regression coverage.

Before pool review picks this up, could you add the missing Surface area and Validation sections to the PR body? The Summary/Changes content is already useful; what's still missing is a quick note on which user-facing/runtime surface this affects and what you ran to verify the allowlist behavior.

Because this touches terminal startup behavior, we'll also put it through a manual QA pass before merge.

💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#6071 until it merges — read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

@lefarcen
lefarcen requested a review from PerishCode July 25, 2026 10:53
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug][Security]: Arbitrary Process Spawn via Unrestricted Terminal Shell Parameter in Open Design

2 participants