Due to privacy concerns about the data contained in API call arguments, we no longer will log the arguments unless:

(1) The API call has been whitelisted because we need the arguments and there is no explicit privacy issue w/r/t the arguments of that API call, or

(2) The browser's activity log testing flag has been set.

I set up an API_action whitelist with a few preliminary ones. We'll probably need to expand it and add similar whitelists for events.

For now, this does not affect DOM API calls because we will be taking a whitelisting approach to DOM API calls to begin with.

Review URL: https://chromiumcodereview.appspot.com/12918020

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

7 files changed, 114 insertions, 4 deletions

diff --git a/chrome/browser/extensions/activity_log.cc b/chrome/browser/extensions/activity_log.cc
index 1814ef5..4a482701 100644
--- a/chrome/browser/extensions/activity_log.cc
+++ b/chrome/browser/extensions/activity_log.cc
@@ -148,6 +148,17 @@ ActivityLog::ActivityLog(Profile* profile) {
   log_activity_to_ui_ = CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kEnableExtensionActivityUI);
 
+  // enable-extension-activity-log-testing
+  // Currently, this just controls whether arguments are collected. In the
+  // future, it may also control other optional activity log features.
+  log_arguments_ = CommandLine::ForCurrentProcess()->
+      HasSwitch(switches::kEnableExtensionActivityLogTesting);
+  if (!log_arguments_) {
+    for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) {
+      arg_whitelist_api_.insert(std::string(APIAction::kAlwaysLog[i]));
+    }
+  }
+
   // If the database cannot be initialized for some reason, we keep
   // chugging along but nothing will get recorded. If the UI is
   // available, things will still get sent to the UI even if nothing
@@ -233,7 +244,13 @@ void ActivityLog::LogAPIAction(const Extension* extension,
                                const ListValue* args,
                                const std::string& extra) {
   if (!IsLogEnabled()) return;
-  LogAPIActionInternal(extension, api_call, args, extra, APIAction::CALL);
+  bool log_args = log_arguments_ ||
+      arg_whitelist_api_.find(api_call) != arg_whitelist_api_.end();
+  LogAPIActionInternal(extension,
+                       api_call,
+                       log_args ? args : new ListValue(),
+                       extra,
+                       APIAction::CALL);
 }
 
 // A wrapper around LogAPIActionInternal, but we know it's actually an event
@@ -247,7 +264,7 @@ void ActivityLog::LogEventAction(const Extension* extension,
   if (!IsLogEnabled()) return;
   LogAPIActionInternal(extension,
                        api_call,
-                       args,
+                       log_arguments_ ? args : new ListValue(),
                        extra,
                        APIAction::EVENT_CALLBACK);
 }
@@ -258,17 +275,22 @@ void ActivityLog::LogBlockedAction(const Extension* extension,
                                    const char* reason,
                                    const std::string& extra) {
   if (!IsLogEnabled()) return;
+  bool log_args = log_arguments_ ||
+      arg_whitelist_api_.find(blocked_call) != arg_whitelist_api_.end();
+  std::string altered_args =
+      log_args ? MakeArgList(args) : MakeArgList(new ListValue());
   scoped_refptr<BlockedAction> action = new BlockedAction(extension->id(),
                                                           base::Time::Now(),
                                                           blocked_call,
-                                                          MakeArgList(args),
+                                                          altered_args,
                                                           std::string(reason),
                                                           extra);
   ScheduleAndForget(&ActivityDatabase::RecordAction, action);
   // Display the action.
   ObserverMap::const_iterator iter = observers_.find(extension);
   if (iter != observers_.end()) {
-    std::string blocked_str = MakeCallSignature(blocked_call, args);
+    std::string blocked_str = MakeCallSignature(blocked_call,
+        log_args ? args : new ListValue());
     iter->second->Notify(&Observer::OnExtensionActivity,
                          extension,
                          ActivityLog::ACTIVITY_EXTENSION_API_BLOCK,
diff --git a/chrome/browser/extensions/activity_log.h b/chrome/browser/extensions/activity_log.h
index 2c01cd5..5fb64bc 100644
--- a/chrome/browser/extensions/activity_log.h
+++ b/chrome/browser/extensions/activity_log.h
@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/hash_tables.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list_threadsafe.h"
 #include "base/synchronization/lock.h"
@@ -199,6 +200,13 @@ class ActivityLog : public ProfileKeyedService,
   bool log_activity_to_stdout_;
   bool log_activity_to_ui_;
 
+  // log_arguments controls whether to log API call arguments. By default, we
+  // don't log most arguments to avoid saving too much data. In testing mode,
+  // argument collection is enabled. We also whitelist some arguments for
+  // collection regardless of whether this bool is true.
+  bool log_arguments_;
+  base::hash_set<std::string> arg_whitelist_api_;
+
   DISALLOW_COPY_AND_ASSIGN(ActivityLog);
 };
 
diff --git a/chrome/browser/extensions/activity_log_unittest.cc b/chrome/browser/extensions/activity_log_unittest.cc
index 42369b5..f678d7b 100644
--- a/chrome/browser/extensions/activity_log_unittest.cc
+++ b/chrome/browser/extensions/activity_log_unittest.cc
@@ -50,6 +50,23 @@ class ActivityLogTest : public ChromeRenderViewHostTestHarness {
     ASSERT_EQ(2, static_cast<int>(i->size()));
   }
 
+  static void Arguments_Missing(
+      scoped_ptr<std::vector<scoped_refptr<Action> > > i) {
+    scoped_refptr<Action> last = i->front();
+    std::string noargs = "ID: odlameecjipmbmbejkplpemijjgpljce, CATEGORY: "
+      "CALL, VERB: UNKNOWN_VERB, TARGET: TABS, API: tabs.testMethod, ARGS: ";
+    ASSERT_EQ(noargs, last->PrettyPrintForDebug());
+  }
+
+  static void Arguments_Present(
+      scoped_ptr<std::vector<scoped_refptr<Action> > > i) {
+    scoped_refptr<Action> last = i->front();
+    std::string args = "ID: odlameecjipmbmbejkplpemijjgpljce, CATEGORY: "
+      "CALL, VERB: UNKNOWN_VERB, TARGET: UNKNOWN_TARGET, API: "
+      "extension.connect, ARGS: \"hello\", \"world\"";
+    ASSERT_EQ(args, last->PrettyPrintForDebug());
+  }
+
  protected:
   ExtensionService* extension_service_;
   Profile* profile_;
@@ -112,5 +129,55 @@ TEST_F(ActivityLogTest, LogAndFetchActions) {
       base::Bind(ActivityLogTest::RetrieveActions_LogAndFetchActions));
 }
 
+TEST_F(ActivityLogTest, LogWithoutArguments) {
+  ActivityLog* activity_log = ActivityLog::GetInstance(profile_);
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder()
+          .SetManifest(DictionaryBuilder()
+                       .Set("name", "Test extension")
+                       .Set("version", "1.0.0")
+                       .Set("manifest_version", 2))
+          .Build();
+  extension_service_->AddExtension(extension);
+  ASSERT_TRUE(ActivityLog::IsLogEnabled());
+
+  scoped_ptr<ListValue> args(new ListValue());
+  args->Set(0, new base::StringValue("hello"));
+  args->Set(1, new base::StringValue("world"));
+  activity_log->LogAPIAction(extension,
+                             std::string("tabs.testMethod"),
+                             args.get(),
+                             "");
+  activity_log->GetActions(
+      extension->id(),
+      0,
+      base::Bind(ActivityLogTest::Arguments_Missing));
+}
+
+TEST_F(ActivityLogTest, LogWithArguments) {
+  ActivityLog* activity_log = ActivityLog::GetInstance(profile_);
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder()
+          .SetManifest(DictionaryBuilder()
+                       .Set("name", "Test extension")
+                       .Set("version", "1.0.0")
+                       .Set("manifest_version", 2))
+          .Build();
+  extension_service_->AddExtension(extension);
+  ASSERT_TRUE(ActivityLog::IsLogEnabled());
+
+  scoped_ptr<ListValue> args(new ListValue());
+  args->Set(0, new base::StringValue("hello"));
+  args->Set(1, new base::StringValue("world"));
+  activity_log->LogAPIAction(extension,
+                            std::string("extension.connect"),
+                            args.get(),
+                            "");
+  activity_log->GetActions(
+      extension->id(),
+      0,
+      base::Bind(ActivityLogTest::Arguments_Present));
+}
+
 }  // namespace extensions
 
diff --git a/chrome/browser/extensions/api_actions.cc b/chrome/browser/extensions/api_actions.cc
index 2a779bf..80437af 100644
--- a/chrome/browser/extensions/api_actions.cc
+++ b/chrome/browser/extensions/api_actions.cc
@@ -15,6 +15,13 @@ const char* APIAction::kTableName = "activitylog_apis";
 const char* APIAction::kTableContentFields[] =
     {"api_type", "api_action_type", "target_type", "api_call", "args", "extra"};
 
+// We should log the arguments to these API calls, even if argument logging is
+// disabled by default.
+const char* APIAction::kAlwaysLog[] =
+    {"extension.connect", "extension.sendMessage",
+     "tabs.executeScript", "tabs.insertCSS" };
+const int APIAction::kSizeAlwaysLog = arraysize(kAlwaysLog);
+
 APIAction::APIAction(const std::string& extension_id,
                      const base::Time& time,
                      const Type type,
diff --git a/chrome/browser/extensions/api_actions.h b/chrome/browser/extensions/api_actions.h
index 6613f6c..d3de3e4 100644
--- a/chrome/browser/extensions/api_actions.h
+++ b/chrome/browser/extensions/api_actions.h
@@ -45,6 +45,8 @@ class APIAction : public Action {
 
   static const char* kTableName;
   static const char* kTableContentFields[];
+  static const char* kAlwaysLog[];
+  static const int kSizeAlwaysLog;
 
   // Create the database table for storing APIActions, or update the schema if
   // it is out of date.  Any existing data is preserved.
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 21cb778..0016cc0 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -487,6 +487,9 @@ const char kEnableExperimentalExtensionApis[] =
 const char kEnableExtensionActivityLogging[] =
     "enable-extension-activity-logging";
 
+const char kEnableExtensionActivityLogTesting[] =
+    "enable-extension-activity-log-testing";
+
 // Enables the extension activity UI.
 const char kEnableExtensionActivityUI[]     = "enable-extension-activity-ui";
 
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 016f50a..17132b4 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -141,6 +141,7 @@ extern const char kEnableDesktopGuestMode[];
 extern const char kEnableDevToolsExperiments[];
 extern const char kEnableExperimentalExtensionApis[];
 extern const char kEnableExtensionActivityLogging[];
+extern const char kEnableExtensionActivityLogTesting[];
 extern const char kEnableExtensionActivityUI[];
 extern const char kEnableFileCookies[];
 extern const char kEnableGoogleNowIntegration[];