Do not elide non-URL display origins in permission prompts.

http://crrev.com/c/768312 addresses a URL spoofing risk in permission
prompts by eliding display origins in prompts from the HEAD. This means
that the most significant part of the origin will be displayed if the
entire origin is too long. http://crrev.com/c/677983 replaces the
extension URL with the extension title when extensions request web
permissions. These two CLs mean that extensions with long names have
their titles elided, which is undesirable.

This CL addresses the issue by not eliding the display origin if it
isn't a URL. This requires a minor API change to
PermissionPrompt::GetDisplayOrigin, making it return a small struct
containing the display origin string and a bool indicating whether or
not the string is a URL. The struct is neccesary to ensure that the bool
state is calculated at the same time as the display origin.

BUG=790958
TBR=tapted@chromium.org

Change-Id: Icdf37feae4448f1fdd7044c7bbe690989f118023
Reviewed-on: https://chromium-review.googlesource.com/807606
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522012}

chrome/browser/permissions/permission_request_manager.cc

@@ -10,6 +10,7 @@
 #include "base/containers/circular_deque.h"
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
+#include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "chrome/browser/permissions/permission_decision_auto_blocker.h"
 #include "chrome/browser/permissions/permission_request.h"
@@ -345,7 +346,8 @@ const std::vector<PermissionRequest*>& PermissionRequestManager::Requests() {
   return requests_;
 }
 
-base::string16 PermissionRequestManager::GetDisplayOrigin() {
+PermissionPrompt::DisplayNameOrOrigin
+PermissionRequestManager::GetDisplayNameOrOrigin() {
   DCHECK(!requests_.empty());
   GURL origin_url = requests_[0]->GetOrigin();
 
@@ -354,15 +356,15 @@ base::string16 PermissionRequestManager::GetDisplayOrigin() {
     base::string16 extension_name =
         extensions::ui_util::GetEnabledExtensionNameForUrl(
             origin_url, web_contents()->GetBrowserContext());
-    if (!extension_name.empty()) {
-      return extension_name;
-    }
+    if (!extension_name.empty())
+      return {extension_name, false /* is_origin */};
   }
 #endif  // !defined(OS_ANDROID)
 
   // Web URLs should be displayed as the origin in the URL.
-  return url_formatter::FormatUrlForSecurityDisplay(
-      origin_url, url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC);
+  return {url_formatter::FormatUrlForSecurityDisplay(
+              origin_url, url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC),
+          true /* is_origin */};
 }
 
 void PermissionRequestManager::Accept() {

chrome/browser/permissions/permission_request_manager.h

@@ -126,7 +126,7 @@ class PermissionRequestManager
 
   // PermissionPrompt::Delegate:
   const std::vector<PermissionRequest*>& Requests() override;
-  base::string16 GetDisplayOrigin() override;
+  PermissionPrompt::DisplayNameOrOrigin GetDisplayNameOrOrigin() override;
   void Accept() override;
   void Deny() override;
   void Closing() override;

chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller_unittest.mm

@@ -96,7 +96,9 @@ class PermissionBubbleControllerTest : public CocoaProfileTest,
     return requests_;
   }
 
-  base::string16 GetDisplayOrigin() override { return base::string16(); }
+  PermissionPrompt::DisplayNameOrOrigin GetDisplayNameOrOrigin() override {
+    return {base::string16(), false /* is_origin */};
+  }
 
   void AddRequest(const std::string& title) {
     std::unique_ptr<MockPermissionRequest> request =

chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.cc

@@ -29,8 +29,9 @@ TestPermissionBubbleViewDelegate::Requests() {
   return requests_;
 }
 
-base::string16 TestPermissionBubbleViewDelegate::GetDisplayOrigin() {
-  return base::string16();
+PermissionPrompt::DisplayNameOrOrigin
+TestPermissionBubbleViewDelegate::GetDisplayNameOrOrigin() {
+  return {base::string16(), false /* is_origin */};
 }
 
 PermissionBubbleBrowserTest::PermissionBubbleBrowserTest() {

chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h

@@ -24,7 +24,7 @@ class TestPermissionBubbleViewDelegate : public PermissionPrompt::Delegate {
   ~TestPermissionBubbleViewDelegate() override;
 
   const std::vector<PermissionRequest*>& Requests() override;
-  base::string16 GetDisplayOrigin() override;
+  PermissionPrompt::DisplayNameOrOrigin GetDisplayNameOrOrigin() override;
 
   void Accept() override {}
   void Deny() override {}

chrome/browser/ui/permission_bubble/permission_prompt.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/strings/string16.h"
 #include "ui/gfx/native_widget_types.h"
 
 class PermissionRequest;
@@ -23,6 +24,13 @@ class WebContents;
 // to the manager for the visible tab.
 class PermissionPrompt {
  public:
+  // Holds the string to be displayed as the origin of the permission prompt,
+  // and whether or not that string is an origin.
+  struct DisplayNameOrOrigin {
+    base::string16 name_or_origin;
+    bool is_origin;
+  };
+
   // The delegate will receive events caused by user action which need to
   // be persisted in the per-tab UI state.
   class Delegate {
@@ -32,7 +40,10 @@ class PermissionPrompt {
     // These pointers should not be stored as the actual request objects may be
     // deleted upon navigation and so on.
     virtual const std::vector<PermissionRequest*>& Requests() = 0;
-    virtual base::string16 GetDisplayOrigin() = 0;
+
+    // Returns the origin to be displayed in the permission prompt. May return
+    // a non-origin, e.g. extension URLs use the name of the extension.
+    virtual DisplayNameOrOrigin GetDisplayNameOrOrigin() = 0;
 
     virtual void Accept() = 0;
     virtual void Deny() = 0;

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

@@ -22,7 +22,6 @@
 #include "chrome/browser/ui/views/page_info/permission_selector_row_observer.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
-#include "components/url_formatter/elide_url.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
@@ -69,7 +68,7 @@ class PermissionsBubbleDialogDelegateView
   PermissionsBubbleDialogDelegateView(
       PermissionPromptImpl* owner,
       const std::vector<PermissionRequest*>& requests,
-      const base::string16& display_origin);
+      const PermissionPrompt::DisplayNameOrOrigin& name_or_origin);
   ~PermissionsBubbleDialogDelegateView() override;
 
   void CloseBubble();
@@ -95,7 +94,7 @@ class PermissionsBubbleDialogDelegateView
 
  private:
   PermissionPromptImpl* owner_;
-  base::string16 display_origin_;
+  PermissionPrompt::DisplayNameOrOrigin name_or_origin_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionsBubbleDialogDelegateView);
 };
@@ -103,8 +102,8 @@ class PermissionsBubbleDialogDelegateView
 PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     PermissionPromptImpl* owner,
     const std::vector<PermissionRequest*>& requests,
-    const base::string16& display_origin)
-    : owner_(owner), display_origin_(display_origin) {
+    const PermissionPrompt::DisplayNameOrOrigin& name_or_origin)
+    : owner_(owner), name_or_origin_(name_or_origin) {
   DCHECK(!requests.empty());
 
   set_close_on_deactivate(false);
@@ -157,6 +156,10 @@ void PermissionsBubbleDialogDelegateView::CloseBubble() {
 }
 
 void PermissionsBubbleDialogDelegateView::AddedToWidget() {
+  // There is no URL spoofing risk from non-origins.
+  if (!name_or_origin_.is_origin)
+    return;
+
   std::unique_ptr<views::Label> title =
       views::BubbleFrameView::CreateDefaultTitleLabel(GetWindowTitle());
 
@@ -183,7 +186,7 @@ ui::AXRole PermissionsBubbleDialogDelegateView::GetAccessibleWindowRole()
 base::string16 PermissionsBubbleDialogDelegateView::GetAccessibleWindowTitle()
     const {
   return l10n_util::GetStringFUTF16(IDS_PERMISSIONS_BUBBLE_ACCESSIBLE_TITLE,
-                                    display_origin_);
+                                    name_or_origin_.name_or_origin);
 }
 
 bool PermissionsBubbleDialogDelegateView::ShouldShowCloseButton() const {
@@ -192,7 +195,7 @@ bool PermissionsBubbleDialogDelegateView::ShouldShowCloseButton() const {
 
 base::string16 PermissionsBubbleDialogDelegateView::GetWindowTitle() const {
   return l10n_util::GetStringFUTF16(IDS_PERMISSIONS_BUBBLE_PROMPT,
-                                    display_origin_);
+                                    name_or_origin_.name_or_origin);
 }
 
 void PermissionsBubbleDialogDelegateView::SizeToContents() {
@@ -310,7 +313,7 @@ void PermissionPromptImpl::Show() {
   DCHECK(browser_->window());
 
   bubble_delegate_ = new PermissionsBubbleDialogDelegateView(
-      this, delegate_->Requests(), delegate_->GetDisplayOrigin());
+      this, delegate_->Requests(), delegate_->GetDisplayNameOrOrigin());
 
   // Set |parent_window| because some valid anchors can become hidden.
   bubble_delegate_->set_parent_window(