Expose ComfyUI telemetry bridge#1069
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a043880e14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const Telemetry = { | ||
| capture: (event: string, properties?: TelemetryProperties): void => { | ||
| ipcRenderer.send('telemetry:capture', { event, properties }) |
There was a problem hiding this comment.
When the hosted ComfyUI code accidentally includes a non-structured-cloneable value in telemetry properties (for example a function, symbol, or DOM object), ipcRenderer.send throws before the main-process asProps filter can drop it. The existing window.api.captureTelemetry wrapper catches this for the renderer, but this new exposed bridge lets a telemetry mistake propagate back into the frontend and break the caller, so the send should be wrapped in a try/catch or the payload should be sanitized in preload first.
Useful? React with 👍 / 👎.
…d-forget `ipcRenderer.send` throws synchronously on non-structured-cloneable payloads (functions, DOM nodes, symbols, circular refs). Since the new `__comfyDesktop2.Telemetry.capture` surface is exposed to the hosted ComfyUI frontend — code we don't fully control — a stray bad payload would otherwise propagate into unrelated frontend code and break it. Mirror the same try/catch contract already used by `window.api.captureTelemetry` in `src/preload/api.ts` (verbatim comment lifted: "telemetry must never break the renderer"). Codex flagged this as P2; agreeing because the contract is established elsewhere in the same repo and the new bridge regressed it.
📝 WalkthroughWalkthroughThis PR extends telemetry array support by introducing separate validation filters for general properties versus person attributes, implementing PII redaction for string array elements, and exposing a new renderer-side ChangesTelemetry Array Support with PII Scrubbing
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/main/lib/ipc/registerTelemetryHandlers.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba60f3b5-b89b-4700-a918-679fae3bbba1
📒 Files selected for processing (4)
src/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/preload/comfyPreload.ts
| 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 |
There was a problem hiding this comment.
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.
3 participants
Summary
Paired change
Note: full pnpm run format:check is pre-existing-broken on this worktree due unrelated repo-wide formatting drift and a parse error in src/renderer/src/components/DetailSection.vue.