Project

General

Profile

Feature #17055

Feature #16356: Upgrade to Tor Browser 9.0 (based on Firefox 68)

Update Unsafe Browser tweaks for Tor Browser 9

Added by intrigeri about 1 month ago. Updated 9 days ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
-
Target version:
Start date:
01/14/2019
Due date:
% Done:

100%

Feature Branch:
feature/16356-tor-browser-9.0+force-all-tests
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Unsafe Browser

Description

config/chroot_local-includes/usr/local/lib/tails-shell-library/chroot-browser.sh needs updating; for example:

  • search extensions are managed differently now: #17039
  • the security slider is displayed
  • the custom browser name is not applied (window name contains "Tor Browser")
  • the custom theme (grep /usr/share/pixmaps/red_dot.png) is not applied
  • probably more, so look at every other customization we attempt and ensure it's applied as intended:
    • in English
    • in another language

addonStartup.json.lz4 (536 Bytes) anonym, 10/06/2019 04:24 PM


Subtasks

Feature #16357: Deal with Torbutton being integrated into Tor BrowserResolved

Bug #17039: Unsafe Browser does not start (segfault) with Tor Browser 9Resolved


Related issues

Related to Tails - Bug #17142: New Unsafe Browser tabs have the "Private Browsing" name and the Tor Browser icon In Progress
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

Associated revisions

Revision 119b779f (diff)
Added by intrigeri about 1 month ago

Adapt the Unsafe Browser branding (name) to Tor Browser 9 (refs: #16357, #17055)

Now that Torbutton is integrated in Tor Browser
(https://trac.torproject.org/projects/tor/ticket/10760),
we only have to patch its own localized branding files.

Revision b337af61 (diff)
Added by intrigeri about 1 month ago

Install an Unsafe Browser red theme compatible with Firefox 68 (refs: #17055)

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes#Documentation
says that "Lightweight themes have been deprecated", which explains why our old,
custom lightweight theme is ignored.

The only options to fix this are:

- Create our own theme, get it automatically signed by Mozilla on AMO, download
the resulting XPI, and somehow make it available at build time.
- Use an existing theme with a GPL-3+ compliant license.

I've chosen the latter option: this theme is almost identical to the one we
had before.

The XPI is 7.7K so I don't think it's worth devising a more complicated
solution than just adding it to Git.

Revision bd8319f3 (diff)
Added by segfault 12 days ago

Don't set browser.uiCustomization.state twice in Unsafe Browser prefs (refs: #17055)

This is a fixup against 05acf4b8eb1671ad9015a116463ea0b78f807385, which
sets the browser.uiCustomization.state in
config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/prefs.js,
but we already write that pref to that file during build time in
apply_prefs_hacks() in config/chroot_local-hooks/10-tbb.

Revision 3d00da9a (diff)
Added by segfault 11 days ago

Create addonStartup.json.lz4 to enable red theme in Unsafe Browser (refs: #17055)

Revision 1307e974 (diff)
Added by segfault 11 days ago

Use a simpler method to hide the security level button in Unsafe Browser (refs: #17055)

Revision c40d2d01 (diff)
Added by segfault 11 days ago

Add comment explaining why we set prefs for the Unsafe Browser (refs: #17055)

Revision 3ed1d036 (diff)
Added by segfault 11 days ago

Remove unneeded pref (refs: #17055)

This is a fixup against 05acf4b8eb1671ad9015a116463ea0b78f807385. The
pref is not actually needed to enable the red theme.

Revision 71f7c423 (diff)
Added by intrigeri 10 days ago

Test suite: update Unsafe Browser reference images (refs: #17055)

Revision 71a0f2ce (diff)
Added by intrigeri 10 days ago

Test suite: drop obsolete Unsafe Browser bookmarks check (refs: #17055)

There's no such thing as "^place:(sort|folder|type)=" anymore.
I've checked the bookmarks setup and I think that checking URIs
is good enough for us.

Revision d8dd21bd (diff)
Added by intrigeri 10 days ago

Test suite: update Unsafe Browser reference image (refs: #17055)

Revision e6f50683 (diff)
Added by segfault 10 days ago

Don't ship python3-lz4 (refs: #17055)

It is only required in the 11-unsafe-browser hook, so we use
ensure_hook_dependency_is_installed() to install/remove it there.

History

#1 Updated by intrigeri about 1 month ago

#2 Updated by intrigeri about 1 month ago

  • Description updated (diff)
  • Assignee deleted (intrigeri)

#3 Updated by intrigeri about 1 month ago

  • Description updated (diff)

#4 Updated by intrigeri about 1 month ago

  • Status changed from Confirmed to In Progress
  • Assignee set to intrigeri

#5 Updated by intrigeri about 1 month ago

  • Description updated (diff)
  • Assignee deleted (intrigeri)

#6 Updated by intrigeri about 1 month ago

the custom theme (grep /usr/share/pixmaps/red_dot.png) is not applied

I'll take this one as this breaks test suite steps before we can test more relevant stuff.

#7 Updated by intrigeri about 1 month ago

intrigeri wrote:

the custom theme (grep /usr/share/pixmaps/red_dot.png) is not applied

I'll take this one as this breaks test suite steps before we can test more relevant stuff.

Well, https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes#Documentation says that "Lightweight themes have been deprecated". This probably explains why our custom lightweightThemes prefs are ignored.

I see two options:

Either way, we'll need to include a (signed) XPI in our Git tree, so there's no benefit in creating our own theme, if we can find an existing one that does the job and has a compatible license.

#8 Updated by intrigeri about 1 month ago

intrigeri wrote:

Done, and it's installed by default (as per about:addons) but not enabled.

#9 Updated by intrigeri about 1 month ago

To be clear, I've released the "lock" I've set on part of this ticket yesterday. I don't plan to work on it again before next Thursday.

#10 Updated by hefee 15 days ago

  • Assignee set to hefee

#11 Updated by hefee 14 days ago

  • Assignee changed from hefee to anonym

I got a working hackish version - i used 05acf4b8eb1671ad9015a116463ea0b78f807385 for user.js.

I needed additionally addonStartup.json.lz4 from a running unsafe-browser:
sudo unsafe-broswer
(while running in a next window)
cp /var/lib/unsafe-browser/chroot/home/clearnet/.unsafe-browser/profile.default/addonStartup.json.lz4 /tmp

(than I modified /usr/local/lib/tails-shell-library/chroot-browser.sh)

diff --git a/usr/local/lib/tails-shell-library/chroot-browser.sh b/usr/local/lib/tails-shell-library/chroot-browser.sh
index 8b31e7d60c..46c80463f8 100644
--- a/config/chroot_local-includes/usr/local/lib/tails-shell-library/chroot-browser.sh
+++ b/config/chroot_local-includes/usr/local/lib/tails-shell-library/chroot-browser.sh
@@ -237,6 +237,10 @@ run_browser_in_chroot () {
     local wm_class="${5}" 
     local profile="$(browser_profile_dir ${browser_name} ${chroot_user})" 

+    cp /tmp/addonStartup.json.lz4 ${profile}/addonStartup.json.lz4
+    set_chroot_browser_permissions "${chroot}" "${browser_name}" \
+           ${profile}/addonStartup.json.lz4
+
     sudo -u "${local_user}" xhost "+SI:localuser:${chroot_user}" 
     chroot "${chroot}" sudo -u "${chroot_user}" /bin/sh -c \
         ". /usr/local/lib/tails-shell-library/tor-browser.sh && \

with this I can see the red theme and don't see the security level button.

#12 Updated by anonym 14 days ago

  • Assignee changed from anonym to hefee

hefee wrote:

I got a working hackish version - i used 05acf4b8eb1671ad9015a116463ea0b78f807385 for user.js.

First of all, I can reproduce!

Second, as I said on XMPP, we can probably avoid setting browser.uiCustomization.state, which is nice since it sets the whole state, so we will quickly desync if upstream changes. The other way to hide it is through userChrome.css, we just have to figure out the element name.

Third, I pushed 7d39b3ac357d8ebc8253a80c86d2103f70650af4.

I needed additionally addonStartup.json.lz4 from a running unsafe-browser:

Sadly, this might be hard for us to solve elegantly.

One solution would be to generate it ourselves at build time. The first problem with that is that Mozilla uses a home-grown lz4 variant, so e.g. liblz4-tool is useless. I found a webextension that can compress/decompress which clearly cannot be used at build time. Any way, I decompressed it, and it has this format:
{
    "app-profile": {
        "addons": { ... one entry per extension, langpack, theme ...},
        "staged": {}
    }
}

I tried generating addonStartup.json.lz4 in the following ways:
  • Empty json
  • Setting addons to an empty {}
  • Setting addons to a proper entry for only the theme

In each case Firefox read it (according to strace) and then wrote its own version over it. It seem to pick up the info I put there: in the last test, I tried putting crazy values in the theme's entry, and they did propagate into the one written by Firefox. So it seems the problem triggers unless we have a full addonStartup.json.lz4, so I guess we'll have to ship and maintain that until we find another solution, I guess.

That extensions.activeThemeID is not respected on first start must be a bug, which we should report to Mozilla. But first we should reproduce in a normal Firefox install, not Tor Browser let alone the Unsafe Browser, and also look if it is fixed in more recent (non-ESR) Firefox. With some luck (frankly I don't know their merge policy) maybe we can get this fixed in ESR68 so we don't have to ship a full addonStartup.json.lz4 for long.

#13 Updated by hefee 14 days ago

  • Assignee deleted (hefee)

It is up for grab again

#14 Updated by segfault 14 days ago

That extensions.activeThemeID is not respected on first start must be a bug, which we should report to Mozilla. But first we should reproduce in a normal Firefox install, not Tor Browser let alone the Unsafe Browser, and also look if it is fixed in more recent (non-ESR) Firefox

I did that with Firefox 69.0.1 on sid. I did what b337af619b931a0b7ee25ad81b7cfb1c012da089 does:

ln -s config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/extensions/red-2.0-an+fx.xpi \
    ~/.mozilla/firefox/*.default/extensions/{91a24c60-0f27-427c-b9a6-96b71f3984a9}.xpi
echo 'user_pref("extensions.activeThemeID", "{91a24c60-0f27-427c-b9a6-96b71f3984a9}");' >> \
    ~/.mozilla/firefox/*.default/prefs.js

When I started Firefox afterwards, it notified me that the add-on "red" was installed by another program on my computer, and whether I want to enable it. When I click "Enable", it does get enabled.

The same also happens if I don't add the extensions.activeThemeID user pref.

So the behavior is different and it doesn't matter whether extensions.activeThemeID is set, because Firefox asks the user whether they want to enable it.

#15 Updated by intrigeri 12 days ago

Semi-random brain dump follows, use with care!

First, I'm worried that we're approaching this problem — and a number of other Firefox-related matters — with a strategy that is unsupported by Mozilla, requires more and more hacks, and may be the root cause for more and more weird behavior (e.g. #17121, #17007). That might be related to our status as a downstream of a downstream vendor (Tor Browser) that already diverges from upstream (Mozilla) vs. the set of available (pre-)configuration "hooks". It started to become apparent to me back when we had to start patching prefs inside omni.ja.

Tor Browser upstream already occupies some of the available customization "hooks", and since years it's doing so deeper and deeper in Firefox internals: setting default prefs in omni.ja, turning some add-ons to "system" ones that can't be disabled by the user, and so on. On top of that, Tor Browser still ships a default profile (that currently boils down to bookmarks.html + 2 × XPIs). On top of that, we patch omni.ja in various ways and repack it with a tool that's not meant to do that, add our own bits to the default profile, move add-ons elsewhere and symlink them from the profile (only to avoid duplicating add-ons when we create the Unsafe Browser profile), add our own add-ons (uBlock and langpacks), and finally do some more tweaks at runtime before we start the browser.

What I'm trying to say is that essentially, we're trying to emulate a human user configuring their Firefox profile. I'm not surprised that this approach starts to break down in various places. And when I read segfault's "was installed by another program on my computer", it becomes obvious to me that if we try to emulate a human user, then we'll get feedback from Firefox that's targeted to human users, instead of behavior optimized for batch operations.

Mozilla provides other ways for "enterprise" deployments to pre-configure Firefox (and to lock down bits of this config). The supported tools to do so have changed a lot over the last few years so I'm glad we did not try one before: we would have had to switch technologies at least twice since. But it seems that the dust is settling now and ESR68 has support for policies set via policies.json, which essentially reimplements CCK2 directly in Firefox, and that apparently provides ways to force installing a given XPI:

Perhaps it would be worth trying this for the red theme add-on?

With some luck (frankly I don't know their merge policy) maybe we can get this fixed in ESR68 so we don't have to ship a full addonStartup.json.lz4 for long.

Two things about this:

  • If the bug is fixed in Firefox VCS upstream (which might require first reporting it), the Tor Browser team might be open to backport the fix for us.
  • Back in the days, we started Firefox at build time (yes, seriously) to generate bits of the profile (for some reason I can't recall). That's definitely ugly but perhaps less so than including addonStartup.json.lz4 in our Git tree and then updating it regularly until this bug is fixed for real.

Second, as I said on XMPP, we can probably avoid setting browser.uiCustomization.state, which is nice since it sets the whole state, so we will quickly desync if upstream changes. The other way to hide it is through userChrome.css, we just have to figure out the element name.

About this, also see "Hide the security level button in the unsafe browser (#16735)" in 10-tbb, which might provide hints to do what you're trying to do (or which might actually break stuff so perhaps it's worth trying without it :)

#16 Updated by intrigeri 12 days ago

  • Priority changed from Elevated to High

#17 Updated by segfault 12 days ago

intrigeri wrote:

Mozilla provides other ways for "enterprise" deployments to pre-configure Firefox (and to lock down bits of this config). The supported tools to do so have changed a lot over the last few years so I'm glad we did not try one before: we would have had to switch technologies at least twice since. But it seems that the dust is settling now and ESR68 has support for policies set via policies.json, which essentially reimplements CCK2 directly in Firefox, and that apparently provides ways to force installing a given XPI:

Perhaps it would be worth trying this for the red theme add-on?

I just tried this. It works flawlessly with my Firefox on sid, but not at all with Tor Browser. I found these relevant issues on trac.torproject.org:

It was noticed that the policies.json doesn't work in Tor Browser, because the update channel is not "release". That was fixed in tor-browser-60.5.1esr-8.5-1.

It was noticed that the policies.json could be used to bypass the the proxy. browser.policies.testing.disallowEnterprise was set to true by default and the fix from 29445 was reverted.

Maybe we could ask whether the fix from 29445 could be reintroduced, so that we can use the policies.json.

Just in case we need this later: Adding this to /etc/tor-browser/profile/prefs.js changed the pref value, setting it in /usr/share/tails/tor-browser-prefs.js did not:

user_pref("browser.policies.testing.disallowEnterprise", false);

#18 Updated by segfault 12 days ago

Maybe we could ask whether the fix from 29445 could be reintroduced, so that we can use the policies.json.

I did that here: https://trac.torproject.org/projects/tor/ticket/31978

#19 Updated by anonym 12 days ago

@intrigeri wrote:

Mozilla provides other ways for "enterprise" deployments to pre-configure Firefox (and to lock down bits of this config).

I feel like an idiot for forgetting to mention this, but I did try one of the old Enterprisy approaches: autoconfig.js. Using it I could change any pref just fine, but it too failed with activeThemeID. My theory was that the existence of a good addonStartup.json.lz4 would make Firefox take different code paths, and in the case of non-existence it overwrites the pref with the default no matter what.

@segfault wrote:

I just tried [the policies.json approach]. It works flawlessly with my Firefox on sid, but not at all with Tor Browser.

What is "works" referring to here? That you can set some pref, or activeThemeID specifically?


In related news, I actually managed to make a minimal addonStartup.json.lz4 that only contains the metadata for the red theme (so we don't have to maintain it unless we update the theme!). Attached! So we actually have a pretty good workaround.

#20 Updated by segfault 12 days ago

  • Assignee deleted (hefee)
  • Priority changed from Elevated to High

anonym wrote:

@segfault wrote:

I just tried [the policies.json approach]. It works flawlessly with my Firefox on sid, but not at all with Tor Browser.

What is "works" here? That you can set some pref, of activeThemeID specifically?

I simply created the following policies.json in /usr/lib/firefox/distribution/ and started Firefox, which then had the red theme enabled. I now reproduced the same with firefox-esr installed on an image built from the feature branch.

{
  "policies": {
    "Extensions": {
      "Install": ["file:///usr/share/tails/chroot-browsers/unsafe-browser/extensions/red-2.0-an+fx.xpi"],
      "Uninstall": [],
      "Locked":  []
    }
  }
}

#21 Updated by segfault 12 days ago

anonym wrote:

In related news, I actually managed to make a minimal addonStartup.json.lz4 that only contains the metadata for the red theme (so we don't have to maintain it unless we update the theme!). Attached! So we actually have a pretty good workaround.

Great! And I found this script on stackexchange.com which can compress and extract Mozilla's jsonlz4:

#!/usr/bin/env python3
from sys import stdin, stdout, argv, stderr
import os
try:
    import lz4.block as lz4
except ImportError:
    import lz4

stdin = os.fdopen(stdin.fileno(), 'rb')
stdout = os.fdopen(stdout.fileno(), 'wb')

if argv[1:] == ['-c']:
    stdout.write(b'mozLz40\0' + lz4.compress(stdin.read()))
elif argv[1:] == ['-d']:
    assert stdin.read(8) == b'mozLz40\0'
    stdout.write(lz4.decompress(stdin.read()))
else:
    stderr.write('Usage: %s -c|-d < infile > outfile\n' % argv[0])
    stderr.write('Compress or decompress Mozilla-flavor LZ4 files.\n\n')
    stderr.write('Examples:\n')
    stderr.write('\t%s -d < infile.json.mozlz4 > outfile.json\n' % argv[0])
    stderr.write('\t%s -c < infile.json > outfile.json.mozlz4\n' % argv[0])
    exit(1)

I just had to install python3-lz4 (and I changed the shebang to use python3).

We could use that to create the addonStartup.json.lz4 at build time.

#22 Updated by segfault 12 days ago

  • Description updated (diff)

intrigeri:

Second, as I said on XMPP, we can probably avoid setting browser.uiCustomization.state, which is nice since it sets the whole state, so we will quickly desync if upstream changes. The other way to hide it is through userChrome.css, we just have to figure out the element name.

About this, also see "Hide the security level button in the unsafe browser (#16735)" in 10-tbb, which might provide hints to do what you're trying to do (or which might actually break stuff so perhaps it's worth trying without it :)

Actually, in 10-tbb, we already set browser.uiCustomization.state during build time, and we use the value from upstream and just strip the security-level-button from it. So we don't have to set it again in /usr/share/tails/chroot-browsers/unsafe-browser/prefs.js. I reverted that part of hefee's commit.

#23 Updated by segfault 12 days ago

anonym wrote:

In related news, I actually managed to make a minimal addonStartup.json.lz4 that only contains the metadata for the red theme (so we don't have to maintain it unless we update the theme!). Attached! So we actually have a pretty good workaround.

I can confirm that copying this addonStartup.json.lz4 to the Unsafe Browser's profile directory enables the red theme. Do you prefer copying this as is to config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/addonStartup.json.lz4, or should we create it via the Python script in a build hook?

#24 Updated by intrigeri 12 days ago

I can confirm that copying this addonStartup.json.lz4 to the Unsafe Browser's profile directory enables the red theme. Do you prefer copying this as is to config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/addonStartup.json.lz4, or should we create it via the Python script in a build hook?

If the extra cost is reasonable, creating it during the build would be quite nicer (as is: one can audit and review changes) than going back to tho (not-)good old times when we were putting all kinds of binary stuff in our Git tree. But that's definitely not worth spending days on it IMO.

#25 Updated by segfault 11 days ago

intrigeri wrote:

I can confirm that copying this addonStartup.json.lz4 to the Unsafe Browser's profile directory enables the red theme. Do you prefer copying this as is to config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/addonStartup.json.lz4, or should we create it via the Python script in a build hook?

If the extra cost is reasonable, creating it during the build would be quite nicer (as is: one can audit and review changes) than going back to tho (not-)good old times when we were putting all kinds of binary stuff in our Git tree. But that's definitely not worth spending days on it IMO.

I implemented something, will test it tomorrow.

#26 Updated by anonym 11 days ago

segfault wrote:

Actually, in 10-tbb, we already set browser.uiCustomization.state during build time, and we use the value from upstream and just strip the security-level-button from it. So we don't have to set it again in /usr/share/tails/chroot-browsers/unsafe-browser/prefs.js. I reverted that part of hefee's commit.

Unless I am missing something you didn't replace that solution with something else. Any way, instead of reading the default browser.uiCustomization.state from omni.ja and filtering it according to our needs, we can hide the security level button much more easily by dropping just this into the Unsafe Browser's userChrome.css:

#security-level-button {display: none !important}

#27 Updated by segfault 11 days ago

anonym wrote:

segfault wrote:

Actually, in 10-tbb, we already set browser.uiCustomization.state during build time, and we use the value from upstream and just strip the security-level-button from it. So we don't have to set it again in /usr/share/tails/chroot-browsers/unsafe-browser/prefs.js. I reverted that part of hefee's commit.

Unless I am missing something you didn't replace that solution with something else.

I didn't replace it with something else because the code is already there. See apply_prefs_hacks in 10-tbb.

Any way, instead of reading the default browser.uiCustomization.state from omni.ja and filtering it according to our needs, we can hide the security level button much more easily by dropping just this into the Unsafe Browser's userChrome.css:
[...]

OK, that does seem easier. I will try to do that during build instead of changing browser.uiCustomization.state.

#28 Updated by segfault 11 days ago

  • Description updated (diff)

I implemented something, will test it tomorrow.

Done, it works.

OK, that does seem easier. I will try to do that during build instead of changing browser.uiCustomization.state.

Did that too, also seems to work (but I only tested that in a running Tails yet).

I think the only thing left to do here is to check whether the other customizations are applied.

#29 Updated by segfault 11 days ago

  • Status changed from In Progress to Needs Validation

I think the only thing left to do here is to check whether the other customizations are applied.

I checked that the other preferences set in config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/prefs.js work as expected. I'm not sure if there are any other customizations. If not, this should be ready for review.

#30 Updated by intrigeri 10 days ago

  • Assignee set to intrigeri

Hi!

I think the only thing left to do here is to check whether the other customizations are applied.

I checked that the other preferences set in config/chroot_local-includes/usr/share/tails/chroot-browsers/unsafe-browser/prefs.js work as expected.

Woohoo! I'll review this today.

I'm not sure if there are any other customizations. If not, this should be ready for review.

I don't remember by heart if we apply other customizations (be it at build time or at runtime) but I'm happy to check this as part of the review.

#31 Updated by intrigeri 10 days ago

  • Status changed from Needs Validation to In Progress

First, congrats you all! It's been an impressive piece of collective work.

My code review yielded only one question: my understanding is that we need python3-lz4 only at build time so perhaps we should not ship it in the images we release? We could use ensure_hook_dependency_is_installed() to install/remove it. Happy to do this myself in time for 4.0~rc1 if you can't.

On the test suite front, some Unsafe Browser scenarios of our test suite still fail:

  • "the Unsafe Browser has no add-ons installed": we're looking for a "You don't have any add-ons […]" string but the GUI does not display anything special anymore when there's no add-on, just an empty page → updated the image to include lots of the blank space, now works fine.
  • "The Unsafe Browser cannot be configured to use Tor and other local proxies" → updated picture.
  • "The Unsafe Browser will not make any connections to the Internet which are not user initiated" sees some DNS requests → see you on #17130

And of course, given we test multiple things in these scenarios, once I fixed the aforementioned issues, it only allowed me to see that the next steps failed too, so I updated them too.

The full unsafe_browser.feature now Works On My Machine™, apart of #17130, yeah!

So, apart of the python3-lz4 thing, the only thing left here is:

intrigeri wrote:

segfault wrote:

I'm not sure if there are any other customizations. If not, this should be ready for review.

I don't remember by heart if we apply other customizations (be it at build time or at runtime) but I'm happy to check this as part of the review.

In the end, I've spent on the test suite the time I thought I would invest into this ⇒ I won't have time to do this today ⇒ someone else could do it, if you'd like and can spare the time :)

#32 Updated by intrigeri 10 days ago

  • Assignee deleted (intrigeri)

#33 Updated by segfault 10 days ago

intrigeri wrote:

My code review yielded only one question: my understanding is that we need python3-lz4 only at build time so perhaps we should not ship it in the images we release? We could use ensure_hook_dependency_is_installed() to install/remove it.

Done

#34 Updated by segfault 10 days ago

I noticed that we still set the unsupported app.update.enabled in /usr/share/tails/chroot-browsers/common/prefs.js.

I'm also wondering why we have these "common" preferences at all. IIUC, it is only used by the Unsafe Browser. Was there a plan to support other chroot browsers? Is that still the case? If not, could we simplify the structure by removing the unnecessary "chroot browsers" abstraction?

#35 Updated by anonym 10 days ago

segfault wrote:

I noticed that we still set the unsupported app.update.enabled in /usr/share/tails/chroot-browsers/common/prefs.js.

Ah, so we need to copy the prefs from e43247dd2558dd391342855796e18c3186a43807 in that file too!

And one Unsafe Browser scenario ("The Unsafe Browser will not make any connections to the Internet which are not user initiated") actually checks that we have app.update.enabled set to false. But I think that test seems kinda pointless and should be removed. I wonder if the "I configure the Unsafe Browser to check for updates more frequently" step needs an update, though, or if those prefs still do what they used to when this test was developed.

I'm also wondering why we have these "common" preferences at all. IIUC, it is only used by the Unsafe Browser. Was there a plan to support other chroot browsers? Is that still the case?

There used to be an I2P Browser at one point. :)

If not, could we simplify the structure by removing the unnecessary "chroot browsers" abstraction?

If you only refer to the files in /usr/share/tails/chroot-browsers, then sure, merge the common part into unsafe-browser. OTOH it seems we need a common part between Tor Browser and Unsafe Browser, e.g. for disabling the update... :)

#36 Updated by segfault 10 days ago

anonym wrote:

segfault wrote:

I noticed that we still set the unsupported app.update.enabled in /usr/share/tails/chroot-browsers/common/prefs.js.

Ah, so we need to copy the prefs from e43247dd2558dd391342855796e18c3186a43807 in that file too!

I don't think so. The preferences we set in /usr/share/tails/tor-browser-prefs.js are also applied to the Unsafe Browser, because they are added at build time, in 10-tbb, to the omni.ja, and the Unsafe Browser uses the same omni.ja. Else, we wouldn't have had #17130, because we only set the update URL in tor-browser-prefs.js, but see the effect in the Unsafe Browser.

I think our whole Tor Browser / Unsafe Browser customization is too complex and not well structured - which becomes apparent by your confusion and the fact that both intrigeri (while implementing) and me (while reviewing) did not think about the consequences of the update URL for the Unsafe Browser. That's why I would like to refactor a few things here.

I'm also wondering why we have these "common" preferences at all. IIUC, it is only used by the Unsafe Browser. Was there a plan to support other chroot browsers? Is that still the case?

There used to be an I2P Browser at one point. :)

Ah, I see.

If not, could we simplify the structure by removing the unnecessary "chroot browsers" abstraction?

If you only refer to the files in /usr/share/tails/chroot-browsers, then sure, merge the common part into unsafe-browser.

I would also like to remove the abstraction from chroot-browser.sh (and rename it to unsafe-browser.sh).

OTOH it seems we need a common part between Tor Browser and Unsafe Browser, e.g. for disabling the update... :)

See my explanation above why /usr/share/tails/tor-browser-prefs.js is already shared between Tor Browser and Unsafe Browser.

#37 Updated by anonym 10 days ago

@segfault, I will read your comments and review your commits tomorrow. I just pushed a few tiny fixes (and a few similar ones to come tomorrow) and wonder if you could review them(?).

#38 Updated by segfault 10 days ago

  • Feature Branch set to feature/16356-tor-browser-9.0+force-all-tests

anonym wrote:

@segfault, I will read your comments and review your commits tomorrow. I just pushed a few tiny fixes (and a few similar ones to come tomorrow) and wonder if you could review them(?).

Sure, will do.

#39 Updated by segfault 10 days ago

segfault wrote:

anonym wrote:

@segfault, I will read your comments and review your commits tomorrow. I just pushed a few tiny fixes (and a few similar ones to come tomorrow) and wonder if you could review them(?).

Sure, will do.

LGTM

#40 Updated by intrigeri 9 days ago

Hi,

segfault wrote:

anonym wrote:

segfault wrote:

I think our whole Tor Browser / Unsafe Browser customization is too complex and not well structured - which becomes apparent by your confusion and the fact that both intrigeri (while implementing) and me (while reviewing) did not think about the consequences of the update URL for the Unsafe Browser. That's why I would like to refactor a few things here.

I'm also wondering why we have these "common" preferences at all. IIUC, it is only used by the Unsafe Browser. Was there a plan to support other chroot browsers? Is that still the case?

There used to be an I2P Browser at one point. :)

Ah, I see.

If not, could we simplify the structure by removing the unnecessary "chroot browsers" abstraction?

If you only refer to the files in /usr/share/tails/chroot-browsers, then sure, merge the common part into unsafe-browser.

I would also like to remove the abstraction from chroot-browser.sh (and rename it to unsafe-browser.sh).

See #12264#note-29 for the next steps wrt. I2P. If we decide on #16531 that we won't reintroduce I2P, then yes, I'm all for simplifying all this!

#41 Updated by anonym 9 days ago

segfault wrote:

anonym wrote:

segfault wrote:

I noticed that we still set the unsupported app.update.enabled in /usr/share/tails/chroot-browsers/common/prefs.js.

Ah, so we need to copy the prefs from e43247dd2558dd391342855796e18c3186a43807 in that file too!

I don't think so. The preferences we set in /usr/share/tails/tor-browser-prefs.js are also applied to the Unsafe Browser, because they are added at build time, in 10-tbb, to the omni.ja, and the Unsafe Browser uses the same omni.ja. Else, we wouldn't have had #17130, because we only set the update URL in tor-browser-prefs.js, but see the effect in the Unsafe Browser.

Derp! :)

I think our whole Tor Browser / Unsafe Browser customization is too complex and not well structured - which becomes apparent by your confusion and the fact that both intrigeri (while implementing) and me (while reviewing) did not think about the consequences of the update URL for the Unsafe Browser. That's why I would like to refactor a few things here.

Got it! But before we get rid of this abstraction, can we perhaps just improve the structure etc of the parts you find too complex?

#42 Updated by anonym 9 days ago

  • Status changed from In Progress to Needs Validation

I've looked at the "other customizations", and my last few commits fixes what I found and needs review.

I think what remains is just checking how the Unsafe Browser works in other locales (and related: #17140).

#43 Updated by intrigeri 9 days ago

  • Assignee set to intrigeri

anonym wrote:

I've looked at the "other customizations", and my last few commits fixes what I found and needs review.

I'll review.

I think what remains is just checking how the Unsafe Browser works in other locales (and related: #17140).

I'll check this and we'll see if I'm lucky and spot the reason for #17140 — but at this point I'm not committing to work on #17140 more than this.

#44 Updated by intrigeri 9 days ago

  • Related to Bug #17142: New Unsafe Browser tabs have the "Private Browsing" name and the Tor Browser icon added

#45 Updated by intrigeri 9 days ago

  • Status changed from Needs Validation to Resolved

intrigeri wrote:

anonym wrote:

I've looked at the "other customizations", and my last few commits fixes what I found and needs review.

I'll review.

LGTM!

I think what remains is just checking how the Unsafe Browser works in other locales (and related: #17140).

I'll check this

All tweaks that depend on the locale look OK in French except:

  • The name of a newly opened tab is "Navigation privée" (~= "Private browsing"). While on Tails 3.16, it's "Nouvel onglet" (~= "New tab") so this is a regression. I've filed #17142 about this so we can finally close this ticket, which is becoming a bit hard for me to follow.
  • The UI is localized but about:preferences is in English. Same in Tor Browser 9.0a7 on my sid system. It looks like this is https://trac.torproject.org/projects/tor/ticket/31747 or https://trac.torproject.org/projects/tor/ticket/28196, which got fixes committed upstream recently, so I'm confident we'll be good once we'll have imported a new set of Tor Browser tarballs.

we'll see if I'm lucky and spot the reason for #17140 — but at this point I'm not committing to work on #17140 more than this.

It turns out that #17140 seems to be an upstream Tor Browser bug, not merely an Unsafe Browser issue, so it's out of scope here.

#46 Updated by intrigeri 9 days ago

  • Assignee deleted (intrigeri)

Also available in: Atom PDF