[bug#42380] [PATCH v5 9/9] gnu: Add torbrowser-unbundle.

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1: Type: text/plain, Size: 11721 bytes --]

Some comments, maybe the have already been addressed previously:

> +++ b/gnu/packages/patches/torbrowser-start-desktop.patch
> @@ -0,0 +1,22 @@
> +Change TorBrowser desktop file in order for it to be agnostic to the
> +path when invoked.
> +
> +--- torbrowser-68.10.0esr-9.5-1/tbb-scripts/RelativeLink/start-tor-browser.desktop.orig    2020-07-05 18:47:40.689484877 -0300
> ++++ torbrowser-68.10.0esr-9.5-1/tbb-scripts/RelativeLink/start-tor-browser.desktop 2020-07-24 14:36:37.720579884 -0300
> +@@ -1,4 +1,4 @@
> +-#!/usr/bin/env ./Browser/execdesktop
> ++#!/usr/bin/env bash
> + #
> + # This file is a self-modifying .desktop file that can be run from the shell.
> + # It preserves arguments and environment for the start-tor-browser script.
> +@@ -28,7 +28,7 @@
> + GenericName=Web Browser
> + Comment=Tor Browser is +1 for privacy and −1 for mass surveillance
> + Categories=Network;WebBrowser;Security;
> +-Exec=sh -c '"$(dirname "$*")"/Browser/start-tor-browser --detach || ([ ! -x "$(dirname "$*")"/Browser/start-tor-browser ] && "$(dirname "$*")"/start-tor-browser --detach)' dummy %k
> +-X-TorBrowser-ExecShell=./Browser/start-tor-browser --detach
> +-Icon=web-browser
> ++Exec=sh -c start-tor-browser
> ++X-TorBrowser-ExecShell=start-tor-browser --detach
> ++Icon=torbrowser

What's the reason for switching the icon from web-browser to torbrowser?
Also, the guixy way would be to simply replace "$(dirname "$*")/STUFF"
by /gnu/store/[...]/MORE-STUF/STUFF.

Otherwise, you're assuming "sh" is in the profile. It would also
be possible to replace "sh" with (string-append (assoc-ref inputs "bash") "/bin/sh")
I guess.

> + StartupWMClass=Tor Browser
> diff --git a/gnu/packages/patches/torbrowser-start-script.patch b/gnu/packages/patches/torbrowser-start-script.patch
> new file mode 100644
> index 0000000000..b8c8d9a26a
> --- /dev/null
> +++ b/gnu/packages/patches/torbrowser-start-script.patch
> @@ -0,0 +1,181 @@
> +Change TorBrowser startup script in order for it to setup needed files
> +outside guix store. Remove tests which are not needed on guix system.
> +
> +--- torbrowser-68.10.0esr-9.5-1/tbb-scripts/RelativeLink/start-tor-browser.orig    2020-07-05 18:47:40.685485004 -0300
> ++++ torbrowser-68.10.0esr-9.5-1/tbb-scripts/RelativeLink/start-tor-browser 2020-07-23 18:13:32.426282743 -0300
> +@@ -5,6 +5,14 @@
> + #
> + # Copyright 2017 The Tor Project.  See LICENSE for licensing information.
> + 
> ++TBB_HOME="${HOME}/.local/share/torbrowser"
> ++TBB_LOGFILE="${TBB_HOME}/torbrowser.log"
> ++TBB_DATA="${TBB_HOME}/Data"
> ++TBB_PROFILE="${TBB_DATA}/Browser/profile.default"
> ++TBB_STORE_PATH=$(dirname $(realpath "$0"))
> ++TBB_STORE_DATA="${TBB_STORE_PATH}/TorBrowser/Data"
> ++TORRC="${TBB_DATA}/Tor/torrc-defaults"
> ++
> + complain_dialog_title="Tor Browser"
> + 
> + # First, make sure DISPLAY is set.  If it isn't, we're hosed; scream
> +@@ -134,8 +142,8 @@
> +           ;;
> +       -l | --log)
> +           if [ -z "$2" -o "${2:0:1}" == "-" ]; then
> +-             printf "Logging Tor Browser debug information to tor-browser.log\n"
> +-             logfile="../tor-browser.log"
> ++             printf "Logging Tor Browser debug information to torbrowser.log\n"

Why rename tor-browser.log to torbrowser.log?

> + [...]
> ++# Try to be agnostic to where we're being started from, check if files are on its
> ++# default paths and chdir to TBB_HOME
> ++if [ -e "${TORRC}" ]; then
> ++   cd "${TBB_HOME}"
> ++else
> ++   mkdir -p "${TBB_HOME}"
> ++   cp -R "${TBB_STORE_DATA}" "${TBB_HOME}"
> ++   chmod -R 700 "${TBB_HOME}"
> ++   mkdir -p "${TBB_PROFILE}"
> ++   echo "user_pref(\"extensions.torlauncher.torrc-defaults_path\", \"${TORRC}\");"\
> ++     > "${TBB_PROFILE}/user.js"
> ++   echo "ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec ${TBB_STORE_PATH}/TorBrowser/Tor/PluggableTransports/obfs4proxy"\
> ++     >> "${TORRC}"
> ++   cd "${TBB_HOME}"
> + fi

"mkdir" and "cp" are from coreutils, which are not guaranteed to be in
the profile. I'd suggest:

(1) (preferred) use substitute* in a build phase to replace
    'mkdir' and 'cp' & co with the absolute store path
(2) or add coreutils to propagated-inputs

Likewise for sed. 

> [...]
> 
> + if [ "$register_desktop_app" -eq 1 ]; then
> + 	mkdir -p "$HOME/.local/share/applications/"
> +-	cp ../start-tor-browser.desktop "$HOME/.local/share/applications/"
> ++	cp "${TBB_STORE_PATH}/start-tor-browser.desktop" "$HOME/.local/share/applications/"
> + 	update-desktop-database "$HOME/.local/share/applications/"
> + 	printf "Tor Browser has been registered as a desktop app for this user in ~/.local/share/applications/\n"
> + 	exit 0

Is this required on Guix and would it work well on Guix? Copying .desktop files around
seems counter to ‘Guix suppots transactional upgrades and roll-backs, [...]. [...]
reproducible operating systems’ and not very functional. Shouldn't
"guix install torbrowser-unbundle" be sufficient?

noscript seems an useful extension for IceCat as well.
Maybe move it to gnuzilla.scm? Maybe move https-everywhere
there as well? (Separate issue: https-everywhere seems to
be bundled in IceCat ...)

> +(define https-everywhere-lib-wasm
> +  (let ((commit "45b1622f1240659aca4762fa336aad1322d6d50f"))
> +    (package
> +      (name "https-everywhere-lib-wasm")
> +      (version "2021.4.15")
> +      (source
> +       (origin
> +         (method git-fetch)
> +         (uri (git-reference
> +               (url "https://github.com/EFForg/https-everywhere-lib-wasm")
> +               (commit commit)))
> +         (file-name (git-file-name name version))
> +         (sha256
> +          (base32
> +           "1lq62rzypdzmnnzvfns3ccvv1g7p7g9s8jx788zzigr3gnmkpffx"))))
> +      (build-system trivial-build-system)
> +      (arguments
> +       `(#:modules ((guix build utils))
> +         #:builder (begin
> +                     (use-modules (guix build utils))
> +                     (format #t "Copying source ...~%")
> +                     (copy-recursively (assoc-ref %build-inputs "source")
> +                                       %output
> +                                       #:log (%make-void-port "w")))))

Why are you copying the source code to somewhere else?
This doesn't seem to accomplish anything. I would suggest
something like:

;; Source code of ‘HTTPS Everywhere WASM Library’,
;; licensed as license:lgpl2.1+, used in 'https-everywhere'
;; as an input.
(define https-everywhere-lib-wasm/source-code
  (origin (method git-fetch) [...]))

Note that you can use 'origin' objects in 'inputs' and 'native-inputs'.


> +      (home-page "https://github.com/EFForg/https-everywhere-lib-wasm")
> +      (synopsis "Browser extension for protection against known attacks")
> +      (description "Browser extension that protects users from a range of
> +known attacks on web browsing activity such as Cross-site scripting, clickjack and
> +makes possible for the users to block or choose on a per site basis which remote
> +javascript to run while browsing the web.")
> +      (license license:gpl2+))))

The license file seems to tell something different: LGPL2.1+ instead of GPL2+:
https://github.com/EFForg/https-everywhere-lib-wasm/blob/master/LICENSE


> [...]
> +      (native-inputs
> +       `(("https-everywhere" ,https-everywhere)
> +	   ("noscript" ,noscript)

noscript and https-everywhere seem more like 'inputs' than
'native-inputs' to me, but IIUC they are source-code only
and not compiled, so it doesn't really matter here I guess.

> [...]
> +             (add-after 'unpack 'make-bundle
> +               (lambda* (#:key inputs native-inputs #:allow-other-keys)
> +		 (let ((tor-launcher (assoc-ref inputs "tor-launcher"))
> +                       (tor-launcher-dir "browser/extensions/tor-launcher")
> +                       (tbb (assoc-ref inputs "tor-browser-build"))
> +                       (tbb-scripts-dir "tbb-scripts"))
> +
> +                   (format #t "Copying tor-launcher ...~%")
> +                   (copy-recursively tor-launcher tor-launcher-dir
> +                                     #:log (%make-void-port "w"))
> +                   (format #t "Copying tor-browser-build ...~%")
> +                   (mkdir tbb-scripts-dir)
> +                   (copy-recursively tbb tbb-scripts-dir
> +                                     #:log (%make-void-port "w"))
> +		   (make-file-writable "browser/app/profile/000-tor-browser.js")
> +		   (make-file-writable (string-append tbb-scripts-dir
> +						      "/RelativeLink/start-tor-browser"))
> +                   (make-file-writable (string-append tbb-scripts-dir
> +						      "/RelativeLink/start-tor-browser.desktop")))
> +		 #t))

Returning #t at the end of a phase is not required anymore.
The warning will disappear when core-updates is merged.

> +             (replace 'configure
> +               (lambda* (#:key inputs outputs configure-flags #:allow-other-keys)
> +		 (let* ((out (assoc-ref outputs "out"))
> +			(bash (which "bash"))
> +			(flags `(,(string-append "--prefix=" out)
> +				 ,@configure-flags)))
> +                   (setenv "SHELL" bash)
> +                   (setenv "AUTOCONF" (string-append
> +                                       (assoc-ref %build-inputs "autoconf")
> +                                       "/bin/autoconf"))

In build phases, use 'inputs' or 'native-inputs' instead of %build-inputs.
It's more explicit, maybe there are other reasons as well. (Here it should
be native-inputs I guess).


> +                   (setenv "CONFIG_SHELL" bash)
> +                   (setenv "PYTHON" (string-append (assoc-ref inputs "python-2")
> +                                                   "/bin/python"))

This most likely should be (assoc-ref (or native-inputs inputs) "python-2")
instead of (assoc-ref inputs "python-2").

> +                   (setenv "CC" "gcc")  ; needed when Stylo is enabled
This most likely should be ,(cc-for-target) instead of "gcc".

(The native-inputs/inputs and "gcc" / (cc-for-target) distinction is important
when cross-compiling (though not all dependencies are cross-compilable currently,
so it's a bit moot for now.))

> +             (add-after 'install-extensions 'link-binaries
> +               (lambda* (#:key inputs native-inputs outputs #:allow-other-keys)
You're not using 'native-inputs' in this build phase so you can remove it
from the arguments list.

> +              [...]
> +             (add-after 'link-binaries 'copy-bundle-data
> +               (lambda* (#:key inputs native-inputs outputs #:allow-other-keys)

Likewise.

> +      (description
> +       "Tor Browser is the Tor Project version of Firefox browser.  It is the only
> +recommended way to anonymously browse the web that is supported by the project.
> +It modifies Firefox in order to avoid many know application level attacks on
> +the privacy of Tor users.
> +
> +WARNING: This is not the official Tor Browser and is currently on testing.  Use
> +at your own risk and please report back on guix channels if you find any
> +issues.")

This seems unnecessarily scary. All packages in guix are ‘at your own risk’
and every new package is ‘in testing’ for a while, whatever that means.
What about

 "Warning: this is not the official built of Tor Browser from upstream.
As such, the Guix version of Tor Browser may have small differences
that might allow a malicious actor to identify you as a Guix user."

?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]