diff --git a/src/__tests__/main/stores/instances.test.ts b/src/__tests__/main/stores/instances.test.ts index f5cc79cf..212d0e15 100644 --- a/src/__tests__/main/stores/instances.test.ts +++ b/src/__tests__/main/stores/instances.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +// Use vi.hoisted to create mock state that's accessible in both mock factory and tests +// This ensures proper scoping and avoids potential test interference +const { mockStoreConstructorCalls } = vi.hoisted(() => ({ + mockStoreConstructorCalls: [] as Array>, +})); + // Mock electron vi.mock('electron', () => ({ app: { @@ -7,10 +13,7 @@ vi.mock('electron', () => ({ }, })); -// Track mock store constructor calls -const mockStoreConstructorCalls: Array> = []; - -// Mock electron-store with a class +// Mock electron-store with a class that tracks constructor calls vi.mock('electron-store', () => { return { default: class MockStore { diff --git a/src/__tests__/main/stores/types.test.ts b/src/__tests__/main/stores/types.test.ts index dd9e8544..fdd257a0 100644 --- a/src/__tests__/main/stores/types.test.ts +++ b/src/__tests__/main/stores/types.test.ts @@ -11,6 +11,7 @@ import type { ClaudeSessionOriginInfo, ClaudeSessionOriginsData, AgentSessionOriginsData, + StoredSession, } from '../../../main/stores/types'; /** @@ -80,21 +81,73 @@ describe('stores/types', () => { }); }); + describe('StoredSession', () => { + it('should have required fields', () => { + const session: StoredSession = { + id: '1', + name: 'Test Session', + toolType: 'claude-code', + cwd: '/path/to/project', + projectRoot: '/path/to/project', + }; + expect(session.id).toBe('1'); + expect(session.toolType).toBe('claude-code'); + }); + + it('should allow optional groupId', () => { + const session: StoredSession = { + id: '1', + groupId: 'group-1', + name: 'Test Session', + toolType: 'claude-code', + cwd: '/path/to/project', + projectRoot: '/path/to/project', + }; + expect(session.groupId).toBe('group-1'); + }); + + it('should allow additional renderer-specific fields', () => { + const session: StoredSession = { + id: '1', + name: 'Test Session', + toolType: 'claude-code', + cwd: '/path/to/project', + projectRoot: '/path/to/project', + // Additional fields from renderer Session type + state: 'idle', + aiLogs: [], + inputMode: 'ai', + }; + expect(session.state).toBe('idle'); + expect(session.aiLogs).toEqual([]); + }); + }); + describe('SessionsData', () => { - it('should have sessions array', () => { + it('should have sessions array with StoredSession items', () => { const data: SessionsData = { - sessions: [{ id: '1', name: 'Test' }], + sessions: [ + { + id: '1', + name: 'Test', + toolType: 'claude-code', + cwd: '/path', + projectRoot: '/path', + }, + ], }; expect(data.sessions).toHaveLength(1); + expect(data.sessions[0].id).toBe('1'); }); }); describe('GroupsData', () => { - it('should have groups array', () => { + it('should have groups array with Group items', () => { const data: GroupsData = { - groups: [{ id: '1', name: 'Group 1' }], + groups: [{ id: '1', name: 'Group 1', emoji: '📁', collapsed: false }], }; expect(data.groups).toHaveLength(1); + expect(data.groups[0].emoji).toBe('📁'); }); }); diff --git a/src/__tests__/main/stores/utils.test.ts b/src/__tests__/main/stores/utils.test.ts index eb5924c6..55e49782 100644 --- a/src/__tests__/main/stores/utils.test.ts +++ b/src/__tests__/main/stores/utils.test.ts @@ -82,7 +82,7 @@ describe('stores/utils', () => { }); it('should return undefined when directory creation fails', () => { - const customPath = '/invalid/path'; + const customPath = '/Users/test/invalid/path'; const mockStore = { get: vi.fn().mockReturnValue(customPath), } as unknown as Store; @@ -102,6 +102,96 @@ describe('stores/utils', () => { `Failed to create custom sync path: ${customPath}, using default` ); }); + + it('should reject relative paths', () => { + const relativePath = 'relative/path/to/data'; + const mockStore = { + get: vi.fn().mockReturnValue(relativePath), + } as unknown as Store; + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = getCustomSyncPath(mockStore); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith(`Custom sync path must be absolute: ${relativePath}`); + }); + + it('should reject paths with traversal sequences', () => { + const traversalPath = '/Users/test/../../../etc/passwd'; + const mockStore = { + get: vi.fn().mockReturnValue(traversalPath), + } as unknown as Store; + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = getCustomSyncPath(mockStore); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith( + `Custom sync path contains traversal sequences: ${traversalPath}` + ); + }); + + it('should allow paths with ".." in directory names (not traversal)', () => { + const validPath = '/Users/test/my..project/data'; + const mockStore = { + get: vi.fn().mockReturnValue(validPath), + } as unknown as Store; + + vi.mocked(fsSync.existsSync).mockReturnValue(true); + + const result = getCustomSyncPath(mockStore); + + // Should be allowed - ".." is part of directory name, not a traversal + expect(result).toBe(validPath); + }); + + it('should reject paths in sensitive system directories', () => { + const sensitivePath = '/etc/maestro'; + const mockStore = { + get: vi.fn().mockReturnValue(sensitivePath), + } as unknown as Store; + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = getCustomSyncPath(mockStore); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith( + `Custom sync path cannot be in sensitive system directory: ${sensitivePath}` + ); + }); + + it('should reject paths that are too short', () => { + const shortPath = '/a'; + const mockStore = { + get: vi.fn().mockReturnValue(shortPath), + } as unknown as Store; + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = getCustomSyncPath(mockStore); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith(`Custom sync path is too short: ${shortPath}`); + }); + + it('should reject paths containing null bytes', () => { + const nullBytePath = '/Users/test/data\0/maestro'; + const mockStore = { + get: vi.fn().mockReturnValue(nullBytePath), + } as unknown as Store; + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = getCustomSyncPath(mockStore); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith( + `Custom sync path contains null bytes: ${nullBytePath}` + ); + }); }); describe('getEarlySettings', () => { diff --git a/src/main/stores/types.ts b/src/main/stores/types.ts index a4eb3a2f..e3921ab2 100644 --- a/src/main/stores/types.ts +++ b/src/main/stores/types.ts @@ -5,7 +5,30 @@ * These types are used across the main process for type-safe store access. */ -import type { SshRemoteConfig } from '../../shared/types'; +import type { SshRemoteConfig, Group } from '../../shared/types'; + +// ============================================================================ +// Stored Session Type (minimal interface for main process storage) +// ============================================================================ + +/** + * Minimal session interface for main process storage. + * The full Session type is defined in renderer/types/index.ts and has 60+ fields. + * This interface captures the required fields that the main process needs to understand, + * while allowing additional properties via index signature for forward compatibility. + * + * Note: We use `any` for the index signature instead of `unknown` to maintain + * backward compatibility with existing code that accesses dynamic session properties. + */ +export interface StoredSession { + id: string; + groupId?: string; + name: string; + toolType: string; + cwd: string; + projectRoot: string; + [key: string]: any; // Allow additional renderer-specific fields +} // ============================================================================ // Bootstrap Store (local-only, determines sync path) @@ -49,7 +72,7 @@ export interface MaestroSettings { // ============================================================================ export interface SessionsData { - sessions: any[]; + sessions: StoredSession[]; } // ============================================================================ @@ -57,7 +80,7 @@ export interface SessionsData { // ============================================================================ export interface GroupsData { - groups: any[]; + groups: Group[]; } // ============================================================================ diff --git a/src/main/stores/utils.ts b/src/main/stores/utils.ts index be75ce56..03fafd93 100644 --- a/src/main/stores/utils.ts +++ b/src/main/stores/utils.ts @@ -7,6 +7,8 @@ * - SSH remote configuration lookup */ +import path from 'path'; + import Store from 'electron-store'; import fsSync from 'fs'; @@ -16,6 +18,107 @@ import type { SshRemoteConfig } from '../../shared/types'; // Re-export getDefaultShell from defaults for backward compatibility export { getDefaultShell } from './defaults'; +// ============================================================================ +// Path Validation Utilities +// ============================================================================ + +/** + * Validates a custom sync path for security and correctness. + * @returns true if the path is valid, false otherwise + */ +function isValidSyncPath(customPath: string): boolean { + // Path must be absolute + if (!path.isAbsolute(customPath)) { + console.error(`Custom sync path must be absolute: ${customPath}`); + return false; + } + + // Check for null bytes (security issue on Unix systems) + if (customPath.includes('\0')) { + console.error(`Custom sync path contains null bytes: ${customPath}`); + return false; + } + + // Check for path traversal BEFORE normalization + // Split by separator and check for literal ".." segments + const segments = customPath.split(/[/\\]/); + if (segments.includes('..')) { + console.error(`Custom sync path contains traversal sequences: ${customPath}`); + return false; + } + + // Normalize the path to resolve any . segments and redundant separators + const normalizedPath = path.normalize(customPath); + + // Reject paths that are too short (likely system directories) + // Minimum reasonable path: /a/b (5 chars on Unix) or C:\a (4 chars on Windows) + const minPathLength = process.platform === 'win32' ? 4 : 5; + if (normalizedPath.length < minPathLength) { + console.error(`Custom sync path is too short: ${customPath}`); + return false; + } + + // Check for Windows reserved names (CON, PRN, AUX, NUL, COM1-9, LPT1-9) + if (process.platform === 'win32') { + const reservedNames = /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i; + const pathSegments = normalizedPath.split(/[/\\]/); + for (const segment of pathSegments) { + // Check the base name without extension + const baseName = segment.split('.')[0]; + if (reservedNames.test(baseName)) { + console.error(`Custom sync path contains Windows reserved name: ${customPath}`); + return false; + } + } + } + + // Reject known sensitive system directories + // For Windows, check common sensitive paths across all drive letters + const sensitiveRoots = + process.platform === 'win32' + ? [ + '\\Windows', + '\\Program Files', + '\\Program Files (x86)', + '\\System', + '\\System32', + '\\SysWOW64', + ] + : ['/bin', '/sbin', '/usr/bin', '/usr/sbin', '/etc', '/var', '/tmp', '/dev', '/proc', '/sys']; + + const lowerPath = normalizedPath.toLowerCase(); + + if (process.platform === 'win32') { + // For Windows, check if any sensitive root appears after the drive letter + // e.g., C:\Windows, D:\Windows, etc. + for (const sensitive of sensitiveRoots) { + const sensitiveLower = sensitive.toLowerCase(); + // Match pattern like "X:\Windows" or "X:\Windows\..." + const drivePattern = /^[a-z]:/i; + if (drivePattern.test(lowerPath)) { + const pathWithoutDrive = lowerPath.slice(2); // Remove "C:" prefix + if ( + pathWithoutDrive === sensitiveLower || + pathWithoutDrive.startsWith(sensitiveLower + '\\') + ) { + console.error(`Custom sync path cannot be in sensitive system directory: ${customPath}`); + return false; + } + } + } + } else { + // Unix path checking + for (const sensitive of sensitiveRoots) { + if (lowerPath === sensitive || lowerPath.startsWith(sensitive + '/')) { + console.error(`Custom sync path cannot be in sensitive system directory: ${customPath}`); + return false; + } + } + } + + return true; +} + // ============================================================================ // Sync Path Utilities // ============================================================================ @@ -23,12 +126,17 @@ export { getDefaultShell } from './defaults'; /** * Get the custom sync path from the bootstrap store. * Creates the directory if it doesn't exist. - * Returns undefined if no custom path is configured or if creation fails. + * Returns undefined if no custom path is configured, validation fails, or creation fails. */ export function getCustomSyncPath(bootstrapStore: Store): string | undefined { const customPath = bootstrapStore.get('customSyncPath'); if (customPath) { + // Validate the path before using it + if (!isValidSyncPath(customPath)) { + return undefined; + } + // Ensure the directory exists if (!fsSync.existsSync(customPath)) { try {