fix(web): bound and scrub resource error telemetry - #7501
Conversation
lefarcen
added
size/L
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@Siri-Ray Thanks for tackling both the privacy exposure and the telemetry flood in one focused change. The overall classification/windowing direction is sound, but this pass found two cases that still violate the PR's core privacy and per-resource counting guarantees. I've left concrete fixes inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| }); | ||
| crossorigin_mode: normalizeCrossorigin(target.getAttribute('crossorigin')), | ||
| }; | ||
| const key = `${tag}\0${resource.normalized_resource}`; |
There was a problem hiding this comment.
Blocking — keep the private resource identity separate from the scrubbed telemetry value. This key uses normalized_resource, but normalization intentionally collapses every same-tag project PDF on the same route to one value such as /api/projects/:project_id/raw/:file.pdf. Consequently, errors for two distinct PDFs share one window: the second PDF loses its immediate first event, and a successful load of either PDF closes the other's repeat window. Alternating a failing resource with loads of another colliding resource can also create fresh immediate events repeatedly, undermining the volume bound. This conflicts with the PR's stated goal of distinguishing affected resources from retries of one resource. Please derive a local-only identity from the actual resource (for example, a canonical origin/path value that is never copied into event properties), use it consistently in both the error and load lookups, and add a regression test with two private PDF URLs that normalize identically to prove they get independent first/count/load behavior.
| const type = resourceType(tag); | ||
| if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { | ||
| return privateResource(parsed.protocol.replace(':', '') || 'opaque', tag); | ||
| } | ||
| const extension = resourceExtension(parsed.pathname); | ||
|
|
||
| if (parsed.origin !== window.location.origin) { | ||
| return { | ||
| category: 'external_media', | ||
| normalized_resource: `external:${type}${extension ? `.${extension}` : ''}`, | ||
| origin_relation: 'cross_origin', | ||
| resource_extension: extension, | ||
| resource_route: 'external', | ||
| resource_type: type, | ||
| }; | ||
| } | ||
|
|
||
| if (parsed.pathname.startsWith('/_next/static/chunks/')) { | ||
| const chunkId = safeNextChunkId(parsed.pathname); | ||
| return { | ||
| category: 'next_chunk', | ||
| normalized_resource: `next:${chunkId}`, | ||
| origin_relation: 'same_origin', | ||
| resource_extension: extension, | ||
| resource_route: '/_next/static/chunks/:chunk', | ||
| resource_type: type, | ||
| }; | ||
| } | ||
|
|
||
| const productRoute = productAssetRoute(parsed.pathname); | ||
| if (productRoute) { | ||
| return { | ||
| category: 'product_asset', | ||
| normalized_resource: parsed.pathname, |
There was a problem hiding this comment.
Blocking — these branches still serialize user-controlled URL text. The non-HTTP branch copies an arbitrary custom scheme into resource_route/normalized_resource, and the product-asset branch copies the full parsed.pathname. Prefix classification does not prove the remainder is a shipped asset: a failed same-origin URL such as /fonts/Customer%20Contract.pdf is classified as product_asset and sends that filename verbatim; similarly, customer-project-123:payload sends customer-project-123. That directly breaks this PR's guarantee that filenames and project identifiers are absent from serialized telemetry. Please make opaque schemes fixed labels and make product assets emit only the route template/type/allowlisted extension (or validate against an exact owned manifest); retain a chunk identifier only under a genuinely restrictive bundler-generated format. Add adversarial fixtures for a private filename beneath a product-looking prefix and a custom scheme, asserting the serialized payload contains neither input.
Visual regression reviewHead: 0 changed · 53 unchanged · 0 new without baseline · 0 failed Unchanged cases
Visual diff is advisory only and does not block merging. |
3 participants
Why
Production stability reporting on 2026-08-25 UTC showed one device emitting 16,839
client_resource_errorevents from a project PDF path. The existing observer sent each failed DOM resource's full URL through the direct safety transport, so project IDs, user filenames, query strings, and external media locations could reach PostHog. The same window still contained real_next/static/chunksfailures across roughly 60–70 devices, so dropping or globally sampling the event would erase an important cross-device reliability signal.This PR makes resource-error telemetry privacy-safe and volume-bounded while preserving an immediate per-device signal for real Next.js chunk failures. It also makes repeated counts explicit so stability analysis can distinguish affected devices/resources from one resource retrying thousands of times.
What users will see
There is no visual UI change. Resource failures continue to produce safety diagnostics, but user and third-party resource URLs are reduced to low-sensitivity classifications and route templates. A repeatedly failing image, PDF, iframe, or media element no longer floods reliability telemetry.
Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Not applicable: this changes the browser safety-telemetry pipeline and has no UI entry point.
Bug fix verification
apps/web/tests/observability/resource-error.test.tsmain:$current_url, with no safe resource classification;pagehidedid not flush the pending repeat count.next_chunk,product_asset,user_artifact, andexternal_mediapayloads contain only their allowed fields;repeat_summaryafter 60 seconds, successful load,pagehide, teardown, or bounded-map eviction;Validation
pnpm exec vitest run -c vitest.config.ts --maxWorkers=2 tests/observability/resource-error.test.ts tests/analytics/error-tracking.test.ts tests/analytics-scrub.test.ts tests/observability/iframe-error.test.ts— 50 passedpnpm --filter @open-design/web typecheck— passedpnpm guard— passedgit diff --check— passedpnpm --filter @open-design/web testwas started but not counted as validation: it was manually interrupted after about nine minutes without a terminal summary while emitting existing jsdom canvas/navigation warnings. The focused web/analytics suites above completed cleanly.