T3: user attachments on reports

[krassx]

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

Mobile parity: Android Report.addAttachment, iOS BugseeAttachment.

• Public ReportAttachmentInput {name, data, mimeType?, fileName?}; writable from ReportHandler.before (addReportAttachment) and UploadOptions.attachments.
• Limits as on iOS: 3 attachments of 3 MB; over-limit dropped via onError. File names sanitised; zip member names are random (<random20>.attachment.bgsfile), manifest entry carries name/mimeType/fileName.

Review focus / known gaps

• Backend handling was verified by reading the worker source only (worker/jobs/bundle.py, manifest_entries.py), not against a running worker/viewer; how the viewer uses attrs.mimeType/fileName is unchecked.
• No Android File/byte[] variants (input is Uint8Array | string).
• Merge fix on main: attachments was moved back onto the public UploadOptions after merging with T2.

🤖 Generated with Claude Code

[cursor]

Deep review: T3 user attachments

Reviewed 5e350aa (feat(core): user attachments on reports) against the assembler, marker stores, Electron renderer IPC, and WebView report pipeline — not the diff alone. Live Node/browser upload() of in-memory attachments is well-tested. The holes are the JSON boundaries this payload now has to cross, and two paths that can still abort the whole report.

Findings

P1 — Binary attachments do not survive JSON, which is how crash recovery, Electron renderers, and WebView actually transport the report

• Location: packages/core/src/attachments.ts:85 (instanceof Uint8Array); persisted via userAttachments on Report (reporting.ts:76)
• Problem: data must be a string or a live Uint8Array. JSON.stringify(new Uint8Array([1,2,3])) becomes {"0":1,"1":2,"2":3}; Node Buffer becomes {"type":"Buffer","data":[...]}. After parse, instanceof Uint8Array is false and the attachment is dropped as malformed. Reproduced.
• Impact: The file comment says attachments usually ride a crash report that is the only record of the incident. That is exactly the path that JSON-persists the request before assembly.
• Scenario:
  1. Capture recovery (Node + browser): submitReport writes JSON.stringify(marker) (node-utils report-marker-store.ts:21, browser-utils idb-report-marker-store.ts:17-18) before async assembly. If the process dies in that window — the window recovery exists for — next launch reassembles from the parsed marker and drops every binary attachment. String data survives. A 64 KiB Uint8Array already JSON-encodes to ~644 KiB (~10×); 3 MB is tens of MB of "0":n,"1":n,... on the crash path.
  2. Electron renderer: encodeReport is JSON.stringify({source, report}). Main never sees a Uint8Array. Binary client.upload({attachments}) / addReportAttachment from a renderer is dropped at assembly. Live path, not just recovery.
  3. WebView: webview-report-pipeline.ts JSON.stringify({ source, report }) — same.
• Fix: Give binary payloads a JSON-stable encoding (e.g. {encoding:'base64', data} or a separate bytes blob keyed from the marker). Decode in resolveUserAttachments before the instanceof check. Do not put raw Uint8Array through JSON.stringify. Copy bytes into the zip (or a sidecar) before the marker write if the crash path cannot wait.

P2 — Non-string fileName throws and loses the report

• Location: packages/core/src/attachments.ts:123 → sanitizeAttachmentFileName (:40, raw.replace)
• Problem: name is type-checked; fileName is not. 0, false, or [] is not nullish, so it is passed through. sanitizeAttachmentFileName assumes a string and throws. The throw escapes resolveUserAttachments / assembleBundle. The trigger pipeline maps it to bundle assembly failed and the incident is gone. Reproduced: a later valid attachment in the same array never runs; onError is not called.
• Impact: Directly contradicts the module contract (“a report is never lost”).
• Scenario: { name: 'log', data: dump, fileName: dump.byteLength } (wrong field) on a crash before handler. Typed TS callers are safe; JS and inferred JSON are not.
• Fix: If fileName is present and not a string, drop that attachment via onError (same as a missing name). Never throw out of resolveUserAttachments.

P2 — report.attachments is still the unsanitized zip-name path

• Location: packages/core/src/reporting.ts:70 vs new userAttachments (:76); assembler bundle-assembler.ts:207-210 vs :216-226
• Problem: T3’s plan says report.attachments writable from before. The safe path is userAttachments / addReportAttachment. Report.attachments remains writable, skips the 3×3 MB cap, and uses attachment.name as the zip member. writeBundleZip throws on duplicate names / __proto__ and aborts the entire bundle (already measured in docs/review/core-D-bundle-upload-recovery.md).
• Impact: One wrong-field assignment on a crash handler can delete the incident (name: 'manifest.json' / 'logs.json' / 'crash.json').
• Scenario: before does request.report.attachments = [{ name: 'logs.json', data }] because that is the Android-shaped name and the plan’s wording.
• Fix: Keep attachments internal (readonly, or a non-exported type). Route application files only through userAttachments. If a caller assigns the old field, treat it as user input (namespace + limits) or reject it via onError without throwing.

Residual (not blocking)

• addReportAttachment is exported from @bugsee/core only. Platform/umbrella barrels re-export ReportAttachmentInput but not the helper, despite the “without reaching into core” pattern.
• Recovery assembleBundle context (node/browser/webworker launch) still omits onError, so recovered drops are silent even after the P1 is fixed.
• Viewer use of attrs.mimeType / attrs.fileName remains unverified (known gap in the PR body).

Overall

1. Overall risk: High
2. Merge recommendation: Request changes — do not treat T3 as done until binary data survives markers and the renderer/WebView JSON hops, and until resolveUserAttachments cannot abort assembly.
3. Fix first: (1) JSON-stable binary encoding + decode before instanceof, (2) never throw from sanitizeAttachmentFileName / resolveUserAttachments, (3) stop exposing the unsanitized report.attachments zip-name field as something application code can assign.
4. What is in good shape: Zip members are random *.attachment.bgsfile (closes the zip-slip / reserved-name collisions for the new path). fileName traversal is stripped. Oversize attachments do not consume a slot. Internal minidumps still coexist. Unit, assembler, client, and e2e coverage of the live in-memory path is real.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

P1: This instanceof Uint8Array check is what drops binary attachments after every JSON hop.

JSON.stringify(new Uint8Array([1,2,3])) is {"0":1,"1":2,"2":3}; Node Buffer becomes {"type":"Buffer","data":[1,2,3]}. After JSON.parse, this condition fails and the attachment is reported malformed and dropped. String data round-trips. Reproduced against this function.

That hop is not hypothetical:

• Report markers persist JSON.stringify(marker) before assembly (node-utils report-marker-store.ts:21, browser-utils idb-report-marker-store.ts:17-18). A crash that dies in the drain/zip window — the window those markers exist for — recovers the report with every binary attachment gone. A 64 KiB Uint8Array already expands to ~644 KiB of index-keyed JSON on that crash path.
• Electron renderer encodeReport and WebView JSON.stringify({ source, report }) are JSON for the live path. Binary upload({attachments}) / addReportAttachment from those environments never arrives as a Uint8Array.

The comment at the top of this file says the attachment usually rides a crash report that is the only record of the incident. Encode bytes in a JSON-stable form (base64 tagged object, or a sidecar the marker points at) and decode here before this check. Do not JSON.stringify a raw Uint8Array.

[cursor]

P2: fileName is not type-checked, and sanitizeAttachmentFileName assumes a string (raw.replace at line 40). A non-string non-nullish value (0, false, []) skips ?? name and throws.

That throw leaves resolveUserAttachments / assembleBundle. The trigger pipeline turns it into bundle assembly failed and the incident is lost — including later valid attachments in the same array. onError is not called. Reproduced with { name: 'ok', data: 'x', fileName: 0 } followed by a good entry.

This is the hole in the “malformed → drop via onError, never throw” contract that name already honors. If fileName is present and not a string, drop this attachment through onError and continue.

[cursor]

P2: This field is still writable application-facing state, and it is the unsafe attachment path. The assembler still uses attachment.name as the zip member (bundle-assembler.ts:207-210) with no 3×3 MB cap and no sanitization.

writeBundleZip throws on a duplicate name or __proto__ and aborts the entire bundle (already measured: manifest.json / logs.json / crash.json / apptoken → report lost). T3’s plan tells people to write report.attachments from before; the safe new field is userAttachments immediately below.

Keep this for SDK-owned minidumps (readonly / internal type). Application files should only go through userAttachments. Assigning here with name: 'logs.json' on a report that also captured logs deletes the incident.

[krassx]

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Binary user attachments now cross the Electron renderer and WebView JSON hops as base64 and SDK .dmp survives the recovery marker (dc5fc29, f1982c5); report.attachments names can no longer abort assembly; hostile attachment lists are contained.

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

[krassx]

Closing instead of merging: this review-only branch is a frozen snapshot that now conflicts with the review base branch after #37/#40/#43/#46 were merged. Its content is already on main, and every finding was addressed there (see the earlier triage comment; #46 was approved at the final review).

🤖 Generated with Claude Code