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

fix: generate stubExecutableExe and sign it #8959

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Mar 14, 2025

fix #8952

Root Cause

when createExecutableStubForExe is executed, WriteZipToSetup writes information to the file, essentially creating a new file, which invalidates the original signature.
Image

https://github.com/Squirrel/Squirrel.Windows/blob/51f5e2cb01add79280a53d51e8d0cfa20f8c9f9f/src/Update/Program.cs#L633-L647

Image

How to fix
Apply a patch to the Squirrel Windows source code(Squirrel/Squirrel.Windows#1903). For the existing stub exe files, don't generate them anymore. Then, a new stub exe can be generated in Electron Builder and signed.

Copy link

changeset-bot bot commented Mar 14, 2025

⚠️ No Changeset found

Latest commit: 9800401

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@beyondkmp beyondkmp marked this pull request as draft March 14, 2025 08:06
@beyondkmp beyondkmp marked this pull request as ready for review March 16, 2025 01:12
@beyondkmp beyondkmp requested a review from mmaietta March 16, 2025 01:47
@@ -19,6 +19,7 @@
<file src="*.pak" target="lib\net45" />
<file src="*.exe.config" target="lib\net45" />
<file src="*.exe.sig" target="lib\net45" />
<file src="*_ExecutionStub.exe" target="lib\net45" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this now being added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since electron-builder now generates and signs *_ExecutionStub.exe, adding this configuration will move the generated *_ExecutionStub.exe to the lib\net45 directory.

@t3chguy
Copy link
Contributor

t3chguy commented Mar 17, 2025

Seems to fail to build at least for ARM64 package: https://github.com/element-hq/element-desktop/actions/runs/13895690921/job/38875908510?pr=2211 looks like my testing is insufficient, doesn't bring in the vendor dir - looks like patch-package doesn't support binary files ds300/patch-package#193

@t3chguy
Copy link
Contributor

t3chguy commented Mar 17, 2025

Looks like package.json files needs updating to include vendor dir

@t3chguy
Copy link
Contributor

t3chguy commented Mar 17, 2025

image

Looks like it works sans the package.json not including vendor in the package - good job @beyondkmp

I also checked that the number of signings remained the same

Copy link
Collaborator

@mmaietta mmaietta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the vendor directory adds a few megabytes (quick maffs) to the repo, which is the antipattern from what electron-builder-binaries is supposed to be used for. IIRC though, the squirrel windows target/package must be installed separately and isn't part of the electron-builder dependency tree, right? In this case, I think it's safe to add the vendor files (albeit I'll still need to verify file origin). In general though, we should avoid adding vendor files directly to electron-builder unless absolutely necessary (which is the case with this fix)

for (const file of files) {
if (path.extname(file.name) === ".exe" && path.basename(file.name, "exe") !== "Squirrel") {
const filePath = path.join(appOutDir, file.name)
log.debug({ file: filePath }, "generating stub executable for exe")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use log.filePath(filePath) here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if (path.extname(file.name) === ".exe" && path.basename(file.name, "exe") !== "Squirrel") {
const filePath = path.join(appOutDir, file.name)
log.debug({ file: filePath }, "generating stub executable for exe")
const fileNameWithoutExt = file.name.slice(0, -4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can extract path.basename(file.name, "exe") from if-statement to a const and then reuse here instead of using slice since basename w/ ext supplied should get us the filenameWithoutExt

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@t3chguy
Copy link
Contributor

t3chguy commented Mar 17, 2025

the squirrel windows target/package must be installed separately and isn't part of the electron-builder dependency tree, right?

Yup, electron-builder-squirrel-windows https://www.npmjs.com/package/electron-builder-squirrel-windows is not a transitive dependency of electron-builder

@beyondkmp
Copy link
Collaborator Author

Looks like package.json files needs updating to include vendor dir

my bad. Added.

@beyondkmp
Copy link
Collaborator Author

These vendor files are copied from the GitHub Actions workflow at https://github.com/beyondkmp/Squirrel.Windows/actions/runs/13871759240/job/38819263248 and the other files(like 7zip,nuget) are copiled from https://github.com/electron/windows-installer/tree/main/vendor.

https://github.com/Squirrel/Squirrel.Windows/pull/1903/files
The code changes for Squirrel Windows are located here.

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

Successfully merging this pull request may close these issues.

Missing signature on Squirrel ExecutionStub
3 participants