only use ephemeral storage for block 3p setting #7647

bridiver · 2021-01-19T00:43:02Z

Resolves brave/brave-browser#12789

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed).
Requested a security/privacy review as needed.

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

bridiver · 2021-01-19T00:44:37Z

still needs tests so technically a wip, but I wanted to get a build

pes10k · 2021-01-19T00:47:50Z

just to keep everything stitched together, manual tests are here: https://dev-pages.brave.software/storage/ephemeral-storage.html

bridiver · 2021-01-19T01:16:17Z

components/content_settings/renderer/brave_content_settings_agent_impl.cc

+         AllowStorageAccessForMainFrameSync(storage_type);
+}
+
+bool BraveContentSettingsAgentImpl::AllowStorageAccessSync(


still need to update the non-sync versions of these methods just for consistency in the results

mrobinson · 2021-01-20T09:39:56Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+  session_storage_namespace_ = content::CreateSessionStorageNamespace(
+      partition, session_partition_id,
+      // clone the namespace if there is an opener
+      // https://html.spec.whatwg.org/multipage/browsers.html#copy-session-storage


Will this clone copy non-ephemeral session storage data into the ephemeral session storage? I'm pretty sure that's why I didn't do this in the past.

it shouldn't be, I'm only passing the id of the parent session storage and it passes all the tests on https://dev-pages.brave.software/storage/ephemeral-storage.html

and since the opener will be suppressed if it's not same domain the owner window will always have ephemeral storage enabled, but it might be worth adding a check in case there's some edge case with subdomains

looks like it should be ok

// If the namespace doesn't exist or it's not populated yet, just create // an empty session storage by not marking it as pending a clone. if (clone_from_ns == namespaces_.end() || !clone_from_ns->second->IsPopulated()) { break; }

would be nice to have a browser test

mrobinson · 2021-01-20T09:41:42Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+  bool block_3p = !cookie_settings->IsCookieAccessAllowed(
+      url, site_for_cookies, top_frame_origin);
+  bool block_1p = !cookie_settings->IsCookieAccessAllowed(
+      url, url, url::Origin::Create(url));


This is a clever way of doing this. Nice!

mrobinson · 2021-01-20T09:42:46Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+  // only use ephemeral storage for block 3p
+  if (block_3p && !block_1p)
+    return true;
+
+  return false;


Nit: Here you could simply do:

return block_3p && !block_1p;

yea, I could still attach the same comment to it for clarity so I'll clean that up

mrobinson · 2021-01-20T09:44:59Z

chromium_src/components/content_settings/core/common/cookie_settings_base.h

+  bool IsEphemeralCookieAccessAllowed(const GURL& url,                    \
+                                      const GURL& first_party_url) const; \


I think you could eliminate this method by giving the three-argument version a default argument:

bool IsEphemeralCookieAccessAllowed( \ const GURL& url, const GURL& site_for_cookies, \ const base::Optional<url::Origin>& top_frame_origin = base::nullopt) const;

I wanted to stay consistent with the original methods for clarity

and also when the two argument version is removed that might cause us to miss something

mrobinson · 2021-01-20T09:50:39Z

chromium_src/net/url_request/url_request_http_job.cc

+  if (use_ephemeral_storage) {                                                 \
+    auto top_frame_url =                                                       \
+        request_->isolation_info().top_frame_origin()->GetURL();               \
+    auto cookie_1p = net::CanonicalCookie::CreateSanitizedCookie(              \
+        top_frame_url, cookie->Name(), cookie->Value(), cookie->Domain(),      \
+        cookie->Path(), cookie->CreationDate(), cookie->ExpiryDate(),          \
+        cookie->LastAccessDate(), cookie->IsSecure(), cookie->IsHttpOnly(),    \
+        cookie->SameSite(), cookie->Priority(), cookie->IsSameParty());        \
+                                                                               \
+    net::CookieOptions::SameSiteCookieContext same_site_context =              \
+        net::cookie_util::ComputeSameSiteContextForResponse(                   \
+            top_frame_url, request_->site_for_cookies(),                       \
+            request_->initiator(), false);                                     \
+    net::CookieOptions options_1p = CreateCookieOptions(same_site_context);    \
+                                                                               \
+    if (!(CanSetCookie(*cookie_1p, &options_1p) &&                             \
+          !CanSetCookie(*cookie, &options)))                                   \
+      use_ephemeral_storage = false;                                           \
+  }                                                                            \


I think you can avoid duplicating this code by making it part of ShouldUseEphemeralStorage and passing in the cookie parameter.

yea, I think there are several places this can be cleaned up so I'll create some follow-up issues

mrobinson · 2021-01-20T09:52:34Z

chromium_src/services/network/restricted_cookie_manager.cc

+  // only use ephemeral storage for block 3p
+  if (block_3p && !block_1p)
+    return true;
+
+  return false;


This could use the same simplification that I mentioned above.

I left a TODO for this https://github.com/brave/brave-core/pull/7647/files#diff-72c915ffc3846c8565c42e47757c71a935649256894742e46a00ecd23f1ca5cdR147

mrobinson · 2021-01-20T09:55:09Z

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

-  MaybeClearAccessDeniedException(storage, *window, &exception_state);
-  if (!base::FeatureList::IsEnabled(net::features::kBraveEphemeralStorage))
+  if (!ShouldUseEphemeralStorage(
+          window, WebContentSettingsClient::StorageType::kSessionStorage))


Should this be WebContentSettingsClient::StorageType::kLocalStorage instead? If not, can the argument be eliminated from the function entirely since each call site passes WebContentSettingsClient::StorageType::kSessionStorage?

Good catch. It won't affect anything because the ephemeral storage settings are the same for session and local storage, but it should be correct in case something changes with that in the future

mkarolin · 2021-01-21T21:54:28Z

chromium_src/net/url_request/url_request_http_job.cc

+    net::CookieInclusionStatus returned_status;                                \
+    auto cookie = net::CanonicalCookie::Create(                                \
+        request_->url(), "a=a", base::Time::Now(), base::nullopt,              \
+        &returned_status);                                                     \


This param has a default value of nullptr and we don't seem to be using returned_status afterwords.

mkarolin

++ on chromium_src

bridiver · 2021-01-22T17:16:37Z

chromium_src/net/base/features.cc

@@ -8,6 +8,6 @@
 namespace net {
 namespace features {
 const base::Feature kBraveEphemeralStorage{"EphemeralStorage",
-                                           base::FEATURE_DISABLED_BY_DEFAULT};
+                                           base::FEATURE_ENABLED_BY_DEFAULT};


removing this change as it was not intentional

…r better webcompat

iefremov · 2021-01-25T14:17:19Z

components/content_settings/renderer/brave_content_settings_agent_impl.h

@@ -8,6 +8,7 @@

 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>

 #include "base/strings/string16.h"


also base/containers/flat_map.h

iefremov · 2021-01-25T14:34:13Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+  auto* rfh = web_contents()->GetOpener();
+  session_storage_namespace_ = content::CreateSessionStorageNamespace(
+      partition, session_partition_id,
+      // clone the namespace if there is an opener


really a nit, but the style guide wants us to use proper punctuation, i.e. start sentenses with a capital and end with a dot.

https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

iefremov · 2021-01-25T15:06:06Z

components/content_settings/renderer/brave_content_settings_agent_impl.cc

+      frame->GetDocument().TopFrameOrigin(),
+      url::Origin(frame->GetDocument().TopFrameOrigin()).GetURL(),
+      frame->GetDocument().TopFrameOrigin(), &result);
+  cached_storage_permissions_[key] = result;


do we need the cache at all? Growing it indefinitely looks pretty scary

this is copied from ContentSettingsAgentImpl and it's per-frame so it doesn't grow indefinitely

iefremov · 2021-01-25T15:33:30Z

LGTM in general, but I'd like to understand whether we have tests for toggling the ephemeral storage on/off?

Also, should we explicitly delete existing ephemeral storage for a given domain when a user disables shields?

iefremov · 2021-01-25T15:34:57Z

oops, something happened with network cookies

bridiver · 2021-01-25T16:27:19Z

@iefremov there are existing tests for cookie behavior that would break if ephemeral storage was turned on and I don't think we would delete the storage just because shields was disabled, but I'll defer to @pes10k

pes10k · 2021-01-25T18:58:02Z

As long as the "when to clear the ephemeral storage" logic doesn't change, i dont think we need to "force clear" it when shields are dropped.

So, if i have two tabs, example.org/1.html and example.org/2.html w/ ephemeral storage enabled, and both pages have some thirdparty frame (e.g., third.org/frame.html). Even if i drop shields on example.org (so ephemeral storage is not being applied to the page), the <example.org, third.org> partition should be cleared when all example.org tabs are closed

iefremov · 2021-01-25T19:56:58Z

@bridiver what happened to network cookies?..

pes10k · 2021-01-25T19:59:50Z

We stripped it from the initial implementation bc (my understanding is) it made implementation much more complicated and ugly, and its by far the less important part of the system. It would still be very good to get it supported, but my preference was to not delay getting this 95%-of-the-benefit part shipped while we figure out whats possible network side.

@bridiver @iefremov if there isn't already a follow up issue for network navigation cookies, would you like me to create one?

bridiver requested review from mrobinson, darkdh and mkarolin January 19, 2021 00:43

bridiver requested a review from a team as a code owner January 19, 2021 00:43

bridiver self-assigned this Jan 19, 2021

bridiver requested a review from iefremov January 19, 2021 00:43

bridiver changed the title ~~only use ephemeral storage when for block 3p setting~~ only use ephemeral storage for block 3p setting Jan 19, 2021

bridiver commented Jan 19, 2021

View reviewed changes

bridiver force-pushed the ephemeral-storage branch 2 times, most recently from 2bc6577 to 2a6b094 Compare January 19, 2021 01:50

bridiver mentioned this pull request Jan 19, 2021

Only use ephemeral storage when storage would otherwise be blocked #7207

Closed

23 tasks

bridiver force-pushed the ephemeral-storage branch from 2a6b094 to 357c273 Compare January 19, 2021 02:07

bridiver added 3 commits January 19, 2021 08:20

only use ephemeral storage when for block 3p setting

54d02cc

only override specific calls to IsCookieAccessAllowed

64e699d

only use ephemeral storage for local and session storage

83a051b

bridiver force-pushed the ephemeral-storage branch from 0439048 to 83a051b Compare January 19, 2021 15:21

mrobinson reviewed Jan 20, 2021

View reviewed changes

mkarolin reviewed Jan 21, 2021

View reviewed changes

mkarolin approved these changes Jan 21, 2021

View reviewed changes

use correct storage type for check

2d2c856

bridiver commented Jan 22, 2021

View reviewed changes

bridiver added 5 commits January 22, 2021 10:16

don't change default

83e12cf

we're back to access denied for dom storage now instead of nullptr fo…

8ed2e0d

…r better webcompat

don't allow all cookies for ephermal storage tests anymore

3cabed1

use url_request to get 1p and 3p cookie settings

0f70edb

disable network tests for now

11650e3

iefremov reviewed Jan 25, 2021

View reviewed changes

remove ephemeral storage for network cookies for now

bed4ed0

iefremov reviewed Jan 25, 2021

View reviewed changes

bridiver merged commit 3dc841e into master Jan 25, 2021

bridiver deleted the ephemeral-storage branch January 25, 2021 19:46

bridiver added this to the 1.21.x - Nightly milestone Jan 28, 2021

bridiver mentioned this pull request Feb 9, 2021

Do not use ephemeral storage for third-party domains that block cookies with a domain override brave/brave-browser#12790

Closed

btlechowski mentioned this pull request Feb 22, 2021

Ephemeral storage should work when third-party cookies are blocked brave/brave-browser#12789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only use ephemeral storage for block 3p setting #7647

only use ephemeral storage for block 3p setting #7647

bridiver commented Jan 19, 2021 •

edited

Loading

bridiver commented Jan 19, 2021

pes10k commented Jan 19, 2021

bridiver Jan 19, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

bridiver Jan 21, 2021

bridiver Jan 21, 2021 •

edited

Loading

iefremov Jan 25, 2021

mrobinson Jan 20, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

bridiver Jan 21, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

mrobinson Jan 20, 2021

bridiver Jan 21, 2021

mkarolin Jan 21, 2021

mkarolin left a comment

bridiver Jan 22, 2021

iefremov Jan 25, 2021

iefremov Jan 25, 2021

iefremov Jan 25, 2021

bridiver Jan 25, 2021

iefremov commented Jan 25, 2021

iefremov commented Jan 25, 2021

bridiver commented Jan 25, 2021

pes10k commented Jan 25, 2021

iefremov commented Jan 25, 2021

pes10k commented Jan 25, 2021

		bool IsEphemeralCookieAccessAllowed(const GURL& url, \
		const GURL& first_party_url) const; \

only use ephemeral storage for block 3p setting #7647

only use ephemeral storage for block 3p setting #7647

Conversation

bridiver commented Jan 19, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bridiver commented Jan 19, 2021

pes10k commented Jan 19, 2021

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 Jan 21, 2021 • 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

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

mkarolin 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

iefremov commented Jan 25, 2021

iefremov commented Jan 25, 2021

bridiver commented Jan 25, 2021

pes10k commented Jan 25, 2021

iefremov commented Jan 25, 2021

pes10k commented Jan 25, 2021

bridiver commented Jan 19, 2021 •

edited

Loading

bridiver Jan 21, 2021 •

edited

Loading