fix(security): close remaining /bin/sh -c shell-injection sites in bundle ID flows (#390)

* fix(security): close remaining /bin/sh -c shell-injection sites in bundle ID flows

Follow-up to #289. PR #289 hardened the `useShell=true` path through
`shellEscapeArg`, but four call sites still hand-built `/bin/sh -c`
strings interpolating user-controlled paths and passed them to the
executor with `useShell=false`:

- `src/utils/bundle-id.ts`
- `src/mcp/tools/project-discovery/get_mac_bundle_id.ts`
- `src/mcp/tools/macos/build_macos.ts`
- `src/utils/macos-steps.ts`

All four now invoke `defaults` and `PlistBuddy` directly with an argv
array, removing shell parsing entirely (option 1 from the issue). The
malicious appPath becomes a single positional argument to `defaults` /
`PlistBuddy` and is never interpreted as a shell expression.

Tests:
- The four `UNFIXED:` regression cases in `bundle-id-injection.test.ts`
  and `mac-bundle-id-injection.test.ts` are flipped to safe assertions
  (no `/bin/sh`, exact argv shape, `useShell=false`).
- New PlistBuddy-fallback cases assert the same shape on the second
  branch.
- Existing fixtures in `get_app_bundle_id.test.ts`,
  `get_mac_bundle_id.test.ts`, and `build_run_device.test.ts` updated
  to the new argv-shape command keys.
- Pre-fix verification: stashed the four source edits and re-ran the
  injection suites — 8/8 fail. Restored the fix — 8/8 pass.

Local gates: typecheck, lint, prettier --check, full vitest run
(1772 passed / 0 failed).

Closes #367

* test: Remove stale shell stub from macOS smoke test

---------

Co-authored-by: voidborne-d <voidborne-d@users.noreply.github.com>
Co-authored-by: Cameron Cooke <web@cameroncooke.com>

Author: voidborne-d

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixed
 
+- Fixed remaining `/bin/sh -c` shell-injection sites in bundle ID extraction and macOS launch flows by invoking `defaults` and `PlistBuddy` directly with argv arrays so user-supplied app paths are no longer interpreted by a shell ([#367](https://github.com/getsentry/XcodeBuildMCP/issues/367)).
 - Fixed simulator test JSONL accuracy by keeping preflight discovery observational, preserving only user-supplied test selectors, discovering multiline parameterized Swift Testing tests, and parsing destination-suffixed xcodebuild test result lines.
 - Removed stale physical-device log session status and shutdown cleanup for deprecated standalone device log capture, and corrected the device build-and-run tool description.
 - Fixed mixed Swift Testing and XCTest summaries so simulator test text output no longer overcounts parameterized Swift Testing results or issue lines.

src/mcp/tools/device/__tests__/build_run_device.test.ts

@@ -109,7 +109,7 @@ describe('build_run_device tool', () => {
           });
         }
 
-        if (command[0] === '/bin/sh') {
+        if (command[0] === 'defaults' || command[0] === '/usr/libexec/PlistBuddy') {
           return createMockCommandResponse({ success: true, output: 'io.sentry.MyApp' });
         }
 
@@ -143,7 +143,7 @@ describe('build_run_device tool', () => {
           });
         }
 
-        if (command[0] === '/bin/sh') {
+        if (command[0] === 'defaults' || command[0] === '/usr/libexec/PlistBuddy') {
           return createMockCommandResponse({ success: true, output: 'io.sentry.MyApp' });
         }
 
@@ -177,7 +177,7 @@ describe('build_run_device tool', () => {
           });
         }
 
-        if (command[0] === '/bin/sh') {
+        if (command[0] === 'defaults' || command[0] === '/usr/libexec/PlistBuddy') {
           return createMockCommandResponse({ success: true, output: 'io.sentry.MyApp' });
         }
 
@@ -218,7 +218,7 @@ describe('build_run_device tool', () => {
           });
         }
 
-        if (command[0] === '/bin/sh') {
+        if (command[0] === 'defaults' || command[0] === '/usr/libexec/PlistBuddy') {
           return createMockCommandResponse({ success: true, output: 'io.sentry.MyApp' });
         }
 
@@ -258,7 +258,7 @@ describe('build_run_device tool', () => {
           });
         }
 
-        if (command[0] === '/bin/sh') {
+        if (command[0] === 'defaults' || command[0] === '/usr/libexec/PlistBuddy') {
           return createMockCommandResponse({ success: true, output: 'io.sentry.MyWatchApp' });
         }
 

src/mcp/tools/macos/build_macos.ts

@@ -107,7 +107,7 @@ export function createBuildMacOSExecutor(
         );
 
         const plistResult = await executor(
-          ['/bin/sh', '-c', `defaults read "${appPath}/Contents/Info" CFBundleIdentifier`],
+          ['defaults', 'read', `${appPath}/Contents/Info`, 'CFBundleIdentifier'],
           'Extract Bundle ID',
           false,
         );

src/mcp/tools/project-discovery/__tests__/get_app_bundle_id.test.ts

@@ -91,7 +91,7 @@ describe('get_app_bundle_id plugin', () => {
 
     it('should return success with bundle ID using defaults read', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/path/to/MyApp.app/Info" CFBundleIdentifier': 'io.sentry.MyApp',
+        'defaults read /path/to/MyApp.app/Info CFBundleIdentifier': 'io.sentry.MyApp',
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
         existsSync: () => true,
@@ -116,10 +116,10 @@ describe('get_app_bundle_id plugin', () => {
 
     it('should fallback to PlistBuddy when defaults read fails', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/path/to/MyApp.app/Info" CFBundleIdentifier': new Error(
+        'defaults read /path/to/MyApp.app/Info CFBundleIdentifier': new Error(
           'defaults read failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/path/to/MyApp.app/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /path/to/MyApp.app/Info.plist':
           'io.sentry.MyApp',
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
@@ -145,10 +145,10 @@ describe('get_app_bundle_id plugin', () => {
 
     it('should return error when both extraction methods fail', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/path/to/MyApp.app/Info" CFBundleIdentifier': new Error(
+        'defaults read /path/to/MyApp.app/Info CFBundleIdentifier': new Error(
           'defaults read failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/path/to/MyApp.app/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /path/to/MyApp.app/Info.plist':
           new Error('Command failed'),
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
@@ -169,10 +169,10 @@ describe('get_app_bundle_id plugin', () => {
 
     it('keeps extraction errors short and preserves diagnostics', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/path/to/MyApp.app/Info" CFBundleIdentifier': new Error(
+        'defaults read /path/to/MyApp.app/Info CFBundleIdentifier': new Error(
           'defaults read failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/path/to/MyApp.app/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /path/to/MyApp.app/Info.plist':
           new Error('Command failed'),
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({

src/mcp/tools/project-discovery/__tests__/get_mac_bundle_id.test.ts

@@ -72,7 +72,7 @@ describe('get_mac_bundle_id plugin', () => {
 
     it('should return success with bundle ID using defaults read', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/Applications/MyApp.app/Contents/Info" CFBundleIdentifier':
+        'defaults read /Applications/MyApp.app/Contents/Info CFBundleIdentifier':
           'io.sentry.MyMacApp',
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
@@ -96,10 +96,10 @@ describe('get_mac_bundle_id plugin', () => {
 
     it('should fallback to PlistBuddy when defaults read fails', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/Applications/MyApp.app/Contents/Info" CFBundleIdentifier': new Error(
+        'defaults read /Applications/MyApp.app/Contents/Info CFBundleIdentifier': new Error(
           'defaults read failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/Applications/MyApp.app/Contents/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /Applications/MyApp.app/Contents/Info.plist':
           'io.sentry.MyMacApp',
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
@@ -123,10 +123,10 @@ describe('get_mac_bundle_id plugin', () => {
 
     it('should return error when both extraction methods fail', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/Applications/MyApp.app/Contents/Info" CFBundleIdentifier': new Error(
+        'defaults read /Applications/MyApp.app/Contents/Info CFBundleIdentifier': new Error(
           'Command failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/Applications/MyApp.app/Contents/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /Applications/MyApp.app/Contents/Info.plist':
           new Error('Command failed'),
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({
@@ -147,10 +147,10 @@ describe('get_mac_bundle_id plugin', () => {
 
     it('keeps extraction errors short and preserves diagnostics', async () => {
       const mockExecutor = createMockExecutorForCommands({
-        'defaults read "/Applications/MyApp.app/Contents/Info" CFBundleIdentifier': new Error(
+        'defaults read /Applications/MyApp.app/Contents/Info CFBundleIdentifier': new Error(
           'Command failed',
         ),
-        '/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "/Applications/MyApp.app/Contents/Info.plist"':
+        '/usr/libexec/PlistBuddy -c Print :CFBundleIdentifier /Applications/MyApp.app/Contents/Info.plist':
           new Error('Command failed'),
       });
       const mockFileSystemExecutor = createMockFileSystemExecutor({

src/mcp/tools/project-discovery/get_mac_bundle_id.ts

@@ -13,8 +13,8 @@ import {
 import { toErrorMessage } from '../../../utils/errors.ts';
 import { createBasicDiagnostics } from '../../../utils/diagnostics.ts';
 
-async function executeSyncCommand(command: string, executor: CommandExecutor): Promise<string> {
-  const result = await executor(['/bin/sh', '-c', command], 'macOS Bundle ID Extraction');
+async function runSpawn(command: string[], executor: CommandExecutor): Promise<string> {
+  const result = await executor(command, 'macOS Bundle ID Extraction', false);
   if (!result.success) {
     throw new Error(result.error ?? 'Command failed');
   }
@@ -49,14 +49,19 @@ export function createGetMacBundleIdExecutor(
       let bundleId: string;
 
       try {
-        bundleId = await executeSyncCommand(
-          `defaults read "${appPath}/Contents/Info" CFBundleIdentifier`,
+        bundleId = await runSpawn(
+          ['defaults', 'read', `${appPath}/Contents/Info`, 'CFBundleIdentifier'],
           executor,
         );
       } catch {
         try {
-          bundleId = await executeSyncCommand(
-            `/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "${appPath}/Contents/Info.plist"`,
+          bundleId = await runSpawn(
+            [
+              '/usr/libexec/PlistBuddy',
+              '-c',
+              'Print :CFBundleIdentifier',
+              `${appPath}/Contents/Info.plist`,
+            ],
             executor,
           );
         } catch (innerError) {

src/smoke-tests/__tests__/e2e-mcp-device-macos.test.ts

@@ -21,7 +21,6 @@ beforeAll(async () => {
       open: { success: true, output: '' },
       kill: { success: true, output: '' },
       pkill: { success: true, output: '' },
-      '/bin/sh': { success: true, output: 'io.sentry.MyApp' },
       'defaults read': { success: true, output: 'io.sentry.MyApp' },
       PlistBuddy: { success: true, output: 'io.sentry.MyApp' },
       xcresulttool: { success: true, output: '{}' },

src/utils/__tests__/bundle-id-injection.test.ts

@@ -4,86 +4,103 @@ import { extractBundleIdFromAppPath } from '../bundle-id.ts';
 import type { CommandExecutor } from '../CommandExecutor.ts';
 
 /**
- * CWE-78 regression tests for bundle-id.ts
+ * CWE-78 regression tests for bundle-id.ts.
  *
- * These tests verify that user-supplied appPath values containing shell
- * metacharacters do NOT result in shell injection when passed through
- * the executeSyncCommand → /bin/sh -c pipeline.
- *
- * CURRENT STATUS: These tests demonstrate the UNFIXED injection vectors
- * identified in the review. The command string passed to /bin/sh -c
- * contains unescaped user input, which would allow command injection.
+ * The implementation now invokes `defaults` and `PlistBuddy` directly with
+ * an argv array (no `/bin/sh -c`), so user-controlled `appPath` values
+ * containing shell metacharacters are passed as opaque positional
+ * arguments and never reach a shell parser.
  */
 
 type CapturedCall = {
   command: string[];
   logPrefix?: string;
+  useShell?: boolean;
 };
 
 const stubProcess = { pid: 1, on: () => stubProcess } as unknown as ChildProcess;
 
 function createCapturingExecutor(calls: CapturedCall[]): CommandExecutor {
-  return async (command, logPrefix) => {
-    calls.push({ command: [...command], logPrefix });
-    // Simulate 'defaults' returning a fake bundle ID
+  return async (command, logPrefix, useShell) => {
+    calls.push({ command: [...command], logPrefix, useShell });
     return { success: true, output: 'com.example.app', process: stubProcess };
   };
 }
 
 describe('bundle-id.ts — CWE-78 shell injection vectors', () => {
-  it('UNFIXED: double-quote breakout in appPath reaches /bin/sh -c unescaped', async () => {
+  it('does not invoke /bin/sh and passes a metacharacter-laden path as an argv element', async () => {
     const calls: CapturedCall[] = [];
     const executor = createCapturingExecutor(calls);
 
-    // Malicious appPath that breaks out of the double-quoted context
     const maliciousPath = '/tmp/evil" $(id) "bar';
     await extractBundleIdFromAppPath(maliciousPath, executor);
 
     expect(calls).toHaveLength(1);
-    const shellCommand = calls[0].command;
-
-    // The command is ['/bin/sh', '-c', '...']
-    expect(shellCommand[0]).toBe('/bin/sh');
-    expect(shellCommand[1]).toBe('-c');
-
-    const cmdString = shellCommand[2];
-
-    // VULNERABILITY: The raw user input is interpolated directly into the
-    // shell command string. The $(id) is NOT escaped and would execute.
-    // A safe implementation would either:
-    // 1. Not use shell at all (pass args array to spawn directly), or
-    // 2. Properly escape the appPath with shellEscapeArg
-    //
-    // This test documents the current vulnerable behavior.
-    // When the fix is applied, update the assertion to verify safety.
-    expect(cmdString).toContain('$(id)');
-
-    // Verify the command reaches shell — it's using /bin/sh -c
-    expect(shellCommand[0]).toBe('/bin/sh');
+    const call = calls[0];
+
+    expect(call.command[0]).toBe('defaults');
+    expect(call.command[0]).not.toBe('/bin/sh');
+    expect(call.useShell).toBe(false);
+
+    expect(call.command).toEqual([
+      'defaults',
+      'read',
+      `${maliciousPath}/Info`,
+      'CFBundleIdentifier',
+    ]);
   });
 
-  it('UNFIXED: semicolon injection in appPath allows command chaining', async () => {
+  it('isolates semicolon-injection attempts as a single argv element', async () => {
     const calls: CapturedCall[] = [];
     const executor = createCapturingExecutor(calls);
 
     const maliciousPath = '/tmp/foo"; rm -rf / ; echo "';
     await extractBundleIdFromAppPath(maliciousPath, executor);
 
-    const cmdString = calls[0].command[2];
-
-    // The rm -rf command is embedded in the shell string unescaped
-    expect(cmdString).toContain('rm -rf');
+    const call = calls[0];
+    expect(call.command[0]).toBe('defaults');
+    expect(call.command[2]).toBe(`${maliciousPath}/Info`);
+    expect(call.command).not.toContain('/bin/sh');
   });
 
-  it('UNFIXED: backtick injection in appPath', async () => {
+  it('isolates backtick-injection attempts as a single argv element', async () => {
     const calls: CapturedCall[] = [];
     const executor = createCapturingExecutor(calls);
 
     const maliciousPath = '/tmp/`touch /tmp/pwned`';
     await extractBundleIdFromAppPath(maliciousPath, executor);
 
-    const cmdString = calls[0].command[2];
-    expect(cmdString).toContain('`touch /tmp/pwned`');
+    const call = calls[0];
+    expect(call.command[0]).toBe('defaults');
+    expect(call.command[2]).toBe(`${maliciousPath}/Info`);
+    expect(call.command).not.toContain('/bin/sh');
+  });
+
+  it('falls back to PlistBuddy without invoking a shell', async () => {
+    const calls: CapturedCall[] = [];
+    const failingExecutor: CommandExecutor = async (command, logPrefix, useShell) => {
+      calls.push({ command: [...command], logPrefix, useShell });
+      if (command[0] === 'defaults') {
+        return { success: false, output: '', error: 'defaults read failed', process: stubProcess };
+      }
+      return { success: true, output: 'com.example.app', process: stubProcess };
+    };
+
+    const maliciousPath = '/tmp/evil" $(id) "bar';
+    const result = await extractBundleIdFromAppPath(maliciousPath, failingExecutor);
+
+    expect(result).toBe('com.example.app');
+    expect(calls).toHaveLength(2);
+
+    const fallback = calls[1];
+    expect(fallback.useShell).toBe(false);
+    expect(fallback.command).toEqual([
+      '/usr/libexec/PlistBuddy',
+      '-c',
+      'Print :CFBundleIdentifier',
+      `${maliciousPath}/Info.plist`,
+    ]);
+    expect(fallback.command).not.toContain('/bin/sh');
   });
 
   it('safe appPath without metacharacters works normally', async () => {
@@ -95,6 +112,11 @@ describe('bundle-id.ts — CWE-78 shell injection vectors', () => {
 
     expect(result).toBe('com.example.app');
     expect(calls).toHaveLength(1);
-    expect(calls[0].command[2]).toContain(safePath);
+    expect(calls[0].command).toEqual([
+      'defaults',
+      'read',
+      `${safePath}/Info`,
+      'CFBundleIdentifier',
+    ]);
   });
 });