Sync: Support omitting sensitive info in ConstructAboutInformation()

ConstructAboutInformation() gathers debugging information about the sync
state. It's employed both to power the sync-internals page - a debugging
page only accessible locally - as well as to compose logs sent to Google
in case of crashes (provided the user has agreed to this). For the
latter use case, the caller used to manually filter a section containing
PII, for privacy. Delegating this responsibility to the caller was bad
since the return value is quite "weakly typed" (base::DictionaryValue).
This meant that one might easily add new sensitive information to the
return value while forgetting to update the filter in the caller.

In this CL we transfer this responsibility to the method, by exposing
an include_sensitive_data parameter. Additionally, we now force the
constructor of a "section" to specify whether it is sensitive or not.
This last part makes the code more privacy-aware, which should make it
less likely to accidentally leak any data (even if this solution isn't
bullet-proof).

Bug: 973820
Change-Id: I99138a741c26755e138ac0a60dd08815aa90a10c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2467939
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816616}

chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc

@@ -393,35 +393,15 @@ void ChromeInternalLogSource::PopulateSyncLogs(SystemLogsResponse* response) {
   if (!profile || !ProfileSyncServiceFactory::HasSyncService(profile))
     return;
 
-  syncer::SyncService* service =
-      ProfileSyncServiceFactory::GetForProfile(profile);
-  std::unique_ptr<base::DictionaryValue> sync_logs(
-      syncer::sync_ui_util::ConstructAboutInformation(service,
-                                                      chrome::GetChannel()));
-
-  // Remove identity section.
-  base::ListValue* details = NULL;
-  sync_logs->GetList(syncer::sync_ui_util::kDetailsKey, &details);
-  if (!details)
-    return;
-  for (auto it = details->begin(); it != details->end(); ++it) {
-    base::DictionaryValue* dict = NULL;
-    if (it->GetAsDictionary(&dict)) {
-      std::string title;
-      dict->GetString("title", &title);
-      if (title == syncer::sync_ui_util::kIdentityTitle) {
-        details->Erase(it, NULL);
-        break;
-      }
-    }
-  }
-
-  // Add sync logs to logs.
-  std::string sync_logs_string;
-  JSONStringValueSerializer serializer(&sync_logs_string);
-  serializer.Serialize(*sync_logs.get());
-
-  response->emplace(kSyncDataKey, sync_logs_string);
+  // Add sync logs to |response|.
+  std::unique_ptr<base::DictionaryValue> sync_logs =
+      syncer::sync_ui_util::ConstructAboutInformation(
+          syncer::sync_ui_util::IncludeSensitiveData(false),
+          ProfileSyncServiceFactory::GetForProfile(profile),
+          chrome::GetChannel());
+  std::string serialized_sync_logs;
+  JSONStringValueSerializer(&serialized_sync_logs).Serialize(*sync_logs);
+  response->emplace(kSyncDataKey, serialized_sync_logs);
 }
 
 void ChromeInternalLogSource::PopulateExtensionInfoLogs(

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -606,9 +606,12 @@ SyncCycleSnapshot ProfileSyncServiceHarness::GetLastCycleSnapshot() const {
 }
 
 std::string ProfileSyncServiceHarness::GetServiceStatus() {
+  // This method is only used in test code for debugging purposes, so it's fine
+  // to include sensitive data in ConstructAboutInformation().
   std::unique_ptr<base::DictionaryValue> value(
-      syncer::sync_ui_util::ConstructAboutInformation(service(),
-                                                      chrome::GetChannel()));
+      syncer::sync_ui_util::ConstructAboutInformation(
+          syncer::sync_ui_util::IncludeSensitiveData(true), service(),
+          chrome::GetChannel()));
   std::string service_status;
   base::JSONWriter::WriteWithOptions(
       *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);

chrome/browser/ui/webui/sync_internals_message_handler.cc

@@ -65,7 +65,11 @@ bool GetIncludeSpecificsInitialState() {
 
 SyncInternalsMessageHandler::SyncInternalsMessageHandler()
     : SyncInternalsMessageHandler(base::BindRepeating(
-          &syncer::sync_ui_util::ConstructAboutInformation)) {}
+          &syncer::sync_ui_util::ConstructAboutInformation,
+          syncer::sync_ui_util::IncludeSensitiveData(true))) {
+  // This class serves to display debug information to the user, so it's fine to
+  // include sensitive data in ConstructAboutInformation() above.
+}
 
 SyncInternalsMessageHandler::SyncInternalsMessageHandler(
     AboutSyncDataDelegate about_sync_data_delegate)

components/sync/driver/about_sync_util.cc

@@ -120,9 +120,8 @@ class Stat : public StatBase {
 // fields.
 class Section {
  public:
-  explicit Section(const std::string& title) : title_(title) {}
-
-  void MarkSensitive() { is_sensitive_ = true; }
+  Section(const std::string& title, bool is_sensitive)
+      : title_(title), is_sensitive_(is_sensitive) {}
 
   Stat<bool>* AddBoolStat(const std::string& key) {
     return AddStat(key, false);
@@ -143,6 +142,8 @@ class Section {
     return result;
   }
 
+  bool is_sensitive() { return is_sensitive_; }
+
  private:
   template <typename T>
   Stat<T>* AddStat(const std::string& key, const T& default_value) {
@@ -161,15 +162,22 @@ class SectionList {
  public:
   SectionList() = default;
 
-  Section* AddSection(const std::string& title) {
-    sections_.push_back(std::make_unique<Section>(title));
+  // WARNING: If this section includes any Personally Identifiable Information,
+  // |is_sensitive| should be set to true.
+  Section* AddSection(const std::string& title, bool is_sensitive) {
+    sections_.push_back(std::make_unique<Section>(title, is_sensitive));
     return sections_.back().get();
   }
 
-  base::Value ToValue() const {
+  // If |include_sensitive_data| is true, returns all added sections. Otherwise,
+  // omits those added with |is_sensitive| set to true.
+  base::Value ToValue(IncludeSensitiveData include_sensitive_data) const {
     base::Value result(base::Value::Type::LIST);
-    for (const std::unique_ptr<Section>& section : sections_)
-      result.Append(section->ToValue());
+    for (const std::unique_ptr<Section>& section : sections_) {
+      if (include_sensitive_data || !section->is_sensitive()) {
+        result.Append(section->ToValue());
+      }
+    }
     return result;
   }
 
@@ -297,13 +305,15 @@ std::string GetConnectionStatus(const SyncTokenStatus& status) {
 // which are grouped into sections and populated with the help of the SyncStat
 // classes defined above.
 std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
+    IncludeSensitiveData include_sensitive_data,
     SyncService* service,
     version_info::Channel channel) {
   auto about_info = std::make_unique<base::DictionaryValue>();
 
   SectionList section_list;
 
-  Section* section_summary = section_list.AddSection("Summary");
+  Section* section_summary =
+      section_list.AddSection("Summary", /*is_sensitive=*/false);
   Stat<std::string>* transport_state =
       section_summary->AddStringStat("Transport State");
   Stat<std::string>* disable_reasons =
@@ -318,13 +328,14 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
       section_summary->AddBoolStat("Setup In Progress");
   Stat<std::string>* auth_error = section_summary->AddStringStat("Auth Error");
 
-  Section* section_version = section_list.AddSection("Version Info");
+  Section* section_version =
+      section_list.AddSection("Version Info", /*is_sensitive=*/false);
   Stat<std::string>* client_version =
       section_version->AddStringStat("Client Version");
   Stat<std::string>* server_url = section_version->AddStringStat("Server URL");
 
-  Section* section_identity = section_list.AddSection(kIdentityTitle);
-  section_identity->MarkSensitive();
+  Section* section_identity =
+      section_list.AddSection(kIdentityTitle, /*is_sensitive=*/true);
   Stat<std::string>* sync_client_id =
       section_identity->AddStringStat("Sync Client ID");
   Stat<std::string>* invalidator_id =
@@ -332,7 +343,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* username = section_identity->AddStringStat("Username");
   Stat<bool>* user_is_primary = section_identity->AddBoolStat("Is Primary");
 
-  Section* section_credentials = section_list.AddSection("Credentials");
+  Section* section_credentials =
+      section_list.AddSection("Credentials", /*is_sensitive=*/false);
   Stat<std::string>* token_request_time =
       section_credentials->AddStringStat("Requested Token");
   Stat<std::string>* token_response_time =
@@ -343,7 +355,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* next_token_request =
       section_credentials->AddStringStat("Next Token Request");
 
-  Section* section_local = section_list.AddSection("Local State");
+  Section* section_local =
+      section_list.AddSection("Local State", /*is_sensitive=*/false);
   Stat<std::string>* server_connection =
       section_local->AddStringStat("Server Connection");
   Stat<std::string>* last_synced = section_local->AddStringStat("Last Synced");
@@ -355,14 +368,16 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* local_backend_path =
       section_local->AddStringStat("Local Backend Path");
 
-  Section* section_network = section_list.AddSection("Network");
+  Section* section_network =
+      section_list.AddSection("Network", /*is_sensitive=*/false);
   Stat<bool>* is_any_throttled_or_backoff =
       section_network->AddBoolStat("Throttled or Backoff");
   Stat<std::string>* retry_time = section_network->AddStringStat("Retry Time");
   Stat<bool>* are_notifications_enabled =
       section_network->AddBoolStat("Notifications Enabled");
 
-  Section* section_encryption = section_list.AddSection("Encryption");
+  Section* section_encryption =
+      section_list.AddSection("Encryption", /*is_sensitive=*/false);
   Stat<bool>* is_using_explicit_passphrase =
       section_encryption->AddBoolStat("Explicit Passphrase");
   Stat<bool>* is_passphrase_required =
@@ -382,8 +397,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* passphrase_time =
       section_encryption->AddStringStat("Passphrase Time");
 
-  Section* section_last_session =
-      section_list.AddSection("Status from Last Completed Session");
+  Section* section_last_session = section_list.AddSection(
+      "Status from Last Completed Session", /*is_sensitive=*/false);
   Stat<std::string>* session_source =
       section_last_session->AddStringStat("Sync Source");
   Stat<std::string>* get_key_result =
@@ -393,7 +408,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<std::string>* commit_result =
       section_last_session->AddStringStat("Commit Step Result");
 
-  Section* section_counters = section_list.AddSection("Running Totals");
+  Section* section_counters =
+      section_list.AddSection("Running Totals", /*is_sensitive=*/false);
   Stat<int>* notifications_received =
       section_counters->AddIntStat("Notifications Received");
   Stat<int>* updates_received =
@@ -409,8 +425,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
   Stat<int>* conflicts_resolved_server_wins =
       section_counters->AddIntStat("Conflicts Resolved: Server Wins");
 
-  Section* section_this_cycle =
-      section_list.AddSection("Transient Counters (this cycle)");
+  Section* section_this_cycle = section_list.AddSection(
+      "Transient Counters (this cycle)", /*is_sensitive=*/false);
   Stat<int>* encryption_conflicts =
       section_this_cycle->AddIntStat("Encryption Conflicts");
   Stat<int>* hierarchy_conflicts =
@@ -421,7 +437,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
       section_this_cycle->AddIntStat("Committed Items");
 
   Section* section_that_cycle = section_list.AddSection(
-      "Transient Counters (last cycle of last completed session)");
+      "Transient Counters (last cycle of last completed session)",
+      /*is_sensitive=*/false);
   Stat<int>* updates_downloaded =
       section_that_cycle->AddIntStat("Updates Downloaded");
   Stat<int>* committed_count =
@@ -433,7 +450,8 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
 
   if (!service) {
     transport_state->Set("Sync service does not exist");
-    about_info->SetKey(kDetailsKey, section_list.ToValue());
+    about_info->SetKey(kDetailsKey,
+                       section_list.ToValue(include_sensitive_data));
     return about_info;
   }
 
@@ -571,7 +589,7 @@ std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
 
   // This list of sections belongs in the 'details' field of the returned
   // message.
-  about_info->SetKey(kDetailsKey, section_list.ToValue());
+  about_info->SetKey(kDetailsKey, section_list.ToValue(include_sensitive_data));
 
   // The values set from this point onwards do not belong in the
   // details list.

components/sync/driver/about_sync_util.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/util/type_safety/strong_alias.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/version_info/version_info.h"
 
@@ -65,10 +66,16 @@ extern const char kOnProtocolEvent[];
 extern const char kOnReceivedIncludeSpecificsInitialState[];
 extern const char kOnReceivedListOfTypes[];
 extern const char kTypes[];
+
+using IncludeSensitiveData =
+    util::StrongAlias<class IncludeSensitiveDataTag, bool>;
 // This function returns a DictionaryValue which contains all the information
 // required to populate the 'About' tab of about:sync.
 // Note that |service| may be null.
+// If |include_sensitive_data| is false, Personally Identifiable Information
+// won't be included in the return value.
 std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
+    IncludeSensitiveData include_sensitive_data,
     SyncService* service,
     version_info::Channel channel);
 

components/sync/driver/about_sync_util_unittest.cc

@@ -18,8 +18,8 @@ TEST(SyncUIUtilTestAbout, ConstructAboutInformationWithUnrecoverableErrorTest) {
   service.SetDisableReasons(
       syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR);
 
-  std::unique_ptr<base::DictionaryValue> strings(
-      ConstructAboutInformation(&service, version_info::Channel::UNKNOWN));
+  std::unique_ptr<base::DictionaryValue> strings(ConstructAboutInformation(
+      IncludeSensitiveData(true), &service, version_info::Channel::UNKNOWN));
 
   EXPECT_TRUE(strings->HasKey("unrecoverable_error_detected"));
 }

ios/components/webui/sync_internals/sync_internals_message_handler.mm

@@ -267,10 +267,12 @@ void SyncInternalsMessageHandler::HandleJsEvent(
 }
 
 void SyncInternalsMessageHandler::SendAboutInfo() {
-  syncer::SyncService* sync_service = GetSyncService();
+  // This class serves to display debug information to the user, so it's fine to
+  // include sensitive data in ConstructAboutInformation().
   std::unique_ptr<base::DictionaryValue> value =
-      syncer::sync_ui_util::ConstructAboutInformation(sync_service,
-                                                      web_ui::GetChannel());
+      syncer::sync_ui_util::ConstructAboutInformation(
+          syncer::sync_ui_util::IncludeSensitiveData(true), GetSyncService(),
+          web_ui::GetChannel());
   DispatchEvent(syncer::sync_ui_util::kOnAboutInfoUpdated, *value);
 }