Opened 4 months ago

Closed 4 weeks ago

Last modified 7 days ago

#31144 closed task (fixed)

ESR68 Network Code Review

Reported by: pili Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201910R, tbb-9.0-must-alpha, tbb-proxy-bypass
Cc: mikeperry Actual Points: 1.25
Parent ID: Points: 10
Reviewer: Sponsor:

Description

This ticket is to cover the network code review for ESR68

Child Tickets

Attachments (1)

FF68_NETWORK_AUDIT (8.4 KB) - added by mikeperry 5 weeks ago.
Attaching notes due to git issues

Download all attachments as: .zip

Change History (27)

comment:1 Changed 4 months ago by pili

Keywords: TorBrowserTeam201908 added

comment:2 Changed 2 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving tickets to September

comment:3 Changed 2 months ago by pili

Points: 10

comment:4 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:5 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201909 removed

comment:6 Changed 5 weeks ago by gk

Keywords: tbb-9.0-alpha-must added
Priority: MediumVery High

comment:7 Changed 5 weeks ago by mikeperry

Ok, I'm wrapping this up. I have the following questions/observations first:

  1. ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?
  2. ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?
  3. ./toolkit/modules/secondscreen/RokuApp.jsm also makes connections.. ISTR disabling this? Is it off?
  4. For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).
  5. Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?
  6. I found a lot of instances where it looks like Android could use Intents to open external apps. Most of the obvious ones route through IntentHelper.openUriExternal() from ./mobile/android/base/java/org/mozilla/gecko/IntentHelper.java, which has some logic to show prompts in private browsing mode.. Do we set private browsing mode? Can users turn it off? Here's the files that call that function:
    • ./mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/ActivityStreamContextMenu.java
    • ./mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
    • ./mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java
    • ./mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java
  7. I have not dug through all of the Android code for *all* Intent usage.. Should I? Has anyone?

Changed 5 weeks ago by mikeperry

Attachment: FF68_NETWORK_AUDIT added

Attaching notes due to git issues

comment:8 in reply to:  7 ; Changed 5 weeks ago by gk

Replying to mikeperry:

Ok, I'm wrapping this up. I have the following questions/observations first:

  1. ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?

It seems we disabled that via pref (see: comment:7:ticket:18546 and comment:10:ticket:16222). We should make sure this still works.

  1. ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?

We have #18862 for that. Arthur checked back then that the prefs are disabling everything. They are still set to false for desktop. We had #22165 as well which got upstreamed meanwhile.

I am not exactly sure what we did when Tor Browser on Android entered the picture. sysrqb: did you look at that?

  1. ./toolkit/modules/secondscreen/RokuApp.jsm also makes connections.. ISTR disabling this? Is it off?

I think it is. We had #16439 for that and are not including RokuApp.jsm and SimpleServiceDiscovery.jsm in our code (it's mobile only since https://bugzilla.mozilla.org/show_bug.cgi?id=1393582 landed and there even governed by a pref we set to false)

  1. For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).
  1. Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?

I don't think so. I thought that would be a nice thing to do during the network code review.

I leave the (other) mobile questions to sysrqb.

Last edited 4 weeks ago by gk (previous) (diff)

comment:9 in reply to:  8 Changed 5 weeks ago by gk

Replying to gk:

Replying to mikeperry:

Ok, I'm wrapping this up. I have the following questions/observations first:

  1. ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?

It seems we disabled that via pref (see: comment:7:ticket:18546 and comment:10:ticket:16222). We should make sure this still works.

  1. ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?

We have #18862 for that Arthur checked back then that the prefs are disabling everything. They are still set to false for desktop. We had #22165 as well which got upstreamed meanwhile.

I am not exactly sure what we did when Tor Browser on Android entered the picture. sysrqb: did you look at that?

It seems we flipped the prefs there explicitly, so we should be good, I guess. sysrqb: I guess that's something we can move to the non-mobile specific prefs file as that beast should be disabled for all platforms.

comment:10 in reply to:  8 Changed 5 weeks ago by mikeperry

  1. Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?

I don't think so. I thought that would be a nice thing to do during the network code review.

Hrmm I don't have a TBB build environment anymore... Maybe we can just ask Ritter if he's run it recently/to rerun it.

I am going to fall into this Intent rabbithole instead for now.

comment:11 Changed 5 weeks ago by mikeperry

Status: newneeds_review

Ok I believe I have completed the android portion now. Here is the full list of found items, removing the ones GeKo said were fixed:

  1. Rust lib check
  2. java.net.URL stream usage (which looks like it bypasses the proxy)
    • GeckoApplication.downloadImageForSetImage uses URL.openStream()
    • GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream()
    • GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?)
    • GeckoMediaDrmBridgeV21.java - uses android.media.MediaDrm which seems to fetch stuff??
    • BitmapUtils.decodeUrl uses openStream for non-jar urls
    • GeckoJarReader - tons of stream use.. Can this be used on remote jars?
    • AbstractCommunicator.openConnectionAndSetHeaders() - uses url.openConnection() (I think we patched this one in #31934?)
    • AbstractCommunicator.sendData() - uses url.getOutputStream().. maybe ok?
  3. IntentHelper openUriExternal usage - maybe we should just patch this to always prompt?
    • ActivityStreamContextMenu.java
    • BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)
    • ChromeCastDisplay.java
    • HomeFragment.java
  4. android.content.Intent startActivity() usage (may or may not be unsafe depending on circumstance :/)
    • ActivityHandlerHelper - Good candidate to patch for external activities, but not everything uses it :/
    • BrowserApp.onUrlOpenWithRefferer () - Might be able to launch other apps if OPEN_WITH_INTENT flag is set?
    • CustomTabsActivity.java - Several methods emit potentially external Intents
    • WebAppActivity.onLoadRequest()
    • BasicGeckoViewPrompt.onFilePrompt()
    • GeckoViewActivity.onExternalResponse()
  5. Intent bindService() usage:
    • SurfaceAllocator - no idea what is happening here :/
    • RemoteManager - no idea what is happening here :/
  6. android.app.PendingIntent
    • ChromeCastDisplay.java - probably want to make sure this is disabled?
    • CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here
  7. android.app.DownloadManager
    • DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it
    • BrowserApp.java uses it to download items without any checks

I committed a rubric of what I did for future audits/tooling here: https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/NETWORK_AUDIT_RUBRIC

I also committed my notes here: https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF68_NETWORK_AUDIT

comment:12 in reply to:  7 Changed 5 weeks ago by tom

Replying to mikeperry:

  1. For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).

That matches my finding from https://bugzilla.mozilla.org/show_bug.cgi?id=1376621#c23

  1. Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?

It runs on all builds in both esr68 and central; however in central several critical functions got allowlisted so it's not very effective anymore.

And when I say 'runs on all builds' I mean if Tor added a rust patch that used a networking function, this check would break the (firefox) build (during that step of the tor browser build process).

comment:13 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed

comment:14 in reply to:  11 Changed 5 weeks ago by sysrqb

Replying to mikeperry:

Ok I believe I have completed the android portion now. Here is the full list of found items, removing the ones GeKo said were fixed:

  1. Rust lib check
  2. java.net.URL stream usage (which looks like it bypasses the proxy)
    • GeckoApplication.downloadImageForSetImage uses URL.openStream()

I don't know why we missed this. We should've found this in #21863.

  • GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream()

Same here.

  • GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?)

This seems like it's only local connections (but it has the potential of bypassing the proxy). Specifically, this is used as a protocol handler for android: URIs. We can reject connections for now, and come back to this.

  • GeckoMediaDrmBridgeV21.java - uses android.media.MediaDrm which seems to fetch stuff??

We disabled DRM by pref (ticket:16285#comment:34).

  • BitmapUtils.decodeUrl uses openStream for non-jar urls

Sigh.

  • GeckoJarReader - tons of stream use.. Can this be used on remote jars?

This is safe, only accepts valid file paths.

  • AbstractCommunicator.openConnectionAndSetHeaders() - uses url.openConnection() (I think we patched this one in #31934?)
  • AbstractCommunicator.sendData() - uses url.getOutputStream().. maybe ok?

Only used in mozstumbler, which we exclude at compile time.

(coming back for the rest.)

comment:15 in reply to:  11 ; Changed 5 weeks ago by sysrqb

(continuing...)

Replying to mikeperry:

  1. IntentHelper openUriExternal usage - maybe we should just patch this to always prompt?

This seems like the easiest solution. I'll add that.

  • ActivityStreamContextMenu.java

Caught by forcing above prompt.

  • BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)

Can you provide a link for this? I'm missing it somehow.

  • ChromeCastDisplay.java

We unset MOZ_NATIVE_DEVICES which excludes this.

  • HomeFragment.java

Caught by forcing above prompt.

  1. android.content.Intent startActivity() usage (may or may not be unsafe depending on circumstance :/)
    • ActivityHandlerHelper - Good candidate to patch for external activities, but not everything uses it :/
    • BrowserApp.onUrlOpenWithRefferer () - Might be able to launch other apps if OPEN_WITH_INTENT flag is set?

Caught by forcing above prompt.

  • CustomTabsActivity.java - Several methods emit potentially external Intents

My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.

  • WebAppActivity.onLoadRequest()

WebActivities can't be installed from PBM (which is the new default tab mode). There will be a weird situation at the next upgrade, because WebApps worked in the current stable due to normal tabs being usable. In the next release, only private tabs will be used (by default). I don't know what will happen if a webapp is loaded in PBM, it seems like they will still work.

  • BasicGeckoViewPrompt.onFilePrompt()
  • GeckoViewActivity.onExternalResponse()

This is not part of the app (geckoview_example). We should patch these in the future.

  1. Intent bindService() usage:
    • SurfaceAllocator - no idea what is happening here :/

Connecting the app to a background service.

  • RemoteManager - no idea what is happening here :/

Needs MediaManager which is excluded.

  1. android.app.PendingIntent
    • ChromeCastDisplay.java - probably want to make sure this is disabled?

Disabled.

  • CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here

These seem like they could be arbitrary actions.

  1. android.app.DownloadManager
    • DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it
    • BrowserApp.java uses it to download items without any checks

This is controlled by browser.download.forward_oma_android_download_manager which is false. (https://bugzilla.mozilla.org/show_bug.cgi?id=1253684 which is restricted?). I'll add this into the override file, just so we aren't surprised by a change later.

comment:16 in reply to:  15 ; Changed 4 weeks ago by mikeperry

Replying to sysrqb:

(continuing...)

Replying to mikeperry:

  • BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)

Can you provide a link for this? I'm missing it somehow.

In ./mobile/android/base/java/org/mozilla/gecko/BrowserApp.java search for BrowserAppDelegate (sorry, no s). That type is used to create a list of things that have Activity related things passed to them. I think it might be harmless though.

  1. android.content.Intent startActivity() usage (may or may not be unsafe depending on circumstance :/)
    • ActivityHandlerHelper - Good candidate to patch for external activities, but not everything uses it :/
    • BrowserApp.onUrlOpenWithRefferer () - Might be able to launch other apps if OPEN_WITH_INTENT flag is set?

Caught by forcing above prompt.

Wait, both of these call startActivity() directly with an intent. Forcing the prompy from IntentHelper will NOT catch these.

If ActivityHandlerHelper was patched to call into IntentHelper (or add its own prompt), then all the things that us it would prompt, but BrowserApp doesn't use either of the Helper classes to handle its Intents.

  1. android.app.PendingIntent
    • ChromeCastDisplay.java - probably want to make sure this is disabled?

Disabled.

  • CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here

These seem like they could be arbitrary actions.

Hrmm.. should we patch that somehow, or assume it is handled when the Intent is finally delivered?

  1. android.app.DownloadManager
    • DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it
    • BrowserApp.java uses it to download items without any checks

This is controlled by browser.download.forward_oma_android_download_manager which is false. (https://bugzilla.mozilla.org/show_bug.cgi?id=1253684 which is restricted?). I'll add this into the override file, just so we aren't surprised by a change later.

Are we sure that pref governs both usages of the download mamager? I did not see any checks in the BrowserApp itself.

comment:17 in reply to:  16 Changed 4 weeks ago by sysrqb

Replying to mikeperry:

Replying to sysrqb:

(continuing...)

Replying to mikeperry:

  • BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)

Can you provide a link for this? I'm missing it somehow.

In ./mobile/android/base/java/org/mozilla/gecko/BrowserApp.java search for BrowserAppDelegate (sorry, no s). That type is used to create a list of things that have Activity related things passed to them. I think it might be harmless though.

Okay, I saw that. Yeah, that is harmless. I don't see any of those delegates implementing onNewIntent() (overriding the empty implementation they inherit).

  1. android.content.Intent startActivity() usage (may or may not be unsafe depending on circumstance :/)
    • ActivityHandlerHelper - Good candidate to patch for external activities, but not everything uses it :/
    • BrowserApp.onUrlOpenWithRefferer () - Might be able to launch other apps if OPEN_WITH_INTENT flag is set?

Caught by forcing above prompt.

Wait, both of these call startActivity() directly with an intent. Forcing the prompy from IntentHelper will NOT catch these.

If ActivityHandlerHelper was patched to call into IntentHelper (or add its own prompt), then all the things that us it would prompt, but BrowserApp doesn't use either of the Helper classes to handle its Intents.

Sorry, I should've been more descriptive. I'm patching them like:

iff --git a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
index 5d6f725dc1c5..ab19b78d54e7 100644
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -175,6 +175,7 @@ import org.mozilla.gecko.util.WindowUtil;
 import org.mozilla.gecko.widget.ActionModePresenter;
 import org.mozilla.gecko.widget.AnchoredPopup;
 import org.mozilla.gecko.widget.AnimatedProgressBar;
+import org.mozilla.gecko.widget.ExternalIntentDuringPrivateBrowsingPromptFragment;
 import org.mozilla.gecko.widget.GeckoActionProvider;
 import org.mozilla.gecko.widget.SplashScreen;
 import org.mozilla.geckoview.DynamicToolbarAnimator;
@@ -2237,7 +2238,8 @@ public class BrowserApp extends GeckoApp
                 if (AppConstants.RELEASE_OR_BETA) {
                     Intent intent = new Intent(Intent.ACTION_VIEW);
                     intent.setData(Uri.parse("market://details?id=" + getPackageName()));
-                    startActivity(intent);
+                    ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser(
+                        this, getSupportFragmentManager(), intent);
                     break;
                 }
 
@@ -4195,7 +4197,8 @@ public class BrowserApp extends GeckoApp
         if (flags.contains(OnUrlOpenListener.Flags.OPEN_WITH_INTENT)) {
             Intent intent = new Intent(Intent.ACTION_VIEW);
             intent.setData(Uri.parse(url));
-            startActivity(intent);
+            ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser(
+                this, getSupportFragmentManager(), intent);
         } else {

And I'm forcing showPromptInPrivateBrowsing as true in ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser.

  1. android.app.PendingIntent
    • ChromeCastDisplay.java - probably want to make sure this is disabled?

Disabled.

  • CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here

These seem like they could be arbitrary actions.

Hrmm.. should we patch that somehow, or assume it is handled when the Intent is finally delivered?

I'd rather break this functionality at this point. Someone can change the default browser on their device to Tor Browser under the assumption that CustomTabs are proxy-safe and other apps will happily use it. I'd rather be safe than sorry here. We can come back later and fix it correctly.

  1. android.app.DownloadManager
    • DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it
    • BrowserApp.java uses it to download items without any checks

This is controlled by browser.download.forward_oma_android_download_manager which is false. (https://bugzilla.mozilla.org/show_bug.cgi?id=1253684 which is restricted?). I'll add this into the override file, just so we aren't surprised by a change later.

Are we sure that pref governs both usages of the download mamager? I did not see any checks in the BrowserApp itself.

Yes, I believe we're safe here (no, it doesn't govern both uses). The DownloadManager is used in two places: BrowserApp.java and DownloadsIntegration.java.

In BrowserApp.java, DownloadManager is only called when a message is dispatched with the Download:AndroidDownloadManager event. This only happens in mobile/android/components/HelperAppDialog.js:_downloadWithAndroidDownloadManager() which is only called if this._shouldForwardToAndroidDownloadManager() === true. That returns false if the pref browser.download.forward_oma_android_download_manager is false, so we're good here.

In DownloadsIntegration.java, DownloadManager is used three times. Each of them is either within a conditional block dependent on useSystemDownloadManager(), or returns early. In useSystemDownloadManager, it returns false if !AppConstants.ANDROID_DOWNLOADS_INTEGRATION. We set ANDROID_DOWNLOADS_INTEGRATION as False at configure time.

comment:18 Changed 4 weeks ago by sysrqb

Status: needs_reviewneeds_revision

I have bug31144_01 for review. Testing this is a bit difficult. I ran some tests, but it will benefit from some more.

comment:19 Changed 4 weeks ago by gk

Keywords: tbb-9.0-must-alpha added; tbb-9.0-alpha-must removed

comment:20 Changed 4 weeks ago by sysrqb

Status: needs_revisionneeds_review

comment:21 Changed 4 weeks ago by mikeperry

Keywords: tbb-proxy-bypass added

comment:22 Changed 4 weeks ago by sysrqb

I pushed bug31144_04 which contains fixups for the missed:

GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?)

This seems like it's only local connections (but it has the potential of bypassing the proxy). Specifically, this is used as a protocol handler for android: URIs. We can reject connections for now, and come back to this.

and

GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream()

Same here.

and

CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here

These seem like they could be arbitrary actions.

Hrmm.. should we patch that somehow, or assume it is handled when the Intent is finally delivered?

I'd rather break this functionality at this point. Someone can change the default browser on their device to Tor Browser under the assumption that CustomTabs are proxy-safe and other apps will happily use it. I'd rather be safe than sorry here. We can come back later and fix it correctly.

and

android.app.DownloadManager

DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it
BrowserApp.java uses it to download items without any checks

This is controlled by browser.download.forward_oma_android_download_manager which is false. (​https://bugzilla.mozilla.org/show_bug.cgi?id=1253684 which is restricted?). I'll add this into the override file, just so we aren't surprised by a change later.

comment:23 Changed 4 weeks ago by sysrqb

CustomTabsActivity.java - Several methods emit potentially external Intents

My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.

This is included in the patch for #26529, but in this case we don't prompt. #26529 seemed like the better commit, but I can move this into its own commit.

diff --git a/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java b/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
index c75962da35a7..438f462755f6 100644
--- a/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
+++ b/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ -618,7 +618,8 @@ public class CustomTabsActivity extends AppCompatActivity
             final Intent intent = new Intent(Intent.ACTION_VIEW);
             intent.setData(uri);
             try {
-                startActivity(intent);
+                // Bug 31144 - Don't know how to handle this case.
+                //startActivity(intent);
             } catch (ActivityNotFoundException e) {
                 Log.w(LOGTAG, "No activity handler found for: " + request.uri);
             }

comment:24 in reply to:  23 ; Changed 4 weeks ago by sysrqb

Replying to sysrqb:

CustomTabsActivity.java - Several methods emit potentially external Intents

My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.

This is included in the patch for #26529, but in this case we don't prompt. #26529 seemed like the better commit, but I can move this into its own commit.

I pushed bug31144_05 with only this patch on top of 68.1.0esr-9.0-3.

comment:25 in reply to:  24 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

Replying to sysrqb:

CustomTabsActivity.java - Several methods emit potentially external Intents

My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.

This is included in the patch for #26529, but in this case we don't prompt. #26529 seemed like the better commit, but I can move this into its own commit.

I pushed bug31144_05 with only this patch on top of 68.1.0esr-9.0-3.

Thanks. I cherry-picked that one on top of tor-bŕowser-68.1.0esr-9.0-3 (commit d5123da897516717e62db98dccbcabd70162dcb3).

The other commits look good to me and I picked them onto tor-browser-68.1.0esr-9.0-3 (commit 947d477eb9289ebcdbbf48ee77609a6d7f395785, 47e35494838222732b9eb9638fb40e7a9707b307, 55b6cd7f3139dd0069ba2c69ec6d08da038594ad, dfdef56ec12ace37d08e2df516430427d0e08d7a, and db19bbcd9c34c933ad7fc84693ed8a5ad3bac3cf).

What is missing here is the patch for #26529 which I'll take care in that bug. We are done here. \o/

mikeperrry, sysrqb: please add your points.

comment:26 Changed 7 days ago by sysrqb

Actual Points: 1.25

Adding my points.

Note: See TracTickets for help on using tickets.