First experimental implementation of the Clear-Site-Data header

content/browser/browsing_data/clear_site_data_throttle.cc

Index: content/browser/browsing_data/clear_site_data_throttle.cc
diff --git a/content/browser/browsing_data/clear_site_data_throttle.cc b/content/browser/browsing_data/clear_site_data_throttle.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d4571bdcf51b03554351e04bcfe62b0afabe7501
--- /dev/null
+++ b/content/browser/browsing_data/clear_site_data_throttle.cc
@@ -0,0 +1,262 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/browsing_data/clear_site_data_throttle.h"
+
+#include "base/command_line.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_string_value_serializer.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/common/origin_util.h"
+#include "net/http/http_response_headers.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+
+namespace content {
+
+namespace {
+
+static const char* kClearSiteDataHeader = "Clear-Site-Data";
+
+static const char* kTypesKey = "types";
+
+// Pretty-printed log output.
+static const char* kConsoleMessagePrefix = "Clear-Site-Data header on '%s': %s";
+static const char* kClearingOneType = "Clearing %s.";
+static const char* kClearingTwoTypes = "Clearing %s and %s.";
+static const char* kClearingThreeTypes = "Clearing %s, %s, and %s.";
+
+// Console logging. Adds a |text| message with |level| to |messages|.
+void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages,

[nasko, 2016/08/11 20:07:22]

nit: Seems an overkill to have a full on method for a single line. Makes code
less readable.

[msramek, 2016/08/12 15:06:27]

Hmm, here I would argue that ConsoleLog(where, what) is semantically clearer
than where->push_back(what), especially with the {} struct initializer. Do you
feel strongly about that?

[nasko, 2016/08/12 21:44:53]

I'm fine as it is.

+                const GURL& url,
+                const std::string& text,
+                ConsoleMessageLevel level) {
+  messages->push_back({url, text, level});
+}
+
+bool AreExperimentalFeaturesEnabled() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+}
+
+}  // namespace
+
+// static
+std::unique_ptr<NavigationThrottle>
+ClearSiteDataThrottle::CreateThrottleForNavigation(NavigationHandle* handle) {
+  if (AreExperimentalFeaturesEnabled())
+    return base::WrapUnique(new ClearSiteDataThrottle(handle));
+
+  return std::unique_ptr<NavigationThrottle>();
+}
+
+ClearSiteDataThrottle::ClearSiteDataThrottle(
+    NavigationHandle* navigation_handle)
+    : NavigationThrottle(navigation_handle),
+      active_tasks_(0),
+      weak_ptr_factory_(this) {}
+
+ClearSiteDataThrottle::~ClearSiteDataThrottle() {
+  // At the end of the navigation we finally have access to the correct
+  // RenderFrameHost. Output the cached console messages. Prefix each sequence
+  // of messages belonging to the same URL with |kConsoleMessagePrefix|.

[nasko, 2016/08/11 20:07:21]

|messages| is a vector and there is no sorting performed here. How do we
guarantee that the same URLs will be combined? Do we care?

[msramek, 2016/08/12 15:06:27]

If we're going through a redirect url1->url2, then all messages for url1 will be
necessarily pushed to the vector before url2, since we're now blocking the
navigation to url2 before url1 is processed. Even if we processed them
asynchronously, they're on the same thread, so the ordering would be retained.

If what you have in mind is a redirect chain url1->url2->url1, then it's going
to output "url1 ... url2 ... url1 ...", which I think makes sense.

I'm basically just trying to compact

"Clear-Site-Data header on https://www.example.com/index.html: <msg1>"
"Clear-Site-Data header on https://www.example.com/index.html: <msg2>"
"Clear-Site-Data header on https://www.example.com/index.html: <msg3>"

into

"Clear-Site-Data header on https://www.google.com/index.html: <msg1>"
"<msg2>"
"<msg3>"

to make it less spammy.

[nasko, 2016/08/12 21:44:53]

Yes, I had the url1->url2->url1 pattern in mind. I'm fine with what is there,
since we can tweak it per dev feedback later on.

+  GURL last_seen_url;
+  for (const ConsoleMessage& message : messages_) {
+    if (message.url == last_seen_url) {
+      navigation_handle()->GetRenderFrameHost()->AddMessageToConsole(
+          message.level, message.text);
+    } else {
+      navigation_handle()->GetRenderFrameHost()->AddMessageToConsole(
+          message.level,
+          base::StringPrintf(kConsoleMessagePrefix, message.url.spec().c_str(),
+                             message.text.c_str()));
+    }
+
+    last_seen_url = message.url;
+  }
+}
+
+ClearSiteDataThrottle::ThrottleCheckResult
+ClearSiteDataThrottle::WillStartRequest() {
+  current_url_ = navigation_handle()->GetURL();
+  return PROCEED;
+}
+
+ClearSiteDataThrottle::ThrottleCheckResult
+ClearSiteDataThrottle::WillRedirectRequest() {
+  // We are processing a redirect from url1 to url2. GetResponseHeaders()
+  // contains headers from url1, but GetURL() is already equal to url2. Handle
+  // the headers before updating the URL, so that |current_url_| corresponds
+  // to the URL that sent the headers.
+  HandleHeader();
+  current_url_ = navigation_handle()->GetURL();
+
+  return active_tasks_ ? DEFER : PROCEED;
+}
+
+ClearSiteDataThrottle::ThrottleCheckResult
+ClearSiteDataThrottle::WillProcessResponse() {
+  HandleHeader();
+  return active_tasks_ ? DEFER : PROCEED;
+}
+
+void ClearSiteDataThrottle::HandleHeader() {
+  NavigationHandleImpl* handle =
+      static_cast<NavigationHandleImpl*>(navigation_handle());
+
+  // Some navigations don't have response headers.
+  if (!handle->GetResponseHeaders())
+    return;
+

[nasko, 2016/08/11 20:07:22]

The spec calls out that Clear-Site-Data is only valid when the response URL is
not "insecure". I don't see such a check in this implementation.

[msramek, 2016/08/12 15:06:27]

You're correct to expect the call to IsOriginSecure() exactly at this line. It
was here, but I moved it a few lines below into the while() loop. The reason was
that I wanted to output an error message if the origin is insecure AND the
header is present. Since you suggested in the next comment to remove the while()
loop, this is no longer necessary.

Note that there is also a test in ClearSiteDataThrottleBrowsertest to verify
that we don't run this on HTTP.

[nasko, 2016/08/12 21:44:53]

Hmm, I wrote the comment and then discarded it. I guess Rietveld thought it
needs to keep it around : (. I did see the check later on, since I grepped for a
different pattern when I wrote the comment.
Yes, the test is very welcome too!

+  // Extract the instances of the header and parse them.
+  size_t iter = 0;
+  std::string header_name;
+  std::string header_value;
+  while (handle->GetResponseHeaders()->EnumerateHeaderLines(&iter, &header_name,

[nasko, 2016/08/11 20:07:21]

Do we want to allow multiple instances of the header? I'd suggest the spec
clarify that only one instance is expected and whether we honor the first(my
preference)/last when duplicates are encountered. This will allow for using
HasHeaderValue and avoiding lots of string copies, which the current
implementation will make.

[msramek, 2016/08/12 15:06:27]

Done.

It's true that currently there's no reason to have more than one instance, since
for example

{ 'types': ['cookies'] }
{ 'types': ['storage'] }

can be merged to

{ 'types': ['cookies', 'storage'] }

and for example

{ 'types': ['cookies'] }
{ 'types': ['cookies'] }

is idempotent.

Note that the original version of the spec didn't have this property, and that
might change again in the future, but for v1, let's simplify it.

It seems that EnumerateHeader(nullptr, ...) is what we want to use, but that
also requires us to list the header in HttpUtil::IsNonCoalescingHeader()
otherwise the JSON will be broken on commas.

[nasko, 2016/08/12 21:44:53]

Doesn't GetNormalizedHeader work? It will likely also have the side effect of
not parsing correctly if there are multiple instances of it.

[msramek, 2016/08/16 13:47:37]

Done.

Ok, let's do it that way. We'll probably update the spec soon to formalize that
only one instance is expected in which case GetNormalizedHeader() indeed works
slightly better by failing to parse instead of silently ignoring other
instances. Or we'll support multiple headers in v2 in which case I'll come back
with a CL for EnumerateHeaders(). But in any case, we'll end up not using
EnumerateHeader().

+                                                            &header_value)) {
+    if (header_name != kClearSiteDataHeader)
+      continue;
+
+    // Only accept the header on secure origins.
+    if (!IsOriginSecure(current_url_)) {
+      ConsoleLog(&messages_, current_url_,
+                 "Not supported for insecure origins.",
+                 CONSOLE_MESSAGE_LEVEL_ERROR);
+      return;
+    }
+
+    bool clear_cookies;
+    bool clear_storage;
+    bool clear_cache;
+
+    if (!ParseHeader(header_value, &clear_cookies, &clear_storage, &clear_cache,
+                     &messages_)) {
+      continue;
+    }
+
+    // If the header is valid, clear the data for this browser context
+    // and origin.
+    BrowserContext* browser_context =
+        navigation_handle()->GetWebContents()->GetBrowserContext();
+    url::Origin origin(current_url_);

[nasko, 2016/08/11 20:07:21]

Do we honor unique origins for deletion too? The spec doesn't call it out.

[msramek, 2016/08/12 15:06:27]

That's a good question.

The current implementation of url::Origin says that unique origins are not equal
to anything, not even to themselves. That means that comparing the origin of
data storage backend to the origin specified in the header will always return
false for unique origin. The deletion cannot be performed.

headers->GetResponseHeaders() already excludes "file:" and "data:" scheme
navigations, and I would expect that an unique origin cannot be
IsOriginSecure(), but let me put a check here just to be sure.

[nasko, 2016/08/12 21:44:53]

Maybe not right now with the current state of affairs, but in general you can
have a document with valid http/https URL and unique origin - for example a
sandboxed iframe hosting web content.

+
+    ++active_tasks_;

[nasko, 2016/08/11 20:07:21]

Honoring only single instance of the header will also avoid counting of tasks,
since we will only have one.

[msramek, 2016/08/12 15:06:27]

Done. But we'll still need at least a boolean.

[nasko, 2016/08/12 21:44:53]

Acknowledged.

+    GetContentClient()->browser()->ClearSiteData(
+        browser_context, origin, clear_cookies, clear_storage, clear_cache,
+        base::Bind(&ClearSiteDataThrottle::TaskFinished,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+bool ClearSiteDataThrottle::ParseHeader(const std::string& header,
+                                        bool* clear_cookies,
+                                        bool* clear_storage,
+                                        bool* clear_cache,
+                                        std::vector<ConsoleMessage>* messages) {
+  std::unique_ptr<base::Value> parsed_header;
+  if (base::IsStringASCII(header))

[nasko, 2016/08/11 20:07:22]

nit: Invert the check and early return.

[msramek, 2016/08/12 15:06:27]

Done.

+    parsed_header = base::JSONReader::Read(header);
+
+  if (!parsed_header) {
+    ConsoleLog(messages, current_url_,
+               "Not a valid JSON or non-ASCII characters present.",

[nasko, 2016/08/11 20:07:21]

Wouldn't we want to be more precise as to what went wrong? As a developer I'd
appreciate if I knew which one of those two conditions I've hit with my header.

[msramek, 2016/08/12 15:06:27]

Done.

+               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)) {
+    ConsoleLog(messages, current_url_,
+               "Expecting 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::vector<std::string> type_names;
+  for (const std::unique_ptr<base::Value>& value : *types) {
+    std::string type;
+    value->GetAsString(&type);
+
+    bool* datatype = nullptr;
+
+    if (type == "cookies") {
+      datatype = clear_cookies;
+    } else if (type == "storage") {
+      datatype = clear_storage;
+    } else if (type == "cache") {
+      datatype = clear_cache;
+    } else {
+      std::string serialized_type;
+      JSONStringValueSerializer serializer(&serialized_type);
+      serializer.Serialize(*value);
+      ConsoleLog(
+          messages, current_url_,
+          base::StringPrintf("Invalid type: %s.", serialized_type.c_str()),
+          CONSOLE_MESSAGE_LEVEL_ERROR);
+      continue;
+    }
+
+    // Each data type should only be processed once.
+    DCHECK(datatype);
+    if (*datatype)
+      continue;
+
+    *datatype = true;
+    type_names.push_back(type);
+  }
+
+  if (!*clear_cookies && !*clear_storage && !*clear_cache) {
+    ConsoleLog(messages, current_url_,
+               "No valid types specified in the 'types' field.",
+               CONSOLE_MESSAGE_LEVEL_ERROR);
+    return false;
+  }
+
+  // Pretty-print which types are to be cleared.
+  std::string output;
+  switch (type_names.size()) {
+    case 1:
+      output = base::StringPrintf(kClearingOneType, type_names[0].c_str());
+      break;
+    case 2:
+      output = base::StringPrintf(kClearingTwoTypes, type_names[0].c_str(),
+                                  type_names[1].c_str());
+      break;
+    case 3:
+      output = base::StringPrintf(kClearingThreeTypes, type_names[0].c_str(),
+                                  type_names[1].c_str(), type_names[2].c_str());
+      break;
+    default:
+      NOTREACHED();
+  }
+  ConsoleLog(messages, current_url_, output, CONSOLE_MESSAGE_LEVEL_LOG);
+
+  return true;
+}
+
+void ClearSiteDataThrottle::TaskFinished() {
+  DCHECK(active_tasks_);
+  if (!--active_tasks_)
+    navigation_handle()->Resume();
+}
+
+}  // namespace content