fix(deps): upgrade electron-builder security patches - #243
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe pull request updates the ChangesElectron Builder Dependency Update
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
The security case holds and the lockfile is honest — I checked both rather than taking them on faith.
26.15.3 remediates GHSA-7g7r-gx96-252g, and electron-builder.json5:99 builds an AppImage that build.yml ships on every GitHub Release, so this lands on an artifact users actually download. Not hygiene.
I was more worried about the 975-line lockfile churn, so I diffed the package set against the live registry. 7zip-bin, app-builder-bin, dmg-license and iconv-corefoundation drop out; pkijs, asn1js, unzipper and @noble/hashes come in. That is upstream moving code signing off native binaries onto JS, not a bad regeneration. Your ajv 6→8 jump falls out of the same chain. Good.
Three things before this can land:
npmDepsHashinnix/package.nixwas not regenerated —nix-check.ymlwill fail. Details inline.- This conflicts with #244 on both
package.jsonandpackage-lock.json. #244 goes first; rebase on top of it. - The body says the upgrade is "from
26.14.0". Main's lockfile resolveselectron-builderandapp-builder-libat26.8.1(builder-util-runtimeat9.5.1). Worth correcting — the real jump is bigger than advertised, which is also why I want the packaging check below.
One packaging risk I'd like verified by hand before merge, flagged inline.
|
I approved CI on this branch and the Nix job failed exactly where I expected, so here is the value rather than making you run the prefetch: Set One caveat: do this after rebasing on #244, not now. That PR rewrites the same lockfile, so this hash will be stale the moment it lands and the Nix job will just fail again on the new resolution. |
26.8.1 -> 26.15.3 remediates GHSA-7g7r-gx96-252g, which is a property of the AppImage this project builds and ships on every GitHub Release and downstream to AUR — so it lands on an artifact users actually download, not just on the build host. The lockfile churn is upstream moving code signing off vendored native binaries onto JS: 7zip-bin, app-builder-bin, dmg-license and iconv-corefoundation drop out, pkijs, asn1js, unzipper and @noble/hashes come in. Verified against the registry rather than taken on faith — dmg-builder and builder-util at 26.15.3 declare exactly what the lockfile records, and every resolved URL is registry.npmjs.org. Regenerated on top of the dev-toolchain bump rather than merged with it: the two PRs re-resolve the same file independently, so combining the two lockfiles textually would have produced a tree neither of them describes. package.json carries both sets of bumps and npm resolved the result. Audit goes 15 -> 4. electron-builder.json5's deb.depends copies app-builder-lib's FpmTarget.getDefaultDepends verbatim, and `depends` replaces rather than appends, so the .deb and pacman artifacts want a build-and-install smoke test before this ships — that list was written against 26.8.x and this crosses seven minors.
b9aa61a to
4d86d74
Compare
The value getopenscreen#244 left behind describes the dependency set before electron-builder moved to 26.15.3, and that upgrade re-resolves a lot: 7zip-bin, app-builder-bin, dmg-license and iconv-corefoundation drop out while pkijs, asn1js, unzipper and @noble/hashes come in, because upstream moved code signing off vendored native binaries onto JS. So the recorded hash no longer matches and `nix build` fails outright for anyone installing through that channel. Taken from the Nix job's own output on this branch rather than computed blind.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Threads cleared, npmDepsHash refreshed, rebased onto #244 with the lockfile regenerated rather than textually merged. Approving.
The security case is real and I verified it rather than taking the title on faith: 26.15.3 remediates GHSA-7g7r-gx96-252g, and electron-builder.json5 builds an AppImage that ships on every GitHub Release and downstream to AUR. The 800-line lockfile churn is upstream moving code signing off vendored native binaries onto JS, checked against the registry — not a bad regeneration.
Audit across both dependency PRs: 21 on main, 15 after #244, 4 with this on top.
Carrying forward one thing that is not resolved by merging: deb.depends in electron-builder.json5 copies app-builder-lib defaults verbatim and replaces rather than appends, and that copy was made against 26.8.x. The .deb and pacman artifacts want an install smoke test before the next release.
3 participants
Summary
electron-builderfrom26.14.0resolved to26.15.3app-builder-lib@26.15.3andbuilder-util-runtime@9.7.0The audit drops from 21 findings (4 critical, 15 high, 2 low) to 7 findings (1 critical, 4 high, 2 low), with no remaining builder/publisher findings.
Related issue
No linked issue; this follows currently published npm/GitHub advisories.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; packaging dependency update only.
Testing
npm exec -- electron-builder --version→26.15.3npm audit --json→ no remaining electron-builder/app-builder-lib/builder-util findingselectron/recording/webm-seek-index.test.tsassertions are Windows-only and returnunsupported-platformon macOSnpm run build-vitenpm run wb:typechecknpm run lint(passes with the repository's 11 existing warnings)Summary by CodeRabbit