Re: A working implementation of AQS (Assisted Query Stats). (issue 10537154)

[ba...@google.com]

It took me a while to figure the failure; it turned out that it was as
simple as
fixing operator= in AutocompleteMatch; I initially changed copy
constructor, but
I didn't realize there was also another place to update...

I addressed all comments, merged with head, resolved all conflicts and bots
are
happy.

The last remaining issue is to perform additional protocol check (must be
https)
before setting the AQS.


https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/autocomplete/autocomplete_unittest.cc
File chrome/browser/autocomplete/autocomplete_unittest.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode532
chrome/browser/autocomplete/autocomplete_unittest.cc:532: // Note: We
use dummy data to work around a compilation error
On 2012/06/19 05:53:43, Peter Kasting wrote:
> Nit: Slightly more informative:

> // MSVC doesn't support zero-length arrays, so supply some dummy
data.

Done.

https://chromiumcodereview.appspot.com/10537154/

[pkas...@chromium.org]

LGTM on what you have so far. Ping me when you add the protocol check and
I'll
take a look at that.

https://chromiumcodereview.appspot.com/10537154/

[ba...@google.com]

On 2012/06/22 19:46:13, Peter Kasting wrote:
> LGTM on what you have so far. Ping me when you add the protocol check and
I'll
> take a look at that.

Done, PTAL.

Currently, running "git try" to verify if all looks good.

https://chromiumcodereview.appspot.com/10537154/

[pkas...@chromium.org]

https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/autocomplete/autocomplete_match.cc
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/autocomplete/autocomplete_match.cc#newcode74
chrome/browser/autocomplete/autocomplete_match.cc:74: if
(match.associated_keyword.get())
Nit: For consistency, we could probably change these conditionals to
look like the unconditional reset() calls in operator=() below. (The
reverse change wouldn't be safe as they need to be unconditional in
operator=().)

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc#newcode219
chrome/browser/search_engines/template_url.cc:219: const std::string
kHttps("https");
Nit: Use chrome::kHttpsScheme from chrome/common/url_constants.h.

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc#newcode221
chrome/browser/search_engines/template_url.cc:221: // Either search or
autocomplete base URL must be using HTTPS
This code isn't actually sufficient.

First, someone could manually create a search engine with the AQS
substitution arg and an HTTP scheme, and we'd happily send the data over
HTTP because we're assuming any URL that uses this is using the Google
base URLs.

Second, along similar lines, did you want to restrict AQS from being
sent to non-Google URLs? If so, you'd also want some sort of
IsGoogleHostname() or similar check besides what you have here.

Perhaps the best way to solve this is by doing something like:

* Recursively call ReplaceSearchTermsUsingTermsData(), but pass in a
SearchTermsArgs that has no AQS string to avoid infinite recursion.
This will give you a trustworthy "final URL".
* Check the scheme and other properties of this URL directly to see
whether it's safe to substitute in the AQS string.
* Add some unittests for TemplateURL that create some URLs that
violate the constraint(s) above to the test suite.

Finally, the comment here is both too short and confusing. Perhaps a
longer explanation of what AQS is, what the constraints on it are, and
why those constraints are necessary, should be added to some canonical
place that talks about AQS, and then this comment can reference there.

https://chromiumcodereview.appspot.com/10537154/

[m...@chromium.org]

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc#newcode221
chrome/browser/search_engines/template_url.cc:221: // Either search or
autocomplete base URL must be using HTTPS

> If so, you'd also want some sort of IsGoogleHostname()...
See candidates at chrome/browser/google/google_util.h

https://chromiumcodereview.appspot.com/10537154/

[ba...@google.com]

PTAL. Please note my question about AQS docs.

https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/autocomplete/autocomplete_match.cc
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/autocomplete/autocomplete_match.cc#newcode74
chrome/browser/autocomplete/autocomplete_match.cc:74: if
(match.associated_keyword.get())

On 2012/06/22 21:48:31, Peter Kasting wrote:
> Nit: For consistency, we could probably change these conditionals to
look like
> the unconditional reset() calls in operator=() below. (The reverse
change
> wouldn't be safe as they need to be unconditional in operator=().)

Actually, to satisfy both performance & consistency, I've moved this
code to the constructor's initialization list.

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc#newcode219
chrome/browser/search_engines/template_url.cc:219: const std::string
kHttps("https");

On 2012/06/22 21:48:31, Peter Kasting wrote:
> Nit: Use chrome::kHttpsScheme from chrome/common/url_constants.h.

Done.

https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/search_engines/template_url.cc#newcode221
chrome/browser/search_engines/template_url.cc:221: // Either search or
autocomplete base URL must be using HTTPS

On 2012/06/22 21:48:31, Peter Kasting wrote:
> This code isn't actually sufficient.

> First, someone could manually create a search engine with the AQS
substitution
> arg and an HTTP scheme, and we'd happily send the data over HTTP
because we're
> assuming any URL that uses this is using the Google base URLs.

Yes, that's a good point. I fixed it according to your suggestion.

> Second, along similar lines, did you want to restrict AQS from being
sent to
> non-Google URLs? If so, you'd also want some sort of
IsGoogleHostname() or
> similar check besides what you have here.

No, we don't want to do that; that's the whole point of privacy
discussion and enforcing HTTPS checks, etc. In case of evil.com, privacy
folks mentioned mechanism of blacklisting providers.

> Perhaps the best way to solve this is by doing something like:

> * Recursively call ReplaceSearchTermsUsingTermsData(), but pass in a
> SearchTermsArgs that has no AQS string to avoid infinite recursion.
This will
> give you a trustworthy "final URL".

Done.

> * Check the scheme and other properties of this URL directly to see
whether
> it's safe to substitute in the AQS string.
> * Add some unittests for TemplateURL that create some URLs that
violate the
> constraint(s) above to the test suite.

> Finally, the comment here is both too short and confusing. Perhaps a
longer
> explanation of what AQS is, what the constraints on it are, and why
those
> constraints are necessary, should be added to some canonical place
that talks
> about AQS, and then this comment can reference there.

Where would you suggest adding the more detailed description of what AQS
is? I want to avoid unnecessary code shifting from one place to another
in case you have another preference than I do.

https://chromiumcodereview.appspot.com/10537154/

[pkas...@chromium.org]

I don't have a strong preference for where I'd put the AQS docs. Probably
in
the SearchTermsArgs struct? It's really up to you.

LGTM with comments


https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url.cc
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url.cc#newcode228
chrome/browser/search_engines/template_url.cc:228: if
(StartsWithASCII(base_url, chrome::kHttpsScheme, false)) {
Nit: Safer:

GURL base_url(ReplaceSearchTermsUsingTermsData(
search_terms_args_without_aqs, search_terms_data);
if (url.SchemeIs(chrome::kHttpsScheme)) { ...

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url_unittest.cc
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url_unittest.cc#newcode405
chrome/browser/search_engines/template_url_unittest.cc:405: // No
{google:baseURL} and protocol is HTTP, we must not substitute AQS.
We should also test that a non-Google base URL _with_ HTTPS allows AQS.

https://chromiumcodereview.appspot.com/10537154/

[ba...@google.com]

Peter, thanks for the long review process and putting up with me :) I hope
that
next reviews will be less painful as I have now more experience with Chrome
code
base.

Assuming that this is looking good now, what does it take to have it
submitted?
I'm not sure if I can do it, or whether you want someone else to do it
instead?

(Running git try in parallel).

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url.cc
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url.cc#newcode228
chrome/browser/search_engines/template_url.cc:228: if
(StartsWithASCII(base_url, chrome::kHttpsScheme, false)) {

On 2012/06/22 23:18:11, Peter Kasting wrote:
> Nit: Safer:

> GURL base_url(ReplaceSearchTermsUsingTermsData(
> search_terms_args_without_aqs, search_terms_data);
> if (url.SchemeIs(chrome::kHttpsScheme)) { ...

Done.

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url_unittest.cc
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/search_engines/template_url_unittest.cc#newcode405
chrome/browser/search_engines/template_url_unittest.cc:405: // No
{google:baseURL} and protocol is HTTP, we must not substitute AQS.

On 2012/06/22 23:18:11, Peter Kasting wrote:
> We should also test that a non-Google base URL _with_ HTTPS allows
AQS.

Done.

https://chromiumcodereview.appspot.com/10537154/

[mpea...@chromium.org]

On 2012/06/23 01:06:30, Bart N wrote:
> Assuming that this is looking good now, what does it take to have it
submitted?
> I'm not sure if I can do it, or whether you want someone else to do it
instead?

Answering on behalf of Peter, you should be able to check the "Commit:" box
on
this code review page and have the system use the commit queue to run some
tests
and submit the code for you. You can use this box even though you are not a
committer because the change has been approved by a committer.

--mark


https://chromiumcodereview.appspot.com/10537154/

[Bart Niechwiej]

On Sat, Jun 23, 2012 at 8:40 AM, <mpea...@chromium.org> wrote:

On 2012/06/23 01:06:30, Bart N wrote:

Assuming that this is looking good now, what does it take to have it

submitted?

I'm not sure if I can do it, or whether you want someone else to do it

instead?

Answering on behalf of Peter, you should be able to check the "Commit:" box on
this code review page and have the system use the commit queue to run some tests
and submit the code for you.  You can use this box even though you are not a
committer because the change has been approved by a committer.

Ah, thanks! I'll wait for a while in case Peter has more comments and then will submit.

Thanks all. 

--mark


https://chromiumcodereview.appspot.com/10537154/

[pkas...@chromium.org]

Once you have an LGTM, you're generally free to commit as soon as you've
addressed any remaining comments to your own satisfaction, unless something
major comes up.

I've checked the commit box.

https://chromiumcodereview.appspot.com/10537154/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ba...@google.com/10537154/70001


https://chromiumcodereview.appspot.com/10537154/

[commi...@chromium.org]

Change committed as 143827

https://chromiumcodereview.appspot.com/10537154/