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

feat: write asar integrity resource on windows #8245

Merged

Conversation

indutny-signal
Copy link
Contributor

Electron 30-x-y added support for ASAR integrity fuse on Windows. When enabled the app would fetch the ELECTRONASAR resource out of the executable file and use it to verify the integrity of the ASAR when reading the data from it.

Copy link

changeset-bot bot commented Jun 3, 2024

🦋 Changeset detected

Latest commit: 52eaa08

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

netlify bot commented Jun 3, 2024

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit 52eaa08
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/666202a307b6d90008be2832
😎 Deploy Preview https://deploy-preview-8245--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@indutny-signal
Copy link
Contributor Author

I'm not sure if I'm supposed to commit the lock file. On one hand, it has to be updated because I installed a new dependency, on the other - it seems like the format has changed and as a result the diff is too large. Any thoughts?

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 4, 2024

@indutny-signal what version of pnpm are you running? Right now, the CI is locked to 8.9.0 so the pnpm lockfile should be regenerated with that version.

Separately, I can look into updating our pnpm version in the CI, but am swamped with work and some life events right now and probably won't be able to pick that up in the near future.

@indutny-signal
Copy link
Contributor Author

@mmaietta thank you for the exact version number! It should be fixed now.

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 4, 2024

Out of curiosity (I'd love to learn more on this topic), how/where did you find the information for resedit? Does it work when cross-compiling on other OS'?

@indutny-signal
Copy link
Contributor Author

@mmaietta electron folks gave me a link to https://github.com/electron/packager/blob/main/src/resedit.ts :)

@indutny-signal
Copy link
Contributor Author

FWIW, I believe that both resedit not pe-edit (its dependency) are pure-JS libraries.

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 4, 2024

Thanks for the link! I'll TAL. Looks like there's an import error during compilation though

Cannot find module 'resedit/cjs' from '../packages/app-builder-lib/out/electron/electronWin.js'

@indutny-signal
Copy link
Contributor Author

@mmaietta thanks! I'm trying to figure it out... It seems to be something specific to how jest does the resolution...

Electron 30-x-y added support for ASAR integrity fuse on Windows. When
enabled the app would fetch the ELECTRONASAR resource out of the
executable file and use it to verify the integrity of the ASAR when
reading the data from it.
@indutny-signal
Copy link
Contributor Author

Alright, I substituted the cjs import with an async esm import. Let me know if this works!

@indutny-signal
Copy link
Contributor Author

That obviously didn't work. I'm surprised that test-windows/test-linux fails while test-mac works fine (as confirmed locally)

@indutny-signal
Copy link
Contributor Author

@mmaietta I didn't realize that CI was running on previous commit (before force-push). Do you mind restarting it?

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 5, 2024

Sure thing! I've restarted the CI, not sure what commit it'll pull from though, so we'll see what happens. At worst, you may just need to push an empty commit to kick the CI to behave 🙂

@indutny-signal
Copy link
Contributor Author

Thank you! Looks like empty commit is the way to go sadly.

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 6, 2024

So I checked this out locally and tried moving this to the top-level

import { NtExecutable, NtExecutableResource, Resource } from "resedit"

Seems the error is coming from within the package though.
Screenshot 2024-06-06 at 10 55 24 AM

    /project/node_modules/.pnpm/[email protected]/node_modules/resedit/dist/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { NtExecutable, NtExecutableResource, Format } from 'pe-library';

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 6, 2024

Bah, even this fails with the same error:

const { NtExecutable, NtExecutableResource, Resource } = require("resedit")

Maybe need to try a different release version of resedit?

Btw, easy way to test this locally instead of waiting for me to kick off the CI/CD is to run locally

TEST_FILES=BuildTest pnpm -w run test-linux

@indutny-signal
Copy link
Contributor Author

@mmaietta downgraded. Looks like 1.7.0 was the last CJS release.

I just tried the test command locally and got:

    /project/node_modules/.pnpm/[email protected]/node_modules/app-builder-bin/linux/x64/app-builder process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE
    Exit code:
    2

      250 |       }
      251 |     } else {
    > 252 |       reject(new ExecError(command, code, out, errorOut))
          |              ^
      253 |     }
      254 |   })
      255 | }

@indutny-signal
Copy link
Contributor Author

Ah, the true error was above:

  ⨯ cannot execute  cause=signal: segmentation fault
                    errorOut=qemu: uncaught target signal 11 (Segmentation fault) - core dumped

                    command=wine /root/.cache/electron-builder/winCodeSign/winCodeSign-2.6.0/rcedit-ia32.exe '/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-AhvwVh/test-project-8/dist/win-unpacked/Test App ßW.exe' --set-version-string FileDescription 'Test Application (test quite “ #378)' --set-version-string ProductName 'Test App ßW' --set-version-string LegalCopyright 'Copyright © 2024 Foo Bar' --set-file-version 1.1.0.42 --set-product-version 1.1.0.42 --set-version-string InternalName 'Test App ßW' --set-version-string OriginalFilename '' --set-version-string CompanyName 'Foo Bar' --set-icon /tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-AhvwVh/test-project-8/build/icon.ico
                    workingDir=

@mmaietta mmaietta merged commit 13e0e0d into electron-userland:master Jun 8, 2024
13 checks passed
@indutny-signal indutny-signal deleted the feature/win-asar-integrity branch June 8, 2024 15:29
@indutny-signal
Copy link
Contributor Author

Thank you!

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.

2 participants