fix(desktop): redact home paths from updater errors sent to Sentry

Merged
SA
2mo ago
+409-1

Problem

When the macOS updater stages an update, it copies the new bundle out of a cache
directory under the user's home folder. When a file it expects is not there, it
reports the failure by printing the full absolute path of that file, and we
hand that message straight to Sentry.

That path begins with the user's home directory, so every one of these reports
carried the account name of the person it came from into the issue title — and
issue titles get republished, including into our own alerting channel. The same
path also contains a per-attempt random staging directory name, so each
occurrence formed a brand-new issue group and one recurring condition shredded
into an unbounded number of issues that could never be triaged as one thing.

The four referenced issues are one condition, not four; they are separate groups
only because that path differs per machine and per attempt. Roughly 108 events
across about a dozen releases in the last 14 days belong to the same family.

Reachability. The reported events come from the packaged desktop app's main
process, which is the only place setupAutoUpdater runs. The change sits on the
one Sentry.captureException in that handler, so it takes effect for every
future occurrence in the first desktop release cut after this merges, and reaches
everyone who auto-updates onto it. It does not retroactively change the events
already captured.

Why code, and not something else. Archiving the issues loses the condition
entirely and does nothing about the account names in the events already there or
the ones that keep arriving. An inbound Sentry filter would drop the reports
wholesale, which is worse: this failure is worth knowing about, it just must not
be worth knowing who. Fixing it further down — at the updater — is a different
question (why the staged bundle is incomplete) and does not stop the path from
being reported. A Sentry-side beforeSend scrubber is forbidden by repo law
(#6164), and correctly so: it would suppress at the wrong layer. The capture
site is the only place that can both keep the report and strip the identity, so
that is where this lands.

Root cause

Sentry.captureException(error) in the autoUpdater.on("error", ...) handler
reported the updater's message verbatim. The message is generated by the staging
tool and is not ours, so nothing along the way had a chance to remove the
user-specific prefix before it became an issue title.

Fix

A new update-error-redaction.ts beside the existing classifier, applied at the
capture call only:

  • the account segment of an absolute home path becomes ~
  • the per-attempt staging directory becomes update.<id>

Everything else in the message — the tool that failed, the file it wanted, the
reason — is left exactly as it was, so the report stays as useful as it is today
and two machines now produce an identical message.

Illustrative shape, with invented account names (not a real captured path):

machine A in : ditto: /Users/ada/Library/Caches/com.superset.desktop.ShipIt/update.qZ4mTb1/Superset.app/Contents/Resources/app.asar: No such file or directory
machine B in : ditto: /Users/grace.h/Library/Caches/com.superset.desktop.ShipIt/update.Kd9wRp7/Superset.app/Contents/Resources/app.asar: No such file or directory
machine A out: ditto: ~/Library/Caches/com.superset.desktop.ShipIt/update.<id>/Superset.app/Contents/Resources/app.asar: No such file or directory
machine B out: ditto: ~/Library/Caches/com.superset.desktop.ShipIt/update.<id>/Superset.app/Contents/Resources/app.asar: No such file or directory
converged    : true

redactUpdateError returns the original error object when there is nothing
to redact, and otherwise preserves name and redacts the stack alongside the
message, so errors that do carry a stack (checksum and code-signature failures)
do not lose it.

Scope notes:

  • isEnvironmentUpdateError is untouched — the file is byte-for-byte
    unchanged, and classification still runs on the raw message. This reports in
    exactly the cases it does today; only the reported text changes.
  • The log lines and the renderer status are untouched. They stay on the user's
    own machine and were never the problem.
  • No typed cause added. Nothing reads one here — no renderer branch, no
    retry decision, no test — so a kind would be code maintained for nothing.
  • Not investigated here: why the staged bundle is incomplete. Separate question.
  • Left alone, as a neighbouring capture that can also carry an absolute path:
    host-service-coordinator.ts:974 puts the host-service outputTail into
    extra. It is secret-redacted but not path-redacted. It does not reach the
    issue title, so it neither leaks into republished titles nor shreds grouping.

What the matcher deliberately does not match

Both patterns are narrow on purpose, because a matcher that rewrites more of the
message than the part it names is a defect. Named inputs it must leave alone, all
covered by tests:

  • /Users/Shared/... — a real macOS directory, not somebody's account (while an
    account merely starting with "Shared" is still redacted)
  • /UsersGuide/... — not a home path at all
  • /Applications/..., /Library/..., /tmp/... — absolute, but not under a home
  • update.zip, update.yml, update.log — real filenames, not the staging dir;
    the staging pattern is anchored on both slashes so it only rewrites a path segment
  • version numbers (1.22.0 → 1.24.0) and error text (sha512 checksum mismatch, expected …, got …) pass through byte-for-byte

Verification

The negative tests were written before the implementation and confirmed to
fail first (Cannot find module './update-error-redaction').

bunx biome check on the three changed files — reformatted one test file with
--write, then re-checked clean:

Checked 3 files in 31ms. No fixes applied.

cd apps/desktop && bun run typecheck:

$ tsc --noEmit
TYPECHECK EXIT: 0

bun test apps/desktop/src/main/lib:

 556 pass
 0 fail
 1087 expect() calls
Ran 556 tests across 40 files. [671.00ms]

The existing classification tests pass unchanged, run on their own:

 6 pass
 0 fail
 26 expect() calls
Ran 6 tests across 1 file. [83.00ms]

New redaction tests (positive, convergence, and negatives):

 8 pass
 0 fail
 24 expect() calls
Ran 8 tests across 1 file. [70.00ms]

Proof the negative tests bite. Widening both patterns — dropping the
/Users/Shared carve-out and dropping the path-segment anchors around the
staging directory — makes exactly the over-match tests fail:

Expected: "ENOENT: no such file or directory, open '/Library/Application Support/Superset/update.log'"
Received: "ENOENT: no such file or directory, open '/Library/Application Support/Superset/update.<id>'"
(fail) redactUpdateErrorMessage > passes messages without a home directory through unchanged

Expected: "ditto: /Users/Shared/Superset/staged.zip: I/O error"
Received: "ditto: ~/Superset/staged.zip: I/O error"
(fail) redactUpdateErrorMessage > leaves paths that are not a user home alone

Expected: "ditto: ./update.zip: Couldn't read pkzip signature."
Received: "ditto: ./update.<id>: Couldn't read pkzip signature."
(fail) redactUpdateErrorMessage > rewrites only the staging directory segment, not similarly named files

 5 pass
 3 fail

The narrow patterns were then restored and the suite re-run green.

Repo-wide bun run lint was not run (it hangs on cross-worktree bun locks).

Refs DESKTOP-127
Refs DESKTOP-122
Refs DESKTOP-11Z
Refs DESKTOP-11T

https://claude.ai/code/session_014LNNETnHMNuF3yKfrM4M3m

Summary by cubic

Redacts home-path segments and per-attempt staging tokens in desktop updater errors before sending to Sentry on macOS, Linux, and Windows. Previously, titles leaked account names and grouped per attempt; now messages and stacks converge without identity leakage while keeping diagnostic value.

  • Applied at the Sentry capture site in the desktop main process; logs and renderer status are unchanged.
  • Redacts both message and stack; returns the original error only when neither changes. Preserves the error name and carries own properties across, redacting string values; non-strings are unchanged.
  • isEnvironmentUpdateError is unchanged and still runs on the raw message; reporting scope is identical.
  • Matchers are narrow and close prior bypasses: handle non-ASCII account names; treat segment boundaries at a path separator or whitespace/quote/colon; carve-outs use the same delimiters. Linux matches only /home/<account> and skips linuxbrew; Windows matches C:\Users\<account> case-insensitively and skips Public, Default, and All Users; macOS skips /Users/Shared. Rewrites only /update.<token>/ to /update.<id>/; does not touch non-home paths or files like update.zip/update.yml.
  • Expected impact: events group per condition and titles no longer include account names; effective in the next desktop release. Addresses DESKTOP-127; relates to DESKTOP-122, DESKTOP-11Z, DESKTOP-11T.

Summary by CodeRabbit

  • Bug Fixes

    • Improved privacy when reporting desktop app update errors by removing personal home-directory names and temporary staging paths across macOS, Linux, and Windows.
    • Preserved useful diagnostic details while leaving shared system paths and unrelated messages unchanged.
    • Maintained error names, stack information, and custom error properties when sanitizing reported errors.

  • Tests

    • Added coverage for platform-specific path redaction, stack handling, unchanged errors, and various update failure scenarios.

Follow-up commit: Linux and Windows coverage (added during triage review)

The first commit redacted only macOS /Users/<account> paths. The updater ships
on Windows (nsis) and Linux (AppImage) as well, and this handler reports all
three with no platform gate — their staging directories live under the user's
home too, which is exactly why the existing classifier already looks for an
-updater path marker alongside shipit. So the same account name was still
reaching the issue title on two of the three shipped platforms.

Added /home/<account> and <drive>:\Users\<account>, each carving out the
directories under that root that are system locations rather than accounts:

  • Windows: Public, Default, All Users
  • Linux: linuxbrew
  • Linux additionally requires /home/ to start a path, so a deeper directory
    that merely ends in /home (/var/lib/home/...) is left alone

The carve-outs were confirmed load-bearing the same way as the originals:
dropping them makes the over-match test fail.

$ bunx biome check <the two changed files>
Checked 2 files in 5ms. No fixes applied.

$ cd apps/desktop && bun run typecheck
TYPECHECK EXIT: 0

$ cd apps/desktop && bun test src/main/lib
 560 pass
 0 fail
 1096 expect() calls
Ran 560 tests across 40 files. [614.00ms]

Second follow-up commit: keep error properties when redacting

Rebuilding the error as a bare new Error(message) dropped every own property
the updater had set. electron-updater builds its errors with
newError(message, code) (builder-util-runtime/out/error.js), which sets an
own code, and Sentry serialises non-standard error properties — so the
redaction was silently discarding the most triageable field on the report,
precisely for the errors it rewrote.

Own properties are now carried across, with string values passed through the
same redaction, so a home path cannot escape on a property instead of the
message. Non-string values cross untouched. The identity path is unchanged: an
error with nothing to redact is still returned as the very same object.

Three tests were added and confirmed to fail first (code, a path-bearing
path, and a numeric statusCode all came back undefined).

$ bunx biome check <the two changed files>
Checked 2 files in 5ms. No fixes applied.

$ cd apps/desktop && bun run typecheck
TYPECHECK EXIT: 0

$ cd apps/desktop && bun test src/main/lib
 563 pass
 0 fail
 1100 expect() calls
Ran 563 tests across 40 files. [794.00ms]

Third follow-up commit: three redaction bypasses (CodeRabbit findings, all accepted)

All three were verified against the code rather than taken on the badge. Each
one let the account name this redaction exists to remove reach Sentry anyway,
so all three are fixed rather than dismissed.

  1. \b carve-outs exempted real accounts. \b matches before . and -,
    so /Users/Shared.dev/, /home/linuxbrew.dev/ and C:\Users\Public.dev\
    were each treated as the system directory they merely resemble and left
    unredacted — all three are valid account names under the pattern's own
    character class. Each carve-out now requires a path separator or end of
    string after the name. The exact system directories stay exempt; accounts
    that merely start with those words no longer do.

  2. Windows matching was case-sensitive. c:\users\ada\... bypassed the
    pattern entirely. It now carries the i flag, which also makes the
    Public/Default/All Users exemptions case-insensitive, as Windows
    means them.

  3. The stack was not inspected before the identity check. redactUpdateError
    returned the original error whenever the message alone was clean, sending
    an unredacted stack to Sentry. That is the ordinary case when the app is
    installed under the user's own Applications folder: the message carries no
    path but every frame does. The stack is now redacted first, and the error is
    returned unchanged only when neither field moves.

Each is pinned by a test confirmed to fail against the previous patterns —
restoring \b, dropping the i flag and dropping the stack check makes exactly
those three tests fail and nothing else. The identity test now sets its own
stack rather than inheriting whatever path the test file lives under, which
would otherwise have redacted itself and made that assertion depend on the
checkout location.

$ bunx biome check <the two changed files>
Checked 2 files in 5ms. No fixes applied.

$ cd apps/desktop && bun run typecheck
TYPECHECK EXIT: 0

$ cd apps/desktop && bun test src/main/lib
 567 pass
 0 fail
 1114 expect() calls
Ran 567 tests across 40 files. [595.00ms]

Fourth follow-up commit: non-ASCII account names (CodeRabbit finding, accepted)

The account-name class was [A-Za-z0-9._-]+, which is ASCII-only. An accented
name was therefore rewritten only in part, and a name containing no ASCII at
all was not matched at all — the whole account name went to Sentry. Account
names are not ASCII in much of the world; the events this redaction was written
for arrived from seven countries.

The account segment is now "everything up to the platform separator" rather
than a character class. Whitespace, quotes and colons also end the segment, so
a path sitting at the end of a sentence cannot run on and swallow the message
after it.

Pinned from both sides, because this class can fail in two opposite ways:

  • narrowing it back to ASCII fails the accented and non-Latin cases (the leak)
  • widening it to a bare "anything but a separator" fails a new case asserting
    the redaction stops at the segment — ditto: /Users/<name>: No such file
    must become ditto: ~: No such file, not ditto: ~, and two paths in one
    message must redact independently rather than merging

The staging-directory pattern stays ASCII-only, deliberately: those names are
generated by the updater, not by a person.

$ bunx biome check <the two changed files>
Checked 2 files in 6ms. No fixes applied.

$ cd apps/desktop && bun run typecheck
TYPECHECK EXIT: 0

$ cd apps/desktop && bun test src/main/lib
 570 pass
 0 fail
 1122 expect() calls
Ran 570 tests across 40 files. [588.00ms]

Fifth follow-up commit, and a note on the approach

The fix: widening the account segment in the previous commit without widening
the carve-out boundary to match left the two disagreeing about where a segment
ends, so a reserved directory named at the end of a sentence
(/Users/Shared: access denied) was rewritten as though it were an account.
Not a leak — the mirror of one — and self-inflicted. The carve-outs now end on
the same delimiter set the segment does. Dropping the boundary entirely fails
three separate tests. 572 pass, typecheck 0, biome clean.

The note, which matters more than the fix. This is the fifth round of
defects on a 13-line family of regexes, and that is evidence about the approach
rather than about any one pattern. A materially more robust instrument exists:
homedir() — already used throughout this process — gives the running user's
home directory exactly, and a literal string replacement of it needs no
character class, no carve-outs, no Unicode handling, no case rules and no
delimiter reasoning. Every bug in rounds 3, 4 and 5 was impossible under that
design.

It is deliberately not done here, for two reasons worth writing down rather
than leaving implicit:

  • It is a narrower guarantee. A literal home replacement only redacts the
    running user's home. That is complete for these events, because each report
    is generated on the machine whose path it carries — but it stops covering an
    arbitrary home-shaped path appearing in a message, which the current patterns
    do cover.
  • The test suite here asserts input-message → output-message over hardcoded
    foreign paths. A homedir-based implementation would fail all of them unless
    the home directory is injected, so switching means restructuring the tests at
    the same time as the implementation. Doing both at the end of a five-round
    session is how a fresh class of bug gets introduced.

The behavioural tests added across these commits are the thing that makes that
swap safe later: they pin what the output must look like, from both directions,
independently of how it is produced. Recommended as the follow-up.