Parse out XSSI guards in Suggestion JSON response

Parse out XSSI guards in Suggestion JSON response

For personalized suggestions in Suggest response, there is a desire for
additional security. In particular, there will be XSSI guards to prevent (well)
XSSI.

We are taking the approach of assuming that XSSI guards will always be there,
and just try to parse it out by looking for the start of valid sugggestion
response. Since the suggestion response is supposed to be a JS array, we look
for "[" in the response. And we repeat the search, in case the XSSI guard
contains "[".

BUG=312458
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231989

[Anuj, 2013-10-28 23:02:34]

Ready for Review PTAL.

[Peter Kasting, 2013-10-29 18:45:49]

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:653: while (true) {
This can infinite loop.

If we find the first '[', trim everything before that, then fail to
Deserialize(), we will loop, and the find() call will return 0 since the same
'[' we found the previous time is still present.

Why does there need to be a loop around this at all?  Why can't we just do all
this once?

[Maria, 2013-10-29 18:55:45]

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:653: while (true) {
I don't think it's likely that XSSI guard would change either. You could
actually look specifically for the guard characters at the start of the string
instead of just any opening square bracket to ensure that we are handling the
case we are expecting instead of possibly an error message with brackets in it.

[Anuj, 2013-10-29 18:58:04]

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:653: while (true) {
Please see the test cases - Hypothetically XSSI guard can itself contain "[".
I think I ran the test after uploading, and didn't upload the fix.

[mariakhomenko, 2013-10-29 19:02:54]

I was under the impression that the XSSI guard used was always: ")]}'\n"


On Tue, Oct 29, 2013 at 11:58 AM, <skanuj@chromium.org> wrote:

>
> https://codereview.chromium.**org/45863006/diff/30001/**
>
chrome/browser/autocomplete/**search_provider.cc<https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomplete/search_provider.cc>
> File chrome/browser/autocomplete/**search_provider.cc (right):
>
> https://codereview.chromium.**org/45863006/diff/30001/**
>
chrome/browser/autocomplete/**search_provider.cc#newcode653<https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomplete/search_provider.cc#newcode653>
> chrome/browser/autocomplete/**search_provider.cc:653: while (true) {
> Please see the test cases - Hypothetically XSSI guard can itself contain
> "[".
> I think I ran the test after uploading, and didn't upload the fix.
>
>
https://codereview.chromium.**org/45863006/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anuj, 2013-10-29 19:03:35]

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/30001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:653: while (true) {
XSSI guard may not change. But looking for response seems a better strategy to
me than stripping out specific XSSI guard.

And in any case, stripping XSSI guard doesn't help against the error message
scenario either.

[Peter Kasting, 2013-10-29 19:03:57]

LGTM

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:652: int error_code = 0;
This doesn't need to be declared outside the loop.  Declare it where it's used.

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:656: size_t response_start_index
= json_data.find("[", lookup_start_index);
Just use a for loop to avoid this temp and write things more clearly and
compactly:

    for (size_t response_start_index = json_data.find("[");
         response_start_index != std::string::npos;
         response_start_index = json_data.find("[", 1)) {
      // Remove any XSSI guards to allow for JSON parsing.
      if (response_start_index > 0)
      ...

[Anuj, 2013-10-29 19:10:34]

Done. CQ now.

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:652: int error_code = 0;
On 2013/10/29 19:03:57, Peter Kasting wrote:
> This doesn't need to be declared outside the loop.  Declare it where it's
used.

Done.

https://codereview.chromium.org/45863006/diff/80001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:656: size_t response_start_index
= json_data.find("[", lookup_start_index);
On 2013/10/29 19:03:57, Peter Kasting wrote:
> Just use a for loop to avoid this temp and write things more clearly and
> compactly:
> 
>     for (size_t response_start_index = json_data.find("[");
>          response_start_index != std::string::npos;
>          response_start_index = json_data.find("[", 1)) {
>       // Remove any XSSI guards to allow for JSON parsing.
>       if (response_start_index > 0)
>       ...

Done.

[Anuj, 2013-10-29 19:11:20]

Same as the reply to your code review comment :)

[commit-bot: I haz the power, 2013-10-29 19:34:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/45863006/180001

[commit-bot: I haz the power, 2013-10-30 00:22:06]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-10-30 01:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/45863006/170002

[commit-bot: I haz the power, 2013-10-30 03:54:19]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-30 06:05:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/45863006/630001

[commit-bot: I haz the power, 2013-10-30 09:57:06]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-30 21:54:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/45863006/810001

[commit-bot: I haz the power, 2013-10-31 04:09:50]

Change committed as 231989