From aae96913dfdae30ba53df27a66dea9b84236edd8 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 18 Aug 2026 13:35:34 -0700 Subject: [PATCH] fix(desktop): register plugin notify handlers only after guards pass; re-resolve activate at the IPC boundary Two hardening follow-ups on the salvaged #84192 work: - dispatchNativeNotification now reports whether the notification actually reached the OS bridge, and dispatchPluginNativeNotification registers its onActivate/onAction closures only on true. Previously a throttled, disabled, or baseline-suppressed notification registered handlers that no click could ever clear, leaking them for the window's lifetime. - The renderer's onNotificationActivate handler re-resolves the activate payload through resolveHermesOpenPath instead of trusting the pre-IPC validation, keeping path validation in one funnel for any future hermesDesktop.notify caller. Adds a regression test covering the throttled and suppressed cases. --- .../contrib/hooks/use-desktop-integrations.ts | 11 ++++- .../src/store/native-notifications.test.ts | 24 ++++++++++ .../desktop/src/store/native-notifications.ts | 44 +++++++++++-------- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts index b5043ca209..ea26871f02 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts @@ -3,7 +3,7 @@ import { useEffect, useRef } from 'react' import { closeActiveTab } from '@/app/chat/close-tab' import { commandFocusedPreview } from '@/app/chat/right-rail/preview-nav' import { openSession } from '@/app/open-session' -import { pathFromHermesDeepLink } from '@/lib/hermes-open-target' +import { pathFromHermesDeepLink, resolveHermesOpenPath } from '@/lib/hermes-open-target' import { storedSessionIdForNotification } from '@/lib/session-ids' import { requestMcpInstallFromDeepLink } from '@/store/mcp-deeplink-install' import { startMcpHealthChecker, stopMcpHealthChecker } from '@/store/mcp-health' @@ -216,7 +216,14 @@ export function useDesktopIntegrations({ } if (payload.activate) { - navigate(payload.activate) + // Defense-in-depth: re-resolve at the IPC boundary rather than trusting + // the pre-IPC validation — any future hermesDesktop.notify caller gets + // funneled through the same resolver. + const path = resolveHermesOpenPath(payload.activate) + + if (path) { + navigate(path) + } } clearPluginNotifyHandlers(payload.notifyId) diff --git a/apps/desktop/src/store/native-notifications.test.ts b/apps/desktop/src/store/native-notifications.test.ts index 987ca56b88..c1fd51d30f 100644 --- a/apps/desktop/src/store/native-notifications.test.ts +++ b/apps/desktop/src/store/native-notifications.test.ts @@ -203,6 +203,30 @@ describe('dispatchPluginNativeNotification', () => { expect(notify).toHaveBeenCalledTimes(2) }) + it('does not register handlers for throttled or suppressed notifications', () => { + const onActivate = vi.fn() + + // First fires and registers; the immediate repeat is throttled per plugin id. + dispatchPluginNativeNotification('leak-plugin', { onActivate: () => undefined, title: 'first' }) + dispatchPluginNativeNotification('leak-plugin', { onActivate, title: 'throttled' }) + expect(notify).toHaveBeenCalledTimes(1) + + // The throttled call must not have registered anything: no notifyId ever + // reached the OS, so its handlers would leak. Invoking with the only + // minted id (from the first call) must not hit the throttled callback. + const payload = notify.mock.calls[0]?.[0] as { notifyId?: string } + invokePluginNotifyActivate(payload.notifyId) + expect(onActivate).not.toHaveBeenCalled() + + // Fully suppressed (kind disabled): nothing registered either. + setNativeNotifyKind('plugin', false) + const suppressed = vi.fn() + dispatchPluginNativeNotification('other-plugin', { onActivate: suppressed, title: 'muted' }) + expect(notify).toHaveBeenCalledTimes(1) + invokePluginNotifyActivate(payload.notifyId) + expect(suppressed).not.toHaveBeenCalled() + }) + it('forwards icon, resolved activate path, and action buttons (deeplink-compatible)', () => { // Unique tag (throttle is per plugin id); activate still uses the plugin deep link. dispatchPluginNativeNotification('index-network-alerts', { diff --git a/apps/desktop/src/store/native-notifications.ts b/apps/desktop/src/store/native-notifications.ts index 2834f2fa0f..3e7e1c8ce7 100644 --- a/apps/desktop/src/store/native-notifications.ts +++ b/apps/desktop/src/store/native-notifications.ts @@ -184,23 +184,26 @@ export interface NativeNotificationInput { notifyId?: string } -export function dispatchNativeNotification(input: NativeNotificationInput): void { +/** Returns true when the notification passed every guard and was handed to the + * OS bridge — callers registering per-notification state (plugin handlers) + * must only do so on true, or suppressed/throttled notifications leak it. */ +export function dispatchNativeNotification(input: NativeNotificationInput): boolean { const prefs = $nativeNotifyPrefs.get() if (!prefs.enabled || !prefs.kinds[input.kind]) { - return + return false } if (withinNativeNotifyBaseline()) { - return + return false } if (!shouldFire(input.kind, input.sessionId, input.global)) { - return + return false } if (throttled(`${input.kind}:${input.sessionId ?? input.tag ?? (input.global ? 'global' : '')}`, Date.now())) { - return + return false } void window.hermesDesktop?.notify({ @@ -215,6 +218,8 @@ export function dispatchNativeNotification(input: NativeNotificationInput): void tag: input.tag, title: input.title }) + + return true } // -- the plugin door (`ctx.os.notify`) ---------------------------------------- @@ -303,25 +308,13 @@ export function dispatchPluginNativeNotification(pluginId: string, input: Plugin const activate = resolveHermesOpenPath(input.activate) ?? undefined const notifyId = input.onActivate || input.actions?.some(a => a.onAction) ? mintNotifyId(pluginId) : undefined - if (notifyId) { - const actions = new Map void>() - - for (const action of input.actions ?? []) { - if (action.onAction) { - actions.set(action.id, action.onAction) - } - } - - pendingPluginNotify.set(notifyId, { actions, onActivate: input.onActivate }) - } - const actions: NativeNotificationAction[] | undefined = input.actions?.map(action => ({ activate: resolveHermesOpenPath(action.activate) ?? undefined, id: action.id, text: action.label })) - dispatchNativeNotification({ + const fired = dispatchNativeNotification({ actions, activate, body: input.body, @@ -333,6 +326,21 @@ export function dispatchPluginNativeNotification(pluginId: string, input: Plugin tag: pluginId, title: input.title }) + + // Register renderer callbacks only for notifications that actually reached + // the OS — a throttled/suppressed one can never be clicked, so registering + // first would leak the closures for the window's lifetime. + if (fired && notifyId) { + const handlers = new Map void>() + + for (const action of input.actions ?? []) { + if (action.onAction) { + handlers.set(action.id, action.onAction) + } + } + + pendingPluginNotify.set(notifyId, { actions: handlers, onActivate: input.onActivate }) + } } // Resolve a pending approval from a notification button, mirroring the in-app