feat: add integrity digest management commands and functionality#380
feat: add integrity digest management commands and functionality#380VerteDinde merged 34 commits intoelectron:mainfrom
Conversation
|
@VerteDinde I saw the Prettier message in the CI run. Sorry about that! Should be good to go now. |
|
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? |
|
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. |
|
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. |
|
I pushed up a few commits to apply |
bin/asar.mjs
Outdated
| extractAll(archive, dest) | ||
| }) | ||
|
|
||
| program.command('integrity-digest <app> <command>') |
There was a problem hiding this comment.
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 (
Lines 26 to 53 in 13bb932
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 { |
There was a problem hiding this comment.
nit: why is this put into a separate function?
There was a problem hiding this comment.
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.
| ); | ||
| } | ||
|
|
||
| // High-level integrity digest management functions |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah yeah, I think I may have also accidentally un-exported some important ones. Will take a look.
There was a problem hiding this comment.
Ended up using the functions above as API in 0677da2.
erickzhao
left a comment
There was a problem hiding this comment.
If it's possible, can we add tests to this PR as well?
dsanders11
left a comment
There was a problem hiding this comment.
Few small comments but overall LGTM.
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
|
||
| const electronVersion = await getTargetElectronVersion(); | ||
| const parsedElectronVersion = semver.parse(electronVersion); | ||
| if (!parsedElectronVersion || 41 > parsedElectronVersion.major) { |
There was a problem hiding this comment.
| if (!parsedElectronVersion || 41 > parsedElectronVersion.major) { | |
| if (!parsedElectronVersion || parsedElectronVersion.major < 41) { |
nit: style (reads a bit better)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
oops sorry, had a slight typo in the original suggestion diff as well
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
|
Sorry @nmggithub, if you wouldn't mind reconciling the small changes with main, I can get this merged for you |
|
@VerteDinde I can handle those since they're related to dependencies on a PR I just merged in |
|
🎉 This PR is included in version 4.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.