diff --git a/src/__tests__/main/ipc/handlers/notifications.test.ts b/src/__tests__/main/ipc/handlers/notifications.test.ts index d5eca76c..a0c62367 100644 --- a/src/__tests__/main/ipc/handlers/notifications.test.ts +++ b/src/__tests__/main/ipc/handlers/notifications.test.ts @@ -1,6 +1,11 @@ /** * Tests for notification IPC handlers * + * IMPORTANT: Custom notification commands have NO WHITELIST and NO VALIDATION. + * Users have full control to specify ANY command, ANY path, ANY arguments. + * This is by design - the feature supports arbitrary shell pipelines for + * maximum flexibility (e.g., fabric | 11s, tee ~/log.txt | say, etc.) + * * Note: Notification command tests are simplified due to the complexity of mocking * child_process spawn with all the event listeners and stdin handling. */ @@ -83,11 +88,11 @@ vi.mock('child_process', async (importOriginal) => { import { registerNotificationsHandlers, - resetTtsState, - getTtsQueueLength, - getActiveTtsCount, - clearTtsQueue, - getTtsMaxQueueSize, + resetNotificationState, + getNotificationQueueLength, + getActiveNotificationCount, + clearNotificationQueue, + getNotificationMaxQueueSize, parseNotificationCommand, } from '../../../../main/ipc/handlers/notifications'; @@ -96,7 +101,7 @@ describe('Notification IPC Handlers', () => { beforeEach(() => { vi.clearAllMocks(); - resetTtsState(); + resetNotificationState(); handlers = new Map(); // Reset mocks @@ -113,7 +118,7 @@ describe('Notification IPC Handlers', () => { afterEach(() => { vi.clearAllMocks(); - resetTtsState(); + resetNotificationState(); }); describe('handler registration', () => { @@ -193,31 +198,44 @@ describe('Notification IPC Handlers', () => { describe('notification state utilities', () => { it('should track notification queue length', () => { - expect(getTtsQueueLength()).toBe(0); + expect(getNotificationQueueLength()).toBe(0); }); it('should track active notification count', () => { - expect(getActiveTtsCount()).toBe(0); + expect(getActiveNotificationCount()).toBe(0); }); it('should clear notification queue', () => { - clearTtsQueue(); - expect(getTtsQueueLength()).toBe(0); + clearNotificationQueue(); + expect(getNotificationQueueLength()).toBe(0); }); it('should reset notification state', () => { - resetTtsState(); - expect(getTtsQueueLength()).toBe(0); - expect(getActiveTtsCount()).toBe(0); + resetNotificationState(); + expect(getNotificationQueueLength()).toBe(0); + expect(getActiveNotificationCount()).toBe(0); }); it('should return max queue size', () => { - expect(getTtsMaxQueueSize()).toBe(10); + expect(getNotificationMaxQueueSize()).toBe(10); }); }); - describe('notification command parsing', () => { - it('should return default command when none provided', () => { + /** + * Custom notification command parsing tests + * + * CRITICAL: These tests verify that there is NO WHITELIST and NO VALIDATION + * on custom notification commands. Users have FULL CONTROL to specify: + * - ANY executable path (absolute or relative) + * - ANY binary name + * - ANY arguments and flags + * - ANY shell pipeline (pipes, redirects, etc.) + * + * This design allows maximum flexibility for users to integrate with + * any tooling they prefer (TTS engines, AI summarizers, logging, etc.) + */ + describe('custom notification command parsing - NO WHITELIST, ANY COMMAND ALLOWED', () => { + it('should return default command (say) when none provided', () => { const result = parseNotificationCommand(); expect(result).toBe('say'); }); @@ -232,55 +250,79 @@ describe('Notification IPC Handlers', () => { expect(result).toBe('say'); }); - it('should accept any command - user has full control', () => { - const result = parseNotificationCommand('say'); - expect(result).toBe('say'); + // Explicit NO WHITELIST tests - any command should be passed through unchanged + it('should NOT validate or whitelist commands - any binary name is allowed', () => { + // These would have been blocked by a whitelist - verify they pass through + expect(parseNotificationCommand('my-custom-binary')).toBe('my-custom-binary'); + expect(parseNotificationCommand('totally-unknown-command')).toBe('totally-unknown-command'); + expect(parseNotificationCommand('arbitrary_executable')).toBe('arbitrary_executable'); }); - it('should accept custom commands with full paths', () => { - const result = parseNotificationCommand('/usr/local/bin/my-tts'); - expect(result).toBe('/usr/local/bin/my-tts'); - }); - - it('should accept commands with arguments', () => { - const result = parseNotificationCommand('say -v Alex'); - expect(result).toBe('say -v Alex'); - }); - - it('should accept command chains with pipes', () => { - const result = parseNotificationCommand('tee ~/log.txt | say'); - expect(result).toBe('tee ~/log.txt | say'); - }); - - it('should accept fabric pattern commands', () => { - const result = parseNotificationCommand( - '/Users/pedram/go/bin/fabric --pattern ped_summarize_conversational --model gpt-5-mini --raw 2>/dev/null | /Users/pedram/.local/bin/11s --voice NFQv27BRKPFgprCm0xgr' + it('should NOT validate or whitelist paths - any absolute path is allowed', () => { + expect(parseNotificationCommand('/usr/local/bin/my-custom-tool')).toBe( + '/usr/local/bin/my-custom-tool' ); - expect(result).toBe( - '/Users/pedram/go/bin/fabric --pattern ped_summarize_conversational --model gpt-5-mini --raw 2>/dev/null | /Users/pedram/.local/bin/11s --voice NFQv27BRKPFgprCm0xgr' + expect(parseNotificationCommand('/Users/pedram/go/bin/fabric')).toBe( + '/Users/pedram/go/bin/fabric' + ); + expect(parseNotificationCommand('/opt/homebrew/bin/anything')).toBe( + '/opt/homebrew/bin/anything' + ); + expect(parseNotificationCommand('/some/deeply/nested/path/to/binary')).toBe( + '/some/deeply/nested/path/to/binary' ); }); - it('should trim leading and trailing whitespace', () => { - const result = parseNotificationCommand(' say '); - expect(result).toBe('say'); + it('should NOT validate or whitelist arguments - any arguments are allowed', () => { + expect(parseNotificationCommand('say -v Alex')).toBe('say -v Alex'); + expect(parseNotificationCommand('cmd --flag1 --flag2=value -x -y -z')).toBe( + 'cmd --flag1 --flag2=value -x -y -z' + ); + expect(parseNotificationCommand('binary arg1 arg2 arg3 "quoted arg"')).toBe( + 'binary arg1 arg2 arg3 "quoted arg"' + ); }); - it('should accept espeak command', () => { - const result = parseNotificationCommand('espeak'); - expect(result).toBe('espeak'); + it('should allow shell pipelines with any commands', () => { + expect(parseNotificationCommand('tee ~/log.txt | say')).toBe('tee ~/log.txt | say'); + expect(parseNotificationCommand('cmd1 | cmd2 | cmd3')).toBe('cmd1 | cmd2 | cmd3'); }); - it('should accept festival command with flags', () => { - const result = parseNotificationCommand('festival --tts'); - expect(result).toBe('festival --tts'); + it('should allow complex command chains with redirects and pipes', () => { + const complexCommand = + '/Users/pedram/go/bin/fabric --pattern ped_summarize_conversational --model gpt-5-mini --raw 2>/dev/null | /Users/pedram/.local/bin/11s --voice NFQv27BRKPFgprCm0xgr'; + expect(parseNotificationCommand(complexCommand)).toBe(complexCommand); + }); + + it('should trim leading and trailing whitespace only', () => { + expect(parseNotificationCommand(' say ')).toBe('say'); + expect(parseNotificationCommand('\t/path/to/cmd\n')).toBe('/path/to/cmd'); + }); + + // Common TTS commands work, but are NOT special-cased or whitelisted + it('should accept common TTS commands (not because whitelisted, but because any command works)', () => { + expect(parseNotificationCommand('say')).toBe('say'); + expect(parseNotificationCommand('espeak')).toBe('espeak'); + expect(parseNotificationCommand('espeak-ng')).toBe('espeak-ng'); + expect(parseNotificationCommand('festival --tts')).toBe('festival --tts'); + expect(parseNotificationCommand('flite')).toBe('flite'); + expect(parseNotificationCommand('spd-say')).toBe('spd-say'); + }); + + // Non-TTS commands are equally valid + it('should accept non-TTS commands for logging, processing, or other purposes', () => { + expect(parseNotificationCommand('tee ~/notifications.log')).toBe('tee ~/notifications.log'); + expect(parseNotificationCommand('cat >> ~/log.txt')).toBe('cat >> ~/log.txt'); + expect(parseNotificationCommand('curl -X POST https://webhook.example.com')).toBe( + 'curl -X POST https://webhook.example.com' + ); }); }); describe('notification queue size limit', () => { it('should reject requests when queue is full', async () => { const handler = handlers.get('notification:speak')!; - const maxSize = getTtsMaxQueueSize(); + const maxSize = getNotificationMaxQueueSize(); // The flow is: // 1. First call: item added to queue, processNextNotification() shifts it out to process @@ -305,7 +347,7 @@ describe('Notification IPC Handlers', () => { await new Promise((resolve) => setTimeout(resolve, 10)); // Verify queue is at capacity - expect(getTtsQueueLength()).toBe(maxSize); + expect(getNotificationQueueLength()).toBe(maxSize); // Now try to add one more - should be rejected immediately // This will resolve immediately with error because queue >= maxSize check triggers @@ -316,7 +358,7 @@ describe('Notification IPC Handlers', () => { expect(result.error).toContain(`max ${maxSize}`); // Clean up - reset all notification state including clearing the queue - resetTtsState(); + resetNotificationState(); }); }); }); diff --git a/src/__tests__/main/preload/notifications.test.ts b/src/__tests__/main/preload/notifications.test.ts index 5457ef0c..20b76b69 100644 --- a/src/__tests__/main/preload/notifications.test.ts +++ b/src/__tests__/main/preload/notifications.test.ts @@ -1,5 +1,8 @@ /** * Tests for notifications preload API + * + * IMPORTANT: Custom notification commands have NO WHITELIST and NO VALIDATION. + * Users have full control to specify ANY command, ANY path, ANY arguments. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -47,8 +50,14 @@ describe('Notification Preload API', () => { }); }); - describe('speak', () => { - it('should invoke notification:speak with text', async () => { + /** + * Custom notification command tests - NO WHITELIST, ANY COMMAND ALLOWED + * + * The speak() method executes a custom command with text piped to stdin. + * There is NO validation or whitelist - users can specify ANY command. + */ + describe('speak - custom notification command (NO WHITELIST)', () => { + it('should invoke notification:speak with text and default command', async () => { mockInvoke.mockResolvedValue({ success: true, ttsId: 123 }); const result = await api.speak('Hello world'); @@ -57,16 +66,17 @@ describe('Notification Preload API', () => { expect(result).toEqual({ success: true, ttsId: 123 }); }); - it('should invoke notification:speak with custom command', async () => { + it('should accept ANY command - no whitelist restriction', async () => { mockInvoke.mockResolvedValue({ success: true, ttsId: 456 }); - const result = await api.speak('Hello', 'espeak'); + // Any command works - not just whitelisted TTS tools + const result = await api.speak('Hello', 'my-custom-tool'); - expect(mockInvoke).toHaveBeenCalledWith('notification:speak', 'Hello', 'espeak'); + expect(mockInvoke).toHaveBeenCalledWith('notification:speak', 'Hello', 'my-custom-tool'); expect(result.ttsId).toBe(456); }); - it('should accept complex command chains with pipes', async () => { + it('should accept complex command chains with pipes (shell pipeline)', async () => { mockInvoke.mockResolvedValue({ success: true, ttsId: 789 }); const complexCommand = 'tee ~/log.txt | say'; @@ -76,9 +86,10 @@ describe('Notification Preload API', () => { expect(result.ttsId).toBe(789); }); - it('should accept commands with full paths and arguments', async () => { + it('should accept ANY absolute path with ANY arguments', async () => { mockInvoke.mockResolvedValue({ success: true, ttsId: 111 }); + // Full paths to custom binaries are allowed const fullPathCommand = '/Users/pedram/go/bin/fabric --pattern ped_summarize_conversational --model gpt-5-mini'; const result = await api.speak('Test', fullPathCommand); @@ -86,6 +97,17 @@ describe('Notification Preload API', () => { expect(mockInvoke).toHaveBeenCalledWith('notification:speak', 'Test', fullPathCommand); expect(result.success).toBe(true); }); + + it('should accept non-TTS commands like curl, tee, cat, etc.', async () => { + mockInvoke.mockResolvedValue({ success: true, ttsId: 222 }); + + // Non-TTS commands are equally valid + const curlCommand = 'curl -X POST -d @- https://webhook.example.com'; + const result = await api.speak('notification payload', curlCommand); + + expect(mockInvoke).toHaveBeenCalledWith('notification:speak', 'notification payload', curlCommand); + expect(result.success).toBe(true); + }); }); describe('stopSpeak', () => { @@ -99,7 +121,7 @@ describe('Notification Preload API', () => { }); }); - describe('onTtsCompleted', () => { + describe('onCommandCompleted (legacy: onTtsCompleted)', () => { it('should register event listener and return cleanup function', () => { const callback = vi.fn(); @@ -109,7 +131,7 @@ describe('Notification Preload API', () => { expect(typeof cleanup).toBe('function'); }); - it('should call callback when event is received', () => { + it('should call callback when notification command completes', () => { const callback = vi.fn(); let registeredHandler: (event: unknown, notificationId: number) => void; diff --git a/src/main/preload/notifications.ts b/src/main/preload/notifications.ts index b7567108..c3228b39 100644 --- a/src/main/preload/notifications.ts +++ b/src/main/preload/notifications.ts @@ -22,7 +22,9 @@ export interface NotificationShowResponse { */ export interface NotificationCommandResponse { success: boolean; - ttsId?: number; // Legacy name kept for backward compatibility + notificationId?: number; + /** @deprecated Use notificationId instead */ + ttsId?: number; error?: string; } @@ -59,6 +61,14 @@ export function createNotificationApi() { * @param handler - Callback when a notification command completes * @returns Cleanup function to unsubscribe */ + onCommandCompleted: (handler: (notificationId: number) => void): (() => void) => { + const wrappedHandler = (_event: Electron.IpcRendererEvent, notificationId: number) => + handler(notificationId); + ipcRenderer.on('notification:commandCompleted', wrappedHandler); + return () => ipcRenderer.removeListener('notification:commandCompleted', wrappedHandler); + }, + + /** @deprecated Use onCommandCompleted instead */ onTtsCompleted: (handler: (notificationId: number) => void): (() => void) => { const wrappedHandler = (_event: Electron.IpcRendererEvent, notificationId: number) => handler(notificationId); diff --git a/src/renderer/global.d.ts b/src/renderer/global.d.ts index 718bec16..67e67282 100644 --- a/src/renderer/global.d.ts +++ b/src/renderer/global.d.ts @@ -1238,9 +1238,11 @@ interface MaestroAPI { speak: ( text: string, command?: string - ) => Promise<{ success: boolean; ttsId?: number; error?: string }>; - stopSpeak: (ttsId: number) => Promise<{ success: boolean; error?: string }>; - onTtsCompleted: (handler: (ttsId: number) => void) => () => void; + ) => Promise<{ success: boolean; notificationId?: number; ttsId?: number; error?: string }>; + stopSpeak: (notificationId: number) => Promise<{ success: boolean; error?: string }>; + onCommandCompleted: (handler: (notificationId: number) => void) => () => void; + /** @deprecated Use onCommandCompleted instead */ + onTtsCompleted: (handler: (notificationId: number) => void) => () => void; }; attachments: { save: (