Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure Trusted Signing fails when there are spaces in the file path #8600

Closed
MikeJerred opened this issue Oct 15, 2024 · 6 comments · Fixed by #8606
Closed

Azure Trusted Signing fails when there are spaces in the file path #8600

MikeJerred opened this issue Oct 15, 2024 · 6 comments · Fixed by #8606

Comments

@MikeJerred
Copy link
Contributor

  • Electron-Builder Version: 25.1.8
  • Node Version: 20.11.0
  • Electron Version: 21.3.3
  • Electron Type (current, beta, nightly):
  • Target: windows

If the path to be signed has spaces in it, the powershell command interprets multiple args.

Example of the error when trying to sign foo bar.exe:

Get-ChildItem: Cannot find path '.\bar' because it does not exist.
SignTool Error: File not found: .\foo
Exception: SignTool failed with exit code 1
@MikeJerred
Copy link
Contributor Author

I think this should be easy to solve by wrapping the file path arg in quotes:

windowsSignAzureManager.ts:

 const params = {
   FileDigest: "SHA256",
   ...extraSigningArgs, // allows overriding FileDigest if provided in config
   Endpoint: endpoint,
   CertificateProfileName: certificateProfileName,
   CodeSigningAccountName: codeSigningAccountName,
-  Files: options.path,
+  Files: `"${options.path}"`,
 }

I would open a PR but I am finding the setup to get this project running locally impossible.

@mmaietta
Copy link
Collaborator

Nice catch! I was unfortunately not able to set up unit tests for the Invoke-TrustedSigning stage of the logic due to not having an azure signing account and their unavailability of being free to opensource projects.

Happy to help get your dev environment setup running though! Here are the generic instructions for getting up n' running for localized testing in your test project: https://github.com/electron-userland/electron-builder/blob/master/CONTRIBUTING.md#to-setup-a-local-dev-environment
Anything I can assist with?

@mmaietta
Copy link
Collaborator

To kickstart you with a test via patch-package, this is the patch on top of the next version of electron-builder

app-builder-lib+26.0.0-alpha.2.patch

diff --git a/node_modules/app-builder-lib/out/codeSign/windowsSignAzureManager.js b/node_modules/app-builder-lib/out/codeSign/windowsSignAzureManager.js
index a78fc4d..c150786 100644
--- a/node_modules/app-builder-lib/out/codeSign/windowsSignAzureManager.js
+++ b/node_modules/app-builder-lib/out/codeSign/windowsSignAzureManager.js
@@ -77,7 +77,7 @@ class WindowsSignAzureManager {
             Endpoint: endpoint,
             CertificateProfileName: certificateProfileName,
             CodeSigningAccountName: codeSigningAccountName,
-            Files: options.path,
+            Files: `"${options.path}"`,
         };
         const paramsString = Object.entries(params)
             .reduce((res, [field, value]) => {

@MikeJerred
Copy link
Contributor Author

Ah good point, I tested now by modifying the .js file directly within node_modules/app-builder-lib and it is working correctly with that fix :)

In terms of project setup, I was able to run pnpm install successfully, but then running pnpm run compile produced 22 errors. I have just pulled in the last 3 commits from master and it works now though, so I guess the code had some errors temporarily.

It is a shame that Azure doesn't provide a free account for open source projects, it does make it tricky to test!

@mmaietta
Copy link
Collaborator

That's odd indeed! I've been developing off of master quite a bit recently and all CI tests have been passing as well https://github.com/electron-userland/electron-builder/commits/master/

Glad to hear that it works with that fix! I'll get a PR set up today and into the next release. Will post back when it's deployed

@mmaietta
Copy link
Collaborator

Released in v26.0.0-alpha.3!

Note, the alpha version also migrates to electron/asar and integrates electron/fuses (electron-builder config object electronFuses). Before I had migrated to electron/asar, several new asar unit tests were added to make sure the migration pathway was clean, so all should be fine. Please do let me know if you experience any issues with it though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants