diff --git a/docs/zh/references/lan-transfer-protocol.md b/docs/zh/references/lan-transfer-protocol.md index 5addd6201..a4c01a23c 100644 --- a/docs/zh/references/lan-transfer-protocol.md +++ b/docs/zh/references/lan-transfer-protocol.md @@ -222,52 +222,6 @@ function sendControlMessage(socket: Socket, message: object): void { {"type":"message_type",...其他字段...}\n ``` -### 4.3 消息发送 - -```typescript -function sendMessage(socket: Socket, message: object): void { - const payload = JSON.stringify(message); - socket.write(`${payload}\n`); -} -``` - -### 4.4 消息接收与解析 - -```typescript -let buffer = ""; - -socket.on("data", (chunk: Buffer) => { - buffer += chunk.toString("utf8"); - - let newlineIndex = buffer.indexOf("\n"); - while (newlineIndex !== -1) { - const line = buffer.slice(0, newlineIndex).trim(); - buffer = buffer.slice(newlineIndex + 1); - - if (line.length > 0) { - const message = JSON.parse(line); - handleMessage(message); - } - - newlineIndex = buffer.indexOf("\n"); - } -}); -``` - -### 4.5 消息类型汇总 - -| 类型 | 方向 | 用途 | -| ---------------- | --------------- | ------------ | -| `handshake` | Client → Server | 握手请求 | -| `handshake_ack` | Server → Client | 握手响应 | -| `ping` | Client → Server | 心跳请求 | -| `pong` | Server → Client | 心跳响应 | -| `file_start` | Client → Server | 开始文件传输 | -| `file_start_ack` | Server → Client | 文件传输确认 | -| `file_chunk` | Client → Server | 文件数据块（流式，无 per-chunk ACK） | -| `file_end` | Client → Server | 文件传输结束 | -| `file_complete` | Server → Client | 传输完成结果 | - --- ## 5. 文件传输协议 @@ -802,7 +756,7 @@ class FileReceiver { ## 附录 A：TypeScript 类型定义 -完整的类型定义位于 `src/types/lanTransfer.ts`： +完整的类型定义位于 `packages/shared/config/types.ts`： ```typescript // 握手消息 diff --git a/packages/shared/config/types.ts b/packages/shared/config/types.ts index 0cc811f6d..56f746b0d 100644 --- a/packages/shared/config/types.ts +++ b/packages/shared/config/types.ts @@ -161,7 +161,7 @@ export const LAN_TRANSFER_MAX_FILE_SIZE = 500 * 1024 * 1024 // 500MB export const LAN_TRANSFER_COMPLETE_TIMEOUT_MS = 60_000 // 60s - wait for file_complete after file_end export const LAN_TRANSFER_GLOBAL_TIMEOUT_MS = 10 * 60 * 1000 // 10 minutes - global transfer timeout -// Binary protocol constants (v3) +// Binary protocol constants (v1) export const LAN_TRANSFER_PROTOCOL_VERSION = '1' export const LAN_BINARY_FRAME_MAGIC = 0x4353 // "CS" as uint16 export const LAN_BINARY_TYPE_FILE_CHUNK = 0x01 @@ -182,7 +182,7 @@ export type LanFileStartMessage = { /** * File chunk data (JSON format) - * @deprecated Use binary frame format in protocol v2. This type is kept for reference only. + * @deprecated Use binary frame format in protocol v1. This type is kept for reference only. */ export type LanFileChunkMessage = { type: 'file_chunk' @@ -217,7 +217,7 @@ export type LanFileStartAckMessage = { /** * Acknowledgment of file chunk received - * @deprecated Protocol v3 uses streaming mode without per-chunk acknowledgment. + * @deprecated Protocol v1 uses streaming mode without per-chunk acknowledgment. * This type is kept for backward compatibility reference only. */ export type LanFileChunkAckMessage = { @@ -235,7 +235,7 @@ export type LanFileCompleteMessage = { success: boolean filePath?: string // Path where file was saved on mobile error?: string - // v3 enhanced error diagnostics + // Enhanced error diagnostics errorCode?: 'CHECKSUM_MISMATCH' | 'INCOMPLETE_TRANSFER' | 'DISK_ERROR' | 'CANCELLED' receivedChunks?: number receivedBytes?: number diff --git a/src/main/services/__tests__/BackupManager.deleteTempBackup.test.ts b/src/main/services/__tests__/BackupManager.deleteTempBackup.test.ts new file mode 100644 index 000000000..631ae0551 --- /dev/null +++ b/src/main/services/__tests__/BackupManager.deleteTempBackup.test.ts @@ -0,0 +1,277 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// Use vi.hoisted to define mocks that are available during hoisting +const { mockLogger } = vi.hoisted(() => ({ + mockLogger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn() + } +})) + +vi.mock('@logger', () => ({ + loggerService: { + withContext: () => mockLogger + } +})) + +vi.mock('electron', () => ({ + app: { + getPath: vi.fn((key: string) => { + if (key === 'temp') return '/tmp' + if (key === 'userData') return '/mock/userData' + return '/mock/unknown' + }) + } +})) + +vi.mock('fs-extra', () => ({ + default: { + pathExists: vi.fn(), + remove: vi.fn(), + ensureDir: vi.fn(), + copy: vi.fn(), + readdir: vi.fn(), + stat: vi.fn(), + readFile: vi.fn(), + writeFile: vi.fn(), + createWriteStream: vi.fn(), + createReadStream: vi.fn() + }, + pathExists: vi.fn(), + remove: vi.fn(), + ensureDir: vi.fn(), + copy: vi.fn(), + readdir: vi.fn(), + stat: vi.fn(), + readFile: vi.fn(), + writeFile: vi.fn(), + createWriteStream: vi.fn(), + createReadStream: vi.fn() +})) + +vi.mock('../WindowService', () => ({ + windowService: { + getMainWindow: vi.fn() + } +})) + +vi.mock('../WebDav', () => ({ + default: vi.fn() +})) + +vi.mock('../S3Storage', () => ({ + default: vi.fn() +})) + +vi.mock('../../utils', () => ({ + getDataPath: vi.fn(() => '/mock/data') +})) + +vi.mock('archiver', () => ({ + default: vi.fn() +})) + +vi.mock('node-stream-zip', () => ({ + default: vi.fn() +})) + +// Import after mocks +import * as fs from 'fs-extra' + +import BackupManager from '../BackupManager' + +describe('BackupManager.deleteTempBackup - Security Tests', () => { + let backupManager: BackupManager + + beforeEach(() => { + vi.clearAllMocks() + backupManager = new BackupManager() + }) + + describe('Normal Operations', () => { + it('should delete valid file in allowed directory', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const validPath = '/tmp/cherry-studio/lan-transfer/backup.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, validPath) + + expect(result).toBe(true) + expect(fs.remove).toHaveBeenCalledWith(validPath) + expect(mockLogger.info).toHaveBeenCalledWith(expect.stringContaining('Deleted temp backup')) + }) + + it('should delete file in nested subdirectory', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const nestedPath = '/tmp/cherry-studio/lan-transfer/sub/dir/file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, nestedPath) + + expect(result).toBe(true) + expect(fs.remove).toHaveBeenCalledWith(nestedPath) + }) + + it('should return false when file does not exist', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(false as never) + + const missingPath = '/tmp/cherry-studio/lan-transfer/missing.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, missingPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + }) + + describe('Path Traversal Attacks', () => { + it('should block basic directory traversal attack (../../../../etc/passwd)', async () => { + const attackPath = '/tmp/cherry-studio/lan-transfer/../../../../etc/passwd' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.pathExists).not.toHaveBeenCalled() + expect(fs.remove).not.toHaveBeenCalled() + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('outside temp directory')) + }) + + it('should block absolute path escape (/etc/passwd)', async () => { + const attackPath = '/etc/passwd' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + expect(mockLogger.warn).toHaveBeenCalled() + }) + + it('should block traversal with multiple slashes', async () => { + const attackPath = '/tmp/cherry-studio/lan-transfer/../../../etc/passwd' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + + it('should block relative path traversal from current directory', async () => { + const attackPath = '../../../etc/passwd' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + + it('should block traversal to parent directory', async () => { + const attackPath = '/tmp/cherry-studio/lan-transfer/../backup/secret.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + }) + + describe('Prefix Attacks', () => { + it('should block similar prefix attack (lan-transfer-evil)', async () => { + const attackPath = '/tmp/cherry-studio/lan-transfer-evil/file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + expect(mockLogger.warn).toHaveBeenCalled() + }) + + it('should block path without separator (lan-transferx)', async () => { + const attackPath = '/tmp/cherry-studio/lan-transferx' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + + it('should block different temp directory prefix', async () => { + const attackPath = '/tmp-evil/cherry-studio/lan-transfer/file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, attackPath) + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + }) + + describe('Error Handling', () => { + it('should return false and log error on permission denied', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockRejectedValue(new Error('EACCES: permission denied') as never) + + const validPath = '/tmp/cherry-studio/lan-transfer/file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, validPath) + + expect(result).toBe(false) + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to delete'), + expect.any(Error) + ) + }) + + it('should return false on fs.pathExists error', async () => { + vi.mocked(fs.pathExists).mockRejectedValue(new Error('ENOENT') as never) + + const validPath = '/tmp/cherry-studio/lan-transfer/file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, validPath) + + expect(result).toBe(false) + expect(mockLogger.error).toHaveBeenCalled() + }) + + it('should handle empty path string', async () => { + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, '') + + expect(result).toBe(false) + expect(fs.remove).not.toHaveBeenCalled() + }) + }) + + describe('Edge Cases', () => { + it('should allow deletion of the temp directory itself', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const tempDir = '/tmp/cherry-studio/lan-transfer' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, tempDir) + + expect(result).toBe(true) + expect(fs.remove).toHaveBeenCalledWith(tempDir) + }) + + it('should handle path with trailing slash', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const pathWithSlash = '/tmp/cherry-studio/lan-transfer/sub/' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, pathWithSlash) + + // path.normalize removes trailing slash + expect(result).toBe(true) + }) + + it('should handle file with special characters in name', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const specialPath = '/tmp/cherry-studio/lan-transfer/file with spaces & (special).zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, specialPath) + + expect(result).toBe(true) + expect(fs.remove).toHaveBeenCalled() + }) + + it('should handle path with double slashes', async () => { + vi.mocked(fs.pathExists).mockResolvedValue(true as never) + vi.mocked(fs.remove).mockResolvedValue(undefined as never) + + const doubleSlashPath = '/tmp/cherry-studio//lan-transfer//file.zip' + const result = await backupManager.deleteTempBackup({} as Electron.IpcMainInvokeEvent, doubleSlashPath) + + // path.normalize handles double slashes + expect(result).toBe(true) + }) + }) +}) diff --git a/src/main/services/__tests__/LocalTransferService.test.ts b/src/main/services/__tests__/LocalTransferService.test.ts new file mode 100644 index 000000000..84f876a60 --- /dev/null +++ b/src/main/services/__tests__/LocalTransferService.test.ts @@ -0,0 +1,481 @@ +import { EventEmitter } from 'events' +import { afterEach, beforeEach, describe, expect, it, type Mock,vi } from 'vitest' + +// Create mock objects before vi.mock calls +const mockLogger = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn() +} + +let mockMainWindow: { + isDestroyed: Mock + webContents: { send: Mock } +} | null = null + +let mockBrowser: EventEmitter & { + start: Mock + stop: Mock + removeAllListeners: Mock +} + +let mockBonjour: { + find: Mock + destroy: Mock +} + +// Mock dependencies before importing the service +vi.mock('@logger', () => ({ + loggerService: { + withContext: () => mockLogger + } +})) + +vi.mock('../WindowService', () => ({ + windowService: { + getMainWindow: vi.fn(() => mockMainWindow) + } +})) + +vi.mock('bonjour-service', () => ({ + default: vi.fn(() => mockBonjour) +})) + +describe('LocalTransferService', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + + // Reset mock objects + mockMainWindow = { + isDestroyed: vi.fn(() => false), + webContents: { send: vi.fn() } + } + + mockBrowser = Object.assign(new EventEmitter(), { + start: vi.fn(), + stop: vi.fn(), + removeAllListeners: vi.fn() + }) + + mockBonjour = { + find: vi.fn(() => mockBrowser), + destroy: vi.fn() + } + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + describe('startDiscovery', () => { + it('should set isScanning to true and start browser', async () => { + const { localTransferService } = await import('../LocalTransferService') + + const state = localTransferService.startDiscovery() + + expect(state.isScanning).toBe(true) + expect(state.lastScanStartedAt).toBeDefined() + expect(mockBonjour.find).toHaveBeenCalledWith({ type: 'cherrystudio', protocol: 'tcp' }) + expect(mockBrowser.start).toHaveBeenCalled() + }) + + it('should clear services when resetList is true', async () => { + const { localTransferService } = await import('../LocalTransferService') + + // First, start discovery and add a service + localTransferService.startDiscovery() + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + fqdn: 'test.local' + }) + + expect(localTransferService.getState().services).toHaveLength(1) + + // Now restart with resetList + const state = localTransferService.startDiscovery({ resetList: true }) + + expect(state.services).toHaveLength(0) + }) + + it('should broadcast state after starting discovery', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + expect(mockMainWindow?.webContents.send).toHaveBeenCalled() + }) + + it('should handle browser.start() error', async () => { + mockBrowser.start.mockImplementation(() => { + throw new Error('Failed to start mDNS') + }) + + const { localTransferService } = await import('../LocalTransferService') + + const state = localTransferService.startDiscovery() + + expect(state.lastError).toBe('Failed to start mDNS') + expect(mockLogger.error).toHaveBeenCalled() + }) + }) + + describe('stopDiscovery', () => { + it('should set isScanning to false and stop browser', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + const state = localTransferService.stopDiscovery() + + expect(state.isScanning).toBe(false) + expect(mockBrowser.stop).toHaveBeenCalled() + }) + + it('should handle browser.stop() error gracefully', async () => { + mockBrowser.stop.mockImplementation(() => { + throw new Error('Stop failed') + }) + + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + // Should not throw + expect(() => localTransferService.stopDiscovery()).not.toThrow() + expect(mockLogger.warn).toHaveBeenCalled() + }) + + it('should broadcast state after stopping', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + vi.clearAllMocks() + + localTransferService.stopDiscovery() + + expect(mockMainWindow?.webContents.send).toHaveBeenCalled() + }) + }) + + describe('browser events', () => { + it('should add service on "up" event', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + fqdn: 'test.local', + type: 'cherrystudio', + protocol: 'tcp' + }) + + const state = localTransferService.getState() + expect(state.services).toHaveLength(1) + expect(state.services[0].name).toBe('Test Service') + expect(state.services[0].port).toBe(12345) + expect(state.services[0].addresses).toContain('192.168.1.100') + }) + + it('should remove service on "down" event', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + // Add service + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + fqdn: 'test.local' + }) + + expect(localTransferService.getState().services).toHaveLength(1) + + // Remove service + mockBrowser.emit('down', { + name: 'Test Service', + host: 'localhost', + port: 12345, + fqdn: 'test.local' + }) + + expect(localTransferService.getState().services).toHaveLength(0) + expect(mockLogger.info).toHaveBeenCalledWith(expect.stringContaining('removed')) + }) + + it('should set lastError on "error" event', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('error', new Error('Discovery failed')) + + const state = localTransferService.getState() + expect(state.lastError).toBe('Discovery failed') + expect(mockLogger.error).toHaveBeenCalled() + }) + + it('should handle non-Error objects in error event', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('error', 'String error message') + + const state = localTransferService.getState() + expect(state.lastError).toBe('String error message') + }) + }) + + describe('getState', () => { + it('should return sorted services by name', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Zebra Service', + host: 'host1', + port: 1001, + addresses: ['192.168.1.1'] + }) + + mockBrowser.emit('up', { + name: 'Alpha Service', + host: 'host2', + port: 1002, + addresses: ['192.168.1.2'] + }) + + const state = localTransferService.getState() + expect(state.services[0].name).toBe('Alpha Service') + expect(state.services[1].name).toBe('Zebra Service') + }) + + it('should include all state properties', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + const state = localTransferService.getState() + + expect(state).toHaveProperty('services') + expect(state).toHaveProperty('isScanning') + expect(state).toHaveProperty('lastScanStartedAt') + expect(state).toHaveProperty('lastUpdatedAt') + }) + }) + + describe('getPeerById', () => { + it('should return peer when exists', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + fqdn: 'test.local' + }) + + const services = localTransferService.getState().services + const peer = localTransferService.getPeerById(services[0].id) + + expect(peer).toBeDefined() + expect(peer?.name).toBe('Test Service') + }) + + it('should return undefined when peer does not exist', async () => { + const { localTransferService } = await import('../LocalTransferService') + + const peer = localTransferService.getPeerById('non-existent-id') + + expect(peer).toBeUndefined() + }) + }) + + describe('normalizeService', () => { + it('should deduplicate addresses', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100', '192.168.1.100', '10.0.0.1'], + referer: { address: '192.168.1.100' } + }) + + const services = localTransferService.getState().services + expect(services[0].addresses).toHaveLength(2) + expect(services[0].addresses).toContain('192.168.1.100') + expect(services[0].addresses).toContain('10.0.0.1') + }) + + it('should filter empty addresses', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100', '', null as any] + }) + + const services = localTransferService.getState().services + expect(services[0].addresses).toEqual(['192.168.1.100']) + }) + + it('should convert txt null/undefined values to empty strings', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + txt: { + version: '1.0', + nullValue: null, + undefinedValue: undefined, + numberValue: 42 + } + }) + + const services = localTransferService.getState().services + expect(services[0].txt).toEqual({ + version: '1.0', + nullValue: '', + undefinedValue: '', + numberValue: '42' + }) + }) + + it('should not include txt when empty', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'], + txt: {} + }) + + const services = localTransferService.getState().services + expect(services[0].txt).toBeUndefined() + }) + }) + + describe('dispose', () => { + it('should clean up all resources', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + mockBrowser.emit('up', { + name: 'Test Service', + host: 'localhost', + port: 12345, + addresses: ['192.168.1.100'] + }) + + localTransferService.dispose() + + expect(localTransferService.getState().services).toHaveLength(0) + expect(localTransferService.getState().isScanning).toBe(false) + expect(mockBrowser.removeAllListeners).toHaveBeenCalled() + expect(mockBonjour.destroy).toHaveBeenCalled() + }) + + it('should handle bonjour.destroy() error gracefully', async () => { + mockBonjour.destroy.mockImplementation(() => { + throw new Error('Destroy failed') + }) + + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + // Should not throw + expect(() => localTransferService.dispose()).not.toThrow() + expect(mockLogger.warn).toHaveBeenCalled() + }) + + it('should be safe to call multiple times', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + + expect(() => { + localTransferService.dispose() + localTransferService.dispose() + }).not.toThrow() + }) + }) + + describe('broadcastState', () => { + it('should not throw when main window is null', async () => { + mockMainWindow = null + + const { localTransferService } = await import('../LocalTransferService') + + // Should not throw + expect(() => localTransferService.startDiscovery()).not.toThrow() + }) + + it('should not throw when main window is destroyed', async () => { + mockMainWindow = { + isDestroyed: vi.fn(() => true), + webContents: { send: vi.fn() } + } + + const { localTransferService } = await import('../LocalTransferService') + + // Should not throw + expect(() => localTransferService.startDiscovery()).not.toThrow() + expect(mockMainWindow.webContents.send).not.toHaveBeenCalled() + }) + }) + + describe('restartBrowser', () => { + it('should destroy old bonjour instance to prevent socket leaks', async () => { + const { localTransferService } = await import('../LocalTransferService') + + // First start + localTransferService.startDiscovery() + expect(mockBonjour.destroy).not.toHaveBeenCalled() + + // Restart - should destroy old instance + localTransferService.startDiscovery() + expect(mockBonjour.destroy).toHaveBeenCalled() + }) + + it('should remove all listeners from old browser', async () => { + const { localTransferService } = await import('../LocalTransferService') + + localTransferService.startDiscovery() + localTransferService.startDiscovery() + + expect(mockBrowser.removeAllListeners).toHaveBeenCalled() + }) + }) +}) diff --git a/src/main/services/lanTransfer/LanTransferClientService.ts b/src/main/services/lanTransfer/LanTransferClientService.ts index 68b6a82b9..a6da2f1a2 100644 --- a/src/main/services/lanTransfer/LanTransferClientService.ts +++ b/src/main/services/lanTransfer/LanTransferClientService.ts @@ -331,11 +331,10 @@ class LanTransferClientService { logger.info('Connection lost, attempting to reconnect...') this.reconnectPromise = this.connectAndHandshake(this.lastConnectOptions) .then(() => { - this.reconnectPromise = null + // Handshake succeeded, connection restored }) - .catch((error) => { + .finally(() => { this.reconnectPromise = null - throw error }) await this.reconnectPromise @@ -406,7 +405,14 @@ class LanTransferClientService { private attachSocketListeners(socket: Socket): void { this.dataHandler = createDataHandler((line) => this.handleControlLine(line)) - socket.on('data', (chunk: Buffer) => this.dataHandler?.handleData(chunk)) + socket.on('data', (chunk: Buffer) => { + try { + this.dataHandler?.handleData(chunk) + } catch (error) { + logger.error('Data handler error', error as Error) + void this.disconnect() + } + }) } private handleControlLine(line: string): void { @@ -419,14 +425,14 @@ class LanTransferClientService { logger.warn('Received invalid JSON control message', { line, consecutiveErrors: this.consecutiveJsonErrors }) if (this.consecutiveJsonErrors >= LanTransferClientService.MAX_CONSECUTIVE_JSON_ERRORS) { - const message = `Protocol error: ${this.consecutiveJsonErrors} consecutive invalid messages` + const message = `Protocol error: ${this.consecutiveJsonErrors} consecutive invalid messages, disconnecting` logger.error(message) this.broadcastClientEvent({ type: 'error', message, timestamp: Date.now() }) - this.consecutiveJsonErrors = 0 + void this.disconnect() } return }