[Autofill/Autocomplete Feature] Substring matching instead of prefix matching.

components/autofill/core/browser/webdata/autofill_table.cc

Index: components/autofill/core/browser/webdata/autofill_table.cc
diff --git a/components/autofill/core/browser/webdata/autofill_table.cc b/components/autofill/core/browser/webdata/autofill_table.cc
index c337f60223f165cecc20a75e0ae7808ddaf7f800..8ba31537cec224792d0ec097f82aed8810fd6749 100644
--- a/components/autofill/core/browser/webdata/autofill_table.cc
+++ b/components/autofill/core/browser/webdata/autofill_table.cc
@@ -28,6 +28,7 @@
 #include "components/autofill/core/browser/webdata/autofill_change.h"
 #include "components/autofill/core/browser/webdata/autofill_entry.h"
 #include "components/autofill/core/common/autofill_switches.h"
+#include "components/autofill/core/common/autofill_util.h"
 #include "components/autofill/core/common/form_field_data.h"
 #include "components/os_crypt/os_crypt.h"
 #include "components/webdata/common/web_database.h"
@@ -456,38 +457,76 @@ bool AutofillTable::GetFormValuesForElementName(
     std::vector<base::string16>* values,
     int limit) {
   DCHECK(values);
-  sql::Statement s;
+  sql::Statement s1;
+  sql::Statement s2;

[please use gerrit instead, 2015/06/30 19:06:23]

I am beginning to think that it would be more correct to use && instead of ||.
Like this:

bool succeeded = false;

if (prefix.empty()) {
  sql::Statement s;
  ...
  succeeded = s.Succeeded();
} else {
  sql::Statement s1;
  ...
  succeeded = s1.Succeeded();

  if (IsFeatureSubstringMatchEnabled()) {
    sql::Statement s2;
    ...
    succeeded &= s2.Succeeded();
  }
}

return succeeded;

[Pritam Nikam, 2015/07/01 17:26:00]

In that case, if |succeeded = s1.Succeeded()| is false, irrespective of
|s2.Succeeded()|'s result |succeeded| will be always false.

IMO, || is appropreate here.

[please use gerrit instead, 2015/07/03 02:05:43]

Is there a case when s1.Succeeded() is false that does not indicate an error?

[Pritam Nikam, 2015/07/03 16:21:27]

Done.

I didn't find so far.

But logically, if |succeeded = s1.Succeeded();| execution results *false* then
|s2.Succeeded()|'s execution results would be immaterial if we *AND* their
results.

 
   if (prefix.empty()) {
-    s.Assign(db_->GetUniqueStatement(
+    s1.Assign(db_->GetUniqueStatement(
         "SELECT value FROM autofill "
         "WHERE name = ? "
         "ORDER BY count DESC "
         "LIMIT ?"));
-    s.BindString16(0, name);
-    s.BindInt(1, limit);
+    s1.BindString16(0, name);
+    s1.BindInt(1, limit);
+
+    values->clear();
+    while (s1.Step())
+      values->push_back(s1.ColumnString16(0));
   } else {
     base::string16 prefix_lower = base::i18n::ToLower(prefix);
     base::string16 next_prefix = prefix_lower;
     next_prefix[next_prefix.length() - 1]++;
 
-    s.Assign(db_->GetUniqueStatement(
+    s1.Assign(db_->GetUniqueStatement(
         "SELECT value FROM autofill "
         "WHERE name = ? AND "
         "value_lower >= ? AND "
         "value_lower < ? "
         "ORDER BY count DESC "
         "LIMIT ?"));
-    s.BindString16(0, name);
-    s.BindString16(1, prefix_lower);
-    s.BindString16(2, next_prefix);
-    s.BindInt(3, limit);
+    s1.BindString16(0, name);
+    s1.BindString16(1, prefix_lower);
+    s1.BindString16(2, next_prefix);
+    s1.BindInt(3, limit);
+
+    values->clear();
+    while (s1.Step())
+      values->push_back(s1.ColumnString16(0));
+
+    if (IsFeatureSubstringMatchEnabled()) {
+      s2.Assign(db_->GetUniqueStatement(
+          "SELECT value FROM autofill "
+          "WHERE name = ? AND ("
+          " value LIKE ? OR "
+          " value LIKE ? OR "
+          " value LIKE ? OR "
+          " value LIKE ? OR "
+          " value LIKE ? OR "
+          " value LIKE ? ESCAPE '!') "
+          "ORDER BY count DESC "
+          "LIMIT ?"));
+      s2.BindString16(0, name);
+      s2.BindString16(
+          1, base::ASCIIToUTF16("% ") + prefix_lower + base::ASCIIToUTF16("%"));

[please use gerrit instead, 2015/06/30 19:06:23]

If "prefix_lower" contains "_", "%%", or "; DROP TABLE autofill;"... will your
code break? Please add some tests for this.

[Pritam Nikam, 2015/07/01 17:26:00]

Yeah, there seems many problems :(
    

[please use gerrit instead, 2015/07/03 02:05:43]

The fix is not to move SQL injection mitigation into SubstringSubstituteText(),
but to do something like this instead:

-------------------------------------------------------------
s2.Assign(db->GetUniqueStatement(
    "SELECT value FROM autofill "
    "WHERE name = ? AND ("
    " value LIKE '%' || ? || '%' OR "
    " value LIKE '%' || ? || '%' OR "
    " value LIKE '%' || ? || '%' OR "
    " value LIKE '%' || ? || '%' OR "
    " value LIKE '%' || ? || '%' OR "
    " value LIKE '%' || ? || '%') "
    " ORDER BY count DESC "
    "LIMIT ?"));
s2.BindString16(0, name);
std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower);
s2.BindString(1, base::StringPrintf(" %s", prefix_lower_utf8));
s2.BindString(2, base::StringPrintf(".%s", prefix_lower_utf8));
s2.BindString(3, base::StringPrintf(",%s", prefix_lower_utf8));
s2.BindString(4, base::StringPrintf("-%s", prefix_lower_utf8));
s2.BindString(5, base::StringPrintf("@%s", prefix_lower_utf8));
s2.BindString(6, base::StringPrintf("_%s", prefix_lower_utf8));
s2.BindInt(7, limit);
-------------------------------------------------------------

BindString() will protect you from "_", "%", and "; DROP TABLE autofill;" in
"prefix_lower". If this does not work, I welcome you emailing me or finding me
on IRC to find a better way to achieve our goal here.

[Pritam Nikam, 2015/07/03 16:21:27]

This SQL query does not work on my chromium Linux build. I can see it treats (_)
& (%) as SQL keywords. Furthermore, as per SQLite documentation (from here [1])
for LIKE operator to handle literal (_), (%) or ESCAPE character as recommended
way is to use ESCAPE clause with ESCAPE character to prepend these.
const char* const prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower).c_str();
s2.BindString(1, base::StringPrintf(" %s", prefix_lower_utf8));


We need string16 version i.e. s2.BindString16() API instead.
I didn't find string formatting API for |wchar_t| on all platforms. Presently
its just for windows platform. Please refer [2].
Sent you details in mail.

References:
[1]. https://www.sqlite.org/lang_expr.html#like
[2].
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

+      s2.BindString16(
+          2, base::ASCIIToUTF16("%.") + prefix_lower + base::ASCIIToUTF16("%"));
+      s2.BindString16(
+          3, base::ASCIIToUTF16("%,") + prefix_lower + base::ASCIIToUTF16("%"));
+      s2.BindString16(
+          4, base::ASCIIToUTF16("%-") + prefix_lower + base::ASCIIToUTF16("%"));
+      s2.BindString16(
+          5, base::ASCIIToUTF16("%@") + prefix_lower + base::ASCIIToUTF16("%"));
+      s2.BindString16(6, base::ASCIIToUTF16("%!_") + prefix_lower +
+                             base::ASCIIToUTF16("%"));

[please use gerrit instead, 2015/06/30 19:06:23]

I am very happy that this works. Good job! Would your code be shorter and easier
to understand if you changed lines 509-520 to something like this?

std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower);
s2.BindString(1, base::StringPrintf("%% %s%%", prefix_lower_utf8));
s2.BindString(2, base::StringPrintf("%%.%s%%", prefix_lower_utf8));
s2.BindString(3, base::StringPrintf("%%,%s%%", prefix_lower_utf8));
s2.BindString(4, base::StringPrintf("%%-%s%%", prefix_lower_utf8));
s2.BindString(5, base::StringPrintf("%%@%s%%", prefix_lower_utf8));
s2.BindString(6, base::StringPrintf("%%!_%s%%", prefix_lower_utf8));

[Pritam Nikam, 2015/07/01 17:26:00]

These doesn't work :(

+      s2.BindInt(7, limit);
+
+      // Append substring matched suggestions.
+      while (s2.Step())
+        values->push_back(s2.ColumnString16(0));
+    }
   }
 
-  values->clear();
-  while (s.Step())
-    values->push_back(s.ColumnString16(0));
-  return s.Succeeded();
+  return s1.Succeeded() || s2.Succeeded();
 }
 
 bool AutofillTable::HasFormElements() {