Refuse invalid SearchProvider and OSDD suggest URLs; etc.

chrome/browser/search_engines/template_url_parser.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_parser.h"
 
 #include <algorithm>
 #include <map>
 #include <vector>
 
@@ skipping 126 matching lines @@
 
   static void StartElementImpl(void* ctx,
                                const xmlChar* name,
                                const xmlChar** atts);
   static void EndElementImpl(void* ctx, const xmlChar* name);
   static void CharactersImpl(void* ctx, const xmlChar* ch, int len);
 
   // Returns a heap-allocated TemplateURL representing the result of parsing.
   // This will be NULL if parsing failed or if the results were invalid for some
   // reason (e.g. the resulting URL was not HTTP[S], a name wasn't supplied,
-  // etc.).
+  // a resulting TempateURLRef was invalid, etc.).
   TemplateURL* GetTemplateURL(Profile* profile, bool show_in_default_list);
 
  private:
   // Key is UTF8 encoded.
   typedef std::map<std::string, ElementType> ElementNameToElementTypeMap;
 
   static void InitMapping();
 
   void ParseURL(const xmlChar** atts);
   void ParseImage(const xmlChar** atts);
@@ skipping 126 matching lines @@
 void TemplateURLParsingContext::CharactersImpl(void* ctx,
                                                const xmlChar* ch,
                                                int len) {
   reinterpret_cast<TemplateURLParsingContext*>(ctx)->string_ +=
       UTF8ToUTF16(std::string(reinterpret_cast<const char*>(ch), len));
 }
 
 TemplateURL* TemplateURLParsingContext::GetTemplateURL(
     Profile* profile,
     bool show_in_default_list) {
-  // Basic legality checks.
-  if (data_.short_name.empty() || !IsHTTPRef(data_.url()) ||
-      !IsHTTPRef(data_.suggestions_url))
-    return NULL;
-
-  // If the image was a data URL, use the favicon from the search URL instead.
-  // (see TODO inEndElementImpl()).
-  GURL url(data_.url());
-  if (derive_image_from_url_ && data_.favicon_url.is_empty())
-    data_.favicon_url = TemplateURL::GenerateFaviconURL(url);
-
-  // TODO(jcampan): http://b/issue?id=1196285 we do not support search engines
-  //                that use POST yet.
-  if (method_ == TemplateURLParsingContext::POST)
+  // TODO(jcampan): Support engines that use POST; see http://crbug.com/18107
+  if (method_ == TemplateURLParsingContext::POST ||
+      data_.short_name.empty() || data_.url().empty() ||

[Peter Kasting, 2012/09/05 23:31:27]

There's no need to add the check for data_.url().empty(); TemplateURLData
guarantees this field is never empty.

[msw, 2012/09/06 20:02:29]

Done.

+      !IsHTTPRef(data_.url()) || !IsHTTPRef(data_.suggestions_url))
     return NULL;
   if (suggestion_method_ == TemplateURLParsingContext::POST)
     data_.suggestions_url.clear();
 
-  data_.SetKeyword(TemplateURLService::GenerateKeyword(url));
+  // If the image was a data URL, use the favicon from the search URL instead.
+  // (see the TODO in EndElementImpl()).
+  GURL search_url(data_.url());

[Peter Kasting, 2012/09/05 23:31:27]

Urf.  Both GenerateFaviconURL() and GenerateKeyword() below DCHECK that
|search_url| is valid.  But I don't think that's actually guaranteed by the
parsing code.  (I think I screwed this up refactoring it months back.)

I think what we need to do here is construct a "temporary" TemplateURL from
|data_|, then call ReplaceSearchTerms() with "x" (or whatever) as the dummy
search string, and use _that_ URL as the search_url here.  We can then check
this for validity (and check that suggestions_url() is either empty() or the
result of ReplaceSearchTerms() on it is valid as well) below; this can replace
the IsValid() calls on the TemplateURLRefs because if those are not valid,
ReplaceSearchTerms() will return the empty string.

The annoying bit about this is that we basically have to construct a TemplateURL
twice, but I don't see what we can do about that.

[msw, 2012/09/06 20:02:29]

Invalid search/suggest URLs bail above in the IsHTTPRef checks anyway.
Adding full support for invalid URLs is out of scope for this *crash fix*.

Removed the DCHECKs anyway, as the functions fallback okay for invalid GURLs:
GenerateFaviconURL returns GURL() for optional TemplateURLData::favicon_url.
GenerateKeyword returns "www" or some best effort via GURL::host(), editable.

+  if (derive_image_from_url_ && data_.favicon_url.is_empty())
+    data_.favicon_url = TemplateURL::GenerateFaviconURL(search_url);
+
+  data_.SetKeyword(TemplateURLService::GenerateKeyword(search_url));
   data_.show_in_default_list = show_in_default_list;
-  return new TemplateURL(profile, data_);
+
+  // Bail if the search URL is empty or if either TemplateURLRef is invalid.
+  scoped_ptr<TemplateURL> template_url(new TemplateURL(profile, data_));
+  if (template_url->url().empty() || !template_url->url_ref().IsValid() ||
+      (!template_url->suggestions_url().empty() &&
+       !template_url->suggestions_url_ref().IsValid()))
+    return NULL;
+
+  return template_url.release();
 }
 
 // static
 void TemplateURLParsingContext::InitMapping() {
   kElementNameToElementTypeMap = new std::map<std::string, ElementType>;
   (*kElementNameToElementTypeMap)[kURLElement] = URL;
   (*kElementNameToElementTypeMap)[kParamElement] = PARAM;
   (*kElementNameToElementTypeMap)[kShortNameElement] = SHORT_NAME;
   (*kElementNameToElementTypeMap)[kImageElement] = IMAGE;
   (*kElementNameToElementTypeMap)[kOpenSearchDescriptionElement] =
@@ skipping 153 matching lines @@
   // &#38; . Unfortunately xmlSubstituteEntitiesDefault affects global state.
   // If this becomes problematic we'll need to provide our own entity
   // type for &amp;, or strip out &#38; by hand after parsing.
   int last_sub_entities_value = xmlSubstituteEntitiesDefault(1);
   TemplateURLParsingContext context(param_filter);
   xmlSAXHandler sax_handler;
   memset(&sax_handler, 0, sizeof(sax_handler));
   sax_handler.startElement = &TemplateURLParsingContext::StartElementImpl;
   sax_handler.endElement = &TemplateURLParsingContext::EndElementImpl;
   sax_handler.characters = &TemplateURLParsingContext::CharactersImpl;
   xmlSAXUserParseMemory(&sax_handler, &context, data, static_cast<int>(length));

[Peter Kasting, 2012/09/05 23:31:27]

I think if this returns nonzero, we should return NULL below.

[msw, 2012/09/06 20:02:29]

Done.

   xmlSubstituteEntitiesDefault(last_sub_entities_value);
 
   return context.GetTemplateURL(profile, show_in_default_list);
 }