mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-01-31 06:42:03 +00:00
security: Fix critical vulnerabilities in worktree init script feature
Fix multiple command injection and security vulnerabilities in the worktree initialization script system: **Critical Fixes:** - Add branch name validation to prevent command injection in create/delete endpoints - Replace string interpolation with array-based command execution using spawnProcess - Implement safe environment variable allowlist to prevent credential exposure - Add script content validation with 1MB size limit and dangerous pattern detection **Code Quality:** - Centralize execGitCommand helper in common.ts using @automaker/platform's spawnProcess - Remove duplicate isGitRepo implementation, standardize imports to @automaker/git-utils - Follow DRY principle by reusing existing platform utilities - Add comprehensive JSDoc documentation with security examples This addresses 6 critical/high severity vulnerabilities identified in security audit: 1. Command injection via unsanitized branch names (delete.ts) 2. Command injection via unsanitized branch names (create.ts) 3. Missing branch validation in init script execution 4. Environment variable exposure (ANTHROPIC_API_KEY and other secrets) 5. Path injection via command substitution 6. Arbitrary script execution without content limits Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -3,15 +3,51 @@
|
||||
*/
|
||||
|
||||
import { createLogger } from '@automaker/utils';
|
||||
import { spawnProcess } from '@automaker/platform';
|
||||
import { exec } from 'child_process';
|
||||
import { promisify } from 'util';
|
||||
import path from 'path';
|
||||
import { getErrorMessage as getErrorMessageShared, createLogError } from '../common.js';
|
||||
import { FeatureLoader } from '../../services/feature-loader.js';
|
||||
|
||||
const logger = createLogger('Worktree');
|
||||
export const execAsync = promisify(exec);
|
||||
const featureLoader = new FeatureLoader();
|
||||
|
||||
// ============================================================================
|
||||
// Secure Command Execution
|
||||
// ============================================================================
|
||||
|
||||
/**
|
||||
* Execute git command with array arguments to prevent command injection.
|
||||
* Uses spawnProcess from @automaker/platform for secure, cross-platform execution.
|
||||
*
|
||||
* @param args - Array of git command arguments (e.g., ['worktree', 'add', path])
|
||||
* @param cwd - Working directory to execute the command in
|
||||
* @returns Promise resolving to stdout output
|
||||
* @throws Error with stderr message if command fails
|
||||
*
|
||||
* @example
|
||||
* ```typescript
|
||||
* // Safe: no injection possible
|
||||
* await execGitCommand(['branch', '-D', branchName], projectPath);
|
||||
*
|
||||
* // Instead of unsafe:
|
||||
* // await execAsync(`git branch -D ${branchName}`, { cwd });
|
||||
* ```
|
||||
*/
|
||||
export async function execGitCommand(args: string[], cwd: string): Promise<string> {
|
||||
const result = await spawnProcess({
|
||||
command: 'git',
|
||||
args,
|
||||
cwd,
|
||||
});
|
||||
|
||||
// spawnProcess returns { stdout, stderr, exitCode }
|
||||
if (result.exitCode === 0) {
|
||||
return result.stdout;
|
||||
} else {
|
||||
const errorMessage = result.stderr || `Git command failed with code ${result.exitCode}`;
|
||||
throw new Error(errorMessage);
|
||||
}
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
// Constants
|
||||
@@ -99,18 +135,6 @@ export function normalizePath(p: string): string {
|
||||
return p.replace(/\\/g, '/');
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a path is a git repo
|
||||
*/
|
||||
export async function isGitRepo(repoPath: string): Promise<boolean> {
|
||||
try {
|
||||
await execAsync('git rev-parse --is-inside-work-tree', { cwd: repoPath });
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a git repository has at least one commit (i.e., HEAD exists)
|
||||
* Returns false for freshly initialized repos with no commits
|
||||
|
||||
@@ -3,7 +3,8 @@
|
||||
*/
|
||||
|
||||
import type { Request, Response, NextFunction } from 'express';
|
||||
import { isGitRepo, hasCommits } from './common.js';
|
||||
import { isGitRepo } from '@automaker/git-utils';
|
||||
import { hasCommits } from './common.js';
|
||||
|
||||
interface ValidationOptions {
|
||||
/** Check if the path is a git repository (default: true) */
|
||||
|
||||
@@ -13,12 +13,14 @@ import { promisify } from 'util';
|
||||
import path from 'path';
|
||||
import * as secureFs from '../../../lib/secure-fs.js';
|
||||
import type { EventEmitter } from '../../../lib/events.js';
|
||||
import { isGitRepo } from '@automaker/git-utils';
|
||||
import {
|
||||
isGitRepo,
|
||||
getErrorMessage,
|
||||
logError,
|
||||
normalizePath,
|
||||
ensureInitialCommit,
|
||||
isValidBranchName,
|
||||
execGitCommand,
|
||||
} from '../common.js';
|
||||
import { trackBranch } from './branch-tracking.js';
|
||||
import { createLogger } from '@automaker/utils';
|
||||
@@ -96,6 +98,26 @@ export function createCreateHandler(events: EventEmitter) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate branch name to prevent command injection
|
||||
if (!isValidBranchName(branchName)) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error:
|
||||
'Invalid branch name. Branch names must contain only letters, numbers, dots, hyphens, underscores, and forward slashes.',
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate base branch if provided
|
||||
if (baseBranch && !isValidBranchName(baseBranch) && baseBranch !== 'HEAD') {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error:
|
||||
'Invalid base branch name. Branch names must contain only letters, numbers, dots, hyphens, underscores, and forward slashes.',
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
if (!(await isGitRepo(projectPath))) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
@@ -145,30 +167,28 @@ export function createCreateHandler(events: EventEmitter) {
|
||||
// Create worktrees directory if it doesn't exist
|
||||
await secureFs.mkdir(worktreesDir, { recursive: true });
|
||||
|
||||
// Check if branch exists
|
||||
// Check if branch exists (using array arguments to prevent injection)
|
||||
let branchExists = false;
|
||||
try {
|
||||
await execAsync(`git rev-parse --verify ${branchName}`, {
|
||||
cwd: projectPath,
|
||||
});
|
||||
await execGitCommand(['rev-parse', '--verify', branchName], projectPath);
|
||||
branchExists = true;
|
||||
} catch {
|
||||
// Branch doesn't exist
|
||||
}
|
||||
|
||||
// Create worktree
|
||||
let createCmd: string;
|
||||
// Create worktree (using array arguments to prevent injection)
|
||||
if (branchExists) {
|
||||
// Use existing branch
|
||||
createCmd = `git worktree add "${worktreePath}" ${branchName}`;
|
||||
await execGitCommand(['worktree', 'add', worktreePath, branchName], projectPath);
|
||||
} else {
|
||||
// Create new branch from base or HEAD
|
||||
const base = baseBranch || 'HEAD';
|
||||
createCmd = `git worktree add -b ${branchName} "${worktreePath}" ${base}`;
|
||||
await execGitCommand(
|
||||
['worktree', 'add', '-b', branchName, worktreePath, base],
|
||||
projectPath
|
||||
);
|
||||
}
|
||||
|
||||
await execAsync(createCmd, { cwd: projectPath });
|
||||
|
||||
// Note: We intentionally do NOT symlink .automaker to worktrees
|
||||
// Features and config are always accessed from the main project path
|
||||
// This avoids symlink loop issues when activating worktrees
|
||||
|
||||
@@ -6,9 +6,11 @@ import type { Request, Response } from 'express';
|
||||
import { exec } from 'child_process';
|
||||
import { promisify } from 'util';
|
||||
import { isGitRepo } from '@automaker/git-utils';
|
||||
import { getErrorMessage, logError } from '../common.js';
|
||||
import { getErrorMessage, logError, isValidBranchName, execGitCommand } from '../common.js';
|
||||
import { createLogger } from '@automaker/utils';
|
||||
|
||||
const execAsync = promisify(exec);
|
||||
const logger = createLogger('Worktree');
|
||||
|
||||
export function createDeleteHandler() {
|
||||
return async (req: Request, res: Response): Promise<void> => {
|
||||
@@ -46,22 +48,25 @@ export function createDeleteHandler() {
|
||||
// Could not get branch name
|
||||
}
|
||||
|
||||
// Remove the worktree
|
||||
// Remove the worktree (using array arguments to prevent injection)
|
||||
try {
|
||||
await execAsync(`git worktree remove "${worktreePath}" --force`, {
|
||||
cwd: projectPath,
|
||||
});
|
||||
await execGitCommand(['worktree', 'remove', worktreePath, '--force'], projectPath);
|
||||
} catch (error) {
|
||||
// Try with prune if remove fails
|
||||
await execAsync('git worktree prune', { cwd: projectPath });
|
||||
await execGitCommand(['worktree', 'prune'], projectPath);
|
||||
}
|
||||
|
||||
// Optionally delete the branch
|
||||
if (deleteBranch && branchName && branchName !== 'main' && branchName !== 'master') {
|
||||
try {
|
||||
await execAsync(`git branch -D ${branchName}`, { cwd: projectPath });
|
||||
} catch {
|
||||
// Branch deletion failed, not critical
|
||||
// Validate branch name to prevent command injection
|
||||
if (!isValidBranchName(branchName)) {
|
||||
logger.warn(`Invalid branch name detected, skipping deletion: ${branchName}`);
|
||||
} else {
|
||||
try {
|
||||
await execGitCommand(['branch', '-D', branchName], projectPath);
|
||||
} catch {
|
||||
// Branch deletion failed, not critical
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -10,7 +10,7 @@
|
||||
import type { Request, Response } from 'express';
|
||||
import path from 'path';
|
||||
import * as secureFs from '../../../lib/secure-fs.js';
|
||||
import { getErrorMessage, logError } from '../common.js';
|
||||
import { getErrorMessage, logError, isValidBranchName } from '../common.js';
|
||||
import { createLogger } from '@automaker/utils';
|
||||
import type { EventEmitter } from '../../../lib/events.js';
|
||||
import { forceRunInitScript } from '../../../services/init-script-service.js';
|
||||
@@ -20,6 +20,9 @@ const logger = createLogger('InitScript');
|
||||
/** Fixed path for init script within .automaker directory */
|
||||
const INIT_SCRIPT_FILENAME = 'worktree-init.sh';
|
||||
|
||||
/** Maximum allowed size for init scripts (1MB) */
|
||||
const MAX_SCRIPT_SIZE_BYTES = 1024 * 1024;
|
||||
|
||||
/**
|
||||
* Get the full path to the init script for a project
|
||||
*/
|
||||
@@ -99,6 +102,31 @@ export function createPutInitScriptHandler() {
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate script size to prevent disk exhaustion
|
||||
const sizeBytes = Buffer.byteLength(content, 'utf-8');
|
||||
if (sizeBytes > MAX_SCRIPT_SIZE_BYTES) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error: `Script size (${Math.round(sizeBytes / 1024)}KB) exceeds maximum allowed size (${Math.round(MAX_SCRIPT_SIZE_BYTES / 1024)}KB)`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Log warning if potentially dangerous patterns are detected (non-blocking)
|
||||
const dangerousPatterns = [
|
||||
/rm\s+-rf\s+\/(?!\s*\$)/i, // rm -rf / (not followed by variable)
|
||||
/curl\s+.*\|\s*(?:bash|sh)/i, // curl | bash
|
||||
/wget\s+.*\|\s*(?:bash|sh)/i, // wget | sh
|
||||
];
|
||||
|
||||
for (const pattern of dangerousPatterns) {
|
||||
if (pattern.test(content)) {
|
||||
logger.warn(
|
||||
`Init script contains potentially dangerous pattern: ${pattern.source}. User responsibility to verify script safety.`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
const scriptPath = getInitScriptPath(projectPath);
|
||||
const automakerDir = path.dirname(scriptPath);
|
||||
|
||||
@@ -193,6 +221,16 @@ export function createRunInitScriptHandler(events: EventEmitter) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate branch name to prevent injection via environment variables
|
||||
if (!isValidBranchName(branch)) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error:
|
||||
'Invalid branch name. Branch names must contain only letters, numbers, dots, hyphens, underscores, and forward slashes.',
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const scriptPath = getInitScriptPath(projectPath);
|
||||
|
||||
// Check if script exists
|
||||
|
||||
@@ -191,22 +191,47 @@ export class InitScriptService {
|
||||
branch,
|
||||
});
|
||||
|
||||
// Spawn the script
|
||||
// Build safe environment - only pass necessary variables, not all of process.env
|
||||
// This prevents exposure of sensitive credentials like ANTHROPIC_API_KEY
|
||||
const safeEnv: Record<string, string> = {
|
||||
// Automaker-specific variables
|
||||
AUTOMAKER_PROJECT_PATH: projectPath,
|
||||
AUTOMAKER_WORKTREE_PATH: worktreePath,
|
||||
AUTOMAKER_BRANCH: branch,
|
||||
|
||||
// Essential system variables
|
||||
PATH: process.env.PATH || '',
|
||||
HOME: process.env.HOME || '',
|
||||
USER: process.env.USER || '',
|
||||
TMPDIR: process.env.TMPDIR || process.env.TEMP || process.env.TMP || '/tmp',
|
||||
|
||||
// Shell and locale
|
||||
SHELL: process.env.SHELL || '',
|
||||
LANG: process.env.LANG || 'en_US.UTF-8',
|
||||
LC_ALL: process.env.LC_ALL || '',
|
||||
|
||||
// Force color output even though we're not a TTY
|
||||
FORCE_COLOR: '1',
|
||||
npm_config_color: 'always',
|
||||
CLICOLOR_FORCE: '1',
|
||||
|
||||
// Git configuration
|
||||
GIT_TERMINAL_PROMPT: '0',
|
||||
};
|
||||
|
||||
// Platform-specific additions
|
||||
if (process.platform === 'win32') {
|
||||
safeEnv.USERPROFILE = process.env.USERPROFILE || '';
|
||||
safeEnv.APPDATA = process.env.APPDATA || '';
|
||||
safeEnv.LOCALAPPDATA = process.env.LOCALAPPDATA || '';
|
||||
safeEnv.SystemRoot = process.env.SystemRoot || 'C:\\Windows';
|
||||
safeEnv.TEMP = process.env.TEMP || '';
|
||||
}
|
||||
|
||||
// Spawn the script with safe environment
|
||||
const child = spawn(shellCmd.shell, [...shellCmd.args, scriptPath], {
|
||||
cwd: worktreePath,
|
||||
env: {
|
||||
...process.env,
|
||||
// Provide useful env vars to the script
|
||||
AUTOMAKER_PROJECT_PATH: projectPath,
|
||||
AUTOMAKER_WORKTREE_PATH: worktreePath,
|
||||
AUTOMAKER_BRANCH: branch,
|
||||
// Force color output even though we're not a TTY
|
||||
FORCE_COLOR: '1',
|
||||
npm_config_color: 'always',
|
||||
CLICOLOR_FORCE: '1',
|
||||
// Git colors
|
||||
GIT_TERMINAL_PROMPT: '0',
|
||||
},
|
||||
env: safeEnv,
|
||||
stdio: ['ignore', 'pipe', 'pipe'],
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user