Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/main/lib/ipc/registerTelemetryHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,31 @@ function isTelemetryValue(v: unknown): v is mainTelemetry.TelemetryValue {
)
}

// Per-key filter to the TelemetryValue contract; renderer payloads cross a
// trust boundary, so non-primitives (incl. arrays) are dropped per-key.
function asProps(value: unknown): Record<string, mainTelemetry.TelemetryValue> {
function isTelemetryValueArray(v: unknown): v is mainTelemetry.TelemetryValue[] {
return Array.isArray(v) && v.every(isTelemetryValue)
}

// Per-key filter to the TelemetryContext contract.
function asProps(value: unknown): mainTelemetry.TelemetryContext {
if (!value || typeof value !== 'object' || Array.isArray(value)) return {}
const out: Record<string, mainTelemetry.TelemetryValue> = {}
const out: mainTelemetry.TelemetryContext = {}
for (const [key, raw] of Object.entries(value)) {
if (typeof key !== 'string') continue
if (isTelemetryValue(raw)) {
out[key] = raw
} else if (isTelemetryValueArray(raw)) {
out[key] = raw
}
// Drop anything else (objects, arrays, functions, symbols, etc.) silently.
}
return out
Comment on lines +37 to +53

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bound telemetry payload size at the IPC trust boundary.

Line 42 and Line 49 accept renderer-controlled objects/arrays with no size limits, so a huge hosted-frontend payload can synchronously stall main-process validation/scrubbing (a little “freeze-lemtry”); cap key count, array length, and string length before writing into out.

Proposed boundary hardening
+const MAX_TELEMETRY_KEYS = 128
+const MAX_TELEMETRY_ARRAY_ITEMS = 128
+const MAX_TELEMETRY_STRING_LENGTH = 2048
+
+function clampTelemetryValue(v: mainTelemetry.TelemetryValue): mainTelemetry.TelemetryValue {
+  return typeof v === 'string' ? v.slice(0, MAX_TELEMETRY_STRING_LENGTH) : v
+}
+
 function isTelemetryValueArray(v: unknown): v is mainTelemetry.TelemetryValue[] {
-  return Array.isArray(v) && v.every(isTelemetryValue)
+  return (
+    Array.isArray(v) &&
+    v.length <= MAX_TELEMETRY_ARRAY_ITEMS &&
+    v.every(isTelemetryValue)
+  )
 }
 
 // Per-key filter to the TelemetryContext contract.
 function asProps(value: unknown): mainTelemetry.TelemetryContext {
   if (!value || typeof value !== 'object' || Array.isArray(value)) return {}
   const out: mainTelemetry.TelemetryContext = {}
-  for (const [key, raw] of Object.entries(value)) {
+  for (const [key, raw] of Object.entries(value).slice(0, MAX_TELEMETRY_KEYS)) {
     if (typeof key !== 'string') continue
     if (isTelemetryValue(raw)) {
-      out[key] = raw
+      out[key] = clampTelemetryValue(raw)
     } else if (isTelemetryValueArray(raw)) {
-      out[key] = raw
+      out[key] = raw.map(clampTelemetryValue)
     }
   }
   return out
 }
 
 function asPersonProps(value: unknown): Record<string, mainTelemetry.TelemetryValue> {
   if (!value || typeof value !== 'object' || Array.isArray(value)) return {}
   const out: Record<string, mainTelemetry.TelemetryValue> = {}
-  for (const [key, raw] of Object.entries(value)) {
+  for (const [key, raw] of Object.entries(value).slice(0, MAX_TELEMETRY_KEYS)) {
     if (typeof key !== 'string') continue
-    if (isTelemetryValue(raw)) out[key] = raw
+    if (isTelemetryValue(raw)) out[key] = clampTelemetryValue(raw)
   }
   return out
 }

Also applies to: 56-63

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/lib/ipc/registerTelemetryHandlers.ts` around lines 37 - 53, The
asProps/isTelemetryValueArray validation currently accepts unbounded
renderer-controlled objects and arrays; limit work done at the IPC boundary by
enforcing caps: in asProps, stop iterating after a maxKeys threshold (e.g., 50)
and ignore additional keys; in isTelemetryValueArray, cap checked/retained array
length to maxArrayLen (e.g., 20) and only validate/trust up to that many items;
when accepting string TelemetryValue (checked by isTelemetryValue), truncate
strings to a maxStringLen (e.g., 200) before assigning into the returned
mainTelemetry.TelemetryContext; update references to
isTelemetryValue/isTelemetryValueArray and the out assignment logic so oversized
inputs are skipped/truncated rather than fully processed.

}

function asPersonProps(value: unknown): Record<string, mainTelemetry.TelemetryValue> {
if (!value || typeof value !== 'object' || Array.isArray(value)) return {}
const out: Record<string, mainTelemetry.TelemetryValue> = {}
for (const [key, raw] of Object.entries(value)) {
if (typeof key !== 'string') continue
if (isTelemetryValue(raw)) out[key] = raw
}
return out
}
Expand All @@ -65,7 +79,7 @@ export function registerTelemetryHandlers(): void {
})

ipcMain.on('telemetry:registerProperties', (_event, properties: unknown) => {
const props = asProps(properties)
const props = asPersonProps(properties)
if (Object.keys(props).length === 0) return
mainTelemetry.registerPersonProperties(props)
})
Expand All @@ -76,7 +90,7 @@ export function registerTelemetryHandlers(): void {
if (!payload || typeof payload !== 'object') return
const userId = asString((payload as Record<string, unknown>).userId)
if (!userId) return
const properties = asProps((payload as Record<string, unknown>).properties)
const properties = asPersonProps((payload as Record<string, unknown>).properties)
mainTelemetry.bindUserId(userId, properties)
})

Expand Down
12 changes: 12 additions & 0 deletions src/main/lib/telemetry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,18 @@ describe('telemetry SDK-level privacy safety nets', () => {
expect(msg).not.toContain('64911')
})

it('scrubs string array entries as a last-resort safety net', () => {
captured.length = 0
telemetry.capture('comfy.desktop.execution.error', {
model_paths: ['C:\\Users\\64911\\Documents\\model.safetensors', 'LoadImage']
})
expect(captured).toHaveLength(1)
const paths = captured[0]!.properties?.model_paths as string[]
expect(paths[0]).toContain('[REDACTED]')
expect(paths[0]).not.toContain('64911')
expect(paths[1]).toBe('LoadImage')
})

it('leaves non-string property types untouched', () => {
captured.length = 0
telemetry.capture('comfy.desktop.execution.completed', {
Expand Down
16 changes: 11 additions & 5 deletions src/main/lib/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -700,11 +700,17 @@ function scrubProperties(properties: TelemetryContext): TelemetryContext {
let mutated: TelemetryContext | null = null
for (const key of Object.keys(properties)) {
const value = properties[key]
if (typeof value !== 'string') continue
const cleaned = scrubAll(value)
if (cleaned === value) continue
if (!mutated) mutated = { ...properties }
mutated[key] = cleaned
if (typeof value === 'string') {
const cleaned = scrubAll(value)
if (cleaned === value) continue
if (!mutated) mutated = { ...properties }
mutated[key] = cleaned
} else if (Array.isArray(value)) {
const cleaned = value.map((entry) => (typeof entry === 'string' ? scrubAll(entry) : entry))
if (cleaned.every((entry, index) => entry === value[index])) continue
if (!mutated) mutated = { ...properties }
mutated[key] = cleaned
}
}
return mutated ?? properties
}
Expand Down
41 changes: 31 additions & 10 deletions src/preload/comfyPreload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,15 @@ const Terminal = {
* the inline injection doesn't need to know its own ID. */
openPopout: (): Promise<void> => ipcRenderer.invoke('terminal-popout-open', null),
onOutput: (callback: (data: string) => void): (() => void) => {
const handler = (_event: IpcRendererEvent, payload: { data: string }) =>
callback(payload.data)
const handler = (_event: IpcRendererEvent, payload: { data: string }) => callback(payload.data)
ipcRenderer.on('terminal-output', handler)
return () => ipcRenderer.removeListener('terminal-output', handler)
},
onExited: (callback: () => void): (() => void) => {
const handler = () => callback()
ipcRenderer.on('terminal-exited', handler)
return () => ipcRenderer.removeListener('terminal-exited', handler)
},
}
}

export interface LogsRestore {
Expand All @@ -71,6 +70,9 @@ export interface LogsOutputMsg {
text: string
}

type TelemetryPrimitive = string | number | boolean | null
type TelemetryProperties = Record<string, TelemetryPrimitive | TelemetryPrimitive[]>

/**
* Read-only logs bridge. Subscribes to the shared per-install log
* broadcast that mirrors every `comfy-output` IPC send. Used by the
Expand All @@ -90,19 +92,39 @@ const Logs = {
* so the inline injection doesn't need to know its own ID. */
openPopout: (): Promise<void> => ipcRenderer.invoke('logs-popout-open', null),
onOutput: (callback: (msg: LogsOutputMsg) => void): (() => void) => {
const handler = (_event: IpcRendererEvent, payload: LogsOutputMsg) =>
callback(payload)
const handler = (_event: IpcRendererEvent, payload: LogsOutputMsg) => callback(payload)
ipcRenderer.on('logs-output', handler)
return () => ipcRenderer.removeListener('logs-output', handler)
},
}
}

const Telemetry = {
capture: (event: string, properties?: TelemetryProperties): void => {
// Telemetry must never break the caller. `ipcRenderer.send` throws
// synchronously on non-structured-cloneable values (functions, DOM
// nodes, symbols, circular refs); since this surface is exposed to
// the hosted ComfyUI frontend, a stray bad payload would otherwise
// propagate into unrelated frontend code. Mirrors the same
// try/catch contract as `window.api.captureTelemetry` in
// `src/preload/api.ts`.
try {
ipcRenderer.send('telemetry:capture', { event, properties })
} catch {
// ignore: telemetry must never break the renderer
}
}
}

contextBridge.exposeInMainWorld('__comfyDesktop2', {
downloadModel: (url: string, filename: string, directory: string): Promise<boolean> => {
return ipcRenderer.invoke('desktop2-download-model', { url, filename, directory })
},
downloadAsset: (url: string, filename: string, authToken?: string): Promise<boolean> => {
return ipcRenderer.invoke('desktop2-download-asset', { url, filename, authToken: authToken || undefined })
return ipcRenderer.invoke('desktop2-download-asset', {
url,
filename,
authToken: authToken || undefined
})
},
pauseDownload: (url: string): Promise<boolean> => {
return ipcRenderer.invoke('model-download-pause', { url })
Expand All @@ -113,9 +135,7 @@ contextBridge.exposeInMainWorld('__comfyDesktop2', {
cancelDownload: (url: string): Promise<boolean> => {
return ipcRenderer.invoke('model-download-cancel', { url })
},
onDownloadProgress: (
callback: (data: ComfyDownloadProgress) => void
): (() => void) => {
onDownloadProgress: (callback: (data: ComfyDownloadProgress) => void): (() => void) => {
const handler = (_event: IpcRendererEvent, data: unknown) =>
callback(data as ComfyDownloadProgress)
ipcRenderer.on('desktop2-download-progress', handler)
Expand All @@ -126,4 +146,5 @@ contextBridge.exposeInMainWorld('__comfyDesktop2', {
},
Terminal,
Logs,
Telemetry
})
Loading