nwjs, nwjs084: added versioning for the nwjs package #310809
Conversation
pkgs/top-level/all-packages.nix
Outdated
| @@ -19221,7 +19221,25 @@ with pkgs; | |||
|
|
|||
| norminette = callPackage ../development/tools/norminette { }; | |||
|
|
|||
| nwjs = callPackage ../development/tools/nwjs { }; | |||
| nwjs = callPackage ../development/tools/nwjs { | |||
| version = "0.85.0"; | |||
There was a problem hiding this comment.
Is there a reason nwjs is reverted to 0.85.0? Only betaflight-configurator has issues prompting the need for 0.84.0 correct?
There was a problem hiding this comment.
Also all-packages may not be the best place for this, perhaps nwjs can be reworked to allow overrideAttrs?
There was a problem hiding this comment.
rolling back to 0.85 was just a mistake, I've fixed it now
There was a problem hiding this comment.
A number of packages were broken by an upgrade to 0.85:
blheli-configurator (new package, https://github.com/NixOS/nixpkgs/pull/310820)
betaflight-configurator
inav-blackbox-explorer (new package, https://github.com/NixOS/nixpkgs/pull/310813)
Probably betaflight-blackbox-explorer is affected, too (I haven't packaged it yet)
There was a problem hiding this comment.
regarding overrideAttrs, I don't see how it's possible to make it work without code duplication. We're interpolating the version in the url, so we'd have to do it again with a new value for version.
I made version and hashes the same kind of input as sdk. Also, python uses a similar approach to allow for multiple versions
a265f95
to
c57c296
Compare
|
I think the general idea is fine, but it also leaves me with some questions. Wouldn't it be better if the package was built from scratch? Because nwjs uses chromium it's susceptible to some of the same vulnerabilities as chrome and by keeping the old binaries as an option we would have to determine which is vulnerable to what CVE and to my knowledge we wouldn't be able to patch those. Edit: |
|
nwjs doesn't appear to backport chromium fixes like electron, so we'd be on our own trying to maintain something as big as that. nwjs from source would be nice, but just looking at the build instructions these are big projects and who knows if hydra may be able to handle it. Ideally upstream for these packages should be notified new versions of nwjs don't work anymore. It's probably safer to just mark these as broken or let users override an older version with an overlay at their own risk. |
|
Okay kinda had the feeling that it just wouldn't be that easy. But, then we agree that leaving this package as is and fixing/mark the packages that may be broken from upgrades of nwjs is the best solution? |
c57c296
to
95c5552
Compare
|
@eclairevoyant what should we do with the |
|
Another option could be to add 0.84.0 with it already marked as insecure
which requires the user to override to build. 0.84.0 itself doesn’t seem to
have any specific vulnerabilities except for using older chromium and
nodejs which aren’t getting patched by nwjs. But could these packages using
vulnerable nwjs be exploited and how so?
Just not sure nixpkgs policy regarding this.
…On Sun, May 12, 2024 at 15:08 Mikael Fangel ***@***.***> wrote:
Okay kinda had the feeling that it just wouldn't be that easy.
But, then we agree that leaving this package as is and fixing/mark the
packages that may be broken from upgrades of nwjs is the best solution?
—
Reply to this email directly, view it on GitHub
<#310809 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BC642ODU72PGQA4CHT6NYFLZB64ZZAVCNFSM6AAAAABHRZ7MJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGM2DMNJTGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Doesn't marking something as insecure require a CVE or eqv.? Also, the packages I'm fixing by the downgrade don't even access the internet (almost), don't open arbitrary files etc. So most probably not exploitable regardless of the vulnerability. |
|
I think since nwjs doesn't maintain backwards compatibility we just have to provide multiple versions. It should be easier once they hit 1.0 (that is if they follow semantic versioning) |
You're correct but because nwjs includes both Chromium and NodeJS it could be susceptible to the same CVE's as both NodeJS and Chromium. Previously we have marked packages including either with their projects' CVE's. |
95c5552
to
489d91d
Compare
|
@qubitnano @MikaelFangel guys, sorry for bumping the conversation but several packages are now broken and several others can't be merged because they're blocked on this change. I'm happy to refactor this but I'd rather do something right now to fix the outstanding issues |
|
I personally wouldn't use all-packages for the hashes. nwjs could be migrated to by-name and finalAttrs, and the nwjs084 package could then simply just use overrideAttrs to set version 0.84.0 and the old hashes. It needs to be by-name anyway per CI. |
|
@qubitnano it's not currently compatible with |
|
Let's just roll back nwjs to 0.84 then. A number of packages are broken by a system-wide upgrade. |
|
I have created a pr reverting the breaking change against 23.11. See: #315071 |
489d91d
to
57cba66
Compare
57cba66
to
64023e3
Compare
64023e3
to
1b8dd2c
Compare
Description of changes
Added nwjs084 package, as later versions break some software, including:
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.