Fix 2 bugs in extensions autoupdate request parameters.

-Separator that should be between id and version parameters was at the end of version
-Missing the "uc" parameter which the gallery server code was expecting as indication
 of an update check

BUG=http://crbug.com/17469
TEST=extensions auto-update should work

Review URL: http://codereview.chromium.org/159224

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@21582 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 116 insertions, 17 deletions

diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index a6a5ca5..bb14931 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -19,6 +19,7 @@
 #include "chrome/common/extensions/extension_error_reporter.h"
 #include "chrome/common/libxml_utils.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/escape.h"
 #include "net/url_request/url_request_status.h"
 #include "libxml/tree.h"
 
@@ -225,8 +226,40 @@ void ExtensionUpdater::OnExtensionInstallFinished(const FilePath& path,
     file_handler_.get(), &ExtensionUpdaterFileHandler::DeleteFile, path));
 }
 
-static const char* kURLEncodedEquals = "%3D";  // '='
-static const char* kURLEncodedAnd = "%26";  // '&'
+
+// Helper function for building up request parameters in update check urls. It
+// appends information about one extension to a request parameter string. The
+// format for request parameters in update checks is:
+//
+//   ?x=EXT1_INFO&x=EXT2_INFO
+//
+// where EXT1_INFO and EXT2_INFO are url-encoded strings of the form:
+//
+//   id=EXTENSION_ID&v=VERSION&uc
+//
+// So for two extensions like:
+//   Extension 1- id:aaaa version:1.1
+//   Extension 2- id:bbbb version:2.0
+//
+// the full update url would be:
+//   http://somehost/path?x=id%3Daaaa%26v%3D1.1%26uc&x=id%3Dbbbb%26v%3D2.0%26uc
+//
+// (Note that '=' is %3D and '&' is %26 when urlencoded.)
+//
+// Again, this function would just append one extension's worth of data, e.g.
+// "x=id%3Daaaa%26v%3D1.1%26uc"
+void AppendExtensionInfo(std::string* str, const Extension& extension) {
+    const Version* version = extension.version();
+    DCHECK(version);
+    std::vector<std::string> parts;
+
+    // Push extension id, version, and uc (indicates an update check to Omaha).
+    parts.push_back("id=" + extension.id());
+    parts.push_back("v=" + version->GetString());
+    parts.push_back("uc");
+
+    str->append("x=" + EscapeQueryParamValue(JoinString(parts, '&')));
+}
 
 void ExtensionUpdater::TimerFired() {
   // Generate a set of update urls for loaded extensions.
@@ -246,18 +279,7 @@ void ExtensionUpdater::TimerFired() {
     // Append extension information to the url.
     std::string full_url_string = update_url.spec();
     full_url_string.append(update_url.has_query() ? "&" : "?");
-    full_url_string.append("x=");
-
-    full_url_string.append("id");
-    full_url_string.append(kURLEncodedEquals);
-    full_url_string.append(extension->id());
-
-    const Version* version = extension->version();
-    DCHECK(version);
-    full_url_string.append("v");
-    full_url_string.append(kURLEncodedEquals);
-    full_url_string.append(version->GetString());
-    full_url_string.append(kURLEncodedAnd);
+    AppendExtensionInfo(&full_url_string, *extension);
 
     GURL full_url(full_url_string);
     if (!full_url.is_valid()) {
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index 999adbd..ebe73fb 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -10,6 +10,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_error_reporter.h"
+#include "net/base/escape.h"
 #include "net/url_request/url_request_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -112,8 +113,10 @@ class MockService : public ExtensionUpdateService {
 const char* kIdPrefix = "000000000000000000000000000000000000000";
 
 // Creates test extensions and inserts them into list. The name and
-// version are all based on their index.
-void CreateTestExtensions(int count, ExtensionList *list) {
+// version are all based on their index. If |update_url| is non-null, it
+// will be used as the update_url for each extension.
+void CreateTestExtensions(int count, ExtensionList *list,
+                          const std::string* update_url) {
   for (int i = 1; i <= count; i++) {
     DictionaryValue input;
     Extension* e = new Extension();
@@ -121,6 +124,8 @@ void CreateTestExtensions(int count, ExtensionList *list) {
                     StringPrintf("%d.0.0.0", i));
     input.SetString(extension_manifest_keys::kName,
                     StringPrintf("Extension %d", i));
+    if (update_url)
+      input.SetString(extension_manifest_keys::kUpdateURL, *update_url);
     std::string error;
     EXPECT_TRUE(e->InitFromValue(input, false, &error));
     list->push_back(e);
@@ -195,6 +200,27 @@ class ServiceForDownloadTests : public MockService {
 
 static const int kUpdateFrequencySecs = 15;
 
+// Takes a string with KEY=VALUE parameters separated by '&' in |params| and
+// puts the key/value pairs into |result|. For keys with no value, the empty
+// string is used. So for "a=1&b=foo&c", result would map "a" to "1", "b" to
+// "foo", and "c" to "".
+static void ExtractParameters(const std::string params,
+                              std::map<std::string, std::string>* result) {
+  std::vector<std::string> pairs;
+  SplitString(params, '&', &pairs);
+  for (size_t i = 0; i < pairs.size(); i++) {
+    std::vector<std::string> key_val;
+    SplitString(pairs[i], '=', &key_val);
+    if (key_val.size() > 0) {
+      std::string key = key_val[0];
+      EXPECT_TRUE(result->find(key) == result->end());
+      (*result)[key] = (key_val.size() == 2) ? key_val[1] : "";
+    } else {
+      NOTREACHED();
+    }
+  }
+}
+
 // All of our tests that need to use private APIs of ExtensionUpdater live
 // inside this class (which is a friend to ExtensionUpdater).
 class ExtensionUpdaterTest : public testing::Test {
@@ -244,11 +270,58 @@ class ExtensionUpdaterTest : public testing::Test {
     EXPECT_TRUE(ExtensionUpdater::Parse(similar_tagnames, &results.get()));
   }
 
+  static void TestUpdateCheckRequests() {
+    // Create an extension with an update_url.
+    ServiceForManifestTests service;
+    ExtensionList tmp;
+    std::string update_url("http://foo.com/bar");
+    CreateTestExtensions(1, &tmp, &update_url);
+    service.set_extensions(tmp);
+
+    // Setup and start the updater.
+    TestURLFetcherFactory factory;
+    URLFetcher::set_factory(&factory);
+    MessageLoop message_loop;
+    scoped_refptr<ExtensionUpdater> updater =
+      new ExtensionUpdater(&service, 60*60*24, &message_loop);
+    updater->Start();
+
+    // Tell the update that it's time to do update checks.
+    updater->TimerFired();
+
+    // Get the url our mock fetcher was asked to fetch.
+    TestURLFetcher* fetcher =
+        factory.GetFetcherByID(ExtensionUpdater::kManifestFetcherId);
+    const GURL& url = fetcher->original_url();
+    EXPECT_FALSE(url.is_empty());
+    EXPECT_TRUE(url.is_valid());
+    EXPECT_TRUE(url.SchemeIs("http"));
+    EXPECT_EQ("foo.com", url.host());
+    EXPECT_EQ("/bar", url.path());
+
+    // Validate the extension request parameters in the query. It should
+    // look something like "?x=id%3D<id>%26v%3D<version>%26uc".
+    EXPECT_TRUE(url.has_query());
+    std::vector<std::string> parts;
+    SplitString(url.query(), '=', &parts);
+    EXPECT_EQ(2u, parts.size());
+    EXPECT_EQ("x", parts[0]);
+    std::string decoded = UnescapeURLComponent(parts[1],
+                                               UnescapeRule::URL_SPECIAL_CHARS);
+    std::map<std::string, std::string> params;
+    ExtractParameters(decoded, &params);
+    EXPECT_EQ(tmp[0]->id(), params["id"]);
+    EXPECT_EQ(tmp[0]->VersionString(), params["v"]);
+    EXPECT_EQ("", params["uc"]);
+
+    STLDeleteElements(&tmp);
+  }
+
   static void TestDetermineUpdates() {
     // Create a set of test extensions
     ServiceForManifestTests service;
     ExtensionList tmp;
-    CreateTestExtensions(3, &tmp);
+    CreateTestExtensions(3, &tmp, NULL);
     service.set_extensions(tmp);
 
     MessageLoop message_loop;
@@ -423,6 +496,10 @@ TEST(ExtensionUpdaterTest, TestXmlParsing) {
   ExtensionUpdaterTest::TestXmlParsing();
 }
 
+TEST(ExtensionUpdaterTest, TestUpdateCheckRequests) {
+  ExtensionUpdaterTest::TestUpdateCheckRequests();
+}
+
 TEST(ExtensionUpdaterTest, TestDetermineUpdates) {
   ExtensionUpdaterTest::TestDetermineUpdates();
 }