Comfy-Org / Comfy-Org/ComfyUI_frontend
Full asset URLs reach Datadog telemetry via raw console.error and reportError messages
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Problem / Goal
Split out of #11410, where `fetchAsBlob` leaked signed cloud asset URLs into error messages. That instance is fixed at the source in 11ad123d7a. The same shape exists elsewhere and was out of that PR's scope.
This is not just console hygiene. `datadogRum.init` (`src/platform/telemetry/initDatadogRum.ts`) runs with `sessionSampleRate: 100` on the cloud hostnames, and RUM's `trackConsoleError` subscribes to `console.error` unconditionally, using the joined arguments as the error message. `datadogRumBeforeSend` drops console-sourced events only when the message starts with `REPORTED_ERROR_PREFIX` or `ASSERTION_FAILURE_PREFIX`, and there is a test asserting ordinary console errors are kept. So any raw `console.error` carrying a URL is retained telemetry, not a local devtools line.
`asset.preview_url` is a signed, expiring URL per `@comfyorg/ingest-types`: the `short_url` doc contrasts itself with `preview_url` on exactly that point, and `AssetInfo.preview_url` is documented as "Signed URL for previewing the asset". Same-origin `/api/view?...` URLs are cookie-authenticated and less sensitive, but still carry filename and subfolder.
Sites carrying a full URL into a logged or reported message:
- `src/platform/telemetry/assetLoadErrorReporting.ts` is the strongest one. It both `console.error`s `{ url, tagName }` and builds `new Error(\`Resource load failed: ${url}\`)` for `reportError`, so the URL reaches Datadog by design rather than incidentally.
- `src/composables/maskeditor/useMaskEditorLoader.ts`, `Invalid image URL: ${url}` and `Failed to load image: ${url}`
- `src/composables/maskeditor/useMaskEditorSaver.ts`, `Failed to load image: ${url}` plus a `console.error` with the URL
- `src/renderer/extensions/layerEditor/composables/useLayerEditorSession.ts`, `failed to load image: ${url}` plus a `console.warn` with the URL
Separately, RUM resource events are not filtered by `datadogRumBeforeSend` at all, since `shouldKeepRumEvent` returns early for any non-error event, and `trackResources` defaults to true. A failed fetch therefore ships its full URL including the query string regardless of any console line. Redacting error messages does not close that channel.
## Proposed Solution
Promote the redaction helper #11410 added privately in `src/base/common/downloadUtil.ts` to a shared location instead of copying it. It keeps origin and path via `new URL().origin` plus `pathname`, which drops userinfo and the query string while preserving enough to identify which object failed.
Apply it at each site above, keeping the diagnostic intact: origin, path, and the status or cause. A redaction that collapses these to "load failed" will be reverted by the next person debugging a 403, so that is explicitly not the goal.
For the resource-event channel, decide separately whether `datadogRumBeforeSend` should scrub query strings on resource events for the asset-storage hosts, rather than returning early for all non-error events.
## Acceptance Criteria
- The redaction helper lives somewhere shared and is used by `downloadUtil.ts` and each site listed above.
- No `console.error`, `console.warn`, `reportError` message, or user-facing toast in those files contains a URL query string or userinfo.
- Each redacted message still contains the origin, the path, and the failure status or cause.
- Regression tests assert both directions for at least `assetLoadErrorReporting.ts`: the signing parameters are absent and the cause is retained. Mutation-check them by reinstating the full URL and confirming they go red.
- A decision is recorded on whether RUM resource events get query-string scrubbing, either implemented or explicitly declined with a reason.
Contributor guide
Research direction
Start with src/base/common/downloadUtil.ts and the telemetry path in src/platform/telemetry/assetLoadErrorReporting.ts, then inspect the three mask-editor/layer-editor files named in the issue. Run the existing telemetry tests and add regression coverage for assetLoadErrorReporting.ts. Done means shared redaction is used at every listed site, causes and paths remain visible, sensitive URL data is absent, and the RUM resource-event decision is documented or implemented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- frontend, observability, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100