Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement first party ephemeral storage support #9624

Merged
merged 8 commits into from
Oct 8, 2021
Merged

Implement first party ephemeral storage support #9624

merged 8 commits into from
Oct 8, 2021

Conversation

goodov
Copy link
Member

@goodov goodov commented Aug 3, 2021 •
edited
Loading

Add support for first party ephemeral storage (1PES) using CONTENT_SETTING_SESSION_ONLY cookie setting value.
Some important things:

  1. We reuse CONTENT_SETTING_SESSION_ONLY value and alter its behavior for our needs when a 1PES feature is enabled. By default, Chromium cleanups everything for a website when this setting is active.
    In our approach when CONTENT_SETTING_SESSION_ONLY is selected for a website ("1PES on"), we introduce all third party restrictions to first party frames of this website, so everything will be blocked except ephemeral cookies and storages.
  2. When 1PES-enabled frame is embedded as 3PES frame, then it should behave as before and nothing should change (its ephemeral storages will be keyed using first-party URL).
  3. When 1PES-enabled frame is embedded on a website with disabled shields, the 1PES-enabled frame will have access to all persistent storages and should behave as if 1PES doesn't exist for the frame, but when a browser is closed, the Chromium logic will still cleanup all persistent storages for 1PES-enabled website.

Resolves brave/brave-browser#15906

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • 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)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • 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:

@goodov goodov requested review from a team as code owners August 3, 2021 12:56
@goodov goodov force-pushed the issues/15906-1pes branch 6 times, most recently from b24c32a to 1b024cb Compare August 5, 2021 10:17
@goodov goodov requested review from bridiver and iefremov August 5, 2021 12:49
@goodov goodov force-pushed the issues/15906-1pes branch from 1b024cb to 4626031 Compare August 5, 2021 14:06
iefremov
iefremov reviewed Aug 12, 2021
View reviewed changes
chromium_src/net/base/features.cc
@@ -17,5 +17,8 @@ const base::FeatureParam<int> kBraveEphemeralStorageKeepAliveTimeInSeconds = {
&kBraveEphemeralStorageKeepAlive,
"BraveEphemeralStorageKeepAliveTimeInSeconds", 30};

const base::Feature kBraveFirstPartyEphemeralStorage{
"BraveFirstPartyEphemeralStorage", base::FEATURE_DISABLED_BY_DEFAULT};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess people would like to have a toggle on brave://flags

@goodov goodov force-pushed the issues/15906-1pes branch 2 times, most recently from 3f18313 to 3a9177c Compare August 12, 2021 16:30
iefremov
iefremov reviewed Aug 13, 2021
View reviewed changes
chromium_src/components/content_settings/core/common/cookie_settings_base.h Outdated
enum class EphemeralStorageAwareType {
kNone,
kAware,
kNotAwareButAllowIn1pEphemeralMode,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it needs comments

iefremov
iefremov reviewed Aug 13, 2021
View reviewed changes
browser/ephemeral_storage/ephemeral_storage_1p_browsertest.cc
// Helper to load easy-to-use Indexed DB API.
void LoadIndexedDbHelper(RenderFrameHost* host) {
const char kLoadIndexMinScript[] =
"new Promise((resolve) => {"
Copy link
Contributor

@iefremov iefremov Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use a raw string literal?

iefremov
iefremov reviewed Aug 13, 2021
View reviewed changes
chromium_src/content/browser/devtools/protocol/network_handler.cc Outdated
#define BRAVE_COOKIE_RETRIEVER_NETWORK_SERVICE_RETRIEVE_BODY \
cookie_options.set_top_frame_origin(top_frame_origin);

#define BRAVE_NETWORK_HANDLER_GET_COOKIES_RETREIVE_CALL_ARGS \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retrIEve

@iefremov
Copy link
Contributor

It's definitely worth adding comments here and there, since this is one of the most complex features and the code is scattered among different overrides

@iefremov
Copy link
Contributor

pls check the following scenario:

  1. Enable the feature, add [*.]yandex.ru to Always clear cookies when windows are closed
  2. open internet.ya.ru, note the yandexuid cookie value
  3. open any site with yandex scripts, e.g. ngs.ru. By default, 3rdparty cookies are blocked
  4. Disable shields on ngs.ru, check yandexuid. ATM the value differs from 1p cookie set by internet.ya.ru (which is not the case when the feature is off)
  5. Also check the cookies view in omnibox -> lock icon -> cookies. In this scenario blocked tab has a bunch of allowed cookies, which does't look right

@goodov goodov force-pushed the issues/15906-1pes branch from 3a9177c to 1b26f9d Compare August 23, 2021 12:17
@goodov goodov force-pushed the issues/15906-1pes branch from 1b26f9d to eed3a97 Compare August 30, 2021 14:40
@goodov goodov force-pushed the issues/15906-1pes branch 2 times, most recently from 2da1b2a to 9e1eb2c Compare September 10, 2021 13:52
@goodov
Copy link
Member Author

goodov commented Sep 10, 2021

The current state is now close to production and it's okay to review it.

@goodov
Copy link
Member Author

goodov commented Sep 13, 2021

Removed DevTools cookies display support in 1PES mode from this PR. Will add this as a separate thing.

@iefremov iefremov self-requested a review September 16, 2021 09:42
mariospr
mariospr reviewed Oct 1, 2021
View reviewed changes
Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patching/chromium_src-wise LGTM, but I very much agree with @iefremov on that all this code needs comments in several places as it's already quite hard to follow how everything fits together.

@goodov goodov force-pushed the issues/15906-1pes branch from d7bf804 to 88c0379 Compare October 1, 2021 13:19
@goodov goodov force-pushed the issues/15906-1pes branch from 88c0379 to ade6e48 Compare October 1, 2021 14:30
@iefremov
Copy link
Contributor

iefremov commented Oct 5, 2021

@mariospr PTAL

@goodov goodov force-pushed the issues/15906-1pes branch 2 times, most recently from 39c5ae9 to 5f19a68 Compare October 6, 2021 13:00
@goodov goodov force-pushed the issues/15906-1pes branch from 5f19a68 to 13eb934 Compare October 7, 2021 07:27
@goodov goodov force-pushed the issues/15906-1pes branch from 13eb934 to c159e2e Compare October 8, 2021 07:29
@goodov goodov merged commit 9a2161c into master Oct 8, 2021
@goodov goodov deleted the issues/15906-1pes branch October 8, 2021 12:55
@goodov goodov added this to the 1.32.x - Nightly milestone Oct 11, 2021
@goodov goodov mentioned this pull request Oct 26, 2021
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariospr mariospr mariospr approved these changes

@iefremov iefremov iefremov approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@goodov goodov

Labels
None yet
Projects
None yet
Milestone
1.32.x - Release
Development

Successfully merging this pull request may close these issues.

Implement 1p Ephemeral Storage Functionality (functionality)
3 participants
@goodov @iefremov @mariospr