Prevent querying of restricted query values.

Prevent querying of restricted query values from the instant extended JS api.

BUG=170785
TEST=NONE


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

[sreeram, 2013-03-05 05:43:44]

I think this QueryState stuff is overkill. You should be able to get by with
just exposing SearchBox::set_query() so that SearchBoxExtension can call it. And
then, just set_query(string16()) whenever the page does a
setValue(restrictedId). Wouldn't that be sufficient?

[robertshield, 2013-03-05 13:28:09]

On 2013/03/05 05:43:44, sreeram wrote:
> I think this QueryState stuff is overkill. You should be able to get by with
> just exposing SearchBox::set_query() so that SearchBoxExtension can call it.
And
> then, just set_query(string16()) whenever the page does a
> setValue(restrictedId). Wouldn't that be sufficient?

Yep, that work and is much less code. I'd been trying to preserve
SearchBox::query_ thinking that others might use it, but looks like that isn't
needed.  PTAL.

[Jered, 2013-03-05 17:47:44]

https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
File chrome/test/data/instant_extended.html (right):

https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
chrome/test/data/instant_extended.html:36: // apiHandle.setRestrictedValue can
work.
Can you explain a bit more? We should document or fix this behavior.

[sreeram, 2013-03-05 17:50:23]

lgtm

[robertshield, 2013-03-05 18:19:04]

https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
File chrome/test/data/instant_extended.html (right):

https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
chrome/test/data/instant_extended.html:36: // apiHandle.setRestrictedValue can
work.
On 2013/03/05 17:47:44, Jered wrote:
> Can you explain a bit more? We should document or fix this behavior.

apiHandle.setRestrictedValue ends up in
SearchBoxExtensionWrapper::SetQueryFromAutocompleteResult which calls
SearchBox::GetAutocompleteResultWithId. 

This last accesses a list called last_autocomplete_results_ which is set only
when SearchBox::GetAutocompleteResults is called.

I added a comment to searchbox_api.js to this effect, let me know if you'd
prefer a more detailed comment somewhere else.

[Jered, 2013-03-05 18:56:35]

On 2013/03/05 18:19:04, robertshield wrote:
>
https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
> File chrome/test/data/instant_extended.html (right):
> 
>
https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
> chrome/test/data/instant_extended.html:36: // apiHandle.setRestrictedValue can
> work.
> On 2013/03/05 17:47:44, Jered wrote:
> > Can you explain a bit more? We should document or fix this behavior.
> 
> apiHandle.setRestrictedValue ends up in
> SearchBoxExtensionWrapper::SetQueryFromAutocompleteResult which calls
> SearchBox::GetAutocompleteResultWithId. 
> 
> This last accesses a list called last_autocomplete_results_ which is set only
> when SearchBox::GetAutocompleteResults is called.
> 
> I added a comment to searchbox_api.js to this effect, let me know if you'd
> prefer a more detailed comment somewhere else.

I was thinking http://dev.chromium.org/embeddedsearch

But perhaps it's obvious or not an issue, since where else would you get an rid?
I suppose there could be confusion about trying to suggest a thumbnail or other
resource identified by an rid (will that work?), and it would be helpful to
clarify that.

[commit-bot: I haz the power, 2013-03-05 19:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12319108/32003

[robertshield, 2013-03-05 19:18:59]

On 2013/03/05 18:56:35, Jered wrote:
> On 2013/03/05 18:19:04, robertshield wrote:
> >
>
https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
> > File chrome/test/data/instant_extended.html (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12319108/diff/25001/chrome/test/data/i...
> > chrome/test/data/instant_extended.html:36: // apiHandle.setRestrictedValue
can
> > work.
> > On 2013/03/05 17:47:44, Jered wrote:
> > > Can you explain a bit more? We should document or fix this behavior.
> > 
> > apiHandle.setRestrictedValue ends up in
> > SearchBoxExtensionWrapper::SetQueryFromAutocompleteResult which calls
> > SearchBox::GetAutocompleteResultWithId. 
> > 
> > This last accesses a list called last_autocomplete_results_ which is set
only
> > when SearchBox::GetAutocompleteResults is called.
> > 
> > I added a comment to searchbox_api.js to this effect, let me know if you'd
> > prefer a more detailed comment somewhere else.
> 
> I was thinking http://dev.chromium.org/embeddedsearch
> 
> But perhaps it's obvious or not an issue, since where else would you get an
rid?

As you say, in practice, it's probably not an issue as callers need to get an
rid first.

> I suppose there could be confusion about trying to suggest a thumbnail or
other
> resource identified by an rid (will that work?), 

It wouldn't work but it might do something. The rids for autocomplete seem to be
ints from 0 to number of suggestions, if the other rid sources use the same
scheme, there could be collisions.

and it would be helpful to
> clarify that.

I'll add a note to the wiki page you linked to.

[commit-bot: I haz the power, 2013-03-05 19:24:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-08 16:18:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12319108/32003

[commit-bot: I haz the power, 2013-03-08 16:18:08]

Failed to apply patch for
chrome/browser/instant/instant_extended_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/instant/instant_extended_browsertest.cc
  Hunk #1 FAILED at 5.
  Hunk #2 succeeded at 1091 with fuzz 2 (offset 259 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/instant/instant_extended_browsertest.cc.rej

Patch:       chrome/browser/instant/instant_extended_browsertest.cc
Index: chrome/browser/instant/instant_extended_browsertest.cc
diff --git a/chrome/browser/instant/instant_extended_browsertest.cc
b/chrome/browser/instant/instant_extended_browsertest.cc
index
4068cc0ddd2220c6ab38ea3bc4cc238de41f1c54..4cf9fe1e22d100167eb805b981a05db32c1ee208
100644
--- a/chrome/browser/instant/instant_extended_browsertest.cc
+++ b/chrome/browser/instant/instant_extended_browsertest.cc
@@ -5,6 +5,10 @@
 #include <sstream>
 
 #include "base/prefs/pref_service.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/autocomplete/autocomplete_provider.h"
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/instant/instant_commit_type.h"
 #include "chrome/browser/instant/instant_ntp.h"
@@ -832,3 +836,79 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_EQ(overlay, active_tab);
 }
+
+IN_PROC_BROWSER_TEST_F(InstantExtendedTest, RestrictedItemReadback) {
+  content::WindowedNotificationObserver observer(
+      chrome::NOTIFICATION_INSTANT_SENT_MOST_VISITED_ITEMS,
+      content::NotificationService::AllSources());
+
+  // Initialize Instant.
+  ASSERT_NO_FATAL_FAILURE(SetupInstant());
+  FocusOmniboxAndWaitForInstantSupport();
+
+  // Get a handle to the NTP and the current state of the JS.
+  ASSERT_NE(static_cast<InstantNTP*>(NULL), instant()->ntp());
+  content::WebContents* preview_tab = instant()->ntp()->contents();
+  EXPECT_TRUE(preview_tab);
+  EXPECT_TRUE(UpdateSearchState(preview_tab));
+
+  // Manufacture a few autocomplete results and get them down to the page.
+  std::vector<InstantAutocompleteResult> autocomplete_results;
+  for (int i = 0; i < 3; ++i) {
+    std::string description(base::StringPrintf("Test Description %d", i));
+    std::string url(base::StringPrintf("http://www.testurl%d.com", i));
+
+    InstantAutocompleteResult res;
+    res.provider = ASCIIToUTF16(AutocompleteProvider::TypeToString(
+        AutocompleteProvider::TYPE_BUILTIN));
+    res.type = ASCIIToUTF16(AutocompleteMatch::TypeToString(
+        AutocompleteMatch::SEARCH_WHAT_YOU_TYPED)),
+    res.description = ASCIIToUTF16(description);
+    res.destination_url = ASCIIToUTF16(url);
+    res.transition = content::PAGE_TRANSITION_TYPED;
+    res.relevance = 42 + i;
+
+    autocomplete_results.push_back(res);
+  }
+  instant()->overlay()->SendAutocompleteResults(autocomplete_results);
+
+  const char kQueryString[] = "Hippos go berzerk!";
+
+  // First set the query text to a non restricted value and ensure it can be
+  // read back.
+  std::ostringstream stream;
+  stream << "apiHandle.setValue('" << kQueryString << "');";
+  EXPECT_TRUE(ExecuteScript(stream.str()));
+  observer.Wait();
+
+  std::string result;
+  EXPECT_TRUE(GetStringFromJS(instant()->GetOverlayContents(),
+                              "apiHandle.value",
+                              &result));
+  EXPECT_EQ(kQueryString, result);
+
+  // Set the query text to the first restricted autocomplete item.
+  int rid = 0;
+  stream.str(std::string());
+  stream << "apiHandle.setRestrictedValue(" << rid << ")";
+  EXPECT_TRUE(ExecuteScript(stream.str()));
+  observer.Wait();
+
+  // Expect that we now receive the empty string when reading the value back.
+  EXPECT_TRUE(GetStringFromJS(instant()->GetOverlayContents(),
+                              "apiHandle.value",
+                              &result));
+  EXPECT_EQ("", result);
+
+  // Now set the query text to a non restricted value and ensure that the
+  // visibility has been reset and the string can again be read back.
+  stream.str(std::string());
+  stream << "apiHandle.setValue('" << kQueryString << "');";
+  EXPECT_TRUE(ExecuteScript(stream.str()));
+  observer.Wait();
+
+  EXPECT_TRUE(GetStringFromJS(instant()->GetOverlayContents(),
+                              "apiHandle.value",
+                              &result));
+  EXPECT_EQ(kQueryString, result);
+}

[commit-bot: I haz the power, 2013-03-08 16:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12319108/41001

[commit-bot: I haz the power, 2013-03-08 17:57:08]

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

[commit-bot: I haz the power, 2013-03-12 16:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12319108/73001

[commit-bot: I haz the power, 2013-03-12 21:40:39]

Change committed as 187654