fix(build): install per-machine so the Windows installer elevates (#264) - #268
Merged
Conversation
`win.requestedExecutionLevel: requireAdministrator` is applied only to Kudu.exe's manifest — app-builder-lib patches it in signAndEditResources and it never reaches the NSIS script. With `nsis.perMachine` unset it defaults to false, so installer.nsi compiled with `RequestExecutionLevel user`: Kudu-Setup-*.exe never asked for elevation, installed into %LOCALAPPDATA%\Programs\Kudu, and then tried to launch a binary whose manifest demands admin. Running the installer elevated by hand made the whole chain consistent, which is why that path worked. Two further consequences of the mismatch: - %LOCALAPPDATA%\Programs is user-writable and applyAutoLaunchWin32 registers Kudu.exe with RunLevel HighestAvailable, so an auto-elevating binary sat in a directory any process running as the user can rewrite. - updateInfo.isAdminRightsRequired is derived from perMachine, so the published metadata advertised false for an app that cannot run without admin. perMachine: true makes the installer request admin up front, install to Program Files, and set isAdminRightsRequired. Existing per-user installs are migrated by electron-builder, which runs uninstallOldVersion against HKEY_CURRENT_USER when the install mode is all. Adds a test pinning the app manifest and installer execution levels together, since nothing in the app's own code paths can catch this.
The js-yaml devDependency added alongside the per-machine fix desynced package-lock.json — npm pruned the nested conventional-commits-parser and conventional-commits-filter entries it considered extraneous locally, which every CI job then rejected with `npm ci` EUSAGE. Restore package.json and package-lock.json to their state on main and read the two options the test cares about directly. The config is flat and hand-maintained, so a top-level block lookup is enough and costs no lockfile churn.
`flushes in batches so a large clean never buffers everything` creates and deletes 1200 real files. That count is load-bearing — flushPending() fires every 500 records, so proving a remainder flush on top of two full batches needs more than 1000 items — and it is the slowest test in the suite at roughly 2s locally. On the Windows CI runner it crossed the 5s default and timed out. Raise the timeout for that one test rather than shrinking the fixture and losing the third-batch assertion. Unrelated to the installer fix; it surfaced as a flake on this branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #264.
The mismatch
win.requestedExecutionLevel: requireAdministratoris applied only toKudu.exe's manifest — app-builder-lib patches it inwinPackager.signAndEditResources→resEdit.patchManifestExecutionLevel, and nothing else in the toolchain reads the option. It never reaches the NSIS script.The installer's own execution level comes from
nsis.perMachine, which was unset and thereforefalse. Soinstaller.nsicompiled with:Kudu-Setup-*.exenever asked for elevation, installed per-user into%LOCALAPPDATA%\Programs\Kudu, and then had to launch a binary whose manifest demands admin. Running the installer elevated by hand made the whole chain consistent — which is the workaround in the issue.Two more consequences of the same mismatch
%LOCALAPPDATA%\Programsis writable by the user, andapplyAutoLaunchWin32registersKudu.exeas a scheduled task withRunLevel HighestAvailable. An auto-elevating binary living in a user-writable directory is replaceable by any process running as that user.updateInfo.isAdminRightsRequiredis derived fromperMachineonly (NsisTarget.js), so the publishedlatest.ymladvertisedfalsefor an app that cannot run without admin. The reporter flagged this independently.The change
nsis.perMachine: true. The installer now requests admin up front (UAC at launch), installs to Program Files, and setsisAdminRightsRequiredso electron-updater elevates when applying updates.Existing per-user installs migrate automatically: electron-builder's install section runs
uninstallOldVersion HKEY_CURRENT_USERwhenever the install mode isall.Adds
src/main/build-config.test.tspinning the app manifest and installer execution levels together — nothing in the app's own code paths can catch a regression here, since it only manifests once a user runs the installer.Confidence
The elevation mismatch, the EoP, and the metadata bug are all confirmed by reading the app-builder-lib 26.8.1 sources.
The hang itself is probable cause, not proven. I traced
.onInitand the install section looking for the reported spin (one thread at 100%, zero I/O delta, no child processes) and every candidate loop —ALLOW_ONLY_ONE_INSTALLER_INSTANCE,GetDParameter,_CHECK_APP_RUNNING,GetProcessInfo— is bounded, and the ones that could stall would spawn children or do I/O. The issue is n=1 and unreproduced here. This PR fixes the configuration defect the reporter's workaround points at and is correct on its own merits regardless; if a non-elevated hang survives it, that's a separate NSIS-level bug.