From 3111908997dae653f906176d339fe1256864018d Mon Sep 17 00:00:00 2001 From: Kevin Overmier Date: Thu, 5 Mar 2026 17:02:20 -0600 Subject: [PATCH] fix(security): remove shell injection surface and block directory traversal Closes #43: Remove shell: true from runGit() in git-helpers.ts. Node.js resolves the git binary via PATH directly without a shell on WSL, Linux, macOS, and Windows. shell: true is unnecessary and allows shell metacharacters in args to be interpreted as shell syntax. Closes #42: Validate module paths in adf create before path.join. Paths containing ".." or absolute paths are rejected with a clear error. A secondary resolved-path check confirms the final path stays within the .ai/ directory, guarding against platform-specific bypass patterns. Co-Authored-By: Claude Sonnet 4.6 --- packages/cli/src/commands/adf.ts | 12 ++++++++++++ packages/cli/src/git-helpers.ts | 15 +++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/commands/adf.ts b/packages/cli/src/commands/adf.ts index c9dad52..ff06b19 100644 --- a/packages/cli/src/commands/adf.ts +++ b/packages/cli/src/commands/adf.ts @@ -480,7 +480,19 @@ function adfCreate(options: CLIOptions, args: string[]): number { const modulePath = moduleArg.endsWith('.adf') ? moduleArg : `${moduleArg}.adf`; const moduleRelPath = modulePath.replace(/\\/g, '/'); + + // Prevent directory traversal: reject paths that escape the .ai/ directory + if (moduleRelPath.includes('..') || path.isAbsolute(moduleRelPath)) { + throw new CLIError(`Invalid module path: "${moduleRelPath}". Path must not contain ".." or be absolute.`); + } + const moduleAbsPath = path.join(aiDir, moduleRelPath); + const resolvedAiDir = path.resolve(aiDir); + const resolvedModulePath = path.resolve(moduleAbsPath); + if (!resolvedModulePath.startsWith(resolvedAiDir + path.sep)) { + throw new CLIError(`Invalid module path: "${moduleRelPath}". Path must stay within ${aiDir}/.`); + } + fs.mkdirSync(path.dirname(moduleAbsPath), { recursive: true }); let fileCreated = false; diff --git a/packages/cli/src/git-helpers.ts b/packages/cli/src/git-helpers.ts index 54b059d..455eed9 100644 --- a/packages/cli/src/git-helpers.ts +++ b/packages/cli/src/git-helpers.ts @@ -1,9 +1,9 @@ /** * Shared git invocation helpers. * - * Centralizes all child-process git calls behind a single `runGit()` that - * uses `shell: true` for cross-platform PATH resolution (fixes WSL, CMD, - * PowerShell parity — see ADX-005 F2). + * Centralizes all child-process git calls behind a single `runGit()`. + * All args are hardcoded call-site strings, never user input — but we + * still avoid `shell: true` to eliminate any shell-injection surface. */ import { execFileSync } from 'node:child_process'; @@ -16,17 +16,16 @@ import type { GitCommit } from '@stackbilt/types'; /** * Run a git command and return its stdout. * - * Uses `shell: true` so that the OS shell resolves the `git` binary via - * PATH. This is the key cross-platform fix: `execFileSync` *without* a - * shell can fail on WSL/Windows when git lives in a PATH entry the Node - * process doesn't see directly. + * Does NOT use `shell: true` — Node resolves `git` via PATH directly, which + * works on WSL, Linux, macOS, and Windows (Git for Windows adds git to PATH + * at install time). Using shell: true is unnecessary here and would allow + * shell metacharacters in args to be interpreted as shell syntax. */ export function runGit(args: string[]): string { return execFileSync('git', args, { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], maxBuffer: 10 * 1024 * 1024, - shell: true, }); }