Allow an external URL with FROM_API qualifier to be forwarded to ARC

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc

Index: chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
diff --git a/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc b/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
index cdf3a3aa6bfc15925130ceb502005aef0a01471c..db9007d2c5b02ed4d6c0e25b9bf534d01977668f 100644
--- a/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
+++ b/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/bind.h"
+#include "base/lazy_instance.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h"
 #include "chrome/browser/chromeos/external_protocol_dialog.h"
@@ -37,6 +38,11 @@ constexpr uint32_t kMinVersionForHandleUrl = 2;
 constexpr uint32_t kMinVersionForRequestUrlHandlerList = 2;
 constexpr uint32_t kMinVersionForAddPreferredPackage = 7;
 
+// TODO(yusukes|djacobo): Find a better way to detect a request loop and remove
+// the global variables.
+base::LazyInstance<GURL> g_last_url = LAZY_INSTANCE_INITIALIZER;
+ui::PageTransition g_last_page_transition;
+
 // Shows the Chrome OS' original external protocol dialog as a fallback.
 void ShowFallbackExternalProtocolDialog(int render_process_host_id,
                                         int routing_id,
@@ -96,6 +102,7 @@ void HandleUrlInArc(int render_process_host_id,
 // A helper function called by GetAction().
 GetActionResult GetActionInternal(
     const GURL& original_url,
+    bool always_ask_user,
     const mojom::IntentHandlerInfoPtr& handler,
     std::pair<GURL, std::string>* out_url_and_package) {
   if (handler->fallback_url.has_value()) {
@@ -112,24 +119,28 @@ GetActionResult GetActionInternal(
                    << " in Chrome. Falling back to ARC...";
     }
     // |fallback_url| which Chrome doesn't support is passed (e.g. market:).
-    return GetActionResult::HANDLE_URL_IN_ARC;
+    return always_ask_user ? GetActionResult::ASK_USER
+                           : GetActionResult::HANDLE_URL_IN_ARC;
   }
 
   // Unlike |handler->fallback_url|, the |original_url| should always be handled
   // in ARC since it's external to Chrome.
   *out_url_and_package = std::make_pair(original_url, handler->package_name);
-  return GetActionResult::HANDLE_URL_IN_ARC;
+  return always_ask_user ? GetActionResult::ASK_USER
+                         : GetActionResult::HANDLE_URL_IN_ARC;
 }
 
 // Gets an action that should be done when ARC has the |handlers| for the
 // |original_url| and the user selects |selected_app_index|. When the user
 // hasn't selected any app, |selected_app_index| must be set to
-// |handlers.size()|.
+// |handlers.size()|. When |always_ask_user| is true, the function never
+// returns HANDLE_URL_IN_ARC.
 //
 // When the returned action is either OPEN_URL_IN_CHROME or HANDLE_URL_IN_ARC,
 // |out_url_and_package| is filled accordingly.
 GetActionResult GetAction(
     const GURL& original_url,
+    bool always_ask_user,
     const std::vector<mojom::IntentHandlerInfoPtr>& handlers,
     size_t selected_app_index,
     std::pair<GURL, std::string>* out_url_and_package) {
@@ -143,7 +154,8 @@ GetActionResult GetAction(
     // If |handlers| has only one element and its package is "Chrome", open
     // the fallback URL in the current tab without showing the dialog.
     if (handlers.size() == 1) {
-      if (GetActionInternal(original_url, handlers[0], out_url_and_package) ==
+      if (GetActionInternal(original_url, always_ask_user, handlers[0],
+                            out_url_and_package) ==
           GetActionResult::OPEN_URL_IN_CHROME) {
         return GetActionResult::OPEN_URL_IN_CHROME;
       }
@@ -158,7 +170,8 @@ GetActionResult GetAction(
         continue;
       // A preferred activity is found. Decide how to open it, either in Chrome
       // or ARC.
-      return GetActionInternal(original_url, handler, out_url_and_package);
+      return GetActionInternal(original_url, always_ask_user, handler,
+                               out_url_and_package);
     }
     // Ask the user to pick one.
     return GetActionResult::ASK_USER;
@@ -166,7 +179,9 @@ GetActionResult GetAction(
 
   // The user has already made the selection. Decide how to open it, either in
   // Chrome or ARC.
-  return GetActionInternal(original_url, handlers[selected_app_index],
+  DCHECK(!always_ask_user)
+      << "|always_ask_user| must be false when |selected_app_index| is valid.";
+  return GetActionInternal(original_url, false, handlers[selected_app_index],
                            out_url_and_package);
 }
 
@@ -174,12 +189,13 @@ GetActionResult GetAction(
 bool HandleUrl(int render_process_host_id,
                int routing_id,
                const GURL& url,
+               bool always_ask_user,
                const std::vector<mojom::IntentHandlerInfoPtr>& handlers,
                size_t selected_app_index,
                GetActionResult* out_result) {
   std::pair<GURL, std::string> url_and_package;
-  const GetActionResult result =
-      GetAction(url, handlers, selected_app_index, &url_and_package);
+  const GetActionResult result = GetAction(
+      url, always_ask_user, handlers, selected_app_index, &url_and_package);
   if (out_result)
     *out_result = result;
 
@@ -208,7 +224,7 @@ GURL GetUrlToNavigateOnDeactivate(
   const GURL empty_url;
   for (size_t i = 0; i < handlers.size(); ++i) {
     std::pair<GURL, std::string> url_and_package;
-    if (GetActionInternal(empty_url, handlers[i], &url_and_package) ==
+    if (GetActionInternal(empty_url, false, handlers[i], &url_and_package) ==
         GetActionResult::OPEN_URL_IN_CHROME) {
       DCHECK(url_and_package.first.SchemeIsHTTPOrHTTPS());
       return url_and_package.first;
@@ -253,8 +269,13 @@ void OnIntentPickerClosed(int render_process_host_id,
                  ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED ||
              close_reason ==
                  ArcNavigationThrottle::CloseReason::ALWAYS_PRESSED) {
-    if (selected_app_index == handlers.size())
+    if (selected_app_index == handlers.size()) {
       close_reason = ArcNavigationThrottle::CloseReason::ERROR;
+    } else {
+      // The user has made a selection. Clear g_last_* variables.
+      g_last_url.Get() = GURL();
+      g_last_page_transition = ui::PageTransition();
+    }
   }
 
   switch (close_reason) {
@@ -268,7 +289,7 @@ void OnIntentPickerClosed(int render_process_host_id,
     }
     case ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED: {
       // Launch the selected app.
-      HandleUrl(render_process_host_id, routing_id, url, handlers,
+      HandleUrl(render_process_host_id, routing_id, url, false, handlers,
                 selected_app_index, nullptr);
       break;
     }
@@ -331,6 +352,7 @@ void OnAppIconsReceived(
 void OnUrlHandlerList(int render_process_host_id,
                       int routing_id,
                       const GURL& url,
+                      bool always_ask_user,
                       std::vector<mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -346,8 +368,8 @@ void OnUrlHandlerList(int render_process_host_id,
 
   // Check if the |url| should be handled right away without showing the UI.
   GetActionResult result;
-  if (HandleUrl(render_process_host_id, routing_id, url, handlers,
-                handlers.size(), &result)) {
+  if (HandleUrl(render_process_host_id, routing_id, url, always_ask_user,
+                handlers, handlers.size(), &result)) {
     if (result == GetActionResult::HANDLE_URL_IN_ARC) {
       ArcNavigationThrottle::RecordUma(
           ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND,
@@ -374,6 +396,29 @@ void OnUrlHandlerList(int render_process_host_id,
                              routing_id, url, base::Passed(&handlers)));
 }
 
+// Returns true if the |url| is safe to be forwarded to ARC without showing the
+// disambig dialog when there is a preferred app on ARC for the |url|. Note that
+// this function almost always returns true (i.e. "safe") except for very rare
+// situations mentioned below.
+// TODO(yusukes|djacobo): Find a better way to detect a request loop and remove
+// these heuristics.
+bool IsSafeToRedirectToArcWithoutUserConfirmation(
+    const GURL& url,
+    ui::PageTransition page_transition,
+    const GURL& last_url,
+    ui::PageTransition last_page_transition) {
+  // Return "safe" unless both transition flags are FROM_API because the only
+  // unsafe situation we know is infinite tab creation loop with FROM_API
+  // (b/30125340).
+  if (!(page_transition & ui::PAGE_TRANSITION_FROM_API) ||
+      !(last_page_transition & ui::PAGE_TRANSITION_FROM_API)) {
+    return true;
+  }
+
+  // Return "safe" unless both URLs are for the same app.
+  return url.scheme() != last_url.scheme();
+}
+
 }  // namespace
 
 bool RunArcExternalProtocolDialog(const GURL& url,
@@ -384,10 +429,31 @@ bool RunArcExternalProtocolDialog(const GURL& url,
   // This function is for external protocols that Chrome cannot handle.
   DCHECK(!url.SchemeIsHTTPOrHTTPS()) << url;
 
-  if (ShouldIgnoreNavigation(page_transition, true /* allow_form_submit */,
+  const bool always_ask_user = !IsSafeToRedirectToArcWithoutUserConfirmation(
+      url, page_transition, g_last_url.Get(), g_last_page_transition);
+  LOG_IF(WARNING, always_ask_user)
+      << "RunArcExternalProtocolDialog: repeatedly handling external protocol "
+      << "redirection to " << url
+      << " started from API: last_url=" << g_last_url.Get();
+
+  // This function is called only on the UI thread. Updating g_last_* variables
+  // without a lock is safe.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  g_last_url.Get() = url;
+  g_last_page_transition = page_transition;
+
+  // For external protocol navigation, always ignore the FROM_API qualifier.
+  // We sometimes do need to forward a request with FROM_API to ARC, or
+  // AppAuth may not work (b/33208965). This is safe as long as we properly
+  // use |always_ask_user|.
+  const ui::PageTransition masked_page_transition =
+      MaskOutPageTransition(page_transition, ui::PAGE_TRANSITION_FROM_API);
+
+  if (ShouldIgnoreNavigation(masked_page_transition,
+                             true /* allow_form_submit */,
                              true /* allow_client_redirect */)) {
     LOG(WARNING) << "RunArcExternalProtocolDialog: ignoring " << url
-                 << " with PageTransition=" << page_transition;
+                 << " with PageTransition=" << masked_page_transition;
     return false;
   }
 
@@ -406,17 +472,18 @@ bool RunArcExternalProtocolDialog(const GURL& url,
   // Show ARC version of the dialog, which is IntentPickerBubbleView. To show
   // the bubble view, we need to ask ARC for a handler list first.
   instance->RequestUrlHandlerList(
-      url.spec(),
-      base::Bind(OnUrlHandlerList, render_process_host_id, routing_id, url));
+      url.spec(), base::Bind(OnUrlHandlerList, render_process_host_id,
+                             routing_id, url, always_ask_user));
   return true;
 }
 
 GetActionResult GetActionForTesting(
     const GURL& original_url,
+    bool always_ask_user,
     const std::vector<mojom::IntentHandlerInfoPtr>& handlers,
     size_t selected_app_index,
     std::pair<GURL, std::string>* out_url_and_package) {
-  return GetAction(original_url, handlers, selected_app_index,
+  return GetAction(original_url, always_ask_user, handlers, selected_app_index,
                    out_url_and_package);
 }
 
@@ -425,4 +492,13 @@ GURL GetUrlToNavigateOnDeactivateForTesting(
   return GetUrlToNavigateOnDeactivate(handlers);
 }
 
+bool IsSafeToRedirectToArcWithoutUserConfirmationForTesting(
+    const GURL& url,
+    ui::PageTransition page_transition,
+    const GURL& last_url,
+    ui::PageTransition last_page_transition) {
+  return IsSafeToRedirectToArcWithoutUserConfirmation(
+      url, page_transition, last_url, last_page_transition);
+}
+
 }  // namespace arc