Add ephemeral storage support for JavaScript cookies #7154

mrobinson · 2020-11-16T14:31:02Z

This change adds a new class EphemeralStoragePartition which provides
access to an ephemeral version of a RestrictedCookieManager for the
content process. In an upcoming change this will also provide a
StoragePartition for navigation requests in third-party frames in order
to add support for navigation cookies.

Resolves brave/brave-browser#12591

Submitter Checklist:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

bridiver · 2020-11-17T02:19:47Z

chromium_src/net/cookies/cookie_monster.h

+#define BRAVE_COOKIE_MONSTER_H                             \
+  std::map<std::string, std::unique_ptr<CookieMonster>>    \
+      ephemeral_cookie_stores_;                            \
+  CookieMonster* GetOrCreateEphemeralCookieStoreForDomain( \


this method doesn't need to be in the header, you can pass in ephemeral_cookie_stores_ to non-class method

This method also uses net_log_ from CookieMonster in order to create the ephemeral monster. Since it's using two instance variables, I think it's simpler for it to be a method. Is there a reason why having fewer methods here is better?

bridiver · 2020-11-17T02:49:16Z

patches/services-network-public-mojom-cookie_manager.mojom.patch

+
+   // Delete session/persistent cookies.
+   CookieDeletionSessionControl session_control = IGNORE_CONTROL;
+


never add blank lines or comments to patches

bridiver · 2020-11-17T03:05:26Z

chromium_src/net/base/url_util.cc

+  // GetDomainAndRegistry might return an empty string if this host is an IP
+  // address or a file URL.
+  if (domain.empty())
+    domain = url::Origin::Create(url.GetOrigin()).Serialize();


I don't believe this is the correct behavior. You should create a URL from the domain even if it's empty and in turn Origin will create an opaque origin for those entries

This would mean that all sites that are accessed with IP addresses share the same ephemeral storage. If that the behaviour that we want, I think it makes sense to make that change in a separate PR that also adds tests for that functionality. Since this PR is just about adding support for ephemeral document.cookie, I'm not sure it belongs here.

no, opaque origins are unique by design, but I think actually it would cause the opposite problem where the same IP in more than one tab would go into different ephemeral storage domains

what is domain for a file url with this code?

The storage domain for a file URL is "file://" and for a IP address it will be "http://127.0.0.1:8000". This means that all file URLs share the same ephemeral storage lifetime and sites accessed via IP address have lifetimes that follow same-origin conventions.

bridiver · 2020-11-17T03:12:51Z

chromium_src/services/network/restricted_cookie_manager.cc

+  if (url::Origin::Create(url) == top_frame_origin)
+    return;
+  options->ephemeral_storage_domain_ =
+      net::URLToEphemeralStorageDomain(top_frame_origin.GetURL());


setting the ephemeral storage domain seems to overly complicate things compared to just setting the top level url and figuring out the storage domain inside the CookieMonster

bridiver · 2020-11-17T03:13:17Z

chromium_src/net/base/url_util.cc

@@ -80,4 +82,16 @@ bool ParseAuthHostAndPort(base::StringPiece input,
  return true;  // Success.
 }

+std::string URLToEphemeralStorageDomain(const GURL& url) {


I don't think this method is needed here https://github.com/brave/brave-core/pull/7154/files#r524857285

Where do you suggest this goes in a way that it can be used in the CookieMonster and also in EphemeralTabStorageHelper or do you prefer to have two versions of this code?

I don't think we need two versions of the code if we just pass in the url and only convert it to ephemeral storage domain when we actually create it. The code would only ever be called from one file

This is also called in EphemeralStorageTabHelper to manage DOM storage. We need to make sure the conversion from URL to storage domain is in sync.

bridiver · 2020-11-17T03:22:07Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.h

@@ -43,6 +44,7 @@ class EphemeralStorageTabHelper
  friend class content::WebContentsUserData<EphemeralStorageTabHelper>;
  scoped_refptr<content::SessionStorageNamespace> local_storage_namespace_;
  scoped_refptr<content::SessionStorageNamespace> session_storage_namespace_;
+  scoped_refptr<content::TLDEphemeralStorage> tld_ephemeral_storage_;


why is this a scoped_refptr?

Reference-counting is used to manage the lifetime of all ephemeral storage. This matches how the lifetime of storage is managed for DOM storage via SessionStorageNamespace in upstream Chromium and in our ephemeral storage implementation.

ok, I see what's happening now with the weakptr map

patches/services-network-restricted_cookie_manager.cc.patch

bridiver · 2020-11-17T03:50:12Z

patches/net-cookies-cookie_monster.cc.patch

+index 140b61a81dcd748aa22e9dec472f1d375bebcfae..527470acda3863843bca0bcb93e918aa2c35961a 100644
+--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
+@@ -579,6 +579,7 @@ void CookieMonster::AttachAccessSemanticsListForCookieList(


I think all of these can be eliminated by subclassing CookieMonster and doing a chromium_src override in content::CreateCookieStore

I did consider subclassing CookieMonster and maybe that does make sense. The issue is that content::CreateCookieStore is not always used to create the CookieStore. Crucially, it isn't used for navigation cookies and instead the constructor is called directly. See net/url_request/url_request_context_builder.cc and in services/network/network_context.cc (using make_unique). If we simply override the factory, there is a chance that this will require more work rebasing onto new versions of Chromium.

I chatted on slack with @bridiver about this and he clarified that by using chromium_src override, we should be able to create a subclass called CookieMonster and then use macros to rename the original CookieMonster. I will give this a shot.

bridiver · 2020-11-17T04:07:01Z

content/browser/tld_ephemeral_storage.cc

+      ->id();
+}
+
+TLDEphemeralStorage::TLDEphemeralStorage(TLDEphemeralStorageKey key,


why does this need to be in content?

This class is the public interface that EphemeralStorageTabHelper uses to access private API in content. Is there a better place for this?

what private api?

the only private apis I see are accessed outside of this class and we're now adding two new patches for this. It would be better I think to just keep it all included in the browser_context override. Also brave/content should never have been created in the first place and the files that are already in there are misplaced

if you created a separate file for tld_ephemeral_storage.cc/.h and included it from browser_context.cc you would put it in chromium_src because if brave/content was a valid location to put files, it would be confusing that these files were not part of brave/content/BUILD.gn

There are three interfaces into private API here CreateSessionStorageNamespace and GetSessionStorageNamespaceId, which used to be in BrowserContext (still part of content). The final piece is TLDEphemeralStorage which talks to the CookieStore in order to manage ephemeral cookie lifetime. The idea is that eventually CreateSessionStorageNamespace and GetSessionStorageNamespaceId would become part of TLDEphemeralStorage.

mrobinson · 2020-11-17T19:01:50Z

@bridiver Okay. I've pushed a new commit, which I believe addresses your concerns. PTAL.

bridiver · 2020-11-17T20:44:39Z

patches/services-network-public-mojom-cookie_manager.mojom.patch

+   CookieSameSiteContext same_site_cookie_context;
+   bool update_access_time = true;
+   bool return_excluded_cookies = false;
+  url.mojom.Url top_frame_url_for_ephemeral_storage;


nit: I don't think we need to specify that this is for ephemeral storage here

bridiver · 2020-11-17T20:46:27Z

browser/brave_tab_helpers.cc

@@ -23,7 +23,7 @@
 #include "brave/components/speedreader/buildflags.h"
 #include "brave/components/tor/buildflags/buildflags.h"
 #include "content/public/browser/web_contents.h"
-#include "third_party/blink/public/common/features.h"
+#include "net/base/features.h"


looks like you need to add //net

mrobinson · 2020-11-18T20:06:58Z

Okay. I've pushed a new version of the branch which I believe addresses the review comments above. Thanks!

iefremov · 2020-12-04T14:10:34Z

chromium_src/content/browser/tld_ephemeral_storage.cc

+    : key_(key), storage_partition_(storage_partition) {
+  DCHECK(active_tld_storage_areas().find(key) ==
+         active_tld_storage_areas().end());
+  active_tld_storage_areas().emplace(key, weak_factory_.GetWeakPtr());


I don't quite follow why do we need a weak pointer instead of a raw one?

In this case the weak pointer is just a sanity check which catches errors where the TLDEphemeralStorage is dereferenced and deleted.

which checks do you have in mind?

.get() on the weak pointer will return nullptr instead of a garbage value, but I will also add a DCHECK to also produce an assertion failure.

using WeakPtr in this way sounds unusual to me, but probably fine - could you write a comment describing this?

Okay. I'll leave a comment.

iefremov · 2020-12-04T14:15:11Z

chromium_src/content/browser/tld_ephemeral_storage.cc

+
+#include <map>
+#include "base/no_destructor.h"
+#include "content/browser/dom_storage/dom_storage_context_wrapper.h"


there are many unused headers, probably if we drop them we could move the whole class outside of chromium_src?

I will definitely remove the unused headers, but the idea is that eventually the private API exposed in browser_context.h can move into this class and it can eventually handle keeping track of all TLD-associated ephemeral DOM storage as well.

I agree if this refactoring is a short-term plan. Otherwise it most probably will stuck forever :)

iefremov · 2020-12-04T14:18:19Z

chromium_src/net/cookies/cookie_monster.h

+#ifndef BRAVE_CHROMIUM_SRC_NET_COOKIES_COOKIE_MONSTER_H_
+#define BRAVE_CHROMIUM_SRC_NET_COOKIES_COOKIE_MONSTER_H_
+
+#define CookieMonster SimpleCookieMonster


ChromiumCookieMonster?

iefremov · 2020-12-04T14:20:51Z

chromium_src/net/cookies/cookie_monster.cc

+
+#include "net/cookies/cookie_monster.h"
+
+#include <iostream>


not needed?

iefremov · 2020-12-04T14:26:33Z

chromium_src/net/cookies/cookie_monster.cc

+CookieMonster::GetOrCreateEphemeralCookieStoreForTopFrameURL(
+    const GURL& top_frame_url) {
+  std::string domain = URLToEphemeralStorageDomain(top_frame_url);
+  auto it = ephemeral_cookie_stores_.find(domain);


no need for this check, emplace will do the trick on its own

Won't this result in creating another instantiation of ChromiumCookieMonster every time we call this method?

yeah, it seems we should leave it alone

iefremov · 2020-12-04T14:35:10Z

chromium_src/content/public/browser/tld_ephemeral_storage.h

+//
+// TODO(mrobinson): Have this class also manage ephemeral local storage and
+// take care of handing out new instances of session storage.
+class CONTENT_EXPORT TLDEphemeralStorage


I'm not sure about future plans for this class, but for now it doesn't look like a "storage", rather something like TLDEphemeralLifetime

I haven't changed this yet, because perhaps it doesn't make sense to rename this class and then rename it back in a followup change. I'm happy to keep discussing it though.

But even with encapsulated DOMStorage - would it still be "storage"? For now we basically only keep namespaces and manage their lifetime in the tab helper, instead of "storing" something

Okay. I will change the name.

iefremov · 2020-12-04T14:40:25Z

Assuming that the patching work is negotiated with @bridiver and most probably cannot be done any better, the whole thing looks good. I'll try to give it test, but for now it fails to build on top of master

iefremov · 2020-12-04T14:41:02Z

(I also assume that commit messages will be updated before merging)

mrobinson · 2020-12-04T15:15:14Z

Assuming that the patching work is negotiated with @bridiver and most probably cannot be done any better, the whole thing looks good. I'll try to give it test, but for now it fails to build on top of master

I've pushed a rebased version of this PR and as well a commit addressing review comments.

mrobinson · 2020-12-07T09:47:05Z

I've pushed a commit which I believe addresses the remaining review comments.

mrobinson · 2020-12-08T12:55:07Z

It seems that the Windows build has an issue with patches to mojom trait headers. I've taken a different tactic, which is to extend the CookieStore API to add two special methods for ephemeral cookie access. This also has the added benefit of reducing the number of Chromium patches necessary by 2. PTAL.

iefremov

pls mention a link to the wiki explainer somewhere in the code (e.g. ephemeral_storage_tab_helper.h and/or ephemeral_storage_lifetime.h)

bridiver · 2020-12-10T21:33:40Z

chromium_src/content/public/browser/browser_context.h

@@ -6,11 +6,14 @@
 #ifndef BRAVE_CHROMIUM_SRC_CONTENT_PUBLIC_BROWSER_BROWSER_CONTEXT_H_
 #define BRAVE_CHROMIUM_SRC_CONTENT_PUBLIC_BROWSER_BROWSER_CONTEXT_H_

-#define IsOffTheRecord IsTor() const; \
-    virtual bool IsOffTheRecord
+#define IsOffTheRecord \


what is this change? Doesn't seem related and also should have 4 space indent for continuation of a line

There have been changes to the lint script which mean that the CI fails unless you do a full format. This will possibly be resolved when #7402 lands.

In the meantime, I can remove these full format changes.

bridiver · 2020-12-10T21:36:56Z

chromium_src/content/browser/tld_ephemeral_lifetime.cc

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "brave/chromium_src/content/public/browser/tld_ephemeral_lifetime.h"


do not use brave/chromium_src in the include path

bridiver · 2020-12-10T21:37:10Z

chromium_src/content/browser/browser_context.cc

 namespace content {
 bool BrowserContext::IsTor() const {
  return false;
 }
 }  // namespace content
+
+#include "../../../../content/browser/browser_context.cc"
+#include "brave/chromium_src/content/browser/tld_ephemeral_lifetime.cc"


do not use brave/chromium_src in the include path

bridiver · 2020-12-10T21:38:10Z

chromium_src/content/public/browser/browser_context.h

 #include "../../../../../content/public/browser/browser_context.h"
 #undef IsOffTheRecord

+#include "brave/chromium_src/content/public/browser/tld_ephemeral_lifetime.h"


don't use brave/chromium_src in the include path

bridiver

approved pending the few small fixes

This change adds a new class EphemeralStoragePartition which provides access to an ephemeral version of a RestrictedCookieManager for the content process. In an upcoming change this will also provide a StoragePartition for navigation requests in third-party frames in order to add support for navigation cookies.

mrobinson · 2020-12-11T14:11:19Z

There are three failures in CI:

Linux and Android: This is failing in the lint stage (while successful in build and testing). Lint has passed on other platforms and locally. There seem to be issues with the new lint script running on CI and the advice given to me is that this can be ignored if it's unrelated to my change.
iOS: It seems that the AdsWrapperTest testPreferencePersistance test is failing. This is a known issue and the current advice is to ignore it. Finally, this change should be unrelated to that test failure.

Given this, I feel fairly confident about merging this change despite the CI failures.

mrobinson requested review from bridiver and iefremov November 16, 2020 14:31

mrobinson requested a review from a team as a code owner November 16, 2020 14:31

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch 2 times, most recently from 7672844 to 6dac143 Compare November 16, 2020 18:55

bridiver reviewed Nov 17, 2020

View reviewed changes

patches/services-network-restricted_cookie_manager.cc.patch Show resolved Hide resolved

bridiver reviewed Nov 17, 2020

View reviewed changes

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch from 06421fe to 1859783 Compare November 17, 2020 19:01

bridiver reviewed Nov 17, 2020

View reviewed changes

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch 2 times, most recently from 1ffa6de to 71d01f4 Compare November 19, 2020 11:04

iefremov reviewed Dec 4, 2020

View reviewed changes

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch from 71d01f4 to 7f6d63a Compare December 4, 2020 14:47

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch from 6297fbe to 42c7c76 Compare December 7, 2020 09:46

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch 4 times, most recently from 1385d1f to be5b657 Compare December 8, 2020 12:31

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch 3 times, most recently from 50a27ab to 7121219 Compare December 8, 2020 17:09

mrobinson mentioned this pull request Dec 8, 2020

Add ephemeral storage support for network cookies #7387

Merged

23 tasks

iefremov approved these changes Dec 9, 2020

View reviewed changes

mihaiplesa assigned mrobinson Dec 9, 2020

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch from 7121219 to 5abef69 Compare December 9, 2020 12:21

mrobinson mentioned this pull request Dec 9, 2020

run clang-format as part of lint #7334

Merged

23 tasks

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch 2 times, most recently from e7b0404 to fa71ccb Compare December 10, 2020 12:24

bridiver reviewed Dec 10, 2020

View reviewed changes

bridiver approved these changes Dec 10, 2020

View reviewed changes

mrobinson force-pushed the ephemeral-cookie-storage-mimimal branch from 4fa55ef to fe5a8ec Compare December 11, 2020 09:30

mrobinson merged commit aca167d into master Dec 11, 2020

mrobinson deleted the ephemeral-cookie-storage-mimimal branch December 11, 2020 14:20

mrobinson added this to the 1.20.x - Nightly milestone Dec 11, 2020

Add ephemeral storage support for JavaScript cookies #7154

Add ephemeral storage support for JavaScript cookies #7154

Conversation

mrobinson commented Nov 16, 2020 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

mrobinson Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Dec 4, 2020 • edited Loading

iefremov commented Dec 4, 2020

mrobinson commented Dec 4, 2020 • edited Loading

mrobinson commented Dec 7, 2020

mrobinson commented Dec 8, 2020 • edited Loading

iefremov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

mrobinson commented Nov 16, 2020 •

edited

Loading

mrobinson Nov 17, 2020 •

edited

Loading

bridiver Nov 17, 2020 •

edited

Loading

bridiver Nov 17, 2020 •

edited

Loading

mrobinson Nov 17, 2020 •

edited

Loading

mrobinson Nov 17, 2020 •

edited

Loading

mrobinson Nov 17, 2020 •

edited

Loading

bridiver Nov 17, 2020 •

edited

Loading

iefremov Dec 4, 2020 •

edited

Loading

iefremov commented Dec 4, 2020 •

edited

Loading

mrobinson commented Dec 4, 2020 •

edited

Loading

mrobinson commented Dec 8, 2020 •

edited

Loading