fix(build-tools): Address PR review findings from Cursor Bugbot and Sentry

- Fix TOCTOU race in create_dmg: validateScriptPath now returns the
  resolved realScriptPath instead of discarding it, eliminating the
  second unguarded realpathSync call
- Fix positional arg misordering in create_dmg: outputPath is only
  passed when appPath is also present, preventing wrong-slot injection
- Fix unused bundleId in codesign_app: removed from .refine() check
  since xcrun notarytool submit does not accept --bundle-id
- Fix validation ordering in pfctl_anchor: rulesFile extension check
  now runs before buildCommand, keeping unvalidated values out of the
  command array
- Fix duplicate --port in serve-mcp.sh: filter --port from passthrough
  args before forwarding to supergateway
- Deduplicate setStructuredOutput into command-result-helpers.ts

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: paul-maas

scripts/serve-mcp.sh

@@ -55,9 +55,24 @@ echo "Workflows:  ${XCODEBUILDMCP_ENABLED_WORKFLOWS}"
 echo "PATH includes: $(which xcodegen 2>/dev/null || echo '(xcodegen not found)'), $(which codesign 2>/dev/null || echo '(codesign not found)')"
 echo ""
 
+# Filter out --port from passthrough args to avoid duplication
+FILTERED_ARGS=()
+skip_next=false
+for arg in "$@"; do
+  if $skip_next; then
+    skip_next=false
+    continue
+  fi
+  if [[ "$arg" == "--port" ]]; then
+    skip_next=true
+    continue
+  fi
+  FILTERED_ARGS+=("$arg")
+done
+
 exec npx supergateway \
   --port "$PORT" \
   --stdio "node ${PROJECT_ROOT}/build/cli.js mcp" \
   --outputTransport streamableHttp \
   --cors \
-  "$@"
+  "${FILTERED_ARGS[@]}"

src/mcp/tools/build-tools/__tests__/create_dmg.test.ts

@@ -52,26 +52,30 @@ describe('create_dmg tool', () => {
 
   describe('validateScriptPath', () => {
     it('should reject absolute scriptPath', () => {
-      const error = _validateScriptPath('/usr/bin/evil', '/project');
-      expect(error).toContain('must be relative');
+      const result = _validateScriptPath('/usr/bin/evil', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('must be relative');
     });
 
     it('should reject path traversal', () => {
-      const error = _validateScriptPath('../evil.sh', '/project');
-      expect(error).toContain('path traversal');
+      const result = _validateScriptPath('../evil.sh', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('path traversal');
     });
 
     it('should reject nested path traversal', () => {
-      const error = _validateScriptPath('Scripts/../../evil.sh', '/project');
-      expect(error).toContain('path traversal');
+      const result = _validateScriptPath('Scripts/../../evil.sh', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('path traversal');
     });
 
     it('should return error when script not found', () => {
       mockedRealpathSync.mockImplementation(() => {
         throw new Error('ENOENT');
       });
-      const error = _validateScriptPath('Scripts/create-dmg.sh', '/project');
-      expect(error).toContain('Script not found');
+      const result = _validateScriptPath('Scripts/create-dmg.sh', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('Script not found');
     });
 
     it('should return error when project path not found', () => {
@@ -80,8 +84,9 @@ describe('create_dmg tool', () => {
         if (pathStr === '/project/Scripts/create-dmg.sh') return pathStr;
         throw new Error('ENOENT');
       });
-      const error = _validateScriptPath('Scripts/create-dmg.sh', '/project');
-      expect(error).toContain('Project path not found');
+      const result = _validateScriptPath('Scripts/create-dmg.sh', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('Project path not found');
     });
 
     it('should reject symlink escape', () => {
@@ -90,18 +95,21 @@ describe('create_dmg tool', () => {
         if (pathStr.includes('create-dmg.sh')) return '/outside/evil.sh';
         return '/project';
       });
-      const error = _validateScriptPath('Scripts/create-dmg.sh', '/project');
-      expect(error).toContain('symlink escape');
+      const result = _validateScriptPath('Scripts/create-dmg.sh', '/project');
+      expect('error' in result).toBe(true);
+      if ('error' in result) expect(result.error).toContain('symlink escape');
     });
 
-    it('should accept valid script within project', () => {
+    it('should accept valid script within project and return realScriptPath', () => {
       mockedRealpathSync.mockImplementation((p: fs.PathLike) => {
         const pathStr = String(p);
         if (pathStr.includes('create-dmg.sh')) return '/project/Scripts/create-dmg.sh';
         return '/project';
       });
-      const error = _validateScriptPath('Scripts/create-dmg.sh', '/project');
-      expect(error).toBeNull();
+      const result = _validateScriptPath('Scripts/create-dmg.sh', '/project');
+      expect('realScriptPath' in result).toBe(true);
+      if ('realScriptPath' in result)
+        expect(result.realScriptPath).toBe('/project/Scripts/create-dmg.sh');
     });
   });
 
@@ -213,6 +221,29 @@ describe('create_dmg tool', () => {
       expect(result.isError()).toBe(true);
     });
 
+    it('should not pass outputPath when appPath is omitted', async () => {
+      mockedRealpathSync.mockImplementation((p: fs.PathLike) => {
+        const pathStr = String(p);
+        if (pathStr.includes('create-dmg.sh')) return '/project/Scripts/create-dmg.sh';
+        return '/project';
+      });
+
+      let capturedCommand: string[] | undefined;
+      const mockExecutor = createMockExecutor({
+        success: true,
+        output: '',
+        onExecute: (cmd) => {
+          capturedCommand = cmd;
+        },
+      });
+
+      await runToolLogic(() =>
+        createDmgLogic({ projectPath: '/project', outputPath: '/dist/App.dmg' }, mockExecutor),
+      );
+
+      expect(capturedCommand).toEqual(['/bin/sh', '/project/Scripts/create-dmg.sh']);
+    });
+
     it('should use default script path when scriptPath not provided', async () => {
       mockedRealpathSync.mockImplementation((p: fs.PathLike) => {
         const pathStr = String(p);

src/mcp/tools/build-tools/codesign_app.ts

@@ -1,10 +1,9 @@
 import * as z from 'zod';
-import type { ToolHandlerContext } from '../../../rendering/types.ts';
 import type { CommandExecutor } from '../../../utils/execution/index.ts';
 import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts';
 import { createTypedTool, getHandlerContext } from '../../../utils/typed-tool-factory.ts';
 import {
-  STRUCTURED_OUTPUT_SCHEMA,
+  setStructuredOutput,
   createCommandSuccess,
   createCommandFailure,
 } from './command-result-helpers.ts';
@@ -32,24 +31,13 @@ const codesignBaseSchema = z.object({
   bundleId: z.string().min(1).optional().describe('Bundle ID (required for notarization)'),
 });
 
-const codesignSchema = codesignBaseSchema.refine(
-  (val) => !val.notarize || (val.teamId != null && val.bundleId != null),
-  { message: 'teamId and bundleId are required when notarize is true', path: ['teamId'] },
-);
+const codesignSchema = codesignBaseSchema.refine((val) => !val.notarize || val.teamId != null, {
+  message: 'teamId is required when notarize is true',
+  path: ['teamId'],
+});
 
 type CodesignParams = z.infer<typeof codesignSchema>;
 
-function setStructuredOutput(
-  ctx: ToolHandlerContext,
-  result: ReturnType<typeof createCommandSuccess>,
-): void {
-  ctx.structuredOutput = {
-    result,
-    schema: STRUCTURED_OUTPUT_SCHEMA,
-    schemaVersion: '1',
-  };
-}
-
 export async function codesignLogic(
   params: CodesignParams,
   executor: CommandExecutor,

src/mcp/tools/build-tools/command-result-helpers.ts

@@ -1,8 +1,20 @@
 import type { CommandResultDomainResult, BasicDiagnostics } from '../../../types/domain-results.ts';
+import type { ToolHandlerContext } from '../../../rendering/types.ts';
 import { createBasicDiagnostics } from '../../../utils/diagnostics.ts';
 
 export const STRUCTURED_OUTPUT_SCHEMA = 'xcodebuildmcp.output.command-result';
 
+export function setStructuredOutput(
+  ctx: ToolHandlerContext,
+  result: CommandResultDomainResult,
+): void {
+  ctx.structuredOutput = {
+    result,
+    schema: STRUCTURED_OUTPUT_SCHEMA,
+    schemaVersion: '1',
+  };
+}
+
 export function createCommandSuccess(
   command: string,
   output: string,

src/mcp/tools/build-tools/create_dmg.ts

@@ -1,12 +1,11 @@
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 import * as z from 'zod';
-import type { ToolHandlerContext } from '../../../rendering/types.ts';
 import type { CommandExecutor } from '../../../utils/execution/index.ts';
 import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts';
 import { createTypedTool, getHandlerContext } from '../../../utils/typed-tool-factory.ts';
 import {
-  STRUCTURED_OUTPUT_SCHEMA,
+  setStructuredOutput,
   createCommandSuccess,
   createCommandFailure,
 } from './command-result-helpers.ts';
@@ -27,23 +26,15 @@ const createDmgSchema = z.object({
 
 type CreateDmgParams = z.infer<typeof createDmgSchema>;
 
-function setStructuredOutput(
-  ctx: ToolHandlerContext,
-  result: ReturnType<typeof createCommandSuccess>,
-): void {
-  ctx.structuredOutput = {
-    result,
-    schema: STRUCTURED_OUTPUT_SCHEMA,
-    schemaVersion: '1',
-  };
-}
-
-function validateScriptPath(resolvedScript: string, projectPath: string): string | null {
+function validateScriptPath(
+  resolvedScript: string,
+  projectPath: string,
+): { error: string } | { realScriptPath: string } {
   if (resolvedScript.startsWith('/')) {
-    return `scriptPath must be relative, not absolute: ${resolvedScript}`;
+    return { error: `scriptPath must be relative, not absolute: ${resolvedScript}` };
   }
   if (resolvedScript.includes('..')) {
-    return `scriptPath must not contain path traversal (..): ${resolvedScript}`;
+    return { error: `scriptPath must not contain path traversal (..): ${resolvedScript}` };
   }
 
   const absoluteScriptPath = path.join(projectPath, resolvedScript);
@@ -52,24 +43,26 @@ function validateScriptPath(resolvedScript: string, projectPath: string): string
   try {
     realScriptPath = fs.realpathSync(absoluteScriptPath);
   } catch {
-    return `Script not found: ${absoluteScriptPath}`;
+    return { error: `Script not found: ${absoluteScriptPath}` };
   }
 
   let realProjectPath: string;
   try {
     realProjectPath = fs.realpathSync(projectPath);
   } catch {
-    return `Project path not found: ${projectPath}`;
+    return { error: `Project path not found: ${projectPath}` };
   }
 
   if (
     realScriptPath !== realProjectPath &&
     !realScriptPath.startsWith(realProjectPath + path.sep)
   ) {
-    return `Script resolves outside project directory (possible symlink escape): ${resolvedScript}`;
+    return {
+      error: `Script resolves outside project directory (possible symlink escape): ${resolvedScript}`,
+    };
   }
 
-  return null;
+  return { realScriptPath };
 }
 
 export async function createDmgLogic(
@@ -80,20 +73,18 @@ export async function createDmgLogic(
   const resolvedScript = params.scriptPath ?? 'Scripts/create-dmg.sh';
   const commandLabel = `create-dmg (${resolvedScript})`;
 
-  const validationError = validateScriptPath(resolvedScript, params.projectPath);
-  if (validationError != null) {
-    const result = createCommandFailure(commandLabel, validationError);
+  const validation = validateScriptPath(resolvedScript, params.projectPath);
+  if ('error' in validation) {
+    const result = createCommandFailure(commandLabel, validation.error);
     setStructuredOutput(ctx, result);
     return;
   }
 
-  const realScriptPath = fs.realpathSync(path.join(params.projectPath, resolvedScript));
-
-  const args: string[] = ['/bin/sh', realScriptPath];
+  const args: string[] = ['/bin/sh', validation.realScriptPath];
   if (params.appPath != null) {
     args.push(params.appPath);
   }
-  if (params.outputPath != null) {
+  if (params.appPath != null && params.outputPath != null) {
     args.push(params.outputPath);
   }
 
@@ -124,6 +115,7 @@ export async function createDmgLogic(
 }
 
 export { validateScriptPath as _validateScriptPath };
+export type ValidateResult = ReturnType<typeof validateScriptPath>;
 
 export const schema = createDmgSchema.shape;
 

src/mcp/tools/build-tools/pfctl_anchor.ts

@@ -1,10 +1,9 @@
 import * as z from 'zod';
-import type { ToolHandlerContext } from '../../../rendering/types.ts';
 import type { CommandExecutor } from '../../../utils/execution/index.ts';
 import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts';
 import { createTypedTool, getHandlerContext } from '../../../utils/typed-tool-factory.ts';
 import {
-  STRUCTURED_OUTPUT_SCHEMA,
+  setStructuredOutput,
   createCommandSuccess,
   createCommandFailure,
 } from './command-result-helpers.ts';
@@ -32,17 +31,6 @@ const pfctlSchema = pfctlBaseSchema.refine(
 
 type PfctlParams = z.infer<typeof pfctlSchema>;
 
-function setStructuredOutput(
-  ctx: ToolHandlerContext,
-  result: ReturnType<typeof createCommandSuccess>,
-): void {
-  ctx.structuredOutput = {
-    result,
-    schema: STRUCTURED_OUTPUT_SCHEMA,
-    schemaVersion: '1',
-  };
-}
-
 function buildCommand(params: PfctlParams): string[] {
   const base = ['sudo', '-n', 'pfctl', '-a', params.anchorName];
 
@@ -58,21 +46,22 @@ function buildCommand(params: PfctlParams): string[] {
 
 export async function pfctlLogic(params: PfctlParams, executor: CommandExecutor): Promise<void> {
   const ctx = getHandlerContext();
-  const command = buildCommand(params);
   const commandLabel = `pfctl -a ${params.anchorName} (${params.action})`;
 
-  try {
-    if (params.action === 'test-syntax' && params.rulesFile != null) {
-      if (!/\.(conf|rules)$/.test(params.rulesFile)) {
-        const result = createCommandFailure(
-          commandLabel,
-          `rulesFile must end with .conf or .rules: ${params.rulesFile}`,
-        );
-        setStructuredOutput(ctx, result);
-        return;
-      }
+  if (params.action === 'test-syntax' && params.rulesFile != null) {
+    if (!/\.(conf|rules)$/.test(params.rulesFile)) {
+      const result = createCommandFailure(
+        commandLabel,
+        `rulesFile must end with .conf or .rules: ${params.rulesFile}`,
+      );
+      setStructuredOutput(ctx, result);
+      return;
     }
+  }
 
+  const command = buildCommand(params);
+
+  try {
     const response = await executor(command, 'PF Anchor Inspection', false);
 
     if (response.success) {

src/mcp/tools/build-tools/xcodegen_generate.ts

@@ -1,10 +1,9 @@
 import * as z from 'zod';
-import type { ToolHandlerContext } from '../../../rendering/types.ts';
 import type { CommandExecutor } from '../../../utils/execution/index.ts';
 import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts';
 import { createTypedTool, getHandlerContext } from '../../../utils/typed-tool-factory.ts';
 import {
-  STRUCTURED_OUTPUT_SCHEMA,
+  setStructuredOutput,
   createCommandSuccess,
   createCommandFailure,
 } from './command-result-helpers.ts';
@@ -16,17 +15,6 @@ const xcodegenSchema = z.object({
 
 type XcodegenParams = z.infer<typeof xcodegenSchema>;
 
-function setStructuredOutput(
-  ctx: ToolHandlerContext,
-  result: ReturnType<typeof createCommandSuccess>,
-): void {
-  ctx.structuredOutput = {
-    result,
-    schema: STRUCTURED_OUTPUT_SCHEMA,
-    schemaVersion: '1',
-  };
-}
-
 export async function xcodegenLogic(
   params: XcodegenParams,
   executor: CommandExecutor,