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

cathiechen · 2020-11-20T08:01:28Z

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:

mrobinson · 2020-12-09T11:47:39Z

This PR will need to be rebased and also include support for network cookies and document.cookie.

Instead of using ephemeral storage for all unblocked third-party frame storage, only use ephemeral storage when third-party storage is blocked. This means that turning on the ephemeral storage flag will always replaced blocked storage in third-party frames with an ephemeral version, regardless of other settings.

cathiechen · 2020-12-18T17:22:05Z

Hi @bridiver @iefremov @mrobinson, I think this patch is ready to review now.
Sorry, somehow I'm not able to add reviewers to this PR.

bridiver · 2020-12-18T18:38:11Z

chromium_src/third_party/blink/renderer/modules/storage/storage_controller.h

+
+#define BRAVE_STORAGE_CONTROLLER_H                         \
+ public:                                                   \
+  static bool CanAccessStorageAreaWithoutEphemeralStorage( \


why is this a static method on StorageController? I can't see any reason for it to be part of StorageController and seems like it would work just as well as an anonymous method added to dom_window_storage.cc

Hmm, IIUC, CanAccessStorageAreaWithoutEphemeralStorage replaces the original CanAccessStorageArea in storage_controller.h. And the current CanAccessStorageArea returns true if cookie allowed or ephemeral flag enabled.
It seems we can't move this to dom_window_storage.cc, for the StorageArea::CanAccessStorage() needs it.

then it can just be a regular method and not a static method on a class that requires a patch

OK, this change has been removed in the new patch.

bridiver · 2020-12-18T18:38:23Z

patches/third_party-blink-renderer-modules-storage-storage_controller.h.patch

+     return task_runner_;
+   }
+
+  BRAVE_STORAGE_CONTROLLER_H


see https://github.com/brave/brave-core/pull/7207/files#r546017684

bridiver · 2020-12-18T18:41:47Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

+  }
+};
+
+IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,


you have two tests with the same name ThirdPartyCookiesEnabled

They are distinguished by the test class names (EphemeralStorageBrowserTest and EphemeralStorageDisabledBrowserTest). So do you prefer to point out the ephemeral flag values explicitly in the test names?

bridiver · 2020-12-18T18:45:23Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

+  EXPECT_EQ("from=b.com", site_a_tab_values.iframe_2.cookies);
+}
+
+IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,


I don't know what "NavigateCookies" is supposed to mean here or "Enabled". Does "Enabled" mean the setting is enabled or that third party cookies are allowed? Using "Blocked" or "Allowed" I think is more useful here. Also it's helpful if you describe the behavior like "UseEphemeralStorageWhenThirdPartyCookiesAreBlocked"

Done, thanks

mrobinson · 2020-12-18T18:45:36Z

patches/third_party-blink-renderer-modules-storage-storage_controller.h.patch

+     return task_runner_;
+   }
+
+  BRAVE_STORAGE_CONTROLLER_H


@bridiver suggested that this might be able to be removed entirely by just moving CanAccessStorageAreaWithoutEphemeralStorage into chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc.

Made an explanation in https://github.com/brave/brave-core/pull/7207/files#r546193351

bridiver · 2020-12-18T18:45:43Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

+  EXPECT_EQ("from=b.com", site_a_tab_values.iframe_2.cookies);
+}
+
+IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,


this also looks like a duplicate test name

bridiver · 2020-12-18T18:46:44Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

@@ -554,3 +584,212 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
  EXPECT_EQ("third-party-a.com", third_party_values.session_storage);
  EXPECT_EQ("name=third-party-a.com", third_party_values.cookies);
 }
+


you also need to test shield and chromium site overrides for block all and block 3p based on the discussion we had with @pes10k about that behavior

also you don't have any tests for BlockAll and I'm fairly certain they won't work correctly with these changes

So all storage should not be accessible when BlockAll cookies, no matter ephemeral flags on or off?
OK, will do.

bridiver · 2020-12-18T18:59:45Z

patches/net-url_request-url_request_http_job.cc.patch

+
+   bool can_get_cookies =
+       (request_info_.privacy_mode == PRIVACY_MODE_DISABLED && CanGetCookies());
+  BRAVE_SETCOOKIEHEADERANDSTART


this patch can probably be eliminated by changing the subclass of URLRequestHttpJob and overriding CanGetCookies

CanGetCookies probably cannot be overridden since it isn't virtual, but I guess it's possible to create a new version of this function or a new method, perhaps with the preprocessor.

It seems that putting this in url_request_http_job.cc is sufficient to remove the patch.

+#define CanGetCookies() \ + (CanGetCookies() || CanUseEphemeralStorage(this)) +

Then, of course, #undef CanGetCookies at the end of the file.

@mrobinson that would also work, but I think you still have the problem discussed in slack of differentiating between block 3p and block all

@mrobinson that is a good idea though and I think we can still use it per the dm discussion with || true

…ges including ephemeral ones

cathiechen · 2020-12-22T17:01:14Z

Hi,
I think this patch is ready for review.
The latest commit makes all the accessible checking relying on the interfaces in CookieSettingsBase which support block all cookies setting. But this seems to lead to more changes for providing the interfaces.

PTAL, thanks!

cathiechen · 2021-01-05T12:31:26Z

Hi,
Let me explain the new patch a bit.
This patch provides two access interfaces for dom storage, navigated cookie and js cookie to check if the storage is accessible for an url and which kind of storge it should use.
All the interfaces will eventually call CookieSettingsBase which provides IsCookieAccessOrEphemeralCookiesAccessAllowed (It contains the ephemeral settings and cookie settings. And returns false if cookie settings is block_all. It determines whether an url is accessible to storage) and IsCookieAccessAllowed (Only contains cookie settings. It determines which storage should be used, normal storage or ephemeral storage).
To use the interfaces in cookie_settings_base.h, we need to add extra interfaces for each storage services.

Dom storage: In order to use the interfaces in DOMWindowStorage, we need to add interfaces in WebContentSettingsClient, BraveContentSettingsAgentImpl, content_settings_manager.mojom, and ContentSettingsManagerImpl. Then they eventually call interfaces in CookieSettingsBase.
The navigated cookies: To use them in URLRequestHttpJob, we need to add interfaces in NetworkDelegate, NetworkServiceNetworkDelegate which calls interfaces in CookieSettingsBase.
The js cookies: It changes RestrictedCookieManager.

bridiver · 2021-01-19T01:51:05Z

closed in favor of #7647

cathiechen requested a review from a team as a code owner November 20, 2020 08:01

mihaiplesa assigned cathiechen Dec 9, 2020

mrobinson marked this pull request as draft December 9, 2020 11:45

mrobinson changed the title ~~Ephemeral dom storage flag~~ Only use ephemeral storage when storage would otherwise be blocked Dec 9, 2020

cathiechen force-pushed the ephemeral-dom-storage-flag branch from e28d018 to 3ab6e3e Compare December 17, 2020 18:19

cathiechen mentioned this pull request Dec 18, 2020

Brave ephemeral storage flag #7473

Closed

23 tasks

cathiechen force-pushed the ephemeral-dom-storage-flag branch from 98c6a1e to 34d0708 Compare December 18, 2020 17:12

cathiechen marked this pull request as ready for review December 18, 2020 17:18

mrobinson self-requested a review December 18, 2020 17:24

bridiver reviewed Dec 18, 2020

View reviewed changes

mrobinson reviewed Dec 18, 2020

View reviewed changes

bridiver reviewed Dec 18, 2020

View reviewed changes

cathiechen added 2 commits December 21, 2020 21:59

Fixed code review comments

7203075

If it blocks all cookies, third party pages don't access to any stora…

160fa33

…ges including ephemeral ones

bridiver closed this Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cathiechen commented Nov 20, 2020 •

edited by mrobinson

Loading

mrobinson commented Dec 9, 2020

cathiechen commented Dec 18, 2020

bridiver Dec 18, 2020

cathiechen Dec 19, 2020

bridiver Dec 30, 2020

cathiechen Jan 5, 2021

bridiver Dec 18, 2020

bridiver Dec 18, 2020

cathiechen Dec 19, 2020

bridiver Dec 18, 2020

cathiechen Dec 19, 2020

mrobinson Dec 18, 2020

cathiechen Dec 19, 2020

bridiver Dec 18, 2020

bridiver Dec 18, 2020

bridiver Dec 18, 2020

cathiechen Dec 19, 2020

bridiver Dec 18, 2020

mrobinson Dec 18, 2020

mrobinson Dec 18, 2020

bridiver Dec 18, 2020

bridiver Dec 18, 2020

cathiechen Dec 21, 2020

cathiechen commented Dec 22, 2020

cathiechen commented Jan 5, 2021

bridiver commented Jan 19, 2021

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

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

Conversation

cathiechen commented Nov 20, 2020 • edited by mrobinson Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

mrobinson commented Dec 9, 2020

cathiechen commented Dec 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

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

cathiechen commented Dec 22, 2020

cathiechen commented Jan 5, 2021

bridiver commented Jan 19, 2021

cathiechen commented Nov 20, 2020 •

edited by mrobinson

Loading