Add google_update::GetUntrustedDataValueFromTag()

chrome/installer/util/google_update_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/installer/util/google_update_util.h"
 
 #include <algorithm>
 #include <map>
 #include <utility>
 #include <vector>
@@ skipping 114 matching lines @@
   return !(c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' ||
            c >= '0' && c <= '9' || c == '-' || c == '_' || c == '$');
 }
 
 // Returns true if |key| from untrusted data is valid.
 bool IsUntrustedDataKeyValid(const std::string& key) {
   return std::find_if(key.begin(), key.end(), IsIllegalUntrustedDataKeyChar)
       == key.end();
 }
 
-// Reads and parses untrusted data passed from Google Update as key-value
-// pairs, then overwrites |untrusted_data_map| with the result.
-// Returns true if data are successfully read.
-bool GetGoogleUpdateUntrustedData(
+bool ParseUntrustedData(

[grt (UTC plus 2), 2014/03/03 15:18:26]

add doc comment

[jackhou1, 2014/03/04 00:56:28]

Done.

+    const std::string& data_string,
     std::map<std::string, std::string>* untrusted_data) {
-  DCHECK(untrusted_data);
-  scoped_ptr<base::Environment> env(base::Environment::Create());
-  std::string data_string;
-  if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string))
-    return false;
-
-  if (data_string.length() > kEnvVariableUntrustedDataMaxLength ||
-      !IsStringPrintable(data_string)) {
-    LOG(ERROR) << "Invalid value in " << kEnvVariableUntrustedData;
+  if (!IsStringPrintable(data_string)) {
+    LOG(ERROR) << "Invalid value in untrusted data string.";
     return false;
   }
-
-  VLOG(1) << kEnvVariableUntrustedData << ": " << data_string;
-
   std::vector<std::pair<std::string, std::string> > kv_pairs;
   if (!base::SplitStringIntoKeyValuePairs(data_string, '=', '&', &kv_pairs)) {
     LOG(ERROR) << "Failed to parse untrusted data: " << data_string;
     return false;
   }
 
   untrusted_data->clear();
   std::vector<std::pair<std::string, std::string> >::const_iterator it;
   for (it = kv_pairs.begin(); it != kv_pairs.end(); ++it) {
     const std::string& key(it->first);
     // TODO(huangs): URL unescape |value|.
     const std::string& value(it->second);
     if (IsUntrustedDataKeyValid(key) && IsStringPrintable(value))
       (*untrusted_data)[key] = value;
     else
       LOG(ERROR) << "Illegal character found in untrusted data.";
   }
   return true;
 }
 
+// Reads and parses untrusted data passed from Google Update as key-value
+// pairs, then overwrites |untrusted_data_map| with the result.
+// Returns true if data are successfully read.
+bool GetGoogleUpdateUntrustedData(
+    std::map<std::string, std::string>* untrusted_data) {
+  DCHECK(untrusted_data);
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  std::string data_string;
+  if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string))

[grt (UTC plus 2), 2014/03/03 15:18:26]

i think "!env" is more idomatic for testing that a scoped ptr is NULL. this may
not have been the case when the code you've moved was written.

[jackhou1, 2014/03/04 00:56:28]

Done.

+    return false;
+
+  if (data_string.length() > kEnvVariableUntrustedDataMaxLength) {
+    LOG(ERROR) << kEnvVariableUntrustedData << " is too long.";
+    return false;
+  }
+
+  VLOG(1) << kEnvVariableUntrustedData << ": " << data_string;

[grt (UTC plus 2), 2014/03/03 15:18:26]

don't log data_string here since IsStringPrintable has yet to have been used to
validate it.

[jackhou1, 2014/03/04 00:56:28]

Done.

+
+  return ParseUntrustedData(data_string, untrusted_data);
+}
+
 }  // namespace
 
 bool EnsureUserLevelGoogleUpdatePresent() {
   VLOG(0) << "Ensuring Google Update is present at user-level.";
 
   bool success = false;
   if (IsGoogleUpdatePresent(false)) {
     success = true;
   } else {
     base::string16 cmd_string;
@@ skipping 30 matching lines @@
   if (GetGoogleUpdateUntrustedData(&untrusted_data)) {
     std::map<std::string, std::string>::const_iterator data_it(
         untrusted_data.find(key));
     if (data_it != untrusted_data.end())
       return data_it->second;
   }
 
   return std::string();
 }
 
+std::string GetUntrustedDataValueFromTag(const std::string& tag,

[grt (UTC plus 2), 2014/03/03 15:18:26]

should the size of |tag| be restricted in the same way as the env var
(kEnvVariableUntrustedDataMaxLength)? if no, why not?

[jackhou1, 2014/03/04 00:56:28]

Done.

I'm don't know if there is a specific reason the env var needs to be restricted
to that size. But it seems reasonable that any string read from elsewhere should
be restricted to a sane size.

+                                         const std::string& key) {
+  std::map<std::string, std::string> untrusted_data;
+  if (ParseUntrustedData(tag, &untrusted_data)) {

[grt (UTC plus 2), 2014/03/03 15:18:26]

without the logging calls (see next comment), i believe that:
  if (ParseUntrustedData(tag, &untrusted_data))
    return untrusted_data[key];
  return std::string();
will accomplish the same thing as lines 236-247.

[jackhou1, 2014/03/04 00:56:28]

Done.

+    std::map<std::string, std::string>::const_iterator data_it(
+        untrusted_data.find(key));
+    if (data_it != untrusted_data.end()) {
+      VLOG(1) << "Key " << key << " is " << data_it->second;

[grt (UTC plus 2), 2014/03/03 15:18:26]

coding style says "Remove most logging calls before checking in." Unless there's
a strong diagnostic reason for the log statements you've added to stay, please
remove them.

[jackhou1, 2014/03/04 00:56:28]

Done.

+      return data_it->second;
+    } else {
+      VLOG(1) << "Key not found: " << key;
+    }
+  }
+
+  return std::string();
+}
+
 }  // namespace google_update