[Activity log] Generalize the whitelist for keeping arguments

Previously, we kept all arguments for only a selected set of API calls,
but preserved arguments for all DOM actions that were logged.  Change
the logic so that the whitelist can cover actions of all types, and make
the default to strip arguments for all types unless in the whitelist.

BUG=253368

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

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

chrome/browser/extensions/activity_log/activity_log_policy.cc

@@ -123,16 +123,13 @@ void ActivityLogPolicy::Util::StripPrivacySensitiveFields(
 }
 
 // static
-void ActivityLogPolicy::Util::StripArguments(
-    const std::set<std::string>& api_whitelist,
-    scoped_refptr<Action> action) {
-  if (action->action_type() != Action::ACTION_API_CALL &&
-      action->action_type() != Action::ACTION_API_EVENT &&
-      action->action_type() != Action::UNUSED_ACTION_API_BLOCKED)
-    return;
-
-  if (api_whitelist.find(action->api_name()) == api_whitelist.end())
+void ActivityLogPolicy::Util::StripArguments(const ApiSet& api_whitelist,
+                                             scoped_refptr<Action> action) {
+  if (api_whitelist.find(
+          std::make_pair(action->action_type(), action->api_name())) ==
+      api_whitelist.end()) {
     action->set_args(scoped_ptr<ListValue>());
+  }
 }
 
 // static

chrome/browser/extensions/activity_log/activity_log_policy.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_POLICY_H_
 #define CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_LOG_POLICY_H_
 
+#include <map>
 #include <set>
 #include <string>
 
@@ -104,6 +105,10 @@ class ActivityLogPolicy {
   // these methods more convenient from within a policy, but they are public.
   class Util {
    public:
+    // A collection of API calls, used to specify whitelists for argument
+    // filtering.
+    typedef std::set<std::pair<Action::ActionType, std::string> > ApiSet;
+
     // Serialize a Value as a JSON string.  Returns an empty string if value is
     // null.
     static std::string Serialize(const base::Value* value);
@@ -117,7 +122,7 @@ class ActivityLogPolicy {
 
     // Strip arguments from most API actions, preserving actions only for a
     // whitelisted set.  Modifies the Action object in-place.
-    static void StripArguments(const std::set<std::string>& api_whitelist,
+    static void StripArguments(const ApiSet& api_whitelist,
                                scoped_refptr<Action> action);
 
     // Given a base day (timestamp at local midnight), computes the timestamp

chrome/browser/extensions/activity_log/activity_log_policy_unittest.cc

@@ -59,8 +59,9 @@ TEST_F(ActivityLogPolicyUtilTest, StripPrivacySensitiveWebRequest) {
 
 // Test that argument values are stripped as appropriate.
 TEST_F(ActivityLogPolicyUtilTest, StripArguments) {
-  std::set<std::string> whitelist;
-  whitelist.insert("tabs.executeScript");
+  ActivityLogPolicy::Util::ApiSet whitelist;
+  whitelist.insert(
+      std::make_pair(Action::ACTION_API_CALL, "tabs.executeScript"));
 
   // API is in whitelist; not stripped.
   scoped_refptr<Action> action =
@@ -78,13 +79,6 @@ TEST_F(ActivityLogPolicyUtilTest, StripArguments) {
   action->mutable_args()->AppendString("woof");
   ActivityLogPolicy::Util::StripArguments(whitelist, action);
   ASSERT_EQ("", ActivityLogPolicy::Util::Serialize(action->args()));
-
-  // Not an API type: not stripped.
-  action = new Action(
-      "punky", base::Time::Now(), Action::ACTION_DOM_ACCESS, "tabs.create");
-  action->mutable_args()->AppendString("woof");
-  ActivityLogPolicy::Util::StripArguments(whitelist, action);
-  ASSERT_EQ("[\"woof\"]", ActivityLogPolicy::Util::Serialize(action->args()));
 }
 
 // Test parsing of URLs serialized to strings.

chrome/browser/extensions/activity_log/counting_policy.cc

@@ -46,6 +46,8 @@ using content::BrowserThread;
 
 namespace {
 
+using extensions::Action;
+
 // Delay between cleaning passes (to delete old action records) through the
 // database.
 const int kCleaningDelayInHours = 12;
@@ -56,8 +58,19 @@ const int kCleaningDelayInHours = 12;
 //
 // TODO(mvrable): The contents of this whitelist should be reviewed and
 // expanded as needed.
-const char* kAlwaysLog[] = {"extension.connect", "extension.sendMessage",
-                            "tabs.executeScript", "tabs.insertCSS"};
+struct ApiList {
+  Action::ActionType type;
+  const char* name;
+};
+
+const ApiList kAlwaysLog[] = {
+    {Action::ACTION_API_CALL, "extension.connect"},
+    {Action::ACTION_API_CALL, "extension.sendMessage"},
+    {Action::ACTION_API_CALL, "tabs.executeScript"},
+    {Action::ACTION_API_CALL, "tabs.insertCSS"},
+    {Action::ACTION_CONTENT_SCRIPT, ""},
+    {Action::ACTION_DOM_ACCESS, "XMLHttpRequest.open"},
+};
 
 // Columns in the main database table.  See the file-level comment for a
 // discussion of how data is stored and the meanings of the _x columns.
@@ -138,7 +151,8 @@ CountingPolicy::CountingPolicy(Profile* profile)
       url_table_("url_ids"),
       retention_time_(base::TimeDelta::FromHours(60)) {
   for (size_t i = 0; i < arraysize(kAlwaysLog); i++) {
-    api_arg_whitelist_.insert(kAlwaysLog[i]);
+    api_arg_whitelist_.insert(
+        std::make_pair(kAlwaysLog[i].type, kAlwaysLog[i].name));
   }
 }
 

chrome/browser/extensions/activity_log/counting_policy.h

@@ -100,7 +100,7 @@ class CountingPolicy : public ActivityLogDatabasePolicy {
   bool CleanStringTables(sql::Connection* db);
 
   // API calls for which complete arguments should be logged.
-  std::set<std::string> api_arg_whitelist_;
+  Util::ApiSet api_arg_whitelist_;
 
   // Tables for mapping strings to integers for shrinking database storage
   // requirements.  URLs are kept in a separate table from other strings to

chrome/browser/extensions/activity_log/counting_policy_unittest.cc

@@ -191,7 +191,7 @@ class CountingPolicyTest : public testing::Test {
     CheckAction(*actions->at(0), "punky", Action::ACTION_API_CALL, "brewster",
                 "", "", "", "", 2);
     CheckAction(*actions->at(1), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "http://www.google.com/", "", "", 1);
+                "", "http://www.google.com/", "", "", 1);
     CheckAction(*actions->at(2), "punky", Action::ACTION_API_CALL,
                 "extension.sendMessage", "[\"not\",\"stripped\"]", "", "", "",
                 1);
@@ -201,7 +201,7 @@ class CountingPolicyTest : public testing::Test {
       scoped_ptr<Action::ActionVector> actions) {
     ASSERT_EQ(2, static_cast<int>(actions->size()));
     CheckAction(*actions->at(0), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "http://www.google.com/", "", "", 1);
+                "", "http://www.google.com/", "", "", 1);
     CheckAction(*actions->at(1), "punky", Action::ACTION_API_CALL, "brewster",
                 "", "", "", "", 1);
   }
@@ -235,25 +235,25 @@ class CountingPolicyTest : public testing::Test {
   static void AllURLsRemoved(scoped_ptr<Action::ActionVector> actions) {
     ASSERT_EQ(2, static_cast<int>(actions->size()));
     CheckAction(*actions->at(0), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "", "", "", 1);
+                "", "", "", "", 1);
     CheckAction(*actions->at(1), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "", "", "", 1);
+                "", "", "", "", 1);
   }
 
   static void SomeURLsRemoved(scoped_ptr<Action::ActionVector> actions) {
     // These will be in the vector in reverse time order.
     ASSERT_EQ(5, static_cast<int>(actions->size()));
     CheckAction(*actions->at(0), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "http://www.google.com/", "Google",
+                "", "http://www.google.com/", "Google",
                 "http://www.args-url.com/", 1);
     CheckAction(*actions->at(1), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "http://www.google.com/", "Google", "", 1);
+                "", "http://www.google.com/", "Google", "", 1);
     CheckAction(*actions->at(2), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "", "", "", 1);
+                "", "", "", "", 1);
     CheckAction(*actions->at(3), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "", "", "http://www.google.com/", 1);
+                "", "", "", "http://www.google.com/", 1);
     CheckAction(*actions->at(4), "punky", Action::ACTION_DOM_ACCESS, "lets",
-                "[\"vamoose\"]", "", "", "", 1);
+                "", "", "", "", 1);
   }
 
   static void CheckDuplicates(scoped_ptr<Action::ActionVector> actions) {