electron-userland / electron-builder

A complete solution to package and build a ready for distribution Electron app with “auto update” support out of the box
https://www.electron.build
MIT License
13.57k stars 1.73k forks source link

/D NSIS Flag does not support paths containing white spaces (e.g. C:/Program Files/) #7946

Open gdotv opened 9 months ago

gdotv commented 9 months ago

The /D flag for the NSIS installer in command line mode is not properly parsing/handling file paths containing spaces. The issue appears to be occurring in the parsing stage of the /D flag in multiUser.nsh (https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/templates/nsis/multiUser.nsh#L45). A very simple test to reproduce the issue (on a Windows device) is to attempt running the NSIS installer in "quiet" mode with an override path containing a white space, for instance in my case: ./gdotv.exe /S /D=C:\Program Files\gdotv

The expected output of this command is for gdotv.exe to be installed under C:\Program Files\gdotv, however it ends up being installed under C:\Program I've also tried different permutations with and without quotes but to no avail, e.g.: ./gdotv.exe /S /D="C:\Program Files\gdotv" fails the installation altogether ./gdotv.exe /S /D='C:\Program Files\gdotv' also fails the installation altogether

For reference, https://nsis.sourceforge.io/Docs/Chapter3.html, section 3.2 suggests the /D flag should allow for white spaces (and therefore be the last flag in the command). I'm unfortunately not familiar with nsh scripts but I expect the solution here would be to modify the parsing of the /D flag such that it either supports quotes or sticking to NSIS standard just captures the rest of the command (everything after /D=, effectively) as the value of the parameter.

This can be reproduced with a minimal, default NSIS build via electron-builder, no extra configuration required.

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days.

ravicious commented 7 months ago

It'd be really nice if /D worked with paths with spaces, given that the common use case one might want to use /D for is to change the install directory for the one-click installer to C:\Program Files\foo.

mmaietta commented 1 month ago

Since /D is supposed to be the last arg provided, would you be willing to try out this patch file with patch-package? electron-updater+6.3.0.patch

diff --git a/node_modules/electron-updater/out/NsisUpdater.js b/node_modules/electron-updater/out/NsisUpdater.js
index 565ab7d..c97b1ad 100644
--- a/node_modules/electron-updater/out/NsisUpdater.js
+++ b/node_modules/electron-updater/out/NsisUpdater.js
@@ -106,15 +106,15 @@ class NsisUpdater extends BaseUpdater_1.BaseUpdater {
         if (options.isForceRunAfter) {
             args.push("--force-run");
         }
-        if (this.installDirectory) {
-            // maybe check if folder exists
-            args.push(`/D=${this.installDirectory}`);
-        }
         const packagePath = this.downloadedUpdateHelper == null ? null : this.downloadedUpdateHelper.packageFile;
         if (packagePath != null) {
             // only = form is supported
             args.push(`--package-file=${packagePath}`);
         }
+        if (this.installDirectory) {
+            // maybe check if folder exists
+            args.push(`/D=${this.installDirectory}`);
+        }
         const callUsingElevation = () => {
             this.spawnLog(path.join(process.resourcesPath, "elevate.exe"), [options.installerPath].concat(args)).catch(e => this.dispatchError(e));
         };

It's basically just moving the /D flag to the last in the list.

ravicious commented 1 week ago

@mmaietta I've been meaning to try out this patch ever since you posted it. I finally found some time, but I've just noticed it's for electron-updater which our project doesn't use. Is there a patch I could try out with electron-builder itself?

mmaietta commented 5 days ago

I took a look at multiUser.nsh and I see the mention of https://github.com/electron-userland/electron-builder/blob/d5d9f3f9aaac0385cc943e30a0841669133afde8/packages/app-builder-lib/templates/nsis/multiUser.nsh#L86-L90

I'm quite unfamiliar with nsis scripting and haven't been able to track down where the installer execution is actually occurring though such that it's installing to a specific directory. @beyondkmp are you able to assist here by chance?