Comfy-Org / Comfy-Org/ComfyUI_frontend

Full asset URLs reach Datadog telemetry via raw console.error and reportError messages

Open
#17,433 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.