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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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', {
|
||||
|
||||
@@ -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<string, () => 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<string, () => 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
|
||||
|
||||
Reference in New Issue
Block a user