Handle onion-location HTTP header & .onion domain #6762

darkdh · 2020-10-02T18:33:59Z

Resolves brave/brave-browser#806

This PR makes it easier for user to interact with onion sites by

Show Open in Tor/Onion Available label when website sends onion-location header with its onion site url
1.2. Opening tab in Tor window when user clicking on label
1.3. Add a preference for automatically opening up onion site in Tor window
Automatically open tab in Tor window when user enter .onion domain in url bar from non-Tor window

Open in Tor in non-Tor window

Onion Available in Tor window

Preference for automatically opening onion site

Clicking Open in Tor in non-Tor window

Clicking Onion Available in Tor window

Submitter Checklist:

Test Plan:

`onion-location` header

Non Tor window

Open https://brave.com in regular window
There should be a label Open in Tor on the right of url bar when page loaded
Click on the label
https://brave5t5rjjg3s6k.onion will be opened in Tor window

Tor window

Open https://brave.com in Tor window
There should be a label Onion Available on the right of url bar when page loaded
Click on the label
https://brave5t5rjjg3s6k.onion will be opened in new tab in the same window

Tor is disabled

Go to brave://settings/extensions and turn off "Private window with Tor"
Open https://brave.com in regular window
There should be NO label Open in Tor on the right of url bar when page loaded

Automatically redirect .onion site

Go to brave://settings/extensions
Toggle on "Automatically redirect .onion site"
Test above Tor window and Non Tor window scenarios
They should go from step 1 directly to step 4
If the original tab is not last tab of the window, it will be closed

`.onion` domain

Non Tor window

Open https://brave5t5rjjg3s6k.onion in regular window
The request will be opened in a tab Tor window

Tor window

Open https://brave5t5rjjg3s6k.onion in Tor window
The request will be opened in same tab in same window

Tor is disabled

Go to brave://settings/extensions and turn off "Private window with Tor"
Open https://brave5t5rjjg3s6k.onion in regular window
The request will be opened in same tab in same window

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

darkdh · 2020-10-07T21:07:05Z

Refactoring tor into components will come up next (brave/brave-browser#1114)
so we will move all related codes, resources, tests into /components/tor, not in the scope of this PR

darkdh · 2020-10-07T21:11:38Z

@diracdeltas could you help checking if the wording is appropriate?

diracdeltas · 2020-10-07T21:23:38Z

@darkdh wording looks good to me!

iefremov · 2020-10-08T09:21:14Z

browser/resources/settings/brave_default_extensions_page/brave_default_extensions_page.html

@@ -130,6 +130,13 @@
        disabled="[[disableTorOption_]]"
        on-settings-boolean-control-change="onTorEnabledChange_">
    </settings-toggle-button>
+    <settings-toggle-button


looks like bad spacing

fixed in 08ec907

iefremov · 2020-10-08T09:21:30Z

browser/tor/BUILD.gn

@@ -36,12 +36,16 @@ source_set("tor") {
      "tor_launcher_factory.h",
      "tor_navigation_throttle.cc",
      "tor_navigation_throttle.h",
+      "onion_location_navigation_throttle.cc",


fixed in 08ec907

iefremov · 2020-10-08T09:25:06Z

browser/ui/views/brave_actions/brave_actions_container.h

@@ -27,6 +27,7 @@
 #endif

 class BraveActionViewController;
+class OnionLocationView;


better to keep these sorted

fixed in 08ec907

iefremov · 2020-10-08T10:02:47Z

browser/tor/onion_location_navigation_throttle.cc

+namespace {
+
+bool GetOnionLocation(const net::HttpResponseHeaders* headers,
+                      std::string* onion_location) {


nit: DCHECK(onion_location)

fixed in 08ec907

iefremov · 2020-10-08T10:15:27Z

browser/tor/onion_location_navigation_throttle.cc

+
+content::NavigationThrottle::ThrottleCheckResult
+OnionLocationNavigationThrottle::WillProcessResponse() {
+  if (navigation_handle()->IsInMainFrame()) {


nit: it's easier to read when

if (!navigation_handle()->IsInMainFrame()) { return {}; }

is placed at the beginning, and then we go on with the good case

actually I think we shouldn't even create the throttle if this is not a main frame navigation. We can check this right in BraveContentBrowserClient::CreateThrottlesForNavigation

fixed in 08ec907

iefremov · 2020-10-08T10:22:03Z

browser/ui/views/brave_actions/brave_actions_container.cc

@@ -38,6 +39,7 @@
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/view.h"

+


fixed in 08ec907

iefremov · 2020-10-08T10:22:42Z

browser/ui/views/location_bar/onion_location_view.h

+#ifndef BRAVE_BROWSER_UI_VIEWS_LOCATION_BAR_ONION_LOCATION_VIEW_H_
+#define BRAVE_BROWSER_UI_VIEWS_LOCATION_BAR_ONION_LOCATION_VIEW_H_
+
+#include "chrome/browser/profiles/profile.h"


no need to include, we have the forward declaration

fixed in 08ec907

iefremov · 2020-10-08T10:32:01Z

browser/ui/views/brave_actions/brave_actions_container.cc

@@ -135,22 +137,25 @@ void BraveActionsContainer::Init() {
  SetLayoutManager(std::move(vertical_container_layout));

  // children
+  onion_location_view_ = new OnionLocationView(browser_->profile());


shouldn't we check the tor group policy or something before adding this?

No, BraveActionsContainer::Init() is only called once per window creation.
And the pref toggle for tor::TorProfileService::IsTorDisabled() doesn't need restarting browser. So if we check and user toggle the pref from off to on, the label will not be shown until next relaunch.
This view will remain invisible until it has onion location and it will be invisible again when #6762 (comment)
Besides, chromium does the same thing in PageActionIcon like zoom, find...etc. Constructing first and then set visibility according to the condition.

iefremov · 2020-10-08T10:36:00Z

browser/tor/onion_location_tab_helper.h

+  explicit OnionLocationTabHelper(content::WebContents* web_contents);
+  friend class content::WebContentsUserData<OnionLocationTabHelper>;
+
+  GURL onion_location_;


I guess we should clean this state if a navigation happened

It will be cleaned up if the new navigation destination doesn't have onion-location header per web_contents
https://github.com/brave/brave-core/pull/6762/files#diff-d8f2fb41afdc92a8124ec8a331c1d44dR83

iefremov · 2020-10-08T10:40:22Z

browser/tor/onion_location_navigation_throttle.cc

+        profiles::SwitchToTorProfile(
+            base::BindRepeating(&OnTorProfileCreated, GURL(onion_location)));
+      } else {
+        OnionLocationTabHelper::SetOnionLocation(


AFAIU there is no way to disable the newly added onion omnibox view, even if I don't want to know about onions - maybe we should consider this.

This will hide OnionLocationView when we navigate away to other website without onion-location header.
This feature will be disabled when user disable tor.

Or do you mean give users another option that they want enabling tor but don't want to see this "Open in Tor"/"Onion Available" label? If so, that sounds very weird, even in Tor browser bundle they don't provide this kind of option. cc @diracdeltas

Yeah, I just want to understand product/UX consequences. There are two facts: (1) "Private window with Tor" pref is enabled by default (2) according to P3A, only 20% of users ever used Tor.

So by always showing this view (for certain websites) we probably will confuse 80% of users who don't care about onion domains at all, and most likely don't want to have any new buttons in the omnibox.

On the other hand, only few sites have the onion counterpart, so for now we are probably ok as is.

simonhong · 2020-10-08T12:42:15Z

browser/ui/views/brave_actions/brave_actions_container.h

@@ -187,6 +190,8 @@ class BraveActionsContainer : public views::View,

  std::unique_ptr<EmptyExtensionsContainer> empty_extensions_container_;

+  OnionLocationView* onion_location_view_ = nullptr;


I think onion view is not brave action.
IMO, OnionLocationView needs another parent view. WDYT?

@petemill do we need another parent for it? Or should we rename BraveActionContainer to something that is designated for Brave Icons?

Well, I'm leaning towards suggesting we put this in the regular PageAction area (or simply in the LocationBar directly and not the BraveActionsContainer. BraveActionsContainer is so named because it's meant to hold only almost-permanent ExtensionActions (extensions that have popups) that are our main brand calls-to-actions. Of course it all changes if the design isn't as logical or separate as this.

But since the onion button is page-specific, I would suggest it goes in to the appropriate page actions area (or location bar) and keep BraveActionsContainer for the more permanent items. The visual design seems to agree with that via the visual separator.

addressed here 6fe10b1c7ae2599f4aca93c31279b7456c036cfa

iefremov · 2020-10-09T12:17:58Z

I've built the PR - all looks good, except a minor nit: when automagical switching to .onion in Tor mode is enabled, we do not close the current tab. This way, the .onion website is opened somewhere in the end of the tabstrip, leaving a tab with "normal" URL behind - probably we should close the original tab?

iefremov · 2020-10-09T12:54:27Z

Also captured a crash, but most probably unrelated to the PR. Clean profile

darkdh · 2020-10-09T16:09:53Z

The crash was fixed in #6820 and I haven't rebase this PR

at browser startup [8357:775:1008/110058.682124:FATAL:remote.h(97)] Check failed: is_bound(). Cannot issue Interface method calls on an unbound Remote 0 libbase.dylib 0x000000010eb7c7b9 base::debug::CollectStackTrace(void**, unsigned long) + 9 1 libbase.dylib 0x000000010ea67f43 base::debug::StackTrace::StackTrace() + 19 2 libbase.dylib 0x000000010ea8b075 logging::LogMessage::~LogMessage() + 261 3 libbase.dylib 0x000000010ea8bd5e logging::LogMessage::~LogMessage() + 14 4 libchrome_dll.dylib 0x0000000114832d6b TorLauncherFactory::OnTorControlCheckComplete() + 219 5 libchrome_dll.dylib 0x00000001148361cb base::internal::Invoker<base::internal::BindState<void (TorLauncherFactory::*)(), base::WeakPtr<TorLauncherFactory> >, void ()>::RunOnce(base::internal::BindStateBase*) + 155 6 libbase.dylib 0x000000010eb09dbb base::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 411

… the window

petemill

LGTM as long as my changes LGTY!

darkdh · 2020-10-14T05:50:56Z

follow up issue created
brave/brave-browser#12133

Merith-TK · 2020-10-14T17:13:49Z

Question:
From what i am seeing, with my inexperienced eyes, i only see you guys focusing on the situation where a website contains a onion-location header, and not something where someone is directly linked to a .onion address, which leads them to a page like this

Wouldnt it be a good idea to have, on this error page, if the domain TLD is .onion, alter the contents of this page to show that this is a .onion address and has to be opened differently than normal webpages?

EDIT: screenshot was taken with

Brave:    1.15.72 Chromium: 86.0.4240.75 (Official Build) (64-bit)
Revision: c69c33933bfc72a159aceb4aeca939eb0087416c-refs/branch-heads/4240@{#1149}

darkdh · 2020-10-14T17:35:01Z

@Merith-TK when user types in .onion address in url bar, we will open a tor window for that.
The version you use doesn't contain this PR. You can try it on 1.17.32 which is available in our current nightly channel

Merith-TK · 2020-10-14T17:50:45Z

alright, thanks for clarifying, i was just curious if the functionality would be implemented at some point,

ghost · 2020-11-21T09:08:08Z

Just a humble user, I think having an option like "Automatically redirect .onion sites", but only when the user is already in a TOR window would be very useful (I thought that was actually what the setting would do). Perhaps a flag to edit the option rather than two options might be preferable.
I disagree with iefremov that Brave should close the original tab when opening a new onion, it should be clear to the user that Brave is not pinging some database (whether local or via the internet) but did require that connection to the site to be made in order to find the onion link (indeed I was rather confused how Brave was finding these, until I learned about the onion property in response headers). However automatically "halting" the page after the request (so it cannot load further resources, refresh itself, etc) should probably be done.
As an aside, I would be very interested in using Brave with TOR but in a normal browsing mode, or at least a mode that saves my history.

Merith-TK · 2020-11-21T14:32:23Z

agree with this, I agree that there should be an option

darkdh force-pushed the onion-location branch from 7376ae9 to f81dbcf Compare October 3, 2020 00:29

darkdh self-assigned this Oct 7, 2020

darkdh force-pushed the onion-location branch 2 times, most recently from bf90cac to 8e44311 Compare October 7, 2020 21:00

darkdh marked this pull request as ready for review October 7, 2020 21:00

darkdh requested a review from bridiver as a code owner October 7, 2020 21:00

darkdh requested review from petemill, simonhong, bbondy and iefremov October 7, 2020 21:01

darkdh requested a review from diracdeltas October 7, 2020 21:11

darkdh added this to the 1.17.x - Nightly milestone Oct 7, 2020

iefremov reviewed Oct 8, 2020

View reviewed changes

simonhong reviewed Oct 8, 2020

View reviewed changes

darkdh force-pushed the onion-location branch from 38d5297 to 14bc579 Compare October 9, 2020 20:43

darkdh and others added 14 commits October 13, 2020 22:02

Add prefs for auto onion location redirect

2fcf841

Fix OnTorProfileCreated lifetime issue

31cb0d2

Add browser test for OnionLocationNavigationThrottle

ba2de91

Check onion location label in browser test

89c36b0

Use Onion Available label button in Tor window

e01eafe

Address review feedbacks from iefremov

8faf100

Add TorDisabled test case

5d74b40

Close original tab after auto onion redirect if it is not last tab of…

6c12465

… the window

Move OnionLocationView to BraveLocationBarView

f14d87a

Remove unused WebContentsObserve from OnionLocationTabHelper

4494e6a

Simplify adding brave trailing views to LocationBarView::Layout()

165c706

nits fix and add comments for .onion handling

15b4acd

"Open In Tor" icon gets rounded border, margin, and vector icon

f097016

darkdh force-pushed the onion-location branch from b833afb to 3d2ecbf Compare October 14, 2020 05:02

Fix browser test for new icon and clang-format

008196f

darkdh force-pushed the onion-location branch from 3d2ecbf to 008196f Compare October 14, 2020 05:37

darkdh mentioned this pull request Oct 14, 2020

Refactor OnionLocationView brave/brave-browser#12133

Open

petemill approved these changes Oct 14, 2020

View reviewed changes

darkdh merged commit 3728080 into master Oct 14, 2020

darkdh deleted the onion-location branch October 14, 2020 07:37

This was referenced Nov 10, 2020

Handle .onions in all contexts brave/brave-browser#806

Closed

"Automatically redirect .onion site" setting does not work in Guest windows brave/brave-browser#12644

Open

da2x mentioned this pull request Nov 23, 2020

Support alt-svc for .onion brave/brave-browser#1121

Open

		@@ -38,6 +39,7 @@
		#include "ui/views/layout/grid_layout.h"
		#include "ui/views/view.h"

		@@ -187,6 +190,8 @@ class BraveActionsContainer : public views::View,

		std::unique_ptr<EmptyExtensionsContainer> empty_extensions_container_;

		OnionLocationView* onion_location_view_ = nullptr;

Handle onion-location HTTP header & .onion domain #6762

Handle onion-location HTTP header & .onion domain #6762

Conversation

darkdh commented Oct 2, 2020 • edited Loading

Submitter Checklist:

Test Plan:

onion-location header

Non Tor window

Tor window

Tor is disabled

Automatically redirect .onion site

.onion domain

Non Tor window

Tor window

Tor is disabled

Reviewer Checklist:

After-merge Checklist:

darkdh commented Oct 7, 2020

darkdh commented Oct 7, 2020

diracdeltas commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

iefremov commented Oct 9, 2020

iefremov commented Oct 9, 2020

darkdh commented Oct 9, 2020

petemill left a comment

Choose a reason for hiding this comment

darkdh commented Oct 14, 2020

Merith-TK commented Oct 14, 2020 • edited Loading

darkdh commented Oct 14, 2020

Merith-TK commented Oct 14, 2020

ghost commented Nov 21, 2020

Merith-TK commented Nov 21, 2020

darkdh commented Oct 2, 2020 •

edited

Loading

`onion-location` header

`.onion` domain

darkdh Oct 8, 2020 •

edited

Loading

iefremov Oct 8, 2020 •

edited

Loading

darkdh Oct 12, 2020 •

edited

Loading

Merith-TK commented Oct 14, 2020 •

edited

Loading