Skip to content

Conversation

@mmaietta
Copy link
Contributor

@mmaietta mmaietta commented Feb 19, 2025

  • When an application uses multiple asars (webapp.asar, anything.asar, etc.), EnableEmbeddedAsarIntegrityValidation fuse breaks the application due to not all asars having integrity generated for them. Fixes: ASAR Integrity assumes a single asar file is present #116
  • Also fixes bug to correctly test makeUniversalApp no asar mode should shim two different app folders, (it was not having an asar integrity generated for the shimmed asar)

Functionality added:

  • Moves all asar integrity generation to after all app assets have been merged/shimmed/copied. This allows other asars that were provided to also be scanned and have asar integrity generated for them.
  • Extracted common Integrity logic to a single file integrity.ts
  • Adds unit test for multi-asar apps

…tyValidation` fuse breaks the application due to not all ASARs having integrity generated for them. Fixes: electron#116
@mmaietta mmaietta marked this pull request as ready for review February 21, 2025 01:20
@mmaietta mmaietta requested a review from a team as a code owner February 21, 2025 01:20
@mmaietta mmaietta force-pushed the multi-asar-integrity branch from e37218f to 61eb5ac Compare February 22, 2025 08:16
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM

@BlackHole1 BlackHole1 merged commit 2b67c90 into electron:main Feb 28, 2025
5 checks passed
@continuous-auth
Copy link

🎉 This PR is included in version 2.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mmaietta mmaietta deleted the multi-asar-integrity branch February 28, 2025 03:29
}
}
if (p.includes('app.asar')) {
if (p.endsWith('.asar')) {
Copy link

@valeriangalliat valeriangalliat Apr 29, 2025

Choose a reason for hiding this comment

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

@BlackHole1 this changed the way some resources are identified i.e. before:

  {
    "relativePath": "Contents/Resources/app.asar",
    "type": 4
  },
  {
    "relativePath": "Contents/Resources/app.asar.unpacked/node_modules/fsevents/fsevents.node",
    "type": 4
  },

And after:

  {
    "relativePath": "Contents/Resources/app.asar",
    "type": 4
  },
  {
    "relativePath": "Contents/Resources/app.asar.unpacked/node_modules/fsevents/fsevents.node",
    "type": 0
  },

(4 = APP_CODE and 0 = MACHO)

I'm not sure if that's intentional or not but in the second case, it's considering fsevents.node as MACHO instead of APP_CODE and it leads into running this check here for this file where it wouldn't hit it before, leading to this error for Electron apps that includes the fsevents package and other packages with universal .node binaries:

Error: Detected file "Contents/Resources/app.asar.unpacked/node_modules/fsevents/fsevents.node" that's the same in both x64 and arm64 builds and not covered by the x64ArchFiles rule

Do you think in the above example app.asar.unpacked/.../fsevents.node should be identified as APP_CODE or MACHO? Trying to figure out what a fix for this would look like

If it should stay APP_CODE, maybe doing if (p.includes('.asar')) is a good enough fix..., or maybe p.endsWith('.asar') || p.includes('.asar.unpacked/') to be stricter

Edit: made a PR #133 with a tentative fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree that all contents in unpacked folder should be APP_CODE, as .node is a macho file. I think your issue being reported here:

other packages with universal .node binaries:

Should be resolved through #126 though

Choose a reason for hiding this comment

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

Oh, even better! I'll watch that PR and pin 2.0.1 in the meantime then, thanks for the pointer 👌

valeriangalliat added a commit to valeriangalliat/electron-universal that referenced this pull request Apr 29, 2025
PR electron#124 introduced a regression where the following path:

    Contents/Resources/app.asar.unpacked/node_modules/fsevents/fsevents.node

Would get identified as `AppFileType.MACHO` instead of
`AppFileType.APP_CODE` as it was previously

We need to make sure that path containing `.asar.unpacked/` are also
treated as `APP_CODE`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ASAR Integrity assumes a single asar file is present

3 participants