Align clear-site-data syntax with the spec.

We've shifted from an explicit JSON dictionary to a list of quoted strings (that can be parsed as JSON for forward-compatibility). That is, rather than `Clear-Site-Data: { "types": [ "cookies", "cache" ] }`, we'll use `Clear-Site-Data: "cookies", "cache"` to mean the same thing. Spec change in w3c/webappsec-clear-site-data#27. BUG: 607897 Change-Id: I051dcc49f9ed108e117347ccd7e249781433302b Reviewed-on: https://chromium-review.googlesource.com/530748 Reviewed-by: Martin Šrámek <[email protected]> Commit-Queue: Mike West <[email protected]> Cr-Commit-Position: refs/heads/master@{#478633}
yzyz · Jun 12, 2017 · cc19a83 · cc19a83
1 parent dca221e
commit cc19a83
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 119 deletions.
diff --git a/content/browser/browsing_data/clear_site_data_throttle.cc b/content/browser/browsing_data/clear_site_data_throttle.cc
@@ -4,11 +4,10 @@
 
 #include "content/browser/browsing_data/clear_site_data_throttle.h"
 
-#include "base/json/json_reader.h"
-#include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/scoped_observer.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
@@ -37,12 +36,10 @@ const char kNameForLogging[] = "ClearSiteDataThrottle";
 
 const char kClearSiteDataHeader[] = "Clear-Site-Data";
 
-const char kTypesKey[] = "types";
-
 // Datatypes.
-const char kDatatypeCookies[] = "cookies";
-const char kDatatypeStorage[] = "storage";
-const char kDatatypeCache[] = "cache";
+const char kDatatypeCookies[] = "\"cookies\"";
+const char kDatatypeStorage[] = "\"storage\"";
+const char kDatatypeCache[] = "\"cache\"";
 
 // Pretty-printed log output.
 const char kConsoleMessageTemplate[] = "Clear-Site-Data header on '%s': %s";
@@ -446,35 +443,13 @@ bool ClearSiteDataThrottle::ParseHeader(const std::string& header,
     return false;
   }
 
-  std::unique_ptr<base::Value> parsed_header = base::JSONReader::Read(header);
-
-  if (!parsed_header) {
-    delegate->AddMessage(current_url, "Expected valid JSON.",
-                         CONSOLE_MESSAGE_LEVEL_ERROR);
-    return false;
-  }
-
-  const base::DictionaryValue* dictionary = nullptr;
-  const base::ListValue* types = nullptr;
-  if (!parsed_header->GetAsDictionary(&dictionary) ||
-      !dictionary->GetListWithoutPathExpansion(kTypesKey, &types)) {
-    delegate->AddMessage(current_url,
-                         "Expected a JSON dictionary with a 'types' field.",
-                         CONSOLE_MESSAGE_LEVEL_ERROR);
-    return false;
-  }
-
-  DCHECK(types);
-
   *clear_cookies = false;
   *clear_storage = false;
   *clear_cache = false;
 
   std::string type_names;
-  for (const base::Value& value : *types) {
-    std::string type;
-    value.GetAsString(&type);
-
+  for (const base::StringPiece& type : base::SplitStringPiece(
+           header, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
     bool* data_type = nullptr;
 
     if (type == kDatatypeCookies) {
@@ -484,31 +459,26 @@ bool ClearSiteDataThrottle::ParseHeader(const std::string& header,
     } else if (type == kDatatypeCache) {
       data_type = clear_cache;
     } else {
-      std::string serialized_type;
-      JSONStringValueSerializer serializer(&serialized_type);
-      serializer.Serialize(value);
-      delegate->AddMessage(
-          current_url,
-          base::StringPrintf("Unrecognized type: %s.", serialized_type.c_str()),
-          CONSOLE_MESSAGE_LEVEL_ERROR);
+      delegate->AddMessage(current_url,
+                           base::StringPrintf("Unrecognized type: %s.",
+                                              type.as_string().c_str()),
+                           CONSOLE_MESSAGE_LEVEL_ERROR);
       continue;
     }
 
     DCHECK(data_type);
 
-    // Each data type should only be processed once.
     if (*data_type)
       continue;
 
     *data_type = true;
     if (!type_names.empty())
       type_names += kConsoleMessageDatatypeSeparator;
-    type_names += type;
+    type_names += type.as_string();
   }
 
   if (!*clear_cookies && !*clear_storage && !*clear_cache) {
-    delegate->AddMessage(current_url,
-                         "No recognized types specified in the 'types' field.",
+    delegate->AddMessage(current_url, "No recognized types specified.",
                          CONSOLE_MESSAGE_LEVEL_ERROR);
     return false;
   }

diff --git a/content/browser/browsing_data/clear_site_data_throttle_browsertest.cc b/content/browser/browsing_data/clear_site_data_throttle_browsertest.cc
@@ -64,7 +64,7 @@ void WaitForTitle(const Shell* shell, const char* expected_title) {
 
 // A value of the Clear-Site-Data header that requests cookie deletion. Reused
 // in tests that need a valid header but do not depend on its value.
-static const char* kClearCookiesHeader = "{ \"types\": [ \"cookies\" ] }";
+static const char* kClearCookiesHeader = "\"cookies\"";
 
 // A helper class to observe BrowsingDataRemover deletion tasks coming from
 // ClearSiteData.
@@ -730,14 +730,13 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest, Types) {
     bool remove_storage;
     bool remove_cache;
   } test_cases[] = {
-      {"{ \"types\": [ \"cookies\" ] }", true, false, false},
-      {"{ \"types\": [ \"storage\" ] }", false, true, false},
-      {"{ \"types\": [ \"cache\" ] }", false, false, true},
-      {"{ \"types\": [ \"cookies\", \"storage\" ] }", true, true, false},
-      {"{ \"types\": [ \"cookies\", \"cache\" ] }", true, false, true},
-      {"{ \"types\": [ \"storage\", \"cache\" ] }", false, true, true},
-      {"{ \"types\": [ \"cookies\", \"storage\", \"cache\" ] }", true, true,
-       true},
+      {"\"cookies\"", true, false, false},
+      {"\"storage\"", false, true, false},
+      {"\"cache\"", false, false, true},
+      {"\"cookies\", \"storage\"", true, true, false},
+      {"\"cookies\", \"cache\"", true, false, true},
+      {"\"storage\", \"cache\"", false, true, true},
+      {"\"cookies\", \"storage\", \"cache\"", true, true, true},
   };
 
   for (const TestCase& test_case : test_cases) {
@@ -793,7 +792,7 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest,
   // worker for "origin1.com", as the header would not be respected outside
   // of the scope.
   GURL url = https_server()->GetURL("origin1.com", "/anything-in-the-scope");
-  AddQuery(&url, "header", "{ \"types\": [ \"storage\" ] }");
+  AddQuery(&url, "header", "\"storage\"");
   NavigateToURL(shell(), url);
   service_workers = GetServiceWorkers();
   EXPECT_EQ(2u, service_workers.size());
@@ -803,7 +802,7 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest,
   // not handled by "worker.js" is the path "resource".
   // The header will be respected and the worker deleted.
   url = https_server()->GetURL("origin1.com", "/resource");
-  AddQuery(&url, "header", "{ \"types\": [ \"storage\" ] }");
+  AddQuery(&url, "header", "\"storage\"");
   NavigateToURL(shell(), url);
 
   // Only "origin2.com" now has a service worker.
@@ -844,7 +843,7 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest, CacheIntegrationTest) {
 
   // Let Clear-Site-Data delete the "cache" of "origin1.com".
   GURL url = https_server()->GetURL("origin1.com", "/clear-site-data");
-  AddQuery(&url, "header", "{ \"types\": [ \"cache\" ] }");
+  AddQuery(&url, "header", "\"cache\"");
   NavigateToURL(shell(), url);
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kTimeoutMs));
 

diff --git a/content/browser/browsing_data/clear_site_data_throttle_unittest.cc b/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
@@ -33,8 +33,7 @@ namespace {
 
 const char kClearSiteDataHeaderPrefix[] = "Clear-Site-Data: ";
 
-const char kClearCookiesHeader[] =
-    "Clear-Site-Data: { \"types\": [ \"cookies\" ] }";
+const char kClearCookiesHeader[] = "Clear-Site-Data: \"cookies\"";
 
 void WaitForUIThread() {
   base::RunLoop run_loop;
@@ -174,36 +173,32 @@ TEST_F(ClearSiteDataThrottleTest, ParseHeaderAndExecuteClearingTask) {
     bool cache;
   } test_cases[] = {
       // One data type.
-      {"{ \"types\": [\"cookies\"] }", true, false, false},
-      {"{ \"types\": [\"storage\"] }", false, true, false},
-      {"{ \"types\": [\"cache\"] }", false, false, true},
+      {"\"cookies\"", true, false, false},
+      {"\"storage\"", false, true, false},
+      {"\"cache\"", false, false, true},
 
       // Two data types.
-      {"{ \"types\": [\"cookies\", \"storage\"] }", true, true, false},
-      {"{ \"types\": [\"cookies\", \"cache\"] }", true, false, true},
-      {"{ \"types\": [\"storage\", \"cache\"] }", false, true, true},
+      {"\"cookies\", \"storage\"", true, true, false},
+      {"\"cookies\", \"cache\"", true, false, true},
+      {"\"storage\", \"cache\"", false, true, true},
 
       // Three data types.
-      {"{ \"types\": [\"storage\", \"cache\", \"cookies\"] }", true, true,
-       true},
-      {"{ \"types\": [\"cache\", \"cookies\", \"storage\"] }", true, true,
-       true},
-      {"{ \"types\": [\"cookies\", \"storage\", \"cache\"] }", true, true,
-       true},
+      {"\"storage\", \"cache\", \"cookies\"", true, true, true},
+      {"\"cache\", \"cookies\", \"storage\"", true, true, true},
+      {"\"cookies\", \"storage\", \"cache\"", true, true, true},
 
       // Different formatting.
-      {"  { \"types\":   [\"cookies\" ]}", true, false, false},
+      {"\"cookies\"", true, false, false},
 
       // Duplicates.
-      {"{ \"types\": [\"cookies\", \"cookies\"] }", true, false, false},
+      {"\"cookies\", \"cookies\"", true, false, false},
 
-      // Other entries in the dictionary.
-      {"{ \"types\": [\"storage\"], \"other_params\": {} }", false, true,
-       false},
+      // Other JSON-formatted items in the list.
+      {"\"storage\", { \"other_params\": {} }", false, true, false},
 
       // Unknown types are ignored, but we still proceed with the deletion for
       // those that we recognize.
-      {"{ \"types\": [\"cache\", \"foo\"] }", false, false, true},
+      {"\"cache\", \"foo\"", false, false, true},
   };
 
   for (const TestCase& test_case : test_cases) {
@@ -251,20 +246,24 @@ TEST_F(ClearSiteDataThrottleTest, InvalidHeader) {
   struct TestCase {
     const char* header;
     const char* console_message;
-  } test_cases[] = {
-      {"", "Expected valid JSON.\n"},
-      {"\"unclosed quote", "Expected valid JSON.\n"},
-      {"\"some text\"", "Expected a JSON dictionary with a 'types' field.\n"},
-      {"{ \"field\" : {} }",
-       "Expected a JSON dictionary with a 'types' field.\n"},
-      {"{ \"types\" : [ \"passwords\" ] }",
-       "Unrecognized type: \"passwords\".\n"
-       "No recognized types specified in the 'types' field.\n"},
-      {"{ \"types\" : [ [ \"list in a list\" ] ] }",
-       "Unrecognized type: [\"list in a list\"].\n"
-       "No recognized types specified in the 'types' field.\n"},
-      {"{ \"types\" : [ \"кукис\", \"сторидж\", \"кэш\" ]",
-       "Must only contain ASCII characters.\n"}};
+  } test_cases[] = {{"", "No recognized types specified.\n"},
+                    {"\"unclosed",
+                     "Unrecognized type: \"unclosed.\n"
+                     "No recognized types specified.\n"},
+                    {"\"passwords\"",
+                     "Unrecognized type: \"passwords\".\n"
+                     "No recognized types specified.\n"},
+                    {"[ \"list\" ]",
+                     "Unrecognized type: [ \"list\" ].\n"
+                     "No recognized types specified.\n"},
+                    {"[ \"list\" ]",
+                     "Unrecognized type: [ \"list\" ].\n"
+                     "No recognized types specified.\n"},
+                    {"{ \"cookies\": [ \"a\" ] }",
+                     "Unrecognized type: { \"cookies\": [ \"a\" ] }.\n"
+                     "No recognized types specified.\n"},
+                    {"\"кукис\", \"сторидж\", \"кэш\"",
+                     "Must only contain ASCII characters.\n"}};
 
   for (const TestCase& test_case : test_cases) {
     SCOPED_TRACE(test_case.header);
@@ -307,7 +306,7 @@ TEST_F(ClearSiteDataThrottleTest, LoadDoNotSaveCookies) {
   throttle.WillProcessResponse(&defer);
   EXPECT_TRUE(defer);
   EXPECT_EQ(1u, console_delegate->messages().size());
-  EXPECT_EQ("Cleared data types: cookies.",
+  EXPECT_EQ("Cleared data types: \"cookies\".",
             console_delegate->messages().front().text);
   EXPECT_EQ(console_delegate->messages().front().level,
             CONSOLE_MESSAGE_LEVEL_INFO);
@@ -405,31 +404,27 @@ TEST_F(ClearSiteDataThrottleTest, DeferAndResume) {
 
       // That includes malformed Clear-Site-Data headers or header values
       // that do not lead to deletion.
-      {REDIRECT, "Clear-Site-Data: { types: cookies } ", false},
-      {REDIRECT, "Clear-Site-Data: { \"types\": [ \"unknown type\" ] }", false},
+      {REDIRECT, "Clear-Site-Data: cookies", false},
+      {REDIRECT, "Clear-Site-Data: \"unknown type\"", false},
 
       // However, redirects are deferred for valid Clear-Site-Data headers.
-      {REDIRECT,
-       "Clear-Site-Data: { \"types\": [ \"cookies\", \"unknown type\" ] }",
-       true},
+      {REDIRECT, "Clear-Site-Data: \"cookies\", \"unknown type\"", true},
       {REDIRECT,
        base::StringPrintf("Content-Type: image/png;\n%s", kClearCookiesHeader),
        true},
       {REDIRECT,
        base::StringPrintf("%s\nContent-Type: image/png;", kClearCookiesHeader),
        true},
 
-      // We expect at most one instance of the header. Multiple instances
-      // will not be parsed currently. This is not an inherent property of
-      // Clear-Site-Data, just a documentation of the current behavior.
+      // Multiple instances of the header will be parsed correctly.
       {REDIRECT,
        base::StringPrintf("%s\n%s", kClearCookiesHeader, kClearCookiesHeader),
-       false},
+       true},
 
       // Final response headers are treated the same way as in the case
       // of redirect.
       {REDIRECT, "Set-Cookie: abc=123;", false},
-      {REDIRECT, "Clear-Site-Data: { types: cookies } ", false},
+      {REDIRECT, "Clear-Site-Data: cookies", false},
       {REDIRECT, kClearCookiesHeader, true},
   };
 
@@ -518,42 +513,46 @@ TEST_F(ClearSiteDataThrottleTest, FormattedConsoleOutput) {
     const char* output;
   } kTestCases[] = {
       // Successful deletion outputs one line.
-      {"{ \"types\": [ \"cookies\" ] }", "https://origin1.com/foo",
+      {"\"cookies\"", "https://origin1.com/foo",
        "Clear-Site-Data header on 'https://origin1.com/foo': "
-       "Cleared data types: cookies.\n"},
+       "Cleared data types: \"cookies\".\n"},
 
       // Another successful deletion.
-      {"{ \"types\": [ \"storage\" ] }", "https://origin2.com/foo",
+      {"\"storage\"", "https://origin2.com/foo",
        "Clear-Site-Data header on 'https://origin2.com/foo': "
-       "Cleared data types: storage.\n"},
+       "Cleared data types: \"storage\".\n"},
 
       // Redirect to the same URL. Unsuccessful deletion outputs two lines.
-      {"{ \"foo\": \"bar\" }", "https://origin2.com/foo",
+      {"\"foo\"", "https://origin2.com/foo",
        "Clear-Site-Data header on 'https://origin2.com/foo': "
-       "Expected a JSON dictionary with a 'types' field.\n"},
+       "Unrecognized type: \"foo\".\n"
+       "Clear-Site-Data header on 'https://origin2.com/foo': "
+       "No recognized types specified.\n"},
 
       // Redirect to another URL. Another unsuccessful deletion.
       {"\"some text\"", "https://origin3.com/bar",
        "Clear-Site-Data header on 'https://origin3.com/bar': "
-       "Expected a JSON dictionary with a 'types' field.\n"},
+       "Unrecognized type: \"some text\".\n"
+       "Clear-Site-Data header on 'https://origin3.com/bar': "
+       "No recognized types specified.\n"},
 
       // Yet another on the same URL.
-      {"{ \"types\" : [ \"passwords\" ] }", "https://origin3.com/bar",
+      {"\"passwords\"", "https://origin3.com/bar",
        "Clear-Site-Data header on 'https://origin3.com/bar': "
        "Unrecognized type: \"passwords\".\n"
        "Clear-Site-Data header on 'https://origin3.com/bar': "
-       "No recognized types specified in the 'types' field.\n"},
+       "No recognized types specified.\n"},
 
       // Successful deletion on the same URL.
-      {"{ \"types\": [ \"cache\" ] }", "https://origin3.com/bar",
+      {"\"cache\"", "https://origin3.com/bar",
        "Clear-Site-Data header on 'https://origin3.com/bar': "
-       "Cleared data types: cache.\n"},
+       "Cleared data types: \"cache\".\n"},
 
       // Redirect to the original URL.
       // Successful deletion outputs one line.
       {"", "https://origin1.com/foo",
        "Clear-Site-Data header on 'https://origin1.com/foo': "
-       "Expected valid JSON.\n"}};
+       "No recognized types specified.\n"}};
 
   bool kThrottleTypeIsNavigation[] = {true, false};
 

diff --git a/content/test/data/browsing_data/worker.js b/content/test/data/browsing_data/worker.js
@@ -13,7 +13,7 @@ self.addEventListener('fetch', function(event) {
   if (url.pathname.match('resource_from_sw')) {
     event.respondWith(new Response(
         'Response content is not important, only the header is.', {
-            'headers': { 'Clear-Site-Data': '{ "types" : [ "cookies" ] }' }
+            'headers': { 'Clear-Site-Data': '"cookies"' }
         }));
     return;
   }
@@ -44,7 +44,7 @@ self.addEventListener('fetch', function(event) {
         origins[3] + 'resource_from_sw',
         origins[4] + 'another_resource_so_that_the_previous_one_isnt_reused',
     ];
-    var header = encodeURIComponent('{ "types": [ "cookies" ] }');
+    var header = encodeURIComponent('"cookies"');
 
     // Fetch all resources and report back to the C++ side by setting
     // the document title.

diff --git a/content/test/data/fuzzer_corpus/clear_site_data/all.txt b/content/test/data/fuzzer_corpus/clear_site_data/all.txt
@@ -1 +1 @@
-{ "types": [ "cookies", "storage", "cache" ] }
+"cookies", "storage", "cache"
diff --git a/content/test/data/fuzzer_corpus/clear_site_data/cache.txt b/content/test/data/fuzzer_corpus/clear_site_data/cache.txt
@@ -1 +1 @@
-{ "types": [ "cache" ] }
+"cache"
diff --git a/content/test/data/fuzzer_corpus/clear_site_data/cookies.txt b/content/test/data/fuzzer_corpus/clear_site_data/cookies.txt
@@ -1 +1 @@
-{ "types": [ "cookies" ] }
+"cookies"
diff --git a/content/test/data/fuzzer_corpus/clear_site_data/extra.txt b/content/test/data/fuzzer_corpus/clear_site_data/extra.txt
@@ -1 +1 @@
-{ "types": [ "cookies", "storage", "cache", "executionContexts" ] }
+"cookies", "storage", "cache", "executionContexts"
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		{ "types": [ "cookies", "storage", "cache" ] }
		"cookies", "storage", "cache"
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		{ "types": [ "cookies", "storage", "cache", "executionContexts" ] }
		"cookies", "storage", "cache", "executionContexts"