[bug#42380] gnu: Add torbrowser.

* gnu/packages/tor.scm (torbrowser): New variable.
(torbrowser-assets): New variable.
* gnu/packages/browser-extensions.scm (noscript): New variable.
(noscript/icecat): New variable.

Change-Id: I73dc53905e4a028108bb34aae07e44256cf16c85
---

Hi, this is a package for Tor Browser.  I initially wanted to base my work on
André's but I believe pretty much everything is new now.  André's work helped
nonetheless, so thank you André.

A few notes:
 - HTTPS-everywhere extension is now built-in.
 - There is a package for Noscript.
 - Bridge support (lyrebird) will come later (patches are being polished and
   are for the testing branch)
 - I took inspiration from OpenBSD's package (they build it too) and from
   Nix (they use the bundle).
 - Some work could be done to improve icecat-minimal inheritance (icons,
   sandbox, wrap-program) but it's not trivial.
 - The name is "torbrowser" because it's obvious that we don't bundle anything
   in Guix, that's how other distros do and it's simpler.
 - It should be FSDG compatible (no DRM, no link to addons.mozilla.org).

Comments are welcome!

Clément

 gnu/packages/browser-extensions.scm |  26 +++
 gnu/packages/gnupg.scm              |   3 +-
 gnu/packages/tor.scm                | 280 ++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+), 2 deletions(-)


base-commit: bb3ab24a296ffa5273b2e82a02ed057e90c095f3

Hi Clément,

ter 12 dez 2023 às 12:21:18 (1702394478), clement@lassieur.org enviou:
> 
> Hi, this is a package for Tor Browser.  I initially wanted to base my work on
> André's but I believe pretty much everything is new now.  André's work helped
> nonetheless, so thank you André.
>

Nice to see someone picking that up. Even though there were quite a lot
of changes both on guix and on Tor Browser which made my package
definition mostly obsolete, I should state beforehand that your code is
both clearer and more concise which also means more maintainable than
mine. Moreover the removal of mozilla's store was a most needed
improvement.

However, I do think that there are things which need to be fixed before
commiting this patch. I've made some intersperced comments bellow, both
to you and other reviewers.

First and foremost:

The noscript addon seems to be missing from the browser. If one goes
to the 'about:addons' tab, it is neither listed nor manageable there.
This makes the security slider almost useless and also implies that
as things stand we would lead guixen to run potentialy harmful and
nonfree javascript code unknowingly and without a warning.

You can check that on https://coveryourtracks.eff.org for the
difference between this browser fingerprint and the upstream one.

Other than that, the current recipe is not deterministic. This is
probably due to the 'BuildID' which is a timestamp.

See: (#$output)/lib/torbrowser/platform.ini

Moreover, both upstream torbrowser and guix' icecat build an
internationalized browser with several locales and the browser as is
offers users on startup to change or set the browser locale even though
we did not provide any other than en-US.

I don't think the current en-US only is a show stopper, but let's make
a note on internationalizing it later.

> A few notes:
>  - HTTPS-everywhere extension is now built-in.

In my understading, the extension got removed as the feature it provided
is now part of firefox itself.

>  - The name is "torbrowser" because it's obvious that we don't bundle anything
>    in Guix, that's how other distros do and it's simpler.

What { is || is not } obvious is highly subjective. Maybe to most people
it is obvious that the distro version of some software is not the
upstream one. On the other hand, maybe it's not obvious to many that,
with regards to TorBrowser's goals, this is a significative difference
as it potentialy implies a reduced anonymity set.

'torbrowser-unbundle' was a pun on the original torbrowser name ("Tor
Browser Bundle") and it was intended as some kind of warning to users
that the guix package cannot live up to a vital upstream goal, namely
that all users are using an identical browser in order to avoid, best
as possible, any leak which could be used to fingerprint/deanonymize
users. It was also kind of an homage to upstream directives if you
will.

However, even if some guix users may be unaware, this is an improvement
to the current situation where people use icecat with tor which
undeniably means a reduced anonymity set. Also, the hint may have been
too weak to convey the intended warning. So I won't strongly oppose
naming it simply 'torbrowser' if I'm the only one who sees a point on
doing otherwise.

>
> diff --git a/gnu/packages/browser-extensions.scm b/gnu/packages/browser-extensions.scm
> index 21c519eda31c..9efa94b77396 100644
> --- a/gnu/packages/browser-extensions.scm
> +++ b/gnu/packages/browser-extensions.scm
> @@ -21,6 +21,7 @@
>  (define-module (gnu packages browser-extensions)
>    #:use-module (guix gexp)
>    #:use-module (guix packages)
> +  #:use-module (guix download)
>    #:use-module (guix git-download)
>    #:use-module (guix build-system copy)
>    #:use-module (guix build-system gnu)
> @@ -221,3 +222,28 @@ (define passff
>  
>  (define-public passff/icecat
>    (make-icecat-extension passff))
> +
> +(define noscript
> +  (package
> +    (name "noscript")
> +    (version "11.4.28")
> +    (source (origin
> +              (method url-fetch/zipbomb)
> +              (uri (string-append
> +                    "https://noscript.net/download/releases/noscript-" version
> +                    ".xpi"))
> +              (sha256
> +               (base32
> +                "051wawi0yjyramp743yjawqaz59g3m2gcivm24b44ibd4arpdl2l"))))
> +    (build-system copy-build-system)
> +    (properties '((addon-id . "{73a6fe31-595d-460b-a920-fcc0f8843232}")))
> +    (arguments
> +     `(#:install-plan '(("." ,(assq-ref properties 'addon-id)))))
> +    (home-page "https://noscript.net")
> +    (synopsis "Software providing extra protection for various browsers.")
> +    (description "The NoScript Security Suite is a software providing extra
> +protection for web browsers.")
> +    (license license:gpl3+)))
> +
> +(define-public noscript/icecat
> +  (make-icecat-extension noscript))

As I understand it, we are not building noscript from source, but getting
a previously built which has minified JS. I never got to build it from
source and also don't think this makes it uncommitable (agains FSDG), but
maybe we could have a note to re-work this definition later in order to
have it built from source (the guix way!).

...

> diff --git a/gnu/packages/tor.scm b/gnu/packages/tor.scm
> index 71f32b3f4331..31e9945f5d39 100644
> --- a/gnu/packages/tor.scm
> +++ b/gnu/packages/tor.scm

...

> +(define-public torbrowser
> +  (package
> +    (inherit icecat-minimal)
> +    (name "torbrowser")
> +    ;; To find the last version, browse
> +    ;; https://archive.torproject.org/tor-package-archive/torbrowser/<version>
> +    ;; (<version> is the version of the `torbrowser-assets` package).  There
> +    ;; should be only one archive that starts with "src-firefox-tor-browser-".
> +    (version "115.5.0esr-13.0-1-build4")

Is there any reason why you chose to use the 'src' version, instead of
the TorBrowser release version (aka torbroser-assets one). At first I
think it would be better if our version were the same as upstream as
it would be clearer to both users and maintainers which version guix
is offering without installing it.

Besides, are you sure this src version number is guaranteed to be
progressive towards higher numbers?

Decomposing it:

Firefox version  |   tb build ver |   tb build attempt
115.5.0esr       |   13.0-1       |   build4

FF version: always increases, but not necessarily in the same step as
torbrowser releases;

tb build version: usually remains the same throughout a major torbrowser
release series;

tb build attempt: varies with the release process and sometimes it
decreases.


> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri
> +        (string-append
> +         "https://archive.torproject.org/tor-package-archive/torbrowser/"
> +         (package-version torbrowser-assets)
> +         "/src-firefox-tor-browser-" version ".tar.xz"))
> +       (sha256
> +        (base32
> +         "0p0qsfc2l2bicqjr1kxciiij5qz7n8xqyvyn8f13fvk0wyg94c6v"))))
> +    (build-system mozilla-build-system)
> +    (arguments
> +     (substitute-keyword-arguments (package-arguments icecat-minimal)
> +       ((#:configure-flags flags '())
> +        #~(cons*
> +           "--without-relative-data-dir" ;store is read-only

Shouldn't we also set '--with-user-appdir=.torbrowser' ?

There is a comment on 'src/browser/config/mozconfigs/tor-browser' that
says we need to set this flag when the relative data dir is unset.

> +           "--disable-base-browser-update"
> +           "--enable-update-channel=release"

Does this mean that users get notified when there is a new torbrowser
release upstream? Shouldn't this flag be removed?

> +           "--with-branding=browser/branding/tb-release"
> +           (string-append "--prefix=" #$output)
> +           (string-append "--with-base-browser-version="
> +                          #$(package-version
> +                             (this-package-input "torbrowser-assets")))
> +           #$flags))
> +       ((#:phases phases)
> +        #~(modify-phases #$phases
> +            (add-before 'configure 'setenv
> +              (lambda _
> +                (setenv "CONFIG_SHELL" (which "bash"))
> +                ;; Install location is prefix/lib/$MOZ_APP_NAME.  Also
> +                ;; $MOZ_APP_NAME is the executable name.  Default is
> +                ;; "firefox".
> +                (setenv "MOZ_APP_NAME" "torbrowser")
> +                ;; Profile location (relative to "~/.").  Default is
> +                ;; lower($MOZ_APP_VENDOR/$MOZ_APP_BASENAME), which is:
> +                ;; ~/.tor project/firefox.
> +                (setenv "MOZ_APP_PROFILE" "torbrowser/browser")
> +                ;; WM_CLASS (default is "$MOZ_APP_NAME-$MOZ_UPDATE_CHANNEL").

This comment was unclear for me at first, probably due to my own
ignorance. To the benefit of others, this is in line with instructions
on 'src/browser/config/mozconfigs/tor-browser' as a hint to window
managers on GNU/Linux.

> +                (setenv "MOZ_APP_REMOTINGNAME" "Tor Browser")
> +                ;; Persistent state directory for the build system (default is
> +                ;; $HOME/.mozbuild).
> +                (setenv "MOZBUILD_STATE_PATH"
> +                        (in-vicinity (getcwd) ".mozbuild"))))

...

> +                    (lambda ()
> +                      (format #t "// first line must be a comment~%")
> +                      ;; Locking prevents these values being written to
> +                      ;; prefs.js, avoiding Store path capture.
> +                      (format #t "lockPref(~s, ~s);~%"
> +                              "extensions.torlauncher.torrc-defaults_path"
> +                              (in-vicinity
> +                               lib "TorBrowser/Data/Tor/torrc-defaults"))
> +                      (format #t "lockPref(~s, ~s);~%"
> +                              "extensions.torlauncher.tor_path"
> +                              (search-input-file inputs "bin/tor"))

This has the undesired side-effect of making impossible to run TorBrowser
with a shepherd tor instance. Is it really needed?

Besides the inefficiency of running two tor processes, using a single one
has the benefit of making eventual onion service auth keys available both
on the browser and to other user software on the same location.

> +                      ;; Required for Guix packaged extensions
> +                      ;; SCOPE_PROFILE=1, SCOPE_APPLICATION=4, SCOPE_SYSTEM=8
> +                      ;; Default is 5.

...

> +            (replace 'install-desktop-entry
> +              (lambda _
> +                (let ((apps (in-vicinity #$output "share/applications")))
> +                  (mkdir-p apps)
> +                  (make-desktop-entry-file
> +                   (in-vicinity apps "torbrowser.desktop")
> +                   #:name "Tor Browser"
> +                   #:exec
> +                   (format #f "~a %u" (in-vicinity #$output "bin/torbrowser"))

Why do away with the 'start-tor-browser.sh'? Part of the logic there is
redundant or not necessary on a system install, but not everything.

> +                   #:comment
> +                   "Tor Browser is +1 for privacy and -1 for mass surveillance"
> +                   #:categories '("Network" "WebBrowser" "Security")
> +                   #:startup-w-m-class "Tor Browser"
> +                   #:icon "tor-browser"))))
> +            (replace 'install-icons
> +              (lambda* (#:key inputs #:allow-other-keys)
> +                (for-each
> +                 (lambda (size)
> +                   (let ((oldpath (string-append
> +                                   "browser/branding/tb-release/default"
> +                                   size ".png"))
> +                         (newpath (string-append #$output
> +                                                 "/share/icons/hicolor/"
> +                                                 size "x" size "/apps")))
> +                     (mkdir-p newpath)
> +                     (copy-file oldpath
> +                                (in-vicinity newpath "tor-browser.png"))))
> +                 '("16" "22" "24" "32" "48" "64" "128" "256"))))))))
> +    (inputs
> +     (modify-inputs (package-inputs icecat-minimal)
> +       (append bash-minimal
> +               tor

Why not tor-client instead? I don't see a legitimate use case of running
relays on the torbrowser.

Also, shouldn't this be a propagated input so as to not be garbage
collected?

> +               torbrowser-assets)))
> +    (propagated-inputs
> +     (list noscript/icecat))

This appears to be insufficient. See comments above.

Thanks for your work on guix and cheers!

Hi Clément,

qui 14 dez 2023 às 18:54:48 (1702590888), nandre@riseup.net enviou:
> 
> First and foremost:
> 
> The noscript addon seems to be missing from the browser. If one goes
> to the 'about:addons' tab, it is neither listed nor manageable there.
> This makes the security slider almost useless and also implies that
> as things stand we would lead guixen to run potentialy harmful and
> nonfree javascript code unknowingly and without a warning.
> 
> You can check that on https://coveryourtracks.eff.org for the
> difference between this browser fingerprint and the upstream one.

Please, disregard what I've said above: noscript is indeed listed on the
addons tab, manageable there and the browser security slider is also
working as expected.

I had just built and run from the store, without installing to a profile
so guix was rightfully not picking up any info on noscript. When properly
installed, it is picked up just fine.

Please accept my appologies for improperly reviewing it, I should've
known that the bug was between the chair and the keyboard.

Hi again!

qui 14 dez 2023 às 18:54:48 (1702590888), nandre@riseup.net enviou:
> 
> Other than that, the current recipe is not deterministic. This is
> probably due to the 'BuildID' which is a timestamp.
> 
> See: (#$output)/lib/torbrowser/platform.ini

This was it. I've just set MOZ_BUILD_DATE environment variable to a
fixed timestamp (yyyymmddhhmmss) and now the build successfully checks.

It's not much, but it's honest computer work! :)

Now the question is: do we just set a fixed timestamp and forget it until
the sun burns out or do we change it each and everytime when updating the
browser so as to mimick the upstream one?

Even though it is a bit of a hassle, I'd vote for mimicking upstream.

> > +       ((#:phases phases)
> > +        #~(modify-phases #$phases
> > +            (add-before 'configure 'setenv
> > +              (lambda _
> > +                (setenv "CONFIG_SHELL" (which "bash"))
> > +                ;; Install location is prefix/lib/$MOZ_APP_NAME.  Also
> > +                ;; $MOZ_APP_NAME is the executable name.  Default is
> > +                ;; "firefox".
> > +                (setenv "MOZ_APP_NAME" "torbrowser")
> > +                ;; Profile location (relative to "~/.").  Default is
> > +                ;; lower($MOZ_APP_VENDOR/$MOZ_APP_BASENAME), which is:
> > +                ;; ~/.tor project/firefox.
> > +                (setenv "MOZ_APP_PROFILE" "torbrowser/browser")
> > +                ;; WM_CLASS (default is "$MOZ_APP_NAME-$MOZ_UPDATE_CHANNEL").

Hi André!

On Thu, Dec 14 2023, André Batista wrote:

> The noscript addon seems to be missing from the browser. If one goes
> to the 'about:addons' tab, it is neither listed nor manageable there.
> This makes the security slider almost useless and also implies that
> as things stand we would lead guixen to run potentialy harmful and
> nonfree javascript code unknowingly and without a warning.
>
> You can check that on https://coveryourtracks.eff.org for the
> difference between this browser fingerprint and the upstream one.

[in an other mail]
> Please, disregard what I've said above: noscript is indeed listed on the
> addons tab, manageable there and the browser security slider is also
> working as expected.
> 
> I had just built and run from the store, without installing to a profile
> so guix was rightfully not picking up any info on noscript. When properly
> installed, it is picked up just fine.
> 
> Please accept my appologies for improperly reviewing it, I should've
> known that the bug was between the chair and the keyboard.

Well you've done the greatest review I've ever seen so no need to
apologize :)  Indeed NoScript is a propagated input, so it needs to be
installed.  You can also test it like this:

    guix shell --emulate-fhs --no-offload --no-cwd --preserve=DISPLAY
    --container --network torbrowser -- torbrowser

Or if you want to add ublock-origin:

    guix shell --emulate-fhs --no-offload --no-cwd --preserve=DISPLAY
    --container --network torbrowser ublock-origin-icecat -- torbrowser

(Note that you'll need either to allow ublock0 to run in private windows
or to not be in a private window.)

My tests with https://coveryourtracks.eff.org give exactly the same
results (between 8 and 9 depending on window size) as the official Tor
Browser.

> Other than that, the current recipe is not deterministic. This is
> probably due to the 'BuildID' which is a timestamp.

Indeed I had forgotten about this.  And Icecat does it well.

> See: (#$output)/lib/torbrowser/platform.ini
>
> Moreover, both upstream torbrowser and guix' icecat build an
> internationalized browser with several locales and the browser as is
> offers users on startup to change or set the browser locale even though
> we did not provide any other than en-US.
>
> I don't think the current en-US only is a show stopper, but let's make
> a note on internationalizing it later.

Yes, but I believe we can add the internationalization as an extension,
it would be nicer than doing what Icecat does: torbrowser-minimal and
torbrowser with internationalization.  When we do this we should
probably fix Icecat as well.  This is, in my opinion, for another patch.

>> A few notes:
>>  - HTTPS-everywhere extension is now built-in.
>
> In my understading, the extension got removed as the feature it provided
> is now part of firefox itself.

Exactly.

>>  - The name is "torbrowser" because it's obvious that we don't bundle anything
>>    in Guix, that's how other distros do and it's simpler.
>
> What { is || is not } obvious is highly subjective. Maybe to most people
> it is obvious that the distro version of some software is not the
> upstream one. On the other hand, maybe it's not obvious to many that,
> with regards to TorBrowser's goals, this is a significative difference
> as it potentialy implies a reduced anonymity set.

I agree that potentially there could be a reduced anonymity, but I've
not seen any footprint difference yet and when we see it I'm hopeful
we'll be able to fix it.

> 'torbrowser-unbundle' was a pun on the original torbrowser name ("Tor
> Browser Bundle") and it was intended as some kind of warning to users
> that the guix package cannot live up to a vital upstream goal, namely
> that all users are using an identical browser in order to avoid, best
> as possible, any leak which could be used to fingerprint/deanonymize
> users. It was also kind of an homage to upstream directives if you
> will.

Are there directives about it?  I haven't been able to find them.  Also
OpenBSD names it "torbrowser" and they build it from source too.

> However, even if some guix users may be unaware, this is an improvement
> to the current situation where people use icecat with tor which
> undeniably means a reduced anonymity set. Also, the hint may have been
> too weak to convey the intended warning. So I won't strongly oppose
> naming it simply 'torbrowser' if I'm the only one who sees a point on
> doing otherwise.

The main benefit of naming it "torbrowser" is, I believe, simplicity,
and the fact that it eases adoption.  People will know it's Tor Browser
and not some variant.

The only real difference with upstream is the fact that we don't store
the profile where the executable is (because our store is read-only).  I
believe this feature is for users who have Torbrowser on a USB dongle
that can be removed and then the system is still clean.  But that's not
really a use-case for us anyway.

>> +(define-public noscript/icecat
>> +  (make-icecat-extension noscript))
>
> As I understand it, we are not building noscript from source, but getting
> a previously built which has minified JS. I never got to build it from
> source and also don't think this makes it uncommitable (agains FSDG), but
> maybe we could have a note to re-work this definition later in order to
> have it built from source (the guix way!).

Does it have minified JS though?  I had a look at several files but
could not find any that is minified.  If it does have minified JS, I
agree we should fix it.  I actually tried to build it from source but
there are a ton of missing Node dependencies. :/

>> +(define-public torbrowser
>> +  (package
>> +    (inherit icecat-minimal)
>> +    (name "torbrowser")
>> +    ;; To find the last version, browse
>> +    ;; https://archive.torproject.org/tor-package-archive/torbrowser/<version>
>> +    ;; (<version> is the version of the `torbrowser-assets` package).  There
>> +    ;; should be only one archive that starts with "src-firefox-tor-browser-".
>> +    (version "115.5.0esr-13.0-1-build4")
>
> Is there any reason why you chose to use the 'src' version, instead of
> the TorBrowser release version (aka torbroser-assets one). At first I
> think it would be better if our version were the same as upstream as
> it would be clearer to both users and maintainers which version guix
> is offering without installing it.

I just wanted the source URL to only depend on the version, and not
anything else.  That makes it easier to maintain, and it reminds people
what it really is: a Firefox.

> Besides, are you sure this src version number is guaranteed to be
> progressive towards higher numbers?
>
> Decomposing it:
>
> Firefox version  |   tb build ver |   tb build attempt
> 115.5.0esr       |   13.0-1       |   build4
>
> FF version: always increases, but not necessarily in the same step as
> torbrowser releases;
>
> tb build version: usually remains the same throughout a major torbrowser
> release series;
>
> tb build attempt: varies with the release process and sometimes it
> decreases.

    base-browser-115.1.0esr-13.0-1-build1 Tagging build1 for 115.1esr-based alpha
    base-browser-115.1.0esr-13.0-1-build2 Tagging build2 for 115.1esr-based alpha
    base-browser-115.2.0esr-13.0-1-build1 Tagging build1 for 115.2.0esr-based Base Browser alpha
    base-browser-115.2.1esr-13.0-1-build1 Tagging build1 for 115.2.1esr-based Base Browser alpha
    base-browser-115.3.0esr-13.0-1-build1 Tagging build1 for 115.3.0esr-based alpha
    base-browser-115.3.1esr-13.0-1-build1 Tagging build1 for 115.3.1esr-based stable
    base-browser-115.4.0esr-13.0-1-build1 Tagging build1 for 115.4.0esr-based stable
    base-browser-115.4.0esr-13.0-1-build2 Tagging build2 for 115.4.0esr-based stable
    base-browser-115.4.0esr-13.5-1-build1 Tagging build1 for 115.4.0esr-based alpha
    base-browser-91.12.0esr-12.0-1-build1 Tagging build1 for 91.12esr-based alpha

Here are some refs I've found in the git repo.  We can see that for the
same "tb build version" (13.0-1) there are several base browser
versions: 115.1.0, 115.2.0, 115.2.1, etc.  build1 goes to build2 only
when both "base version" and "tb build version" don't change.

In this example we can see a 13.5-1, which means alpha, which we never
want.  So version string being monotonically increasing doesn't really
help: guix package --upgrade won't work anyway.

I think the version should describe fully what we are packaging, and in
this example, we can see that 13.0.1 isn't enough.  I might be wrong
though, what do you think?

>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri
>> +        (string-append
>> +         "https://archive.torproject.org/tor-package-archive/torbrowser/"
>> +         (package-version torbrowser-assets)
>> +         "/src-firefox-tor-browser-" version ".tar.xz"))
>> +       (sha256
>> +        (base32
>> +         "0p0qsfc2l2bicqjr1kxciiij5qz7n8xqyvyn8f13fvk0wyg94c6v"))))
>> +    (build-system mozilla-build-system)
>> +    (arguments
>> +     (substitute-keyword-arguments (package-arguments icecat-minimal)
>> +       ((#:configure-flags flags '())
>> +        #~(cons*
>> +           "--without-relative-data-dir" ;store is read-only
>
> Shouldn't we also set '--with-user-appdir=.torbrowser' ?
>
> There is a comment on 'src/browser/config/mozconfigs/tor-browser' that
> says we need to set this flag when the relative data dir is unset.

They say it indeed, but they don't use it in the code^^.

    set_define("MOZ_USER_DIR", user_appdir)

[...]

    #define DEFAULT_PRODUCT_DIR nsLiteralCString(MOZ_USER_DIR)

[...]

    #if !defined(TOR_BROWSER)
      rv = localDir->AppendRelativeNativePath(DEFAULT_PRODUCT_DIR);
      if (NS_FAILED(rv)) {
        return rv;
      }
    #endif

But you are right, I'll add it anyway, we never know, they might change
the code later.

>> +           "--disable-base-browser-update"
>> +           "--enable-update-channel=release"
>
> Does this mean that users get notified when there is a new torbrowser
> release upstream? Shouldn't this flag be removed?

No, there is a channel anyway, we just change it from "default" to
"release".  Otherwise this code gets executed:

    @depends("--enable-update-channel")
    def tor_browser_nightly_build(channel):
        if channel and channel[0] in ["default", "nightly"]:
            return True

And we get warnings of instability because it thinks it's a nightly
while it's not.  I'll add a comment!

>> +                ;; WM_CLASS (default is "$MOZ_APP_NAME-$MOZ_UPDATE_CHANNEL").
>
> This comment was unclear for me at first, probably due to my own
> ignorance. To the benefit of others, this is in line with instructions
> on 'src/browser/config/mozconfigs/tor-browser' as a hint to window
> managers on GNU/Linux.

Yeah it's just a way for windows (e.g. in Gnome) to know to which
left-bar button they are associated.

>> +                      (format #t "// first line must be a comment~%")
>> +                      ;; Locking prevents these values being written to
>> +                      ;; prefs.js, avoiding Store path capture.
>> +                      (format #t "lockPref(~s, ~s);~%"
>> +                              "extensions.torlauncher.torrc-defaults_path"
>> +                              (in-vicinity
>> +                               lib "TorBrowser/Data/Tor/torrc-defaults"))
>> +                      (format #t "lockPref(~s, ~s);~%"
>> +                              "extensions.torlauncher.tor_path"
>> +                              (search-input-file inputs "bin/tor"))
>
> This has the undesired side-effect of making impossible to run TorBrowser
> with a shepherd tor instance. Is it really needed?

I don't think so, I'll change it.

> Besides the inefficiency of running two tor processes, using a single one
> has the benefit of making eventual onion service auth keys available both
> on the browser and to other user software on the same location.

Yeah I agree.

>> +            (replace 'install-desktop-entry
>> +              (lambda _
>> +                (let ((apps (in-vicinity #$output "share/applications")))
>> +                  (mkdir-p apps)
>> +                  (make-desktop-entry-file
>> +                   (in-vicinity apps "torbrowser.desktop")
>> +                   #:name "Tor Browser"
>> +                   #:exec
>> +                   (format #f "~a %u" (in-vicinity #$output "bin/torbrowser"))
>
> Why do away with the 'start-tor-browser.sh'? Part of the logic there is
> redundant or not necessary on a system install, but not everything.

The file is 384 lines long, and most of it is not compatible with having
a read-only store.  Patching it would require a huge patch for almost 0
gain, we would be better off with a small wrapper if really there are
things we need to wrap.  But are there things we need from this file?  I
haven't found any.

>> +    (inputs
>> +     (modify-inputs (package-inputs icecat-minimal)
>> +       (append bash-minimal
>> +               tor
>
> Why not tor-client instead? I don't see a legitimate use case of running
> relays on the torbrowser.

Indeed!

> Also, shouldn't this be a propagated input so as to not be garbage
> collected?
>
>> +               torbrowser-assets)))
>> +    (propagated-inputs
>> +     (list noscript/icecat))

I don't think being propagated would change anything regarding to
garbage collection.  Normal inputs are protected as well.  But your
point is good, I need to test that everything goes well when tor is
upgraded and the previous one garbage collected.  I believe worst case
scenario is we need to use "lockPref ... tor_path" instead of "pref
... tor_path" to prevent the store paths to go into the profile.

> Thanks for your work on guix and cheers!

I'll send an updated patch soon, and I'll test garbage collecting tor.

Also, I'm working on packaging Mullvad Browser too, which is almost the
same work as Tor Browser, so we'll have 2 browsers for the same price!
It's more-or-less a Tor Browser without the Tor network, that encourages
the use of a VPN.  (And WebRTC is enabled in Mullvadbrowser while it's
not yet enabled in Tor Browser).

Thank you again for this great review André :)

Clément

On Tue, Dec 19 2023, Clément Lassieur wrote:

> I'll send an updated patch soon, and I'll test garbage collecting tor.

So I sent an updated patch, and I can confirm that references to
non-existing tor binaries don't cause any issue, those references are
updated upon starting Tor Browser.

Cheers,
Clément

qui 21 dez 2023 às 16:05:44 (1703185544), clement@lassieur.org enviou:
> On Tue, Dec 19 2023, Clément Lassieur wrote:
> 
> > I'll send an updated patch soon, and I'll test garbage collecting tor.
> 
> So I sent an updated patch, and I can confirm that references to
> non-existing tor binaries don't cause any issue, those references are
> updated upon starting Tor Browser.

Will you do the honors, then?

Didn't have the time to test it yet, but I suppose we are beyond testing
phase? I, for one, will be merging this to my local channel.

Thanks and do tell me if you need any more input, I'll reply to you
after my next guix pull.

On Fri, Dec 22 2023, André Batista wrote:

> qui 21 dez 2023 às 16:05:44 (1703185544), clement@lassieur.org enviou:
>> On Tue, Dec 19 2023, Clément Lassieur wrote:
>> 
>> > I'll send an updated patch soon, and I'll test garbage collecting tor.
>> 
>> So I sent an updated patch, and I can confirm that references to
>> non-existing tor binaries don't cause any issue, those references are
>> updated upon starting Tor Browser.
>
> Will you do the honors, then?

Sure!  I thought it would make sense to add a mention like:

    Co-authored-by: André Batista <nandre@riseup.net>

given all the work you've done which was useful to me.  Does it sound
good to you?

> Didn't have the time to test it yet, but I suppose we are beyond testing
> phase? I, for one, will be merging this to my local channel.
>
> Thanks and do tell me if you need any more input, I'll reply to you
> after my next guix pull.

seg 25 dez 2023 às 16:28:00 (1703532480), clement@lassieur.org enviou:
> On Fri, Dec 22 2023, André Batista wrote:
> 
> > qui 21 dez 2023 às 16:05:44 (1703185544), clement@lassieur.org enviou:
> >> On Tue, Dec 19 2023, Clément Lassieur wrote:
> >> 
> >> > I'll send an updated patch soon, and I'll test garbage collecting tor.
> >> 
> >> So I sent an updated patch, and I can confirm that references to
> >> non-existing tor binaries don't cause any issue, those references are
> >> updated upon starting Tor Browser.
> >
> > Will you do the honors, then?
> 
> Sure!  I thought it would make sense to add a mention like:
> 
>     Co-authored-by: André Batista <nandre@riseup.net>
> 
> given all the work you've done which was useful to me.  Does it sound
> good to you?

That would be lovely, thank you!

I've included it to my local channel and did some minimal testing and
everything seem to be working as expected.

The only thing of note was that I couldn't find here the
prerequisite-patch-id that you've mentioned on the version 2. It built
and ran just fine over 5bd80ccd6, even though it did not apply cleanly.

Cheers!

On Wed, Dec 27 2023, André Batista wrote:

> seg 25 dez 2023 às 16:28:00 (1703532480), clement@lassieur.org enviou:
>> On Fri, Dec 22 2023, André Batista wrote:
>> 
>> > qui 21 dez 2023 às 16:05:44 (1703185544), clement@lassieur.org enviou:
>> >> On Tue, Dec 19 2023, Clément Lassieur wrote:
>> >> 
>> >> > I'll send an updated patch soon, and I'll test garbage collecting tor.
>> >> 
>> >> So I sent an updated patch, and I can confirm that references to
>> >> non-existing tor binaries don't cause any issue, those references are
>> >> updated upon starting Tor Browser.
>> >
>> > Will you do the honors, then?
>> 
>> Sure!  I thought it would make sense to add a mention like:
>> 
>>     Co-authored-by: André Batista <nandre@riseup.net>
>> 
>> given all the work you've done which was useful to me.  Does it sound
>> good to you?
>
> That would be lovely, thank you!
>
> I've included it to my local channel and did some minimal testing and
> everything seem to be working as expected.
>
> The only thing of note was that I couldn't find here the
> prerequisite-patch-id that you've mentioned on the version 2. It built
> and ran just fine over 5bd80ccd6, even though it did not apply cleanly.

This was probably a local unrelated commit.

Pushed!  Thanks for testing :)

Clément