From 7a84d96d726908a460f269b9e7f2354f8a752eaa Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 3 Feb 2026 20:17:47 -0600 Subject: [PATCH] fix(#274): Add input validation to prevent command injection in git operations Implemented strict whitelist-based validation for git branch names and repository URLs to prevent command injection vulnerabilities in worktree operations. Security fixes: - Created git-validation.util.ts with whitelist validation functions - Added custom DTO validators for branch names and repository URLs - Applied defense-in-depth validation in WorktreeManagerService - Comprehensive test coverage (31 tests) for all validation scenarios Validation rules: - Branch names: alphanumeric + hyphens + underscores + slashes + dots only - Repository URLs: https://, http://, ssh://, git:// protocols only - Blocks: option injection (--), command substitution ($(), ``), shell operators - Prevents: SSRF attacks (localhost, internal networks), credential injection Defense layers: 1. DTO validation (first line of defense at API boundary) 2. Service-level validation (defense-in-depth before git operations) Fixes #274 Co-Authored-By: Claude Sonnet 4.5 --- .../src/api/agents/dto/spawn-agent.dto.ts | 57 +++++ .../src/git/git-validation.util.spec.ts | 238 ++++++++++++++++++ .../src/git/git-validation.util.ts | 174 +++++++++++++ .../src/git/worktree-manager.service.ts | 6 + docs/scratchpads/274-command-injection.md | 80 ++++++ 5 files changed, 555 insertions(+) create mode 100644 apps/orchestrator/src/git/git-validation.util.spec.ts create mode 100644 apps/orchestrator/src/git/git-validation.util.ts create mode 100644 docs/scratchpads/274-command-injection.md diff --git a/apps/orchestrator/src/api/agents/dto/spawn-agent.dto.ts b/apps/orchestrator/src/api/agents/dto/spawn-agent.dto.ts index 9941873..181b48d 100644 --- a/apps/orchestrator/src/api/agents/dto/spawn-agent.dto.ts +++ b/apps/orchestrator/src/api/agents/dto/spawn-agent.dto.ts @@ -7,10 +7,65 @@ import { IsOptional, ArrayNotEmpty, IsIn, + Validate, + ValidatorConstraint, + ValidatorConstraintInterface, + ValidationArguments, } from "class-validator"; import { Type } from "class-transformer"; import { AgentType } from "../../../spawner/types/agent-spawner.types"; import { GateProfileType } from "../../../coordinator/types/gate-config.types"; +import { validateBranchName, validateRepositoryUrl } from "../../../git/git-validation.util"; + +/** + * Custom validator for git branch names + * Uses whitelist-based validation to prevent command injection + */ +@ValidatorConstraint({ name: "isValidBranchName", async: false }) +export class IsValidBranchName implements ValidatorConstraintInterface { + validate(branchName: string, _args: ValidationArguments): boolean { + try { + validateBranchName(branchName); + return true; + } catch { + return false; + } + } + + defaultMessage(args: ValidationArguments): string { + try { + validateBranchName(args.value as string); + return "Branch name is invalid"; + } catch (error) { + return error instanceof Error ? error.message : "Branch name is invalid"; + } + } +} + +/** + * Custom validator for git repository URLs + * Prevents SSRF and command injection via dangerous protocols + */ +@ValidatorConstraint({ name: "isValidRepositoryUrl", async: false }) +export class IsValidRepositoryUrl implements ValidatorConstraintInterface { + validate(repositoryUrl: string, _args: ValidationArguments): boolean { + try { + validateRepositoryUrl(repositoryUrl); + return true; + } catch { + return false; + } + } + + defaultMessage(args: ValidationArguments): string { + try { + validateRepositoryUrl(args.value as string); + return "Repository URL is invalid"; + } catch (error) { + return error instanceof Error ? error.message : "Repository URL is invalid"; + } + } +} /** * Context DTO for agent spawn request @@ -18,10 +73,12 @@ import { GateProfileType } from "../../../coordinator/types/gate-config.types"; export class AgentContextDto { @IsString() @IsNotEmpty() + @Validate(IsValidRepositoryUrl) repository!: string; @IsString() @IsNotEmpty() + @Validate(IsValidBranchName) branch!: string; @IsArray() diff --git a/apps/orchestrator/src/git/git-validation.util.spec.ts b/apps/orchestrator/src/git/git-validation.util.spec.ts new file mode 100644 index 0000000..097dc57 --- /dev/null +++ b/apps/orchestrator/src/git/git-validation.util.spec.ts @@ -0,0 +1,238 @@ +/** + * Git Validation Utility Tests + * + * Tests for command injection prevention in git operations + */ + +import { describe, it, expect } from "vitest"; +import { BadRequestException } from "@nestjs/common"; +import { + validateBranchName, + validateRepositoryUrl, + validateSpawnContext, +} from "./git-validation.util"; + +describe("validateBranchName", () => { + describe("Valid branch names", () => { + it("should accept standard branch names", () => { + expect(() => validateBranchName("main")).not.toThrow(); + expect(() => validateBranchName("develop")).not.toThrow(); + expect(() => validateBranchName("master")).not.toThrow(); + }); + + it("should accept feature branch names with slashes", () => { + expect(() => validateBranchName("feature/add-login")).not.toThrow(); + expect(() => validateBranchName("fix/bug-123")).not.toThrow(); + expect(() => validateBranchName("hotfix/security-patch")).not.toThrow(); + }); + + it("should accept branch names with hyphens and underscores", () => { + expect(() => validateBranchName("feature-branch")).not.toThrow(); + expect(() => validateBranchName("feature_branch")).not.toThrow(); + expect(() => validateBranchName("feature-branch_v2")).not.toThrow(); + }); + + it("should accept branch names with dots", () => { + expect(() => validateBranchName("release/1.0.0")).not.toThrow(); + expect(() => validateBranchName("v2.5.1")).not.toThrow(); + }); + + it("should accept branch names with numbers", () => { + expect(() => validateBranchName("feature-123")).not.toThrow(); + expect(() => validateBranchName("123-bugfix")).not.toThrow(); + }); + }); + + describe("Invalid branch names (Command Injection)", () => { + it("should reject empty or whitespace-only names", () => { + expect(() => validateBranchName("")).toThrow(BadRequestException); + expect(() => validateBranchName(" ")).toThrow(BadRequestException); + expect(() => validateBranchName("\t")).toThrow(BadRequestException); + }); + + it("should reject names starting with hyphen (option injection)", () => { + expect(() => validateBranchName("--config")).toThrow(BadRequestException); + expect(() => validateBranchName("-malicious")).toThrow(BadRequestException); + }); + + it("should reject names with double dots (range specification)", () => { + expect(() => validateBranchName("feature..main")).toThrow(BadRequestException); + expect(() => validateBranchName("..malicious")).toThrow(BadRequestException); + }); + + it("should reject names with path traversal patterns", () => { + expect(() => validateBranchName("../etc/passwd")).toThrow(BadRequestException); + expect(() => validateBranchName("feature/../main")).toThrow(BadRequestException); + expect(() => validateBranchName("malicious/..")).toThrow(BadRequestException); + }); + + it("should reject names ending with .lock (reserved by git)", () => { + expect(() => validateBranchName("feature.lock")).toThrow(BadRequestException); + expect(() => validateBranchName("main.lock")).toThrow(BadRequestException); + }); + + it("should reject names with special shell characters", () => { + expect(() => validateBranchName("feature;rm -rf /")).toThrow(BadRequestException); + expect(() => validateBranchName("feature$malicious")).toThrow(BadRequestException); + expect(() => validateBranchName("feature`whoami`")).toThrow(BadRequestException); + expect(() => validateBranchName("feature$(whoami)")).toThrow(BadRequestException); + expect(() => validateBranchName("feature|malicious")).toThrow(BadRequestException); + expect(() => validateBranchName("feature&malicious")).toThrow(BadRequestException); + }); + + it("should reject names with control characters", () => { + expect(() => validateBranchName("feature\x00malicious")).toThrow(BadRequestException); + expect(() => validateBranchName("feature\x1Fmalicious")).toThrow(BadRequestException); + expect(() => validateBranchName("feature\x7Fmalicious")).toThrow(BadRequestException); + }); + + it("should reject names exceeding maximum length", () => { + const longName = "a".repeat(256); + expect(() => validateBranchName(longName)).toThrow(BadRequestException); + }); + + it("should reject names with spaces", () => { + expect(() => validateBranchName("feature branch")).toThrow(BadRequestException); + expect(() => validateBranchName("feature branch")).toThrow(BadRequestException); + }); + }); +}); + +describe("validateRepositoryUrl", () => { + describe("Valid repository URLs", () => { + it("should accept HTTPS URLs", () => { + expect(() => validateRepositoryUrl("https://github.com/user/repo.git")).not.toThrow(); + expect(() => validateRepositoryUrl("https://gitlab.com/group/project.git")).not.toThrow(); + expect(() => validateRepositoryUrl("https://bitbucket.org/user/repo.git")).not.toThrow(); + }); + + it("should accept HTTP URLs (for development)", () => { + expect(() => validateRepositoryUrl("http://git.example.com/repo.git")).not.toThrow(); + }); + + it("should accept SSH URLs with git@ format", () => { + expect(() => validateRepositoryUrl("git@github.com:user/repo.git")).not.toThrow(); + expect(() => validateRepositoryUrl("ssh://git@gitlab.com/user/repo.git")).not.toThrow(); + }); + + it("should accept git:// protocol", () => { + expect(() => validateRepositoryUrl("git://github.com/user/repo.git")).not.toThrow(); + }); + }); + + describe("Invalid repository URLs (Security Risks)", () => { + it("should reject empty or whitespace-only URLs", () => { + expect(() => validateRepositoryUrl("")).toThrow(BadRequestException); + expect(() => validateRepositoryUrl(" ")).toThrow(BadRequestException); + }); + + it("should reject dangerous protocols (file://)", () => { + expect(() => validateRepositoryUrl("file:///etc/passwd")).toThrow(BadRequestException); + expect(() => validateRepositoryUrl("file://C:/Windows/System32")).toThrow( + BadRequestException + ); + }); + + it("should reject dangerous protocols (javascript:, data:)", () => { + expect(() => validateRepositoryUrl("javascript:alert('XSS')")).toThrow(BadRequestException); + expect(() => validateRepositoryUrl("data:text/html,")).toThrow( + BadRequestException + ); + }); + + it("should reject localhost URLs (SSRF protection)", () => { + expect(() => validateRepositoryUrl("https://localhost/repo.git")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://127.0.0.1/repo.git")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://0.0.0.0/repo.git")).toThrow(BadRequestException); + expect(() => validateRepositoryUrl("http://::1/repo.git")).toThrow(BadRequestException); + }); + + it("should reject internal network URLs (SSRF protection)", () => { + expect(() => validateRepositoryUrl("https://192.168.1.1/repo.git")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://10.0.0.1/repo.git")).toThrow(BadRequestException); + expect(() => validateRepositoryUrl("https://172.16.0.1/repo.git")).toThrow( + BadRequestException + ); + }); + + it("should reject URLs with embedded credentials", () => { + expect(() => validateRepositoryUrl("https://user:pass@github.com/repo.git")).toThrow( + BadRequestException + ); + }); + + it("should reject URLs with shell special characters", () => { + expect(() => validateRepositoryUrl("https://github.com/repo.git;whoami")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://github.com/repo.git|malicious")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://github.com/repo.git&malicious")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://github.com/repo.git$malicious")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("https://github.com/repo.git`whoami`")).toThrow( + BadRequestException + ); + }); + + it("should reject URLs exceeding maximum length", () => { + const longUrl = "https://github.com/" + "a".repeat(2000) + ".git"; + expect(() => validateRepositoryUrl(longUrl)).toThrow(BadRequestException); + }); + + it("should reject unknown/dangerous protocols", () => { + expect(() => validateRepositoryUrl("ftp://example.com/repo.git")).toThrow( + BadRequestException + ); + expect(() => validateRepositoryUrl("telnet://example.com")).toThrow(BadRequestException); + }); + }); +}); + +describe("validateSpawnContext", () => { + it("should validate both repository and branch", () => { + expect(() => + validateSpawnContext({ + repository: "https://github.com/user/repo.git", + branch: "feature/add-login", + }) + ).not.toThrow(); + }); + + it("should reject invalid repository", () => { + expect(() => + validateSpawnContext({ + repository: "file:///etc/passwd", + branch: "main", + }) + ).toThrow(BadRequestException); + }); + + it("should reject invalid branch", () => { + expect(() => + validateSpawnContext({ + repository: "https://github.com/user/repo.git", + branch: "--config malicious", + }) + ).toThrow(BadRequestException); + }); + + it("should reject both invalid repository and branch", () => { + expect(() => + validateSpawnContext({ + repository: "javascript:alert('XSS')", + branch: "$(whoami)", + }) + ).toThrow(BadRequestException); + }); +}); diff --git a/apps/orchestrator/src/git/git-validation.util.ts b/apps/orchestrator/src/git/git-validation.util.ts new file mode 100644 index 0000000..e11b75c --- /dev/null +++ b/apps/orchestrator/src/git/git-validation.util.ts @@ -0,0 +1,174 @@ +/** + * Git Input Validation Utility + * + * Provides strict validation for git references (branch names, repository URLs) + * to prevent command injection vulnerabilities. + * + * Security: Whitelist-based approach - only allow known-safe characters. + */ + +import { BadRequestException } from "@nestjs/common"; + +/** + * Validates a git branch name for safety + * + * Allowed format: alphanumeric, hyphens, underscores, forward slashes + * Examples: "main", "feature/add-login", "fix/bug_123" + * + * Rejected: Special characters that could be interpreted as git syntax + * Examples: "--option", "$(command)", ";malicious", "`command`" + * + * @param branchName - The branch name to validate + * @throws BadRequestException if branch name is invalid + */ +export function validateBranchName(branchName: string): void { + // Check for empty or whitespace-only + if (!branchName || branchName.trim().length === 0) { + throw new BadRequestException("Branch name cannot be empty"); + } + + // Check length (git has a 255 char limit for ref names) + if (branchName.length > 255) { + throw new BadRequestException("Branch name exceeds maximum length (255 characters)"); + } + + // Whitelist: only allow alphanumeric, hyphens, underscores, forward slashes, dots + // This prevents all forms of command injection + const safePattern = /^[a-zA-Z0-9/_.-]+$/; + if (!safePattern.test(branchName)) { + throw new BadRequestException( + `Branch name contains invalid characters. Only alphanumeric, hyphens, underscores, slashes, and dots are allowed.` + ); + } + + // Prevent git option injection (branch names starting with -) + if (branchName.startsWith("-")) { + throw new BadRequestException( + "Branch name cannot start with a hyphen (prevents option injection)" + ); + } + + // Prevent double dots (used for range specifications in git) + if (branchName.includes("..")) { + throw new BadRequestException("Branch name cannot contain consecutive dots (..)"); + } + + // Prevent path traversal patterns + if (branchName.includes("/../") || branchName.startsWith("../") || branchName.endsWith("/..")) { + throw new BadRequestException("Branch name cannot contain path traversal patterns"); + } + + // Prevent ending with .lock (reserved by git) + if (branchName.endsWith(".lock")) { + throw new BadRequestException("Branch name cannot end with .lock (reserved by git)"); + } + + // Prevent control characters + // eslint-disable-next-line no-control-regex + if (/[\x00-\x1F\x7F]/.test(branchName)) { + throw new BadRequestException("Branch name cannot contain control characters"); + } +} + +/** + * Validates a git repository URL for safety + * + * Allowed protocols: https, http (dev only), ssh (git@) + * Prevents: file://, javascript:, data:, and other dangerous protocols + * + * @param repositoryUrl - The repository URL to validate + * @throws BadRequestException if URL is invalid or uses dangerous protocol + */ +export function validateRepositoryUrl(repositoryUrl: string): void { + // Check for empty or whitespace-only + if (!repositoryUrl || repositoryUrl.trim().length === 0) { + throw new BadRequestException("Repository URL cannot be empty"); + } + + // Check length (reasonable limit for URLs) + if (repositoryUrl.length > 2000) { + throw new BadRequestException("Repository URL exceeds maximum length (2000 characters)"); + } + + // Remove whitespace + const url = repositoryUrl.trim(); + + // Whitelist allowed protocols + const httpsPattern = /^https:\/\//i; + const httpPattern = /^http:\/\//i; // Only for development + const sshGitPattern = /^git@[a-zA-Z0-9.-]+:/; // git@host:repo format + const sshUrlPattern = /^ssh:\/\/git@[a-zA-Z0-9.-]+(\/|:)/; // ssh://git@host/repo or ssh://git@host:repo + + if ( + !httpsPattern.test(url) && + !httpPattern.test(url) && + !sshGitPattern.test(url) && + !sshUrlPattern.test(url) && + !url.startsWith("git://") + ) { + throw new BadRequestException( + "Repository URL must use https://, http://, ssh://, git://, or git@ protocol" + ); + } + + // Prevent dangerous protocols + const dangerousProtocols = [ + "file://", + "javascript:", + "data:", + "vbscript:", + "about:", + "chrome:", + "view-source:", + ]; + + for (const dangerous of dangerousProtocols) { + if (url.toLowerCase().startsWith(dangerous)) { + throw new BadRequestException( + `Repository URL cannot use ${dangerous} protocol (security risk)` + ); + } + } + + // Prevent localhost/internal network access (SSRF protection) + const localhostPatterns = [ + /https?:\/\/(localhost|127\.0\.0\.1|0\.0\.0\.0|::1)/i, + /https?:\/\/192\.168\./i, + /https?:\/\/10\./i, + /https?:\/\/172\.(1[6-9]|2\d|3[01])\./i, + ]; + + for (const pattern of localhostPatterns) { + if (pattern.test(url)) { + throw new BadRequestException( + "Repository URL cannot point to localhost or internal networks (SSRF protection)" + ); + } + } + + // Prevent credential injection in URL + if (url.includes("@") && !sshGitPattern.test(url) && !sshUrlPattern.test(url)) { + // Extract the part before @ to check if it looks like credentials + const beforeAt = url.split("@")[0]; + if (beforeAt.includes("://") && beforeAt.split("://")[1].includes(":")) { + throw new BadRequestException("Repository URL cannot contain embedded credentials"); + } + } + + // Prevent control characters and dangerous characters in URL + // eslint-disable-next-line no-control-regex + if (/[\x00-\x1F\x7F`$;|&]/.test(url)) { + throw new BadRequestException("Repository URL contains invalid or dangerous characters"); + } +} + +/** + * Validates a complete agent spawn context + * + * @param context - The spawn context with repository and branch + * @throws BadRequestException if any field is invalid + */ +export function validateSpawnContext(context: { repository: string; branch: string }): void { + validateRepositoryUrl(context.repository); + validateBranchName(context.branch); +} diff --git a/apps/orchestrator/src/git/worktree-manager.service.ts b/apps/orchestrator/src/git/worktree-manager.service.ts index 860c2c6..bb872d3 100644 --- a/apps/orchestrator/src/git/worktree-manager.service.ts +++ b/apps/orchestrator/src/git/worktree-manager.service.ts @@ -3,6 +3,7 @@ import { simpleGit, SimpleGit } from "simple-git"; import * as path from "path"; import { GitOperationsService } from "./git-operations.service"; import { WorktreeInfo, WorktreeError } from "./types"; +import { validateBranchName } from "./git-validation.util"; /** * Result of worktree cleanup operation @@ -70,6 +71,10 @@ export class WorktreeManagerService { throw new Error("taskId is required"); } + // Validate baseBranch to prevent command injection + // This is defense-in-depth - DTO validation should catch this first + validateBranchName(baseBranch); + const worktreePath = this.getWorktreePath(repoPath, agentId, taskId); const branchName = this.getBranchName(agentId, taskId); @@ -79,6 +84,7 @@ export class WorktreeManagerService { const git = this.getGit(repoPath); // Create worktree with new branch + // baseBranch is validated above to prevent command injection await git.raw(["worktree", "add", worktreePath, "-b", branchName, baseBranch]); this.logger.log(`Successfully created worktree at ${worktreePath}`); diff --git a/docs/scratchpads/274-command-injection.md b/docs/scratchpads/274-command-injection.md new file mode 100644 index 0000000..fd2f55c --- /dev/null +++ b/docs/scratchpads/274-command-injection.md @@ -0,0 +1,80 @@ +# Issue #274: Sanitize agent spawn command payloads (command injection risk) + +## Objective + +Add input validation and sanitization to agent spawn command payloads to prevent command injection vulnerabilities in git operations. + +## Security Impact + +**Severity:** P0 (Critical) - Blocks production deployment +**Attack Vector:** Federated instances can inject malicious commands via branch names +**Risk:** Command injection in git operations allowing arbitrary code execution + +## Vulnerability Details + +### Attack Flow + +1. Attacker sends federation command with malicious branch name +2. Payload passes through command service without validation +3. Branch name used directly in `git worktree add` command +4. Malicious git syntax executed on orchestrator + +### Vulnerable Code + +**File:** `apps/orchestrator/src/git/worktree-manager.service.ts:82` + +```typescript +await git.raw(["worktree", "add", worktreePath, "-b", branchName, baseBranch]); +``` + +**Input Source:** Federation command payload → no validation → git command + +### Attack Example + +```json +{ + "commandType": "agent.spawn", + "payload": { + "context": { + "branch": "feature/--config user.core.sshCommand=malicious" + } + } +} +``` + +## Approach + +### 1. Add Input Validation DTOs + +- Strict regex for branch names (alphanumeric + hyphens + underscores + slashes) +- Repository URL validation (https/ssh only) +- Reject dangerous characters (`;`, `$`, `` ` ``, `--`, etc.) + +### 2. Create Sanitization Utility + +- Whitelist-based approach +- Validate before any git operation +- Clear error messages on rejection + +### 3. Apply at Multiple Layers + +- DTO validation (first line of defense) +- Service-level sanitization (defense in depth) +- Git operation wrapper (last resort) + +## Progress + +- [ ] Create validation utility +- [ ] Update SpawnAgentDto with strict validation +- [ ] Update SpawnAgentCommandPayload type +- [ ] Add sanitization in WorktreeManagerService +- [ ] Add tests for validation +- [ ] Add tests for sanitization +- [ ] Security vulnerability FIXED +- [ ] Create PR +- [ ] Merge to develop +- [ ] Close issue #274 + +## Implementation Status + +**IN PROGRESS** - Adding input validation and sanitization