[NTP] Add better URL validation and adjust input field requirements for custom link dialog

Use URL validation provided by GURL, and added URL formatting using
FixupURL. This tries to make "smart" adjustments to obviously invalid
input (see url_fixer.h). The fixed URL will default to "https://" if a
scheme was not specified.

Also disabled spellcheck in the URL field and removed "required" from
the Name field. The URL will be set as the title if Name is left empty.

Bug: 874194
Change-Id: I0d40f551f3e79d77aa93ce3dd0f36f736bf987ee
Reviewed-on: https://chromium-review.googlesource.com/1182418
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586024}

chrome/browser/resources/local_ntp/custom_links_edit.css

@@ -72,7 +72,7 @@ input {
   width: calc(100% - 16px);
 }
 
-#url-field:not(.text-modified) {
+input::placeholder {
   color: rgba(32, 33, 36, 0.38);
 }
 

chrome/browser/resources/local_ntp/custom_links_edit.html

@@ -17,15 +17,15 @@
         <label id="title-field-name" class="field-title"></label>
         <div class="input-container">
           <input id="title-field" class="field-input" type="text"
-              autocomplete="off" tabindex="0" required></input>
+              autocomplete="off" tabindex="0"></input>
           <div class="underline"></div>
         </div>
       </div>
       <div id="url" class="field-container">
         <label id="url-field-name" class="field-title"></label>
         <div class="input-container">
-          <input id="url-field" class="field-input" type="text" value="https://"
-              autocomplete="off" tabindex="0" required></input>
+          <input id="url-field" class="field-input" type="text" placeholder="https://"
+              autocomplete="off" spellcheck="false" tabindex="0" required></input>
           <div class="underline"></div>
         </div>
         <div id="invalid-url" class="error-msg"></div>

chrome/browser/resources/local_ntp/custom_links_edit.js

@@ -35,17 +35,6 @@ const IDS = {
 };
 
 
-/**
- * Enum for classes.
- * @enum {string}
- * @const
- */
-const CLASSES = {
-  // Applied if the input field has been modified.
-  TEXT_MODIFIED: 'text-modified',
-};
-
-
 /**
  * Enum for key codes.
  * @enum {int}
@@ -105,19 +94,6 @@ let editLinkTitle = '';
 let deleteLinkTitle = '';
 
 
-/**
- * True if the provided url is valid.
- * @type {string}
- */
-function isValidURL(url) {
-  let a = document.createElement('a');
-  a.href = url;
-  // Invalid URLs will not match the current host.
-  let isValid = a.host && a.host != window.location.host;
-  return isValid;
-}
-
-
 /**
  * Handler for the 'linkData' message from the host page. Pre-populates the url
  * and title fields with link's data obtained using the rid. Called if we are
@@ -135,7 +111,6 @@ function prepopulateFields(rid) {
   prepopulatedLink.rid = rid;
   $(IDS.TITLE_FIELD).value = prepopulatedLink.title = data.title;
   $(IDS.URL_FIELD).value = prepopulatedLink.url = data.url;
-  $(IDS.URL_FIELD).classList.add(CLASSES.TEXT_MODIFIED);
 
   // Set accessibility names.
   $(IDS.DELETE).setAttribute('aria-label', deleteLinkTitle + ' ' + data.title);
@@ -175,20 +150,25 @@ function showInvalidUrlUntilTextInput() {
  * completed. If the fields were unchanged, does not update the link data.
  */
 function finishEditLink() {
+  let newUrl = '';
+  let newTitle = '';
+
+  const urlValue = $(IDS.URL_FIELD).value;
+  if (urlValue != prepopulatedLink.url) {
+    newUrl = chrome.embeddedSearch.newTabPage.fixupAndValidateUrl(urlValue);
     // Show error message for invalid urls.
-  if (!isValidURL($(IDS.URL_FIELD).value)) {
+    if (!newUrl) {
       showInvalidUrlUntilTextInput();
       disableSubmitUntilTextInput();
       return;
     }
+  }
 
-  let newUrl = '';
-  let newTitle = '';
-  if ($(IDS.URL_FIELD).value != prepopulatedLink.url &&
-      $(IDS.URL_FIELD).value != 'https://')
-    newUrl = $(IDS.URL_FIELD).value;
-  if ($(IDS.TITLE_FIELD).value != prepopulatedLink.title)
-    newTitle = $(IDS.TITLE_FIELD).value;
+  const titleValue = $(IDS.TITLE_FIELD).value;
+  if (!titleValue)  // Set the URL input as the title if no title is provided.
+    newTitle = urlValue;
+  else if (titleValue != prepopulatedLink.title)
+    newTitle = titleValue;
 
   // Update the link only if a field was changed.
   if (!!newUrl || !!newTitle) {
@@ -219,7 +199,6 @@ function closeDialog() {
   window.setTimeout(() => {
     $(IDS.FORM).reset();
     $(IDS.URL_FIELD_CONTAINER).classList.remove('invalid');
-    $(IDS.URL_FIELD).classList.remove(CLASSES.TEXT_MODIFIED);
     $(IDS.DELETE).disabled = false;
     $(IDS.DONE).disabled = false;
     prepopulatedLink.rid = -1;
@@ -321,10 +300,6 @@ function init() {
       closeDialog();
     }
   };
-  $(IDS.URL_FIELD).addEventListener('input', (event) => {
-    if (!$(IDS.URL_FIELD).classList.contains(CLASSES.TEXT_MODIFIED))
-      $(IDS.URL_FIELD).classList.add(CLASSES.TEXT_MODIFIED);
-  });
   $(IDS.DELETE).addEventListener('click', deleteLink);
   $(IDS.CANCEL).addEventListener('click', closeDialog);
   $(IDS.FORM).addEventListener('submit', (event) => {

chrome/renderer/searchbox/searchbox.cc

@@ -19,6 +19,7 @@
 #include "chrome/renderer/searchbox/searchbox_extension.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/favicon_base/favicon_url_parser.h"
+#include "components/url_formatter/url_fixer.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
@@ -203,6 +204,23 @@ bool TranslateIconRestrictedUrl(const GURL& transient_url,
   return true;
 }
 
+std::string FixupAndValidateUrl(const std::string& url) {
+  GURL gurl = url_formatter::FixupURL(url, /*desired_tld=*/std::string());
+  if (!gurl.is_valid())
+    return std::string();
+
+  // Unless "http" was specified, replaces FixupURL's default "http" with
+  // "https".
+  if (url.find(std::string("http://")) == std::string::npos &&
+      gurl.SchemeIs(url::kHttpScheme)) {
+    GURL::Replacements replacements;
+    replacements.SetSchemeStr(url::kHttpsScheme);
+    gurl = gurl.ReplaceComponents(replacements);
+  }
+
+  return gurl.spec();
+}
+
 }  // namespace internal
 
 SearchBox::IconURLHelper::IconURLHelper() = default;
@@ -367,6 +385,10 @@ void SearchBox::ResetCustomLinks() {
   embedded_search_service_->ResetCustomLinks(page_seq_no_);
 }
 
+std::string SearchBox::FixupAndValidateUrl(const std::string& url) const {
+  return internal::FixupAndValidateUrl(url);
+}
+
 void SearchBox::SetCustomBackgroundURL(const GURL& background_url) {
   embedded_search_service_->SetCustomBackgroundURL(background_url);
 }

chrome/renderer/searchbox/searchbox.h

@@ -136,6 +136,11 @@ class SearchBox : public content::RenderFrameObserver,
   // Sends ResetCustomLinks to the browser.
   void ResetCustomLinks();
 
+  // Attempts to fix obviously invalid URLs. Uses the "https" scheme unless
+  // otherwise specified. Returns the fixed URL if valid, otherwise returns an
+  // empty string.
+  std::string FixupAndValidateUrl(const std::string& url) const;
+
   // Updates the NTP custom background preferences, sometimes this includes
   // image attributions.
   void SetCustomBackgroundURL(const GURL& background_url);

chrome/renderer/searchbox/searchbox_extension.cc

@@ -624,6 +624,7 @@ class NewTabPageBindings : public gin::Wrappable<NewTabPageBindings> {
                                const std::string& title);
   static void UndoCustomLinkAction();
   static void ResetCustomLinks();
+  static std::string FixupAndValidateUrl(const std::string& url);
   static void LogEvent(int event);
   static void LogMostVisitedImpression(
       int position,
@@ -679,6 +680,8 @@ gin::ObjectTemplateBuilder NewTabPageBindings::GetObjectTemplateBuilder(
       .SetMethod("undoCustomLinkAction",
                  &NewTabPageBindings::UndoCustomLinkAction)
       .SetMethod("resetCustomLinks", &NewTabPageBindings::ResetCustomLinks)
+      .SetMethod("fixupAndValidateUrl",
+                 &NewTabPageBindings::FixupAndValidateUrl)
       .SetMethod("logEvent", &NewTabPageBindings::LogEvent)
       .SetMethod("logMostVisitedImpression",
                  &NewTabPageBindings::LogMostVisitedImpression)
@@ -886,6 +889,14 @@ void NewTabPageBindings::ResetCustomLinks() {
   search_box->LogEvent(NTPLoggingEventType::NTP_CUSTOMIZE_SHORTCUT_RESTORE_ALL);
 }
 
+// static
+std::string NewTabPageBindings::FixupAndValidateUrl(const std::string& url) {
+  SearchBox* search_box = GetSearchBoxForCurrentContext();
+  if (!search_box || !HasOrigin(GURL(chrome::kChromeSearchMostVisitedUrl)))
+    return std::string();
+  return search_box->FixupAndValidateUrl(url);
+}
+
 // static
 void NewTabPageBindings::LogEvent(int event) {
   SearchBox* search_box = GetSearchBoxForCurrentContext();

chrome/renderer/searchbox/searchbox_unittest.cc

@@ -77,6 +77,9 @@ bool TranslateIconRestrictedUrl(const GURL& transient_url,
                                 const SearchBox::IconURLHelper& helper,
                                 GURL* url);
 
+// Defined in searchbox.cc
+std::string FixupAndValidateUrl(const std::string& url);
+
 TEST(SearchBoxUtilTest, ParseViewIdAndRestrictedIdSuccess) {
   int view_id = -1;
   InstantRestrictedID rid = -1;
@@ -264,4 +267,55 @@ TEST(SearchBoxUtilTest, TranslateIconRestrictedUrlFailure) {
   }
 }
 
+TEST(SearchBoxUtilTest, FixupAndValidateUrlReturnsEmptyIfInvalid) {
+  struct TestCase {
+    const char* url;
+    bool is_valid;
+  } test_cases[] = {
+      {"   ", false},
+      {"^&*@)^)", false},
+      {"foo", true},
+      {"http://foo", true},
+      {"\thttp://foo", true},
+      {"    http://foo", true},
+      {"https://foo", true},
+      {"foo.com", true},
+      {"http://foo.com", true},
+      {"https://foo.com", true},
+      {"blob://foo", true},
+
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    const std::string& url = FixupAndValidateUrl(test_case.url);
+    EXPECT_EQ(!url.empty(), test_case.is_valid)
+        << " for test_case '" << test_case.url << "'";
+  }
+}
+
+TEST(SearchBoxUtilTest, FixupAndValidateUrlDefaultsToHttps) {
+  struct TestCase {
+    const char* url;
+    const char* expected_scheme;
+  } test_cases[] = {
+      // No scheme.
+      {"foo.com", url::kHttpsScheme},
+      // With "http".
+      {"http://foo.com", url::kHttpScheme},
+      // With "http" and whitespace.
+      {"\thttp://foo", url::kHttpScheme},
+      {"    http://foo", url::kHttpScheme},
+      // With "https".
+      {"https://foo.com", url::kHttpsScheme},
+      // Non "http"/"https".
+      {"blob://foo", url::kBlobScheme},
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    const GURL url(FixupAndValidateUrl(test_case.url));
+    EXPECT_TRUE(url.SchemeIs(test_case.expected_scheme))
+        << " for test case '" << test_case.url << "'";
+  }
+}
+
 }  // namespace internal