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..f48907b0b6058e2229df197275fb88544f9cd721 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
+// this heuristics.

[djacobo, 2016/12/01 23:47:23]

nit: these*

[Yusuke Sato, 2016/12/02 00:44:37]

Done.

+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();

[djacobo, 2016/12/01 23:47:23]

qq: Does this means that two consecutive calls to the intent picker via
arc_external_protocol_dialog cannot be considered safe since their scheme is the
same? 

We certainly don't have a NavigationHandle here, so emulating something like
this could be the proper way to go?
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe...


I am ok with the fix, I could take a look at the TODO soon, actually I also
think this is a very unlikely situation but certainly needs proper handling. 

[Yusuke Sato, 2016/12/02 00:24:17]

Yes, but it's considered "possibly unsafe" only when the previous and current
navigations are FROM_API (which is very rare), and the previous navigation was
handled by ARC's preferred app without showing the disambig dialog.
Re right fix:
When |url| is sent to ArcIntentHelper, the helper converts the URL to an Intent
object, right? I'm wondering if it is possible to add some EXTRAs to the intent
object so that ArcIntentHelper can ignore the intent when it is bounced back to
ArcIntentHelper. Just a random idea, and I'm not really sure if it is feasible
though.

+}
+
 }  // 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