Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

πŸ™ˆοΈ Remove webkitgtk24x #75040

Merged
merged 9 commits into from Dec 7, 2019

Conversation

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 5, 2019

Motivation for this change

I hope it goes without saying, but please πŸ˜‚οΈ

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@worldofpeace worldofpeace requested a review from jtojnar Dec 5, 2019
@worldofpeace worldofpeace changed the title Remove webkitgtk24x πŸ™ˆοΈ πŸ™ˆοΈ Remove webkitgtk24x Dec 5, 2019
@worldofpeace worldofpeace added this to To do in Picking up garbage via automation Dec 5, 2019
@worldofpeace worldofpeace moved this from To do to In progress in Picking up garbage Dec 5, 2019
@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

#18312 basically did this in an elegant way, but we can for sure remove this now.

@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from 541e8d0 to 9568d61 Dec 5, 2019
@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

#18312 basically did this in an elegant way, but we can for sure remove this now.

@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from 9568d61 to b32ce1f Dec 5, 2019
worldofpeace added 4 commits Dec 5, 2019
It uses insecure webkitgtk24x.
You know cannot enable optional withWebKit when
withGtk2 is enabled.
@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from b32ce1f to 7822d83 Dec 5, 2019
@worldofpeace worldofpeace mentioned this pull request Dec 5, 2019
0 of 10 tasks complete
@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from 7822d83 to 7ecd894 Dec 5, 2019
@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 5, 2019

See mmex in all-packages

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

@jtojnar Running unpackPhase on emacs25 shows

  WEBKIT_REQUIRED=1.4.0
  WEBKIT_MODULES="webkitgtk-3.0 >= $WEBKIT_REQUIRED"

So I think the source in nixpkgs predates this commit.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

See mmex in all-packages

Did 3e1ee5b.
This reminds me wxGTK needs to default to gtk3 #73145, but it's pretty tedious TBH.

@ofborg ofborg bot added the 8.has: clean-up label Dec 5, 2019
@ofborg ofborg bot requested review from globin, viric and fpletz Dec 5, 2019
uzbl/uzbl#408

Frightening version of webkitgtk used here.
πŸ˜€. This for sure has been superseded in the present.
Nothing uses this in nixpkgs also.
It goes without saying that we should remove this πŸ˜…οΈ.
@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from 3e1ee5b to 06553f3 Dec 5, 2019
@ofborg ofborg bot requested review from peti, jwiegley, the-kenny and lovek323 Dec 5, 2019
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 5, 2019

https://git.savannah.gnu.org/cgit/emacs.git/commit/src/xwidget.c?id=a36ed9b5e95afea5716256bac24d883263aefbaf seems to build:

diff --git a/pkgs/applications/editors/emacs/25.nix b/pkgs/applications/editors/emacs/25.nix
index f3989be52c0..15e9fbdd726 100644
--- a/pkgs/applications/editors/emacs/25.nix
+++ b/pkgs/applications/editors/emacs/25.nix
@@ -48,7 +48,16 @@ stdenv.mkDerivation rec {
     })
   ] ++ [
     # Backport patch so we can use webkitgtk with xwidgets.
-    ./0001-xwidget-Use-WebKit2-API.patch
+    (fetchurl {
+      name = "0001-Omit-unnecessary-includes-from-xwidget-c.patch";
+      url = "https://git.savannah.gnu.org/cgit/emacs.git/patch?id=a36ed9b5e95afea5716256bac24d883263aefbaf";
+      sha256 = "0a4ax1kjcn6g4c59a0wikplx47r6r3lh5igc9vrlqkpk5lw30j0j";
+    })
+    (fetchurl {
+      name = "0002-xwidget-Use-WebKit2-API.patch";
+      url = "https://git.savannah.gnu.org/cgit/emacs.git/patch?id=d781662873f228b110a128f7a2b6583a4d5e0a3a";
+      sha256 = "1m2fnblsk4dg3scxiawz6vsa83bvyg9bxis9mhrjd5f7cdmcnn4z";
+    })
   ];
 
   nativeBuildInputs = [ pkgconfig autoconf automake texinfo ]
@worldofpeace worldofpeace force-pushed the worldofpeace:remove-terror-webkitgtk branch from 06553f3 to eda1ea5 Dec 5, 2019
@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 5, 2019

@jtojnar Fetched those patches. I had to do it from the GitHub mirror because of the ongoing DOS (or maybe I'm blacklisted).

@jtojnar
jtojnar approved these changes Dec 5, 2019
@worldofpeace worldofpeace merged commit 0c8dadd into NixOS:master Dec 7, 2019
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
claws-mail, emacs25, mmex, mu on aarch64-linux Success
Details
claws-mail, emacs25, mmex, mu on x86_64-darwin Success
Details
claws-mail, emacs25, mmex, mu on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Picking up garbage automation moved this from In progress to Done Dec 7, 2019
@worldofpeace worldofpeace deleted the worldofpeace:remove-terror-webkitgtk branch Dec 7, 2019
@orivej
Copy link
Contributor

@orivej orivej commented Dec 8, 2019

Claws Mail will build with a supported WebKit once it switches to GTK 3, which seems to progress well:
https://git.claws-mail.org/?p=claws.git;a=shortlog;h=refs/heads/gtk3
https://git.claws-mail.org/?p=claws.git;a=history;f=src/plugins/fancy;hb=refs/heads/gtk3
Meanwhile Claws Mail 3.17.4 has added a new litehtml-based HTML renderer plugin. I suppose I'll try to package it, since lack of HTML support noticebly reduces its usability. Alternatively it would make sense to provide a gtk3 branch snapshot of Claws Mail.
EDIT: Note that Claws Mail uses webkitgtk with javascript disabled, and therefore does not put its users at risk of known security issues.

@orivej
Copy link
Contributor

@orivej orivej commented Dec 9, 2019

I have tried the litehtml plugin β€” it is as simple as adding gumbo to claws-mail build inputs (and enabling it in Configuration > Plugins) β€” but it's rendering is very bad, with huge spacing between words and lines, and the text can not be selected and copied.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Dec 9, 2019

Claws Mail will build with a supported WebKit once it switches to GTK 3, which seems to progress well:
https://git.claws-mail.org/?p=claws.git;a=shortlog;h=refs/heads/gtk3
https://git.claws-mail.org/?p=claws.git;a=history;f=src/plugins/fancy;hb=refs/heads/gtk3
Meanwhile Claws Mail 3.17.4 has added a new litehtml-based HTML renderer plugin. I suppose I'll try to package it, since lack of HTML support noticebly reduces its usability. Alternatively it would make sense to provide a gtk3 branch snapshot of Claws Mail.

It seems the only mostly working solution would be a gtk3 variant package. Though it's not good to distribute non-released/non-production code (some people even request that you don't), it's an option.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 9, 2019

EDIT: Note that Claws Mail uses webkitgtk with javascript disabled, and therefore does not put its users at risk of known security issues.

While disabling JavaScript will reduce the attack surface greatly, I doubt JavaScriptCore is the only vulnerable component.

orivej-nixos pushed a commit that referenced this pull request Mar 13, 2020
This branch currently seems an almost adequate replacement for gtk2 claws-mail,
except that clicking links in the web view opens them in the email window even
when "open links with external browser" is enabled.

Related: #75040
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Mar 14, 2020
This branch currently seems an almost adequate replacement for gtk2 claws-mail,
except that clicking links in the web view opens them in the email window even
when "open links with external browser" is enabled.

Related: NixOS#75040
(cherry picked from commit b0d9764)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.