Project

General

Profile

Bug #17142

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

New Unsafe Browser tabs have the "Private Browsing" name and the Tor Browser icon

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

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
-
Target version:
Start date:
Due date:
% Done:

0%

Feature Branch:
bugfix/17142-unsafe-browser-tabs
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Unsafe Browser

Description

Regression with Tor Browser 9.0a7.

Might be because Torbutton, which is now integrated, in Tor Browser, forces private browsing mode even for the Unsafe Browser.


Related issues

Related to Tails - Feature #17055: Update Unsafe Browser tweaks for Tor Browser 9 Resolved 01/14/2019
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

Associated revisions

Revision d1c0dec4 (diff)
Added by segfault about 1 month ago

Fix private browsing mode enabled by default in the Unsafe Browser (refs: #17142)

Revision 2acaeac9 (diff)
Added by segfault about 1 month ago

Remove Tor Browser icons in Unsafe Browser (refs: #17142)

Revision 5552dffb (diff)
Added by segfault about 1 month ago

Remove Tor Browser icons in Unsafe Browser (refs: #17142)

Revision 1186b584 (diff)
Added by segfault 30 days ago

Fix private browsing mode enabled by default in the Unsafe Browser (refs: #17142)

Revision 07d7daae (diff)
Added by segfault 30 days ago

Remove Tor Browser icons in Unsafe Browser (refs: #17142)

Revision 99485622 (diff)
Added by intrigeri 29 days ago

Ensure browser/omni.ja has the expected permissions (refs: #17142)

We do this in delete_chroot_browser_searchplugins(), presumably
to avoid issues when run under a strict umask. Let's make
these operations consistent.

Revision cf815094
Added by anonym 29 days ago

Merge remote-tracking branch 'origin/bugfix/17142-unsafe-browser-tabs' into testing

Fix-committed: #17142

History

#1 Updated by intrigeri about 1 month ago

#2 Updated by intrigeri about 1 month ago

  • Related to Feature #17055: Update Unsafe Browser tweaks for Tor Browser 9 added

#3 Updated by intrigeri about 1 month ago

  • Parent task set to #16356

#4 Updated by segfault about 1 month ago

Maybe setting browser.privatebrowsing.autostart to false fixes the private browsing part.

#5 Updated by segfault about 1 month ago

  • Assignee set to segfault

segfault wrote:

Maybe setting browser.privatebrowsing.autostart to false fixes the private browsing part.

That seems to work.

Regarding the icon: I tried to fix this by removing the icons in /var/lib/unsafe-browser/chroot//usr/local/lib/tor-browser/browser/chrome/icons/default in the configure_chroot_browser() function, but that didn't work. I have no idea where else Tor Browser gets this icon from.

#6 Updated by intrigeri about 1 month ago

I have no idea where else Tor Browser gets this icon from.

It could be bundled in one of the omni.ja (be it in a separate file or as data in some XML or whatever file).

#7 Updated by segfault about 1 month ago

  • Status changed from Confirmed to In Progress

#8 Updated by segfault about 1 month ago

  • Status changed from In Progress to Confirmed
  • Feature Branch set to bugfix/17142-unsafe-browser-tabs

intrigeri wrote:

I have no idea where else Tor Browser gets this icon from.

It could be bundled in one of the omni.ja (be it in a separate file or as data in some XML or whatever file).

Indeed, there are also Tor Browser icons in chrome/browser/content/branding in /usr/local/lib/tor-browser/browser/omni.ja, and when those are removed, the tabs don't have the icon anymore.

#9 Updated by segfault about 1 month ago

  • Status changed from Confirmed to In Progress

#10 Updated by segfault about 1 month ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)

#11 Updated by intrigeri 30 days ago

  • Assignee set to intrigeri

#12 Updated by segfault 30 days ago

  • Status changed from Needs Validation to In Progress

#13 Updated by intrigeri 30 days ago

  • Status changed from In Progress to Needs Validation

Here as well, I've transplanted the branch onto testing (git rebase --onto testing devel bugfix/17142-unsafe-browser-tabs) so we can have fix in 4.0.

#14 Updated by intrigeri 29 days ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to segfault

Hi segfault,

unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?

One thing I've noticed while reviewing the branch is that this introduces a 2nd case when we're de+recompressing "${chroot}/${TBB_INSTALL}/browser/omni.ja" when starting the Unsafe Browser. I'm a bit concerned that it could make it even slower to start than it already is. But I'm not sure how we could fix that, short of updating this archive when we build images. Perhaps combining the newly introduced code with delete_chroot_browser_searchplugins() would help. Anyway, I did not measure the actual impact of this branch so for now: whatever.

#15 Updated by segfault 29 days ago

intrigeri wrote:

Hi segfault,

unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?

I'll take a look now. If I can't fix this today, I won't have time before Sunday evening, so in that case you might want to take over.

One thing I've noticed while reviewing the branch is that this introduces a 2nd case when we're de+recompressing "${chroot}/${TBB_INSTALL}/browser/omni.ja" when starting the Unsafe Browser.

No, it does not. It only uses 7z d to delete files from an archive without de+recompressing the whole thing. That's done in a split second.

I'm a bit concerned that it could make it even slower to start than it already is. But I'm not sure how we could fix that, short of updating this archive when we build images.

I also thought about that, and actually even implemented something in the 11-unsafe-browser hook. But that adds another 17M to the image, and I'm not sure if that's worth it. Also, while debugging the Unsafe Browser setup script, I noticed that compared to the start time of the Browser itself, the de+recompressing doesn't take that long.

Perhaps combining the newly introduced code with delete_chroot_browser_searchplugins() would help.

We could combine set_chroot_browser_name() and delete_chroot_browser_searchplugins() to only decompress once.

#16 Updated by segfault 29 days ago

  • Assignee changed from segfault to intrigeri

segfault wrote:

intrigeri wrote:

Hi segfault,

unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?

I'll take a look now. If I can't fix this today, I won't have time before Sunday evening, so in that case you might want to take over.

In the videos of the failed test scenarios the Unsafe Browser just doesn't start. But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start. So I don't know how to debug this further. Maybe you could ssh into a Jenkins VM and investigate there.

We could combine set_chroot_browser_name() and delete_chroot_browser_searchplugins() to only decompress once.

Regarding this, I noticed that the decompression in delete_chroot_browser_searchplugins() is actually unnecessary. I pushed a commit which removes that.

#17 Updated by intrigeri 29 days ago

Hi,

first, I've read your comments wrt. omni.ja-related stuff and I fully agree with your conclusions.

Regarding the test suite regression:

segfault wrote:

In the videos of the failed test scenarios the Unsafe Browser just doesn't start. But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start. So I don't know how to debug this further. Maybe you could ssh into a Jenkins VM and investigate there.

I took a look at the Journal for the first failing scenario and I see this:

Oct 18 04:10:06 amnesia unsafe-browser.desktop[6228]: * Starting Unsafe Browser
Oct 18 04:10:06 amnesia sudo[8031]:     root : TTY=tty2 ; PWD=/home/amnesia ; USER=amnesia ; COMMAND=/usr/bin/xhost +SI:localuser:clearnet
Oct 18 04:10:06 amnesia sudo[8031]: pam_unix(sudo:session): session opened for user amnesia by (uid=0)
Oct 18 04:10:06 amnesia unsafe-browser.desktop[6228]: localuser:clearnet being added to access control list
Oct 18 04:10:06 amnesia sudo[8031]: pam_unix(sudo:session): session closed for user amnesia
Oct 18 04:10:07 amnesia systemd[4872]: gnome-terminal-server.service: Succeeded.
Oct 18 04:10:12 amnesia unsafe-browser.desktop[6228]: Fontconfig warning: "/usr/local/lib/tor-browser/TorBrowser/Data/fontconfig/fonts.conf", line 85: unknown element "blank" 
Oct 18 04:10:22 amnesia unsafe-browser.desktop[6228]: Segmentation fault

Interesting. I'll try to reproduce locally.

Note: we had a similar issue a few weeks ago in the very same area, with the very same result, that I fixed with 6a14bcfac80a1d1f9a197a78355999a861045150 back then.

#18 Updated by intrigeri 29 days ago

Interestingly, the "Unsafe Browser can be used in all languages supported in Tails" scenario passes. The main difference is that in that scenario, we start it from a terminal instead of the Activities Overview. I have a hunch that it's related to umask and files permissions (elsewhere we explicitly do chmod a+r "${pack}" but delete_chroot_browser_icons() does not); I will play a bit with this.

Regarding this, I noticed that the decompression in delete_chroot_browser_searchplugins() is actually unnecessary. I pushed a commit which removes that.

Sweet! The updated code implicitly relies on the fact that there's no shell expansion for "${searchplugins_dir}"/*/manifest.json (and thus the glob is passed to 7z as-is). I'll push a follow-up commit to fix that. Not that I think this glob will ever expand in practice, but developers would have to wonder about this every time they work on this piece of code, which is a tiny bit distracting.

#19 Updated by intrigeri 29 days ago

But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start.

I can reproduce this but only if I run sudo unsafe-browser in a Terminal (non-login shell, umask = 022). But if I start the Unsafe Browser from the Activities Overview (where our custom umask = 077 is honored), then I reproduce the problem that Jenkins pointed us to. The umask divergence could be seen as a bug, feel free to file a ticket about it if you'd like. Anyhow, this shows us that it's important, when developing, to test stuff in a way that's as close as possible to what actual users would do :)

I've pushed a commit that fixes this in my manual tests. Let's see if this fixes the test suite!

#20 Updated by intrigeri 29 days ago

Forgot to say, since I was distracted by the regression: I confirm that this branch fixes the bug this ticket is about :)

#21 Updated by intrigeri 29 days ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (intrigeri)

I've seen the full non-fragile test suite pass locally.

#22 Updated by anonym 29 days ago

  • Assignee set to anonym

#23 Updated by anonym 28 days ago

  • Status changed from Needs Validation to Fix committed
  • % Done changed from 0 to 100

#24 Updated by anonym 28 days ago

  • Assignee deleted (anonym)
  • % Done changed from 100 to 0

Note that the Unsafe Browser in Tails 3.16 also uses Tor Browser's icon, so this was not a regression, but it's still nice to have it fixed! :)

Code looks excellent, nothing to remark except that @segfault's 09fcbc152a021485773c3310180c4b6fedef6813 really tickled my gray matter! Thanks! :)

#25 Updated by intrigeri 28 days ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF