feat(suite-desktop): turn on ASAR integrity check via afterPack hook #17050
Conversation
|
🚀 Expo preview is ready!
|
Lemonexe
force-pushed
the
feat/ASAR-verification-afterPack-hook
branch
from
e786e79 to
f4017bd
Compare
WalkthroughThe changes introduce the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/scripts/setElectronFuses.js (1)
4-9: Consider adding source URL as a constant.The platform mapping is correctly implemented, but the source URL in the comment could be defined as a constant for better maintainability.
+const PLATFORM_MAPPING_SOURCE = 'https://github.com/electron-userland/electron-builder/blob/04be5699c664e6a93e093b820a16ad516355b5c7/packages/app-builder-lib/src/platformPackager.ts#L430-L434'; -// copied from https://github.com/electron-userland/electron-builder/blob/04be5699c664e6a93e093b820a16ad516355b5c7/packages/app-builder-lib/src/platformPackager.ts#L430-L434 +// copied from ${PLATFORM_MAPPING_SOURCE}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/suite-desktop-core/package.json(1 hunks)packages/suite-desktop-core/scripts/setElectronFuses.js(1 hunks)packages/suite-desktop/electron-builder-config.js(1 hunks)packages/suite-desktop/package.json(1 hunks)scripts/list-outdated-dependencies/foundation-dependencies.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: build-deploy
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (7)
packages/suite-desktop-core/scripts/setElectronFuses.js (3)
1-2: LGTM! Dependencies are correctly imported.The required dependencies from
@electron/fusesandpathare properly imported.
14-24: LGTM! Platform handling is well-documented and implemented.The code correctly implements Windows-only ASAR integrity check with clear documentation about platform support status.
33-37:✅ Verification successful
Verify the fuse options are secure and up-to-date.
The implementation uses two fuse options:
- EnableEmbeddedAsarIntegrityValidation
- OnlyLoadAppFromAsar
Let's verify these are the recommended settings for security.
🌐 Web query:
What are the recommended fuse options for @electron/fuses in Electron 34?💡 Result:
For Electron 34 applications using
@electron/fuses, these security-focused configurations are recommended based on industry practices and official documentation:Core Security Fuses
RunAsNode: false
DisablesELECTRON_RUN_AS_NODEenvironment variable usage[3][6]
EnableNodeOptionsEnvironmentVariable: false
BlocksNODE_OPTIONSmanipulation[6]
EnableNodeCliInspectArguments: false
Disables debug arguments like--inspect[3][6]ASAR Integrity & Validation
EnableEmbeddedAsarIntegrityValidation: true
Enforces macOS app.asar validation[6]
OnlyLoadAppFromAsar: true
Restricts app loading exclusively from ASAR[6]Advanced Configurations
const { FuseV1Options } = require('@electron/fuses'); // Recommended Forge plugin configuration new FusesPlugin({ version: FuseVersion.V1, [FuseV1Options.GrantFileProtocolExtraPrivileges]: false, [FuseV1Options.LoadBrowserProcessSpecificV8Snapshot]: true, resetAdHocDarwinSignature: true // Required for Apple Silicon builds })Key considerations:
- Use
strictlyRequireAllFuses: trueto future-proof configurations[6]- Validate fuse states post-build with:
npx @electron/fuses read --app dist/your-app.app- Combine with code signing for tamper protection[2][3]
These settings balance security with functionality while maintaining Electron 34's enhanced sandboxing capabilities[10]. Implementation details vary slightly between packaging tools (Forge vs direct CLI)[2][9].
Citations:
- 1: https://www.eaton.com/content/dam/eaton/products/electrical-circuit-protection/fuses/bussmann-series-high-speed-fuses/bus-ele-cat-10506-hsf.pdf
- 2: https://www.electronforge.io/config/plugins/fuses
- 3: https://electronjs.org/docs/latest/tutorial/fuses
- 4: https://www.swe-check.com.au/pdfs/fuse_bible_complete.pdf
- 5: https://www.e34chassis.com/fuse-and-relay-diagrams/
- 6: https://www.npmjs.com/package/@electron/fuses
- 7: Electron Fuses support electron-userland/electron-builder#6365
- 8: https://www.sandc.com/en/products--services/products/sm-power-fuses/
- 9: https://classic.yarnpkg.com/en/package/@electron-forge/plugin-fuses
- 10: https://electronjs.org/blog
ASAR Fuse Options Are Secure
The fuse options set at lines 33–37 in
packages/suite-desktop-core/scripts/setElectronFuses.js—namely,[FuseV1Options.EnableEmbeddedAsarIntegrityValidation]: trueand[FuseV1Options.OnlyLoadAppFromAsar]: true—match the recommended configurations for ASAR integrity and validation in Electron 34. These settings enforce the necessary integrity checks and restrict app loading solely from ASAR archives, aligning with industry best practices.
- The web query confirms that for ASAR security, these two fuse options are the recommended settings.
- Note: For broader security hardening, consider reviewing additional core fuses (such as
RunAsNode: false,EnableNodeOptionsEnvironmentVariable: false, andEnableNodeCliInspectArguments: false) based on your application’s overall security strategy.packages/suite-desktop/electron-builder-config.js (1)
165-165: LGTM! afterPack hook is correctly configured.The afterPack hook is properly configured to use the setElectronFuses.js script.
scripts/list-outdated-dependencies/foundation-dependencies.txt (1)
27-27: LGTM! Dependency correctly added to foundation dependencies.@electron/fuses is properly added under the DESKTOP section.
packages/suite-desktop/package.json (1)
33-33:✅ Verification successful
Verify @electron/fuses version compatibility.
Let's ensure version ^1.8.0 is compatible with Electron 34.1.0 used in the project.
🌐 Web query:
What is the latest version of @electron/fuses and is version 1.8.0 compatible with Electron 34?💡 Result:
The latest version of
@electron/fusesis 1.8.0 as confirmed by npm registry entries from March 2024[4][10]. This version remains compatible with Electron 34 (released January 2025) based on evidence from official documentation:Compatibility Details
Electron Forge Integration
The Fuses plugin explicitly supports@electron/fusesv1.x with Electron 34 through its configuration system[1]. Example code demonstrates usingFuseVersion.V1andFuseV1Optionsin Electron 34 projects[1].API Consistency
No breaking changes were introduced in Electron 34's fuse handling system[3][6]. The fuse options used in v1.8.0 (RunAsNode,EnableCookieEncryption, etc.) remain valid[4][7].Security Considerations
The package includes special handling for Apple Silicon macOS builds, which remains relevant in Electron 34's arm64 distribution model[4][7].For new projects using Electron 34, you can safely install with:
npm install @electron/[email protected]Citations:
- 1: https://www.electronforge.io/config/plugins/fuses
- 2: https://electronjs.org/blog
- 3: https://electronjs.org/blog/electron-34-0
- 4: https://www.npmjs.com/package/@electron/fuses
- 5: Electron Fuses support electron-userland/electron-builder#6365
- 6: https://github.com/electron/electron/releases/
- 7: https://www.electron.build/tutorials/adding-electron-fuses.html
- 8: https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/scheme.json
- 9: https://releases.electronjs.org/releases
- 10: https://www.npmjs.com/package/@electron/fuses/v/1.4.0
@electron/fuses version validated.
The dependency is set to "^1.8.0", which is confirmed as the latest release and fully compatible with Electron 34.1.0. No changes are required.packages/suite-desktop-core/package.json (1)
50-50: New Dependency Addition: "@electron/fuses"
The new dev dependency"@electron/fuses": "^1.8.0"has been added to support the ASAR integrity check via an afterPack hook. This aligns with the PR objective, ensuring that the fuse settings are applied for Windows builds. Please confirm that using a dev dependency is appropriate for your build process and that the version aligns with the project's compatibility and security requirements.
| @@ -162,5 +162,6 @@ module.exports = { | |||
| category: 'Utility', | |||
| target: ['AppImage'], | |||
| }, | |||
| afterPack: '../suite-desktop-core/scripts/setElectronFuses.js', | |||
There was a problem hiding this comment.
Wait, why is the new script .js, but the other two are .ts? (notarize and sign-windows)
Because they're not really typescript! They have the extension, but they are written in CJS.
It's actually quite a mess.
That rhymes!
Anyway, in next PR I'll turn on TS (ofc it's not running currently) so you can look forward to me valiantly fighting skeletons in closet 💀
|
Idk why the playwright test keeps failing, I rerun it manually from this branch and it runs ✔️ |
Description
After the original implementation #16951 was reverted in #16972 because of failing macOS builds, let's just turn on the ASAR integrity for Windows, for the time being.
The previous attempt was very simple - just several lines of config for electron-builder, which takes care of flipping the electron fuses during build for all platforms (but it takes effect only on macOS & Windows).
If we want to turn on the fuses only for selected platform(s), we need to do it manually via an
afterPackhook.Related Issue
Resolve https://github.com/trezor/trezor-suite-private/issues/16
QA instructions
@coderabbitai ignore