Password manager internals page: Improve security

components/autofill/core/common/save_password_progress_logger.cc

Index: components/autofill/core/common/save_password_progress_logger.cc
diff --git a/components/autofill/core/common/save_password_progress_logger.cc b/components/autofill/core/common/save_password_progress_logger.cc
index cdb8dd7a177e97f1542db30a714749683b3094f1..5f051ccf96dd8b821064d72f05e133e5aab1073b 100644
--- a/components/autofill/core/common/save_password_progress_logger.cc
+++ b/components/autofill/core/common/save_password_progress_logger.cc
@@ -4,9 +4,14 @@
 
 #include "components/autofill/core/common/save_password_progress_logger.h"
 
+#include <algorithm>
+
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/autofill/core/common/password_form.h"
 
@@ -20,6 +25,76 @@ namespace autofill {
 
 namespace {
 
+// Note 1: Caching the ID->string map in an array would be probably faster, but
+// the switch statement is (a) robust against re-ordering, and (b) checks in
+// compile-time, that all IDs get a string assigned. The expected frequency of
+// calls is low enough (in particular, zero if password manager internals page
+// is not open), that optimizing for code robustness is preferred against speed.
+// Note 2: The messages can be used as dictionary keys. Do not use '.' in them.
+std::string GetStringFromID(SavePasswordProgressLogger::StringID id) {
+  switch (id) {
+    case SavePasswordProgressLogger::STRING_DECISION_ASK:
+      return "Decision: ASK the user";
+    case SavePasswordProgressLogger::STRING_DECISION_DROP:
+      return "Decision: DROP the password";
+    case SavePasswordProgressLogger::STRING_DECISION_SAVE:
+      return "Decision: SAVE the password";
+    case SavePasswordProgressLogger::STRING_METHOD:
+      return "Form method";
+    case SavePasswordProgressLogger::STRING_METHOD_GET:
+      return "GET";
+    case SavePasswordProgressLogger::STRING_METHOD_POST:
+      return "POST";
+    case SavePasswordProgressLogger::STRING_METHOD_EMPTY:
+      return "(empty)";
+    case SavePasswordProgressLogger::STRING_OTHER:
+      return "(other)";
+    case SavePasswordProgressLogger::STRING_SCHEME_HTML:
+      return "HTML";
+    case SavePasswordProgressLogger::STRING_SCHEME_BASIC:
+      return "Basic";
+    case SavePasswordProgressLogger::STRING_SCHEME_DIGEST:
+      return "Digest";
+    case SavePasswordProgressLogger::STRING_SCHEME_MESSAGE:
+      return "Scheme";
+    case SavePasswordProgressLogger::STRING_SIGNON_REALM:
+      return "Signon realm";
+    case SavePasswordProgressLogger::STRING_ORIGINAL_SIGNON_REALM:
+      return "Original signon realm";
+    case SavePasswordProgressLogger::STRING_ORIGIN:
+      return "Origin";
+    case SavePasswordProgressLogger::STRING_ACTION:
+      return "Action";
+    case SavePasswordProgressLogger::STRING_USERNAME_ELEMENT:
+      return "Username element";
+    case SavePasswordProgressLogger::STRING_PASSWORD_ELEMENT:
+      return "Password element";
+    case SavePasswordProgressLogger::STRING_PASSWORD_AUTOCOMPLETE_SET:
+      return "Password autocomplete set";
+    case SavePasswordProgressLogger::STRING_OLD_PASSWORD_ELEMENT:
+      return "Old password element";
+    case SavePasswordProgressLogger::STRING_SSL_VALID:
+      return "SSL valid";
+    case SavePasswordProgressLogger::STRING_PASSWORD_GENERATED:
+      return "Password generated";
+    case SavePasswordProgressLogger::STRING_TIMES_USED:
+      return "Times used";
+    case SavePasswordProgressLogger::STRING_USE_ADDITIONAL_AUTHENTICATION:
+      return "Use additional authentication";
+    case SavePasswordProgressLogger::STRING_PSL_MATCH:
+      return "PSL match";
+    case SavePasswordProgressLogger::STRING_NAME_OR_ID:
+      return "Form name or ID";
+    case SavePasswordProgressLogger::STRING_MESSAGE:
+      return "Message";
+    case SavePasswordProgressLogger::STRING_INVALID:
+      return "INVALID";
+      // Intentionally no default: clause here -- all IDs need to get covered.
+  }
+  NOTREACHED();  // Win compilers don't believe this is unreachable.
+  return std::string();
+};
+
 // Removes privacy sensitive parts of |url| (currently all but host and scheme).
 std::string ScrubURL(const GURL& url) {
   if (url.is_valid())
@@ -27,112 +102,150 @@ std::string ScrubURL(const GURL& url) {
   return std::string();
 }
 
+// Returns true for all characters which we don't want to see in the logged IDs
+// or names of HTML elements.
+bool IsUnwantedInElementID(char c) {
+  return !(c == '_' || c == '-' || IsAsciiAlpha(c) || IsAsciiDigit(c));
+}
+
+// Replaces all characters satisfying IsUnwantedInElementID by a ' ', and turns
+// all characters to lowercase. This damages some valid HTML element IDs or
+// names, but it is likely that it will be still possible to match the scrubbed
+// string to the original ID or name in the HTML doc. That's good enough for the
+// logging purposes, and provides some security benefits.
+std::string ScrubElementID(std::string element_id) {
+  std::replace_if(
+      element_id.begin(), element_id.end(), IsUnwantedInElementID, ' ');
+  return StringToLowerASCII(element_id);
+}
+
+std::string ScrubElementID(const base::string16& element_id) {
+  return ScrubElementID(base::UTF16ToUTF8(element_id));
+}
+
 std::string FormSchemeToString(PasswordForm::Scheme scheme) {
+  SavePasswordProgressLogger::StringID result_id =
+      SavePasswordProgressLogger::STRING_INVALID;
   switch (scheme) {
     case PasswordForm::SCHEME_HTML:
-      return "HTML";
+      result_id = SavePasswordProgressLogger::STRING_SCHEME_HTML;
+      break;
     case PasswordForm::SCHEME_BASIC:
-      return "BASIC";
+      result_id = SavePasswordProgressLogger::STRING_SCHEME_BASIC;
+      break;
     case PasswordForm::SCHEME_DIGEST:
-      return "DIGEST";
+      result_id = SavePasswordProgressLogger::STRING_SCHEME_DIGEST;
+      break;
     case PasswordForm::SCHEME_OTHER:
-      return "OTHER";
+      result_id = SavePasswordProgressLogger::STRING_OTHER;
+      break;
   }
-  NOTREACHED();  // Win compilers don't believe this is unreachable.
-  return std::string();
+  return GetStringFromID(result_id);
 }
 
-StringValue DecisionToStringValue(
-    SavePasswordProgressLogger::Decision decision) {
-  switch (decision) {
-    case SavePasswordProgressLogger::DECISION_SAVE:
-      return StringValue("SAVE the password");
-    case SavePasswordProgressLogger::DECISION_ASK:
-      return StringValue("ASK the user whether to save the password");
-    case SavePasswordProgressLogger::DECISION_DROP:
-      return StringValue("DROP the password");
-  }
-  NOTREACHED();  // Win compilers don't believe this is unreachable.
-  return StringValue(std::string());
+std::string FormMethodToString(const std::string& method) {
+  std::string method_processed;
+  base::TrimWhitespaceASCII(
+      StringToLowerASCII(method), base::TRIM_ALL, &method_processed);
+  SavePasswordProgressLogger::StringID result_id =
+      SavePasswordProgressLogger::STRING_OTHER;
+  if (method_processed.empty())
+    result_id = SavePasswordProgressLogger::STRING_METHOD_EMPTY;
+  else if (method_processed == "get")
+    result_id = SavePasswordProgressLogger::STRING_METHOD_GET;
+  else if (method_processed == "post")
+    result_id = SavePasswordProgressLogger::STRING_METHOD_POST;
+  return GetStringFromID(result_id);
 }
 
 }  // namespace
 
-SavePasswordProgressLogger::SavePasswordProgressLogger() {}
+SavePasswordProgressLogger::SavePasswordProgressLogger() {
+}
 
-SavePasswordProgressLogger::~SavePasswordProgressLogger() {}
+SavePasswordProgressLogger::~SavePasswordProgressLogger() {
+}
 
-void SavePasswordProgressLogger::LogPasswordForm(const std::string& message,
-                                                 const PasswordForm& form) {
+void SavePasswordProgressLogger::LogPasswordForm(
+    SavePasswordProgressLogger::StringID label,
+    const PasswordForm& form) {
   DictionaryValue log;
-  // Do not use the "<<" operator for PasswordForms, because it also prints
-  // passwords. Also, that operator is only for testing.
-  log.SetString("scheme", FormSchemeToString(form.scheme));
-  log.SetString("signon realm", ScrubURL(GURL(form.signon_realm)));
-  log.SetString("original signon realm",
+  log.SetString(GetStringFromID(STRING_SCHEME_MESSAGE),
+                FormSchemeToString(form.scheme));
+  log.SetString(GetStringFromID(STRING_SCHEME_MESSAGE),
+                FormSchemeToString(form.scheme));
+  log.SetString(GetStringFromID(STRING_SIGNON_REALM),
+                ScrubURL(GURL(form.signon_realm)));
+  log.SetString(GetStringFromID(STRING_ORIGINAL_SIGNON_REALM),
                 ScrubURL(GURL(form.original_signon_realm)));
-  log.SetString("origin", ScrubURL(form.origin));
-  log.SetString("action", ScrubURL(form.action));
-  log.SetString("username element", form.username_element);
-  log.SetString("password element", form.password_element);
-  log.SetBoolean("password autocomplete set", form.password_autocomplete_set);
-  log.SetString("old password element", form.old_password_element);
-  log.SetBoolean("ssl valid", form.ssl_valid);
-  log.SetBoolean("password generated",
+  log.SetString(GetStringFromID(STRING_ORIGIN), ScrubURL(form.origin));
+  log.SetString(GetStringFromID(STRING_ACTION), ScrubURL(form.action));
+  log.SetString(GetStringFromID(STRING_USERNAME_ELEMENT),
+                ScrubElementID(form.username_element));
+  log.SetString(GetStringFromID(STRING_PASSWORD_ELEMENT),
+                ScrubElementID(form.password_element));
+  log.SetBoolean(GetStringFromID(STRING_PASSWORD_AUTOCOMPLETE_SET),
+                 form.password_autocomplete_set);
+  log.SetString(GetStringFromID(STRING_OLD_PASSWORD_ELEMENT),
+                ScrubElementID(form.old_password_element));
+  log.SetBoolean(GetStringFromID(STRING_SSL_VALID), form.ssl_valid);
+  log.SetBoolean(GetStringFromID(STRING_PASSWORD_GENERATED),
                  form.type == PasswordForm::TYPE_GENERATED);
-  log.SetInteger("times used", form.times_used);
-  log.SetBoolean("use additional authentication",
+  log.SetInteger(GetStringFromID(STRING_TIMES_USED), form.times_used);
+  log.SetBoolean(GetStringFromID(STRING_USE_ADDITIONAL_AUTHENTICATION),
                  form.use_additional_authentication);
-  log.SetBoolean("is PSL match", form.IsPublicSuffixMatch());
-  LogValue(message, log);
+  log.SetBoolean(GetStringFromID(STRING_PSL_MATCH), form.IsPublicSuffixMatch());
+  LogValue(label, log);
 }
 
-void SavePasswordProgressLogger::LogHTMLForm(const std::string& message,
-                                             const std::string& name_or_id,
-                                             const std::string& method,
-                                             const GURL& action) {
+void SavePasswordProgressLogger::LogHTMLForm(
+    SavePasswordProgressLogger::StringID label,
+    const std::string& name_or_id,
+    const std::string& method,
+    const GURL& action) {
   DictionaryValue log;
-  log.SetString("name_or_id", name_or_id);
-  log.SetString("method", method);
-  log.SetString("action", ScrubURL(action));
-  LogValue(message, log);
-}
-
-void SavePasswordProgressLogger::LogURL(const std::string& message,
-                                        const GURL& url) {
-  LogValue(message, StringValue(ScrubURL(url)));
+  log.SetString(GetStringFromID(STRING_NAME_OR_ID), ScrubElementID(name_or_id));
+  log.SetString(GetStringFromID(STRING_METHOD), FormMethodToString(method));
+  log.SetString(GetStringFromID(STRING_ACTION), ScrubURL(action));
+  LogValue(label, log);
 }
 
-void SavePasswordProgressLogger::LogBoolean(const std::string& message,
-                                            bool value) {
-  LogValue(message, FundamentalValue(value));
+void SavePasswordProgressLogger::LogURL(
+    SavePasswordProgressLogger::StringID label,
+    const GURL& url) {
+  LogValue(label, StringValue(ScrubURL(url)));
 }
 
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           int value) {
-  LogValue(message, FundamentalValue(value));
+void SavePasswordProgressLogger::LogBoolean(
+    SavePasswordProgressLogger::StringID label,
+    bool truth_value) {
+  LogValue(label, FundamentalValue(truth_value));
 }
 
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           size_t value) {
-  LogValue(message, FundamentalValue(checked_cast<int, size_t>(value)));
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    int signed_number) {
+  LogValue(label, FundamentalValue(signed_number));
 }
 
-void SavePasswordProgressLogger::LogFinalDecision(Decision decision) {
-  LogValue("Final decision taken", DecisionToStringValue(decision));
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    size_t unsigned_number) {
+  int signed_number = checked_cast<int, size_t>(unsigned_number);
+  LogNumber(label, signed_number);
 }
 
-void SavePasswordProgressLogger::LogMessage(const std::string& message) {
-  LogValue("Message", StringValue(message));
+void SavePasswordProgressLogger::LogMessage(
+    SavePasswordProgressLogger::StringID message) {
+  LogValue(STRING_MESSAGE, StringValue(GetStringFromID(message)));
 }
 
-void SavePasswordProgressLogger::LogValue(const std::string& name,
-                                          const Value& log) {
+void SavePasswordProgressLogger::LogValue(StringID label, const Value& log) {
   std::string log_string;
   bool conversion_to_string_successful = base::JSONWriter::WriteWithOptions(
       &log, base::JSONWriter::OPTIONS_PRETTY_PRINT, &log_string);
   DCHECK(conversion_to_string_successful);
-  SendLog(name + ": " + log_string);
+  SendLog(GetStringFromID(label) + ": " + log_string);
 }
 
 }  // namespace autofill