diff --git a/CHANGELOG.md b/CHANGELOG.md index 9194236bd..536c3018e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Fixed +- Fixed malformed simulator discovery responses, project discovery path-boundary checks, and compiler diagnostic filenames containing glob metacharacters ([#424](https://github.com/getsentry/XcodeBuildMCP/issues/424)). - Fixed `suppressWarnings` being ignored in settled build, build-run, and test output. The flag was honored only while streaming, so warnings still reached the final MCP tool response ([#447](https://github.com/getsentry/XcodeBuildMCP/issues/447)). ## [2.6.2] diff --git a/src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts b/src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts index f15af6cea..f164cfa4a 100644 --- a/src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts +++ b/src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts @@ -130,6 +130,96 @@ describe('discover_projs plugin', () => { expect(result.workspaces).toEqual([]); expect(readdirCallCount).toBe(3); }); + + it('defaults prefix-matching sibling scan paths to the workspace root', async () => { + const scannedPaths: string[] = []; + const mockFileSystemExecutor = createMockFileSystemExecutor({ + stat: async (filePath) => { + scannedPaths.push(filePath); + return { isDirectory: () => true, mtimeMs: 0 }; + }, + readdir: async (directoryPath) => { + scannedPaths.push(directoryPath); + return []; + }, + }); + + await discoverProjects( + { workspaceRoot: '/workspace/app', scanPath: '../application' }, + mockFileSystemExecutor, + ); + + expect(scannedPaths).toEqual(['/workspace/app', '/workspace/app']); + }); + + it('skips recursive entries in prefix-matching sibling directories', async () => { + const mockFileSystemExecutor = createMockFileSystemExecutor({ + stat: async () => ({ isDirectory: () => true, mtimeMs: 0 }), + readdir: async () => [ + { + name: '../application/Outside.xcodeproj', + isDirectory: () => true, + isSymbolicLink: () => false, + }, + ], + }); + + const result = await discoverProjects( + { workspaceRoot: '/workspace/app' }, + mockFileSystemExecutor, + ); + + expect(result.projects).toEqual([]); + }); + + it('uses the containing directory as the recursive boundary for bundle workspace roots', async () => { + const mockFileSystemExecutor = createMockFileSystemExecutor({ + stat: async () => ({ isDirectory: () => true, mtimeMs: 0 }), + readdir: async () => [ + { name: 'App.xcodeproj', isDirectory: () => true, isSymbolicLink: () => false }, + ], + }); + + const result = await discoverProjects( + { workspaceRoot: '/workspace/App.xcodeproj' }, + mockFileSystemExecutor, + ); + + expect(result.projects).toEqual(['/workspace/App.xcodeproj']); + }); + + it('uses the containing directory for relative bundle workspace roots', async () => { + const scannedPaths: string[] = []; + const mockFileSystemExecutor = createMockFileSystemExecutor({ + stat: async (filePath) => { + scannedPaths.push(filePath); + return { isDirectory: () => true, mtimeMs: 0 }; + }, + readdir: async () => [], + }); + + await discoverProjects({ workspaceRoot: 'App.xcodeproj' }, mockFileSystemExecutor); + + expect(scannedPaths).toEqual([process.cwd()]); + }); + + it('resolves an explicit dot scan path from a relative bundle workspace root', async () => { + const scannedPaths: string[] = []; + const mockFileSystemExecutor = createMockFileSystemExecutor({ + stat: async (filePath) => { + scannedPaths.push(filePath); + return { isDirectory: () => true, mtimeMs: 0 }; + }, + readdir: async () => [], + }); + + await discoverProjects( + { workspaceRoot: 'App.xcodeproj', scanPath: '.' }, + mockFileSystemExecutor, + ); + + expect(scannedPaths).toEqual([process.cwd()]); + }); }); describe('Logic error handling', () => { diff --git a/src/mcp/tools/project-discovery/discover_projs.ts b/src/mcp/tools/project-discovery/discover_projs.ts index 1f5cf4273..862feb8f0 100644 --- a/src/mcp/tools/project-discovery/discover_projs.ts +++ b/src/mcp/tools/project-discovery/discover_projs.ts @@ -25,6 +25,16 @@ interface DirentLike { isSymbolicLink(): boolean; } +function isPathWithin(rootPath: string, candidatePath: string): boolean { + const relativePath = path.relative(path.resolve(rootPath), path.resolve(candidatePath)); + return ( + relativePath === '' || + (relativePath !== '..' && + !relativePath.startsWith(`..${path.sep}`) && + !path.isAbsolute(relativePath)) + ); +} + function getErrorDetails( error: unknown, fallbackMessage: string, @@ -62,8 +72,6 @@ async function _findProjectsRecursive( } log('debug', `Scanning directory: ${currentDirAbs} at depth ${currentDepth}`); - const normalizedWorkspaceRoot = path.normalize(workspaceRootAbs); - try { const entries = await fileSystemExecutor.readdir(currentDirAbs, { withFileTypes: true }); for (const rawEntry of entries) { @@ -81,7 +89,7 @@ async function _findProjectsRecursive( continue; } - if (!path.normalize(absoluteEntryPath).startsWith(normalizedWorkspaceRoot)) { + if (!isPathWithin(workspaceRootAbs, absoluteEntryPath)) { log( 'warn', `Skipping entry outside workspace root: ${absoluteEntryPath} (Workspace: ${workspaceRootAbs})`, @@ -166,38 +174,26 @@ function isBundleLikePath(workspaceRoot: string): boolean { ); } -function resolveScanBase(workspaceRoot: string, scanPath?: string): string { - if (scanPath) { - return scanPath; - } - - if (isBundleLikePath(workspaceRoot)) { - return path.dirname(workspaceRoot); - } - - return '.'; -} - async function discoverProjectsOrError( params: DiscoverProjectsParams, fileSystemExecutor: FileSystemExecutor, ): Promise { - const scanPath = resolveScanBase(params.workspaceRoot, params.scanPath); + const scanPath = params.scanPath ?? '.'; const maxDepth = params.maxDepth ?? DEFAULT_MAX_DEPTH; const workspaceRoot = params.workspaceRoot; - const requestedScanPath = path.resolve(workspaceRoot, scanPath); - let absoluteScanPath = requestedScanPath; const workspaceBoundary = isBundleLikePath(workspaceRoot) ? path.dirname(workspaceRoot) : workspaceRoot; - const normalizedWorkspaceRoot = path.normalize(workspaceBoundary); - if (!path.normalize(absoluteScanPath).startsWith(normalizedWorkspaceRoot)) { + const absoluteWorkspaceBoundary = path.resolve(workspaceBoundary); + const requestedScanPath = path.resolve(absoluteWorkspaceBoundary, scanPath); + let absoluteScanPath = requestedScanPath; + if (!isPathWithin(absoluteWorkspaceBoundary, absoluteScanPath)) { log( 'warn', `Requested scan path '${scanPath}' resolved outside workspace root '${workspaceRoot}'. Defaulting scan to workspace root.`, ); - absoluteScanPath = normalizedWorkspaceRoot; + absoluteScanPath = absoluteWorkspaceBoundary; } const context: DiscoverProjectsExecutionContext = { @@ -228,7 +224,7 @@ async function discoverProjectsOrError( const results: DiscoverProjectsResult = { projects: [], workspaces: [] }; await _findProjectsRecursive( absoluteScanPath, - workspaceRoot, + absoluteWorkspaceBoundary, 0, maxDepth, results, diff --git a/src/utils/__tests__/simulator-resolver.test.ts b/src/utils/__tests__/simulator-resolver.test.ts new file mode 100644 index 000000000..c913a18d5 --- /dev/null +++ b/src/utils/__tests__/simulator-resolver.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest'; +import { createMockExecutor } from '../../test-utils/mock-executors.ts'; +import { resolveSimulatorIdToName, resolveSimulatorNameToId } from '../simulator-resolver.ts'; + +describe('simulator resolver', () => { + it('returns a contract error for invalid JSON from simctl', async () => { + const result = await resolveSimulatorNameToId( + createMockExecutor({ output: 'not-json' }), + 'iPhone 17 Pro', + ); + + expect(result).toMatchObject({ + success: false, + error: expect.stringContaining('Failed to parse simulator list:'), + }); + }); + + it('returns a contract error for a malformed devices payload', async () => { + const result = await resolveSimulatorIdToName( + createMockExecutor({ output: JSON.stringify({ devices: { 'iOS 27.0': null } }) }), + 'SIMULATOR-ID', + ); + + expect(result).toEqual({ + success: false, + error: 'Failed to parse simulator list: simctl returned an invalid devices payload.', + }); + }); +}); diff --git a/src/utils/__tests__/simulator-steps.test.ts b/src/utils/__tests__/simulator-steps.test.ts new file mode 100644 index 000000000..eabf34b7b --- /dev/null +++ b/src/utils/__tests__/simulator-steps.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from 'vitest'; +import { createMockExecutor } from '../../test-utils/mock-executors.ts'; +import { findSimulatorById } from '../simulator-steps.ts'; + +describe('simulator steps', () => { + it('returns a contract error for invalid JSON from simctl', async () => { + const result = await findSimulatorById( + 'SIMULATOR-ID', + createMockExecutor({ output: 'not-json' }), + ); + + expect(result).toMatchObject({ + simulator: null, + error: expect.stringContaining('Failed to parse simulator list:'), + }); + }); +}); diff --git a/src/utils/renderers/__tests__/event-formatting.test.ts b/src/utils/renderers/__tests__/event-formatting.test.ts index e179ec3dc..d50e01bb2 100644 --- a/src/utils/renderers/__tests__/event-formatting.test.ts +++ b/src/utils/renderers/__tests__/event-formatting.test.ts @@ -1,3 +1,5 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { @@ -126,6 +128,35 @@ describe('event formatting', () => { ); }); + it('treats glob metacharacters in compiler diagnostic filenames literally', () => { + const projectBaseDir = mkdtempSync(join(tmpdir(), 'xcodebuildmcp-diagnostic-')); + const sourceDir = join(projectBaseDir, 'Sources'); + const decoyDir = join(projectBaseDir, 'Decoy'); + const literalFile = join(sourceDir, 'ContentView[1].swift'); + + try { + mkdirSync(sourceDir, { recursive: true }); + mkdirSync(decoyDir, { recursive: true }); + writeFileSync(literalFile, ''); + writeFileSync(join(decoyDir, 'ContentView1.swift'), ''); + + const rendered = formatHumanCompilerErrorEvent( + { + type: 'compiler-error', + operation: 'BUILD', + message: 'unterminated string literal', + rawLine: 'ContentView[1].swift:16:18: error: unterminated string literal', + }, + { baseDir: projectBaseDir }, + ); + + expect(rendered).toContain(`${literalFile}:16:18`); + expect(rendered).not.toContain('Decoy/ContentView1.swift'); + } finally { + rmSync(projectBaseDir, { recursive: true, force: true }); + } + }); + it('formats tool-originated errors in xcodebuild-style form', () => { expect( formatHumanCompilerErrorEvent({ diff --git a/src/utils/renderers/event-formatting.ts b/src/utils/renderers/event-formatting.ts index 75fcdba85..180a9bf16 100644 --- a/src/utils/renderers/event-formatting.ts +++ b/src/utils/renderers/event-formatting.ts @@ -1,6 +1,6 @@ import { existsSync } from 'node:fs'; import path from 'node:path'; -import { globSync } from 'glob'; +import { escape, globSync } from 'glob'; import type { ArtifactRenderItem, BuildStageRenderItem, @@ -211,7 +211,7 @@ function resolveDiagnosticPathCandidate( return cached ?? filePath; } - const matches = globSync(`**/${filePath}`, { + const matches = globSync(`**/${escape(filePath)}`, { cwd: options.baseDir, nodir: true, ignore: DIAGNOSTIC_PATH_IGNORE_PATTERNS, diff --git a/src/utils/simulator-resolver.ts b/src/utils/simulator-resolver.ts index f0b9b580f..31a045436 100644 --- a/src/utils/simulator-resolver.ts +++ b/src/utils/simulator-resolver.ts @@ -7,6 +7,41 @@ export type SimulatorResolutionResult = type SimulatorDevice = { udid: string; name: string }; +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function isSimulatorDevice(value: unknown): value is SimulatorDevice { + return isRecord(value) && typeof value.udid === 'string' && typeof value.name === 'string'; +} + +function parseSimulatorDevices( + output: string, +): { devices: Record } | { error: string } { + let parsed: unknown; + try { + parsed = JSON.parse(output) as unknown; + } catch (parseError) { + return { error: `Failed to parse simulator list: ${parseError}` }; + } + + if (!isRecord(parsed) || !isRecord(parsed.devices)) { + return { error: 'Failed to parse simulator list: simctl returned an invalid devices payload.' }; + } + + const devices: Record = {}; + for (const [runtime, runtimeDevices] of Object.entries(parsed.devices)) { + if (!Array.isArray(runtimeDevices) || !runtimeDevices.every(isSimulatorDevice)) { + return { + error: 'Failed to parse simulator list: simctl returned an invalid devices payload.', + }; + } + devices[runtime] = runtimeDevices; + } + + return { devices }; +} + async function fetchSimulatorDevices( executor: CommandExecutor, ): Promise<{ devices: Record } | { error: string }> { @@ -20,11 +55,7 @@ async function fetchSimulatorDevices( return { error: `Failed to list simulators: ${result.error}` }; } - try { - return JSON.parse(result.output) as { devices: Record }; - } catch (parseError) { - return { error: `Failed to parse simulator list: ${parseError}` }; - } + return parseSimulatorDevices(result.output); } function findSimulator( diff --git a/src/utils/simulator-steps.ts b/src/utils/simulator-steps.ts index 00a8fac22..14809f544 100644 --- a/src/utils/simulator-steps.ts +++ b/src/utils/simulator-steps.ts @@ -62,9 +62,17 @@ export async function findSimulatorById( return { simulator: null, error: listResult.error ?? 'Failed to list simulators' }; } - const simulatorsData = JSON.parse(listResult.output) as { - devices: Record; - }; + let simulatorsData: { devices: Record }; + try { + simulatorsData = JSON.parse(listResult.output) as { + devices: Record; + }; + } catch (error) { + return { + simulator: null, + error: `Failed to parse simulator list: ${toErrorMessage(error)}`, + }; + } for (const runtime in simulatorsData.devices) { const devices = simulatorsData.devices[runtime];