Merge pull request #219 from pedramamini/code-refactor

fix: address PR feedback for stores module

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<Record<string, unknown>>,
+}));
+
 // Mock electron
 vi.mock('electron', () => ({
 	app: {
@@ -7,10 +13,7 @@ vi.mock('electron', () => ({
 	},
 }));
 
-// Track mock store constructor calls
-const mockStoreConstructorCalls: Array<Record<string, unknown>> = [];
-
-// Mock electron-store with a class
+// Mock electron-store with a class that tracks constructor calls
 vi.mock('electron-store', () => {
 	return {
 		default: class MockStore {

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('📁');
 		});
 	});
 

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<BootstrapSettings>;
@@ -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<BootstrapSettings>;
+
+			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<BootstrapSettings>;
+
+			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<BootstrapSettings>;
+
+			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<BootstrapSettings>;
+
+			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<BootstrapSettings>;
+
+			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<BootstrapSettings>;
+
+			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', () => {

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[];
 }
 
 // ============================================================================

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<BootstrapSettings>): 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 {