Skip to content

fix(security): harden Windows subprocess argument handling - #2252

Merged
tejaskash merged 1 commit into
mainfrom
fix/p505-windows-shell-injection
Sep 9, 2026
Merged

fix(security): harden Windows subprocess argument handling#2252
tejaskash merged 1 commit into
mainfrom
fix/p505-windows-shell-injection

Conversation

@tejaskash

@tejaskash tejaskash commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

On Windows, the subprocess helpers spawned with shell: true, which let the shell interpret the contents of arguments passed to a child process. This hardens that path.

The five subprocess helpers now delegate spawning to cross-spawn, which resolves Windows .cmd/.bat wrappers and escapes arguments so their contents reach the child process intact rather than being interpreted by a shell. Nothing spawns through a shell, and the shell option is removed from SubprocessOptions.

cross-spawn was already present transitively (via eslint/vite); this promotes it to a direct dependency and adds @types/cross-spawn. Delegating also removes the hand-rolled Windows quoting and PATHEXT resolution.

Changes

  • Promote cross-spawn to a direct dependency; add @types/cross-spawn
  • runSubprocess, checkSubprocess, runSubprocessCapture, runSubprocessCaptureSync, checkSubprocessSync call crossSpawn / crossSpawn.sync
  • Drop shell from SubprocessOptions and the now-redundant shell: false in initGitRepo
  • Behavioral test: an argument containing shell metacharacters arrives at the child as a single argv element

Testing

  • tsc --noEmit clean, eslint clean, 18 subprocess unit tests pass
  • Follow-up before merge: re-run the Windows verification against this build, since the fix now depends on cross-spawn's escaping

Out of scope (follow-up)

Other callers (codezip-dev-server.ts, container.ts, update/action.ts, import/phase2-import.ts, and others) still spawn directly and bypass these hardened helpers. Routing them through the helpers closes the same class of issue everywhere.

@tejaskash
tejaskash requested a review from a team September 9, 2026 15:36
@github-actions github-actions Bot added the size/m PR size: M label Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added agentcore-harness-reviewing AgentCore Harness review in progress claude-security-reviewing Claude Code /security-review in progress labels Sep 9, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
Comment thread src/lib/utils/subprocess.ts Dismissed
Comment thread src/lib/utils/subprocess.ts Dismissed

@agentcore-devx-automation agentcore-devx-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AgentCore Harness Review

Verdict: Looks good

This is a clean security/correctness fix. On Windows, spawn previously defaulted to shell: true and the resolveCommand helper concatenated command + args with .join(' '), so any argument containing shell metacharacters (&, |, >, quotes, spaces, etc.) would be re-parsed by cmd.exe — a real command-injection surface when paths/user input flowed into e.g. git, uv, or npm invocations. Replacing all five wrappers with cross-spawn eliminates that path entirely while keeping .cmd/.bat shim resolution working on Windows.

Things I checked:

  • Verified no remaining callers pass shell: true (only removals were the redundant shell: false overrides in src/cli/operations/init/files.ts, correctly cleaned up here).
  • SubprocessOptions.shell is removed from the exported interface; no external consumers were relying on it (greped for SubprocessOptions and .shell references).
  • cross-spawn is a module.exports = spawn default with .sync attached; esModuleInterop: true is set in tsconfig.build.json, so import crossSpawn from 'cross-spawn' and crossSpawn.sync(...) both resolve correctly.
  • encoding: 'utf-8' is passed through untouched to spawnSync inside cross-spawn, so runSubprocessCaptureSync still returns strings.
  • The new regression test exercises runSubprocessCapture end-to-end with a real subprocess (no mocking), which is exactly the right level.

Telemetry: N/A — this is a security refactor, not a new user-facing feature.

Nothing blocking. Ship it.

@agentcore-devx-automation agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Sep 9, 2026
@tejaskash
tejaskash force-pushed the fix/p505-windows-shell-injection branch from 0e1ee91 to 2e71cf8 Compare September 9, 2026 15:40
@tejaskash tejaskash changed the title fix(security): prevent Windows command injection in subprocess helpers (P505979923) fix(security): harden Windows subprocess argument handling Sep 9, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 9, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@tejaskash
tejaskash force-pushed the fix/p505-windows-shell-injection branch from 2e71cf8 to 7a2d8de Compare September 9, 2026 15:46
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-0.28.1.tgz

How to install

gh release download pr-2252-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.28.1.tgz

@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 41.08% 15742 / 38319
🔵 Statements 40.34% 16790 / 41618
🔵 Functions 35.19% 2712 / 7705
🔵 Branches 34.24% 10526 / 30738
Generated in workflow #4459 for commit 872eb92 by the Vitest Coverage Report Action

@tejaskash
tejaskash force-pushed the fix/p505-windows-shell-injection branch from 7a2d8de to b0b568e Compare September 9, 2026 16:06
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
The subprocess helpers spawned with shell: true on Windows, which let the shell
interpret argument contents. They now delegate spawning to cross-spawn, which
resolves Windows .cmd/.bat wrappers and escapes arguments so their contents reach
the child process intact rather than being interpreted. Nothing spawns through a
shell, and the shell option is removed from SubprocessOptions.

- Promote cross-spawn to a direct dependency; add @types/cross-spawn
- runSubprocess, checkSubprocess, runSubprocessCapture, runSubprocessCaptureSync,
  checkSubprocessSync now call crossSpawn / crossSpawn.sync
- Behavioral test: an argument containing shell metacharacters arrives at the
  child as a single argv element
@tejaskash
tejaskash force-pushed the fix/p505-windows-shell-injection branch from b0b568e to 872eb92 Compare September 9, 2026 19:56
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026
@tejaskash
tejaskash merged commit d7c0add into main Sep 9, 2026
37 checks passed
@tejaskash
tejaskash deleted the fix/p505-windows-shell-injection branch September 9, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants