[WebShare] Check share target url_template syntax at parse time

Invalid share targets will now be ignored, rather than showing up in the
picker and being rejected at launch time. This brings the implementation
in line with the spec: https://wicg.github.io/web-share-target/

This CL:
- Makes manifest_parser.cc check the share target url_template placeholder
  syntax.
- Refactors ReplacePlaceholders() so that it does no string copying when
 called with url_template_filled==nullptr

BUG=694380, 788191

Change-Id: Icd2a5af5e3100fd93a3fa3cd9ba00a23905355b7
Reviewed-on: https://chromium-review.googlesource.com/927581
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543763}

chrome/browser/webshare/share_service_impl.cc

@@ -126,9 +126,8 @@ void ShareServiceImpl::OnPickerClosed(const std::string& title,
   if (!content::ReplaceWebShareUrlPlaceholders(result->url_template(), title,
                                                text, share_url,
                                                &url_template_filled)) {
-    // TODO(mgiuca): This error should not be possible at share time, because
-    // targets with invalid templates should not be chooseable. Fix
-    // https://crbug.com/694380 and replace this with a DCHECK.
+    // This error should not be possible at share time. content::ManifestParser
+    // should have filtered out invalid targets at manifest parse time.
     std::move(callback).Run(blink::mojom::ShareError::INTERNAL_ERROR);
     return;
   }

content/common/manifest_share_target_util_unittest.cc

@@ -18,48 +18,70 @@ constexpr char kUrlSpec[] = "https://www.google.com/";
 
 }  // namespace
 
-TEST(ManifestShareTargetUtilTest,
-     ReplaceWebShareUrlPlaceholdersInvalidTemplate) {
-  const GURL kUrl(kUrlSpec);
-  GURL url_template_filled;
-
+TEST(ManifestShareTargetUtilTest, ReplaceUrlPlaceholdersInvalidTemplate) {
   // Badly nested placeholders.
   GURL url_template = GURL("http://example.com/?q={");
-  bool succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText,
-                                                  kUrl, &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   url_template = GURL("http://example.com/?q={title");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   url_template = GURL("http://example.com/?q={title{text}}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   url_template = GURL("http://example.com/?q={title{}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   url_template = GURL("http://example.com/?q={{title}}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   // Placeholder with non-identifier character.
   url_template = GURL("http://example.com/?q={title?}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
+
+  // Placeholder with digit character.
+  url_template = GURL("http://example.com/?q={title1}");
+  EXPECT_TRUE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_TRUE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
+
+  // Empty placeholder.
+  url_template = GURL("http://example.com/?q={}");
+  EXPECT_TRUE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_TRUE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
 
   // Invalid placeholder in URL fragment.
   url_template = GURL("http://example.com/#{title?}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_FALSE(succeeded);
+  EXPECT_FALSE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_FALSE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
+
+  // { in path.
+  url_template = GURL("http://example.com/subpath{/");
+  EXPECT_TRUE(ValidateWebShareUrlTemplate(url_template));
+  EXPECT_TRUE(
+      ReplaceWebShareUrlPlaceholders(url_template, "", "", GURL(), nullptr));
+
+  // Invalid placeholder. Non-empty title, text, share URL and non-empty output
+  // parameter.
+  GURL url_template_filled;
+  url_template = GURL("http://example.com/?q={");
+  EXPECT_FALSE(ReplaceWebShareUrlPlaceholders(url_template, "text", "title",
+                                              GURL("http://www.google.com"),
+                                              &url_template_filled));
 }
 
 TEST(ManifestShareTargetUtilTest, ReplaceWebShareUrlPlaceholders) {
@@ -73,13 +95,6 @@ TEST(ManifestShareTargetUtilTest, ReplaceWebShareUrlPlaceholders) {
   EXPECT_TRUE(succeeded);
   EXPECT_EQ(url_template, url_template_filled);
 
-  // Empty |url_template|
-  url_template = GURL();
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_TRUE(succeeded);
-  EXPECT_EQ(GURL(), url_template_filled);
-
   // One title placeholder.
   url_template = GURL("http://example.com/#{title}");
   succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
@@ -147,13 +162,6 @@ TEST(ManifestShareTargetUtilTest, ReplaceWebShareUrlPlaceholders) {
       "text=My%20text&title=My%20title&url=https%3A%2F%2Fwww.google.com%2F",
       url_template_filled.spec());
 
-  // Placeholder with digit character.
-  url_template = GURL("http://example.com/#{title1}");
-  succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,
-                                             &url_template_filled);
-  EXPECT_TRUE(succeeded);
-  EXPECT_EQ("http://example.com/#", url_template_filled.spec());
-
   // Empty placeholder.
   url_template = GURL("http://example.com/#{}");
   succeeded = ReplaceWebShareUrlPlaceholders(url_template, kTitle, kText, kUrl,

content/public/common/manifest_share_target_util.cc

@@ -19,27 +19,16 @@ bool IsIdentifier(char c) {
   return base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) || c == '-' || c == '_';
 }
 
-// Returns to |out| the result of running the "replace placeholders" algorithm
-// on |template_string|. The algorithm is specified at
+// Returns to |out| the result of running the "replace placeholders"
+// algorithm on |url_template|. The algorithm is specified at
 // https://wicg.github.io/web-share-target/#dfn-replace-placeholders
+// Does not copy any string data. The resulting vector can be concatenated
+// together with base::StrCat to produce the final string.
 bool ReplacePlaceholders(base::StringPiece template_string,
-                         base::StringPiece title,
-                         base::StringPiece text,
-                         const GURL& share_url,
-                         std::string* out) {
-  constexpr char kTitlePlaceholder[] = "title";
-  constexpr char kTextPlaceholder[] = "text";
-  constexpr char kUrlPlaceholder[] = "url";
-
-  std::map<base::StringPiece, std::string> placeholder_to_data;
-  placeholder_to_data[kTitlePlaceholder] =
-      net::EscapeQueryParamValue(title, false);
-  placeholder_to_data[kTextPlaceholder] =
-      net::EscapeQueryParamValue(text, false);
-  placeholder_to_data[kUrlPlaceholder] =
-      net::EscapeQueryParamValue(share_url.spec(), false);
+                         const std::map<base::StringPiece, std::string>& data,
+                         std::vector<base::StringPiece>* out) {
+  DCHECK(out);
 
-  std::vector<base::StringPiece> split_template;
   bool last_saw_open = false;
   size_t start_index_to_copy = 0;
   for (size_t i = 0; i < template_string.size(); ++i) {
@@ -47,10 +36,10 @@ bool ReplacePlaceholders(base::StringPiece template_string,
       if (template_string[i] == '}') {
         base::StringPiece placeholder = template_string.substr(
             start_index_to_copy + 1, i - 1 - start_index_to_copy);
-        auto it = placeholder_to_data.find(placeholder);
-        if (it != placeholder_to_data.end()) {
+        auto it = data.find(placeholder);
+        if (it != data.end()) {
           // Replace the placeholder text with the parameter value.
-          split_template.push_back(it->second);
+          out->push_back(it->second);
         }
 
         last_saw_open = false;
@@ -64,8 +53,8 @@ bool ReplacePlaceholders(base::StringPiece template_string,
         // Error: Saw close, with no corresponding open.
         return false;
       } else if (template_string[i] == '{') {
-        split_template.push_back(template_string.substr(
-            start_index_to_copy, i - start_index_to_copy));
+        out->push_back(template_string.substr(start_index_to_copy,
+                                              i - start_index_to_copy));
 
         last_saw_open = true;
         start_index_to_copy = i;
@@ -76,31 +65,56 @@ bool ReplacePlaceholders(base::StringPiece template_string,
     // Error: Saw open that was never closed.
     return false;
   }
-  split_template.push_back(template_string.substr(
+  out->push_back(template_string.substr(
       start_index_to_copy, template_string.size() - start_index_to_copy));
-
-  *out = base::StrCat(split_template);
   return true;
 }
 
 }  // namespace
 
+bool ValidateWebShareUrlTemplate(const GURL& url_template) {
+  return ReplaceWebShareUrlPlaceholders(url_template, /*title=*/"", /*text=*/"",
+                                        /*share_url=*/GURL(),
+                                        /*url_template_filled=*/nullptr);
+}
+
 bool ReplaceWebShareUrlPlaceholders(const GURL& url_template,
                                     base::StringPiece title,
                                     base::StringPiece text,
                                     const GURL& share_url,
                                     GURL* url_template_filled) {
-  std::string new_query;
-  std::string new_ref;
-  if (!ReplacePlaceholders(url_template.query_piece(), title, text, share_url,
-                           &new_query) ||
-      !ReplacePlaceholders(url_template.ref_piece(), title, text, share_url,
-                           &new_ref)) {
+  constexpr char kTitlePlaceholder[] = "title";
+  constexpr char kTextPlaceholder[] = "text";
+  constexpr char kUrlPlaceholder[] = "url";
+  std::map<base::StringPiece, std::string> replacements;
+  replacements[kTitlePlaceholder] = net::EscapeQueryParamValue(title, false);
+  replacements[kTextPlaceholder] = net::EscapeQueryParamValue(text, false);
+  replacements[kUrlPlaceholder] =
+      net::EscapeQueryParamValue(share_url.spec(), false);
+
+  std::vector<base::StringPiece> new_query_split;
+  std::vector<base::StringPiece> new_ref_split;
+  if (!ReplacePlaceholders(url_template.query_piece(), replacements,
+                           &new_query_split) ||
+      !ReplacePlaceholders(url_template.ref_piece(), replacements,
+                           &new_ref_split)) {
     return false;
   }
 
-  // Check whether |url_template| has a query in order to preserve the '?' in a
-  // URL with an empty query. e.g. http://www.google.com/?
+  // Early-return optimization, since ReplaceWebShareUrlPlaceholders() is called
+  // at manifest parse time just to get the success boolean, ignoring the
+  // result.
+  if (!url_template_filled)
+    return true;
+
+  // Ensure that the replacements are not deleted prior to
+  // GURL::ReplaceComponents() being called. GURL::Replacements::SetQueryStr()
+  // does not make a copy.
+  std::string new_query = base::StrCat(new_query_split);
+  std::string new_ref = base::StrCat(new_ref_split);
+
+  // Check whether |url_template| has a query in order to preserve the '?' in
+  // a URL with an empty query. e.g. http://www.google.com/?
   GURL::Replacements url_replacements;
   if (url_template.has_query())
     url_replacements.SetQueryStr(new_query);

content/public/common/manifest_share_target_util.h

@@ -14,6 +14,10 @@ class GURL;
 
 namespace content {
 
+// Determines whether |url_template| is valid; that is, whether
+// ReplaceWebShareUrlPlaceholders() would succeed for the given template.
+CONTENT_EXPORT bool ValidateWebShareUrlTemplate(const GURL& url_template);
+
 // Writes to |url_template_filled|, a copy of |url_template| with all
 // instances of "{title}", "{text}", and "{url}" in the query and fragment
 // parts of the URL replaced with |title|, |text|, and |url| respectively.

content/renderer/manifest/manifest_parser.cc

@@ -15,6 +15,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "content/public/common/manifest.h"
+#include "content/public/common/manifest_share_target_util.h"
 #include "content/public/common/manifest_util.h"
 #include "content/renderer/manifest/manifest_uma_util.h"
 #include "third_party/WebKit/public/platform/WebColor.h"
@@ -349,8 +350,16 @@ std::vector<Manifest::Icon> ManifestParser::ParseIcons(
 
 GURL ManifestParser::ParseShareTargetURLTemplate(
     const base::DictionaryValue& share_target) {
-  return ParseURL(share_target, "url_template", manifest_url_,
-                  ParseURLOriginRestrictions::kSameOriginOnly);
+  GURL url_template = ParseURL(share_target, "url_template", manifest_url_,
+                               ParseURLOriginRestrictions::kSameOriginOnly);
+  if (!ValidateWebShareUrlTemplate(url_template)) {
+    AddErrorInfo(
+        "property 'url_template' ignored. Placeholders have incorrect "
+        "syntax.");
+    return GURL();
+  }
+
+  return url_template;
 }
 
 base::Optional<Manifest::ShareTarget> ManifestParser::ParseShareTarget(

content/renderer/manifest/manifest_parser_unittest.cc

@@ -1089,9 +1089,24 @@ TEST_F(ManifestParserTest, ShareTargetUrlTemplateParseRules) {
     Manifest manifest = ParseManifestWithURLs(
         "{ \"share_target\": {\"url_template\": \"share/?title={title\" } }",
         manifest_url, document_url);
+    ASSERT_FALSE(manifest.share_target.has_value());
+    EXPECT_TRUE(manifest.IsEmpty());
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'url_template' ignored. Placeholders have incorrect "
+        "syntax.",
+        errors()[0]);
+  }
+
+  // Smoke test: Contains share_target and url_template, and url_template
+  // contains unknown placeholder.
+  {
+    Manifest manifest = ParseManifestWithURLs(
+        "{ \"share_target\": {\"url_template\": \"share/?title={abcxyz}\" } }",
+        manifest_url, document_url);
     ASSERT_TRUE(manifest.share_target.has_value());
     EXPECT_EQ(manifest.share_target.value().url_template.spec(),
-              "https://foo.com/share/?title={title");
+              "https://foo.com/share/?title={abcxyz}");
     EXPECT_FALSE(manifest.IsEmpty());
     EXPECT_EQ(0u, GetErrorCount());
   }