Skip to content

feat: add integrity digest management commands and functionality#380

Merged
VerteDinde merged 34 commits intoelectron:mainfrom
nmggithub:main
Mar 5, 2026
Merged

feat: add integrity digest management commands and functionality#380
VerteDinde merged 34 commits intoelectron:mainfrom
nmggithub:main

Conversation

@nmggithub
Copy link
Contributor

This is to add support for the additional integrity digest API I am adding in this pull request: electron/electron#48587. I believe this package is the best place to put this functionality. Once merged, this functionality can be pulled into tools like Electron Forge as needed.

@nmggithub
Copy link
Contributor Author

@VerteDinde I saw the Prettier message in the CI run. Sorry about that! Should be good to go now.

@nmggithub
Copy link
Contributor Author

Just curious, when do we want to merge this in? From what I understand, the changes this interacts with are in Electron 41, which just recently had its Alpha release. Do we want to wait until the Beta, or even the Stable release? Or can we merge it in now?

@erickzhao
Copy link
Member

Ah, thanks for the reminder ping @nmggithub. cc @electron/wg-ecosystem this needs a review and we should get a new release out before the next stable version drops.

@socket-security
Copy link

socket-security bot commented Feb 25, 2026

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

@erickzhao
Copy link
Member

I pushed up a few commits to apply yarn.lock changes myself in accordance to our policies and then merged in main and fixed the merge conflicts.

bin/asar.mjs Outdated
extractAll(archive, dest)
})

program.command('integrity-digest <app> <command>')
Copy link
Member

Choose a reason for hiding this comment

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

note(non-blocking): I don't think this necessarily needs to land in this PR, but since this adds a new command, we should update the usage section of the README as well (

asar/README.md

Lines 26 to 53 in 13bb932

### Usage
```bash
$ asar --help
Usage: asar [options] [command]
Commands:
pack|p <dir> <output>
create asar archive
list|l <archive>
list files of asar archive
extract-file|ef <archive> <filename>
extract one file from archive
extract|e <archive> <dest>
extract archive
Options:
-h, --help output usage information
-V, --version output the version number
```
)

I think it'd make sense to both update the list of commands and add a new section with more prose on usage.


type AsarIntegrity = Record<string, Pick<FileRecord['integrity'], 'algorithm' | 'hash'>>;

function calculateIntegrityDigestV1(asarIntegrity: AsarIntegrity): IntegrityDigestV1 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: why is this put into a separate function?

Copy link
Contributor Author

@nmggithub nmggithub Feb 25, 2026

Choose a reason for hiding this comment

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

For separation in case we ever have a V2. I'd rather not inline it in the later switch statement. EDIT: Just realized I'm not switching on this one specifically. Will consider this more. EDIT 2: if you were questioning why its a separate function than the one below, I believe I did that to codify the explicit conversion between Info.plist and the resulting integrity hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted in e7ae2d0.

);
}

// High-level integrity digest management functions
Copy link
Member

Choose a reason for hiding this comment

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

issue(blocking): If we want these to be usable via the JavaScript API (for integration in other tools), we would need to re-export them via src/asar.ts as well.

It would also be helpful to add TSDoc comments at least to the functions that get exported so that they can appear in the API docs as well.

Copy link
Contributor Author

@nmggithub nmggithub Feb 25, 2026

Choose a reason for hiding this comment

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

Ah yeah, I think I may have also accidentally un-exported some important ones. Will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up using the functions above as API in 0677da2.

@nmggithub nmggithub requested a review from erickzhao February 25, 2026 22:02
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

If it's possible, can we add tests to this PR as well?

@nmggithub nmggithub requested a review from erickzhao March 4, 2026 01:00
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Few small comments but overall LGTM.

@nmggithub nmggithub requested a review from dsanders11 March 4, 2026 07:33
@socket-security
Copy link

socket-security bot commented Mar 4, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​plist@​3.0.51001007380100
Added@​types/​semver@​7.7.11001007581100
Added@​electron/​fuses@​2.1.01001009581100
Addedplist@​3.1.010010010082100
Added@​electron/​get@​4.0.29910010082100
Addedsemver@​7.7.410010010090100

View full report

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

LGTM


const electronVersion = await getTargetElectronVersion();
const parsedElectronVersion = semver.parse(electronVersion);
if (!parsedElectronVersion || 41 > parsedElectronVersion.major) {
Copy link
Member

@erickzhao erickzhao Mar 4, 2026

Choose a reason for hiding this comment

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

Suggested change
if (!parsedElectronVersion || 41 > parsedElectronVersion.major) {
if (!parsedElectronVersion || parsedElectronVersion.major < 41) {

nit: style (reads a bit better)

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 chose the number-first pattern on purpose for style, but I can see your point here. The only issue is that your new logic does not handle a case where parsedElectronVersion is undefined. I will make some changes to this, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as of 47b17c3.

Copy link
Member

Choose a reason for hiding this comment

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

oops sorry, had a slight typo in the original suggestion diff as well

nmggithub and others added 2 commits March 4, 2026 16:49
@VerteDinde
Copy link
Member

Sorry @nmggithub, if you wouldn't mind reconciling the small changes with main, I can get this merged for you

@erickzhao
Copy link
Member

@VerteDinde I can handle those since they're related to dependencies on a PR I just merged in

@VerteDinde VerteDinde merged commit ccaf57f into electron:main Mar 5, 2026
6 checks passed
@electron-npm-package-publisher

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants