[merge to m54] Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG=639126

Review-Url: https://codereview.chromium.org/2368593002
Cr-Commit-Position: refs/heads/master@{#422467}
(cherry picked from commit fa34e547d6ee25ea0692436ba7462ed0a0ef45f4)

Review URL: https://codereview.chromium.org/2411473002 .

Cr-Commit-Position: refs/branch-heads/2840@{#708}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}
diff --git a/chrome/browser/ui/bookmarks/bookmark_utils.cc b/chrome/browser/ui/bookmarks/bookmark_utils.cc
index 13c2e18..ffebaca 100644
--- a/chrome/browser/ui/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/ui/bookmarks/bookmark_utils.cc
@@ -126,13 +126,20 @@
 
 base::string16 FormatBookmarkURLForDisplay(const GURL& url) {
   // Because this gets re-parsed by FixupURL(), it's safe to omit the scheme
-  // and trailing slash, and unescape most characters.  However, it's
+  // and trailing slash, and unescape most characters. However, it's
   // important not to drop any username/password, or unescape anything that
   // changes the URL's meaning.
-  return url_formatter::FormatUrl(
-      url, url_formatter::kFormatUrlOmitAll &
-               ~url_formatter::kFormatUrlOmitUsernamePassword,
-      net::UnescapeRule::SPACES, nullptr, nullptr, nullptr);
+  url_formatter::FormatUrlTypes format_types =
+      url_formatter::kFormatUrlOmitAll &
+      ~url_formatter::kFormatUrlOmitUsernamePassword;
+
+  // If username is present, we must not omit the scheme because FixupURL() will
+  // subsequently interpret the username as a scheme. crbug.com/639126
+  if (url.has_username())
+    format_types &= ~url_formatter::kFormatUrlOmitHTTP;
+
+  return url_formatter::FormatUrl(url, format_types, net::UnescapeRule::SPACES,
+                                  nullptr, nullptr, nullptr);
 }
 
 bool IsAppsShortcutEnabled(Profile* profile) {
diff --git a/chrome/browser/ui/bookmarks/bookmark_utils.h b/chrome/browser/ui/bookmarks/bookmark_utils.h
index 1cb09ce..1a5bf29 100644
--- a/chrome/browser/ui/bookmarks/bookmark_utils.h
+++ b/chrome/browser/ui/bookmarks/bookmark_utils.h
@@ -53,9 +53,8 @@
 // all tabs. This is a preference modifier, not a visual modifier.
 void ToggleBookmarkBarWhenVisible(content::BrowserContext* browser_context);
 
-// Returns a formatted version of |url| appropriate to display to a user with
-// the given |prefs|, which may be NULL.  When re-parsing this URL, clients
-// should call url_formatter::FixupURL().
+// Returns a formatted version of |url| appropriate to display to a user.
+// When re-parsing this URL, clients should call url_formatter::FixupURL().
 base::string16 FormatBookmarkURLForDisplay(const GURL& url);
 
 // Returns whether the Apps shortcut is enabled. If true, then the visibility
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
index 5a9a095..4098467 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
@@ -259,6 +259,45 @@
   [controller_ cancel:nil];
 }
 
+using BookmarkEditorControllerEditKeepsSchemeTest = CocoaProfileTest;
+TEST_F(BookmarkEditorControllerEditKeepsSchemeTest, EditKeepsScheme) {
+  // Edits the bookmark and ensures resulting URL keeps the same scheme, even
+  // when userinfo is present in the URL
+  ASSERT_TRUE(profile());
+
+  BookmarkModel* model = BookmarkModelFactory::GetForBrowserContext(profile());
+  const BookmarkNode* kParent = model->bookmark_bar_node();
+  const base::string16 kTitle = ASCIIToUTF16("EditingKeepsScheme");
+
+  const GURL kUrl = GURL("http://javascript:scripttext@example.com/");
+  const BookmarkNode* kNode = model->AddURL(kParent, 0, base::string16(), kUrl);
+
+  BookmarkEditorController* controller = [[BookmarkEditorController alloc]
+      initWithParentWindow:test_window()
+                   profile:profile()
+                    parent:kParent
+                      node:kNode
+                       url:GURL()
+                     title:base::string16()
+             configuration:BookmarkEditor::SHOW_TREE];
+
+  [controller runAsModalSheet];
+
+  // We expect only the trailing / to be trimmed when userinfo is present
+  EXPECT_NSEQ(base::SysUTF8ToNSString(kUrl.spec()),
+              [[controller displayURL] stringByAppendingString:@"/"]);
+
+  [controller setDisplayName:base::SysUTF16ToNSString(kTitle)];
+
+  EXPECT_TRUE([controller okButtonEnabled]);
+  [controller ok:nil];
+
+  ASSERT_EQ(1, kParent->child_count());
+  const BookmarkNode* kChild = kParent->GetChild(0);
+  EXPECT_EQ(kTitle, kChild->GetTitle());
+  EXPECT_EQ(kUrl, kChild->url());
+}
+
 class BookmarkEditorControllerTreeTest : public CocoaProfileTest {
 
  public:
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
index 20c893c..2abe3d7 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
@@ -73,6 +73,13 @@
       editor_->url_tf_->SetText(text);
   }
 
+  base::string16 GetURLText() const {
+    if (editor_->details_.type != BookmarkEditor::EditDetails::NEW_FOLDER)
+      return editor_->url_tf_->text();
+
+    return base::string16();
+  }
+
   void ApplyEdits() {
     editor_->ApplyEdits();
   }
@@ -332,6 +339,34 @@
   EXPECT_EQ(ASCIIToUTF16("new_a"), new_node->GetTitle());
 }
 
+// Edits the bookmark and ensures resulting URL keeps the same scheme, even
+// when userinfo is present in the URL
+TEST_F(BookmarkEditorViewTest, EditKeepsScheme) {
+  const BookmarkNode* kBBNode = model_->bookmark_bar_node();
+
+  const GURL kUrl = GURL("http://javascript:scripttext@example.com/");
+
+  CreateEditor(profile_.get(), kBBNode,
+               BookmarkEditor::EditDetails::AddNodeInFolder(kBBNode, 1, kUrl,
+                                                            base::string16()),
+               BookmarkEditorView::SHOW_TREE);
+
+  // We expect only the trailing / to be trimmed when userinfo is present
+  EXPECT_EQ(ASCIIToUTF16(kUrl.spec()), GetURLText() + ASCIIToUTF16("/"));
+
+  const base::string16& kTitle = ASCIIToUTF16("EditingKeepsScheme");
+  SetTitleText(kTitle);
+
+  ApplyEdits(editor_tree_model()->GetRoot()->GetChild(0));
+
+  ASSERT_EQ(4, kBBNode->child_count());
+
+  const BookmarkNode* kNewNode = kBBNode->GetChild(1);
+
+  EXPECT_EQ(kTitle, kNewNode->GetTitle());
+  EXPECT_EQ(kUrl, kNewNode->url());
+}
+
 // Creates a new folder.
 TEST_F(BookmarkEditorViewTest, NewFolder) {
   const BookmarkNode* bb_node = model_->bookmark_bar_node();