Repository navigation
fix(updater): re-prove the retained Linux package before a privileged retry - #12395
Conversation
… retry The recovery card's "Try Automatic Install Again" handed electron-updater the cached .deb/.rpm path with no re-verification. That path is user-writable, so the digest proven when the card rendered says nothing about the bytes dpkg or rpm would read as root minutes later — and the card's other actions (copy command, reveal) validated while the one that actually installs did not. Re-hash the artifact immediately before the install, ahead of any destructive quit prep, and abort with copy that tells the user to download again. This narrows the window rather than closing it; only an immutable handoff would close it, which is a larger change. Also fix a macOS-only failure this suite gained with the platform-conditional pre-commit copy: the expectation hardcoded the non-Darwin string, so the suite was red on any Mac.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLinux package installation now performs fresh retained-package digest validation before teardown and native installation. The updater blocks overlapping install requests, handles missing or changed artifacts, records diagnostics, and preserves recovery state for transient failures. Tests cover validation concurrency, staged Debian and RPM packages, digest failures, installation failures, retries, and lifecycle outcomes. The recovery card shows checking progress during automatic retries, and preload wiring relays aborted installations. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review findings on the original fix: 1. The abort force-sent its error status with no staleness guard, so a verdict from a hash that outlived its cycle overwrote whatever card had replaced it (a fresh 'available' from Check for Updates became a stale "package no longer matches" error). Now keyed on an install-cycle signature, the same protection failLinuxPackageRecovery already had. 2. 'read-failed' (EMFILE/EIO/EACCES mid-stream) was described as a digest mismatch and tore down the recovery card. It now reuses the accurate per-reason copy and keeps the card, exactly as the Copy/Show paths do for the same reason. It still fails closed: chmod 000 on a swapped file would otherwise be a one-line bypass, since root can read what we cannot. 3. The check was keyed on the recovery status, so it only covered the retry. The primary 'downloaded -> Restart to Update' install, whose window is hours rather than seconds, handed the same user-writable path to dpkg/rpm unverified. Moved into performQuitAndInstall keyed on the tracked artifact, so both paths are covered; non-Linux keeps its exact timing through a synchronous artifact guard. 4. The async prologue had moved the "quit timer is always cleared" invariant out of a try/finally. The re-proof now owns a flag cleared in finally, and a rejection fails closed instead of wedging the updater. 5. The install re-proof no longer joins an in-flight validation, so its proof cannot predate the click that asked for it. 6. The retry button gained the pending affordance the other actions have, since the click now streams the whole package before anything happens. Tests: real packages are staged in a real updater cache for the whole Linux block (a path that never existed would now abort every install); new cases cover the swapped primary install, the stale-verdict drop, the preserved card on read-failed, the rejecting re-proof, the concurrent second click, and the fresh-hash guarantee. Each was verified to fail with only its source change reverted.
|
Addressed the review findings in f657a51. #1 stale abort clobbers a newer cycle (fixed). The abort now captures an install-cycle signature before hashing and drops its status if the cycle changed — the same protection #2 read-failed copy and card teardown (fixed). Both messages are gone; the abort now reuses #3 primary "Restart to Update" path unprotected (fixed). The re-proof moved into #4 wedge on rejection (fixed). The re-proof owns a flag cleared in #5 in-flight reuse (fixed). #6 retry affordance (fixed). The retry action gained Test fixtures: the whole Linux block now stages real packages in a real updater cache, so a path that never existed would abort every install rather than silently pass. Verified: 115/115 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/updater.test.ts (1)
3914-3918: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse one constant for the pre-commit failure message.
The same platform-conditional literal exists at lines 1832-1835. Two copies must stay in sync with
getPreCommitInstallFailureMessageinsrc/main/updater.ts. MovePRE_COMMIT_FAILURE_MESSAGEto module scope and use it in both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48e46916-2c03-4778-9908-51e2538cd5ee
📒 Files selected for processing (7)
src/main/linux-package-update-recovery.test.tssrc/main/linux-package-update-recovery.tssrc/main/updater-linux-package-recovery-actions.test.tssrc/main/updater.test.tssrc/main/updater.tssrc/renderer/src/components/LinuxPackageInstallRecoveryCard.test.tsxsrc/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx
…tall The cycle guard that stops a stale digest verdict from clobbering a newer card also withheld the only signal the renderer has that the restart was called off. The preload abort relay keys on an 'error' status, so with the status suppressed the window stays restart-prepared for the rest of the session: Terminal/Settings skip their unsaved-work prompts and the shutdown checkpoint stays deduped, so a later real quit stages no fresh snapshot. Push the abandon from performQuitAndInstall's single return-false site, so it cannot depend on what the reporter decides about the status text, and relay it to the existing relay.abort() (a no-op unless the renderer armed a restart). The status stays cycle-guarded exactly as before. Tests: the stale-verdict and swapped-primary-install cases now assert the push, a committed install asserts its absence, and the preload relay test covers the new channel. Each fails with only its source change reverted.
|
Round-2 finding #1 (should-fix, updater.ts:1906) — fixed in Verified the claim first. Confirmed the stale-cycle branch in Fix (4 lines of source): push the abandon from if (getTrackedLinuxPackageArtifact() && !(await proveRetainedLinuxPackage(pendingVersion))) {
mainWindowRef?.webContents.send('updater:quitAndInstallAborted')
return
}Deliberately not done: no new retry/timeout/reorder machinery, no relaxing of the cycle guard (the stale status stays withheld — the round-1 clobber fix is untouched), no new abort trigger in the relay's status handling (broadening it to non-error states would spuriously abort a legit install when a background nudge check pushes Tests (all fail with only the source reverted, verified by patch-revert):
Verification: 236/236 across the 7 touched suites, 155/155 across adjacent updater/UpdateCard/beforeunload suites, both tsconfigs clean, oxlint + oxfmt clean, |
Found by the
v1.4.167..v1.4.168-rc.1production release scan.The defect
The Linux package recovery card (#12183) offers three actions. Copy Install Command and Show Package both call
validateTrackedArtifact()and re-hash the retained.deb/.rpm. Try Automatic Install Again — the one that actually runs a package manager as root — calledquitAndInstall()with no re-verification at all.The cached path is user-writable (
$XDG_CACHE_HOME/~/.cache/orca-updater/pending/). electron-updater'sDebUpdaterruns["dpkg","-i",installerPath]throughrunCommandWithSudoIfNeeded. So the digest proven when the recovery card rendered said nothing about the bytes a root package manager would read minutes later, after a failed escalation left the card on screen.Scope, stated honestly
The privileged-install primitive is not new —
v1.4.167already invoked the same electron-updater path, so this is not a regression introduced by 1.4.168. What #12183 added is a long-lived retry affordance that widens the window, plus a copy-command path that did not exist before.This change narrows the window; it does not close it. A same-UID attacker can still race between the hash and
dpkg -i. Closing it properly needs an immutable handoff — installing from a held descriptor or a root-owned copy — which is a larger change and deliberately out of scope here.The fix
Re-hash the artifact immediately before the install and ahead of any destructive quit prep, so a replaced or vanished package aborts with copy telling the user to download again, instead of being installed as root. The check runs only on the recovery retry path, where the user has already had one failure.
Also
Fixes a macOS-only failure this suite gained from the platform-conditional pre-commit copy:
updater.test.tshardcoded the non-Darwin string, so the unit suite was red on any Mac. The sibling block already had the platform guard.Verification
New test drives a real staged package in a real temp cache through the real validator: install fails, a local process swaps the file, the retry aborts — no PTY kill, no quit, correct error status and lifecycle event. The existing retry test now also runs against a genuinely valid artifact rather than a path that never existed.
src/main/updater.test.ts109/109 on macOS; updater/window/preload suites 363 passing.