Skip to content

feat: add integrity digest management commands and functionality - #380

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

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

Conversation

@nmggithub

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@nmggithub

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/integrity-digest.ts Outdated
@socket-security

socket-security Bot commented Feb 25, 2026

Copy link
Copy Markdown

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
Copy Markdown
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.

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

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

Copy link
Copy Markdown
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.

Comment thread src/integrity-digest.ts

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

function calculateIntegrityDigestV1(asarIntegrity: AsarIntegrity): IntegrityDigestV1 {

Copy link
Copy Markdown
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?

@nmggithub nmggithub Feb 25, 2026

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

Comment thread src/integrity-digest.ts
);
}

// High-level integrity digest management functions

Copy link
Copy Markdown
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.

@nmggithub nmggithub Feb 25, 2026

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

Comment thread src/integrity-digest.ts
Comment thread bin/asar.mjs Outdated
@nmggithub
nmggithub requested a review from erickzhao February 25, 2026 22:02

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

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?

Comment thread src/integrity-digest.ts Outdated
Comment thread src/integrity-digest.ts Outdated
@nmggithub
nmggithub requested a review from erickzhao March 4, 2026 01:00

@dsanders11 dsanders11 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread test/integrity-digest-integration-spec.ts Outdated
Comment thread src/integrity-digest.ts Outdated
Comment thread src/integrity-digest.ts Outdated
Comment thread bin/asar.mjs Outdated
@nmggithub
nmggithub requested a review from dsanders11 March 4, 2026 07:33
@socket-security

socket-security Bot commented Mar 4, 2026

Copy link
Copy Markdown

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

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

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) {

@erickzhao erickzhao Mar 4, 2026

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@erickzhao

Copy link
Copy Markdown
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

Copy link
Copy Markdown

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

erickzhao added a commit that referenced this pull request Mar 31, 2026
erickzhao added a commit that referenced this pull request Mar 31, 2026
…tional… (#424)

* Revert "feat: add integrity digest management commands and functionality (#380)"

This reverts commit ccaf57f.

* revert the picomatch change
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