Refuse invalid SearchProvider and OSDD suggest URLs; etc.

Refuse invalid SearchProvider and OSDD suggest URLs; etc.

Change SearchProvider::CreateSuggestFetcher:
Validate the TemplateURL[Ref] and suggest URL on search.
Return NULL early if invalid; this prevents crashes.

Change TemplateURLParsingContext::GetTemplateURL:
Validate the TemplateURLRefs from OSDDs import.
Return NULL early if invalid (don't import bad engines).

Return NULL early on TemplateURLParser::Parse error.

TODO(followup): Validate existing TemplateURLs in profile data?
TODO(followup): Support invalid GURLs that may be valid after replacements.

BUG=145852
TEST=Less crashes as reported in bug, import succeeds on [1], fails (no-op) on [2] and [3]...

[1] http://ready.to/search/en/?sna=MSW_TEST_SUGGEST&prf=http%3A%2F%2Fsearch.yahoo.com%2Fsearch%3Fei%3D%7BinputEncoding%7D%26amp%3Bfr%3Dcrmas%26amp%3Bp%3D&des=MSW_TEST_SUGGEST&opt=%26lt%3BUrl%20type%3D%26quot%3Bapplication%2Fx-suggestions%2Bjson%26quot%3B%20template%3D%26quot%3Bhttp%3A%2F%2Fff.search.yahoo.com%2Fgossip%3Foutput%3Dfxjson%26amp%3Bcommand%3D%7BsearchTerms%7D%26quot%3B%2F%26gt%3B&in=utf&ou=ono&mod=pn#

[2] http://ready.to/search/en/?sna=MSW_TEST_SUGGEST_BADSCHEME&prf=http%3A%2F%2Fsearch.yahoo.com%2Fsearch%3Fei%3D%7BinputEncoding%7D%26amp%3Bfr%3Dcrmas%26amp%3Bp%3D&des=MSW_TEST_SUGGEST_BADSCHEME&opt=%26lt%3BUrl%20type%3D%26quot%3Bapplication%2Fx-suggestions%2Bjson%26quot%3B%20template%3D%26quot%3Bbadscheme%3A%2F%2Fff.search.yahoo.com%2Fgossip%3Foutput%3Dfxjson%26amp%3Bcommand%3D%7BsearchTerms%7D%26quot%3B%2F%26gt%3B&in=utf&ou=ono&mod=pn#

[3] http://ready.to/search/en/?sna=MSW_TEST_SUGGEST_BADOPENBRACE&prf=http%3A%2F%2Fsearch.yahoo.com%2Fsearch%3Fei%3D%7BinputEncoding%7D%26amp%3Bfr%3Dcrmas%26amp%3Bp%3D&des=MSW_TEST_SUGGEST_BADOPENBRACE&opt=%26lt%3BUrl%20type%3D%26quot%3Bapplication%2Fx-suggestions%2Bjson%26quot%3B%20template%3D%26quot%3Bhttp%3A%2F%2Fff.search.yahoo.com%2Fgossip%3Foutput%3Dfxjson%26amp%3Bcommand%3D%7BsearchTerms%7D%26amp%3Bfoo%3D%7Bbad%26quot%3B%2F%26gt%3B&in=utf&ou=ono&mod=pn

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155315

[msw, 2012-08-31 19:32:43]

Hey Peter, please take a look; thanks!
I'm investigating the importation of invalid OSDD separately.

[msw, 2012-09-04 22:51:44]

Hey Peter, please take a look; thanks!

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

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/autocomplete...
chrome/browser/autocomplete/search_provider.cc:638:
!template_url->suggestions_url_ref().IsValid())
You don't need to check IsValid(); if it fails, ReplaceSearchTerms() below will
return the empty string and you'll fail the next conditional.

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/autocomplete...
chrome/browser/autocomplete/search_provider.cc:643:
TemplateURLRef::SearchTermsArgs(text)));
Nit: Indent 4 from beginning of line

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_parser.cc (right):

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_parser.cc:296:
data_.short_name.empty() || data_.url().empty() ||
There's no need to add the check for data_.url().empty(); TemplateURLData
guarantees this field is never empty.

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_parser.cc:304: GURL
search_url(data_.url());
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.

http://codereview.chromium.org/10908044/diff/8001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_parser.cc:492:
xmlSAXUserParseMemory(&sax_handler, &context, data, static_cast<int>(length));
I think if this returns nonzero, we should return NULL below.

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

Addressed comments, please take another look, thanks!

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.cc:638:
!template_url->suggestions_url_ref().IsValid())
On 2012/09/05 23:31:27, Peter Kasting wrote:
> You don't need to check IsValid(); if it fails, ReplaceSearchTerms() below
will
> return the empty string and you'll fail the next conditional.

Done.

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.cc:643:
TemplateURLRef::SearchTermsArgs(text)));
On 2012/09/05 23:31:27, Peter Kasting wrote:
> Nit: Indent 4 from beginning of line

Done.

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_parser.cc (right):

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.cc:296:
data_.short_name.empty() || data_.url().empty() ||
On 2012/09/05 23:31:27, Peter Kasting wrote:
> There's no need to add the check for data_.url().empty(); TemplateURLData
> guarantees this field is never empty.

Done.

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.cc:304: GURL
search_url(data_.url());
On 2012/09/05 23:31:27, Peter Kasting wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/10908044/diff/8001/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.cc:492:
xmlSAXUserParseMemory(&sax_handler, &context, data, static_cast<int>(length));
On 2012/09/05 23:31:27, Peter Kasting wrote:
> I think if this returns nonzero, we should return NULL below.

Done.

[Peter Kasting, 2012-09-06 20:28:31]

I missed that IsHTTPRef() enforces that things are empty-or-valid.  That's
actually too strong a guarantee, but I'm OK with not fixing that here.

Please don't remove the DCHECKs on those functions.  We want to continue to
enforce that all callers must pass in valid GURLs.

LGTM once those are restored.

https://chromiumcodereview.appspot.com/10908044/diff/7003/chrome/browser/sear...
File chrome/browser/search_engines/template_url_parser.cc (right):

https://chromiumcodereview.appspot.com/10908044/diff/7003/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.cc:147: // a resulting
TempateURLRef was invalid, etc.).
Nit: Tempate -> Template

[msw, 2012-09-06 20:38:35]

Thanks, landing.

https://chromiumcodereview.appspot.com/10908044/diff/7003/chrome/browser/sear...
File chrome/browser/search_engines/template_url_parser.cc (right):

https://chromiumcodereview.appspot.com/10908044/diff/7003/chrome/browser/sear...
chrome/browser/search_engines/template_url_parser.cc:147: // a resulting
TempateURLRef was invalid, etc.).
On 2012/09/06 20:28:31, Peter Kasting wrote:
> Nit: Tempate -> Template

Done.

[commit-bot: I haz the power, 2012-09-06 20:41:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10908044/9002

[commit-bot: I haz the power, 2012-09-07 05:29:13]

Change committed as 155315