Skip to content

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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34d0708
Only activate ephemeral storage for otherwise blocked storage
mrobinson Dec 9, 2020
7203075
Fixed code review comments
cathiechen Dec 21, 2020
160fa33
If it blocks all cookies, third party pages don't access to any stora…
cathiechen Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
267 changes: 253 additions & 14 deletions browser/ephemeral_storage/ephemeral_storage_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,30 @@ content::EvalJsResult GetCookiesInFrame(RenderFrameHost* host) {
return content::EvalJs(host, "document.cookie");
}

content::EvalJsResult GetStorageInFrame(RenderFrameHost* host,
StorageType storage_type) {
std::string script = base::StringPrintf("%sStorage;", ToString(storage_type));
return content::EvalJs(host, script);
}

void AssertEmptyInFrame(RenderFrameHost* frame) {
EXPECT_EQ(nullptr, GetStorageInFrame(frame, StorageType::Local));
EXPECT_EQ(nullptr, GetStorageInFrame(frame, StorageType::Session));
EXPECT_EQ("", GetCookiesInFrame(frame));
}

void AssertEmptyInSubframes(WebContents* web_contents) {
RenderFrameHost* main_frame = web_contents->GetMainFrame();
AssertEmptyInFrame(content::ChildFrameAt(main_frame, 0));
AssertEmptyInFrame(content::ChildFrameAt(main_frame, 1));
}

} // namespace

class EphemeralStorageBrowserTest : public InProcessBrowserTest {
class EphemeralStorageBaseBrowserTest : public InProcessBrowserTest {
public:
EphemeralStorageBrowserTest()
: https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
scoped_feature_list_.InitAndEnableFeature(
net::features::kBraveEphemeralStorage);
}
EphemeralStorageBaseBrowserTest()
: https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}

void SetUpOnMainThread() override {
InProcessBrowserTest::SetUpOnMainThread();
Expand Down Expand Up @@ -116,6 +131,13 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest {
content_settings, brave_shields::ControlType::ALLOW, GURL());
}

void BlockThirdPartyCookies() {
auto* content_settings =
HostContentSettingsMapFactory::GetForProfile(browser()->profile());
brave_shields::SetCookieControlType(
content_settings, brave_shields::ControlType::BLOCK_THIRD_PARTY, GURL());
}

void SetValuesInFrame(RenderFrameHost* frame,
std::string storage_value,
std::string cookie_value) {
Expand Down Expand Up @@ -180,11 +202,19 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest {
GURL c_site_ephemeral_storage_url_;

private:
DISALLOW_COPY_AND_ASSIGN(EphemeralStorageBrowserTest);
DISALLOW_COPY_AND_ASSIGN(EphemeralStorageBaseBrowserTest);
};

class EphemeralStorageBrowserTest : public EphemeralStorageBaseBrowserTest {
public:
EphemeralStorageBrowserTest() {
scoped_feature_list_.InitAndEnableFeature(
net::features::kBraveEphemeralStorage);
}
};

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest, StorageIsPartitioned) {
AllowAllCookies();
BlockThirdPartyCookies();

WebContents* first_party_tab = LoadURLInNewTab(b_site_ephemeral_storage_url_);
WebContents* site_a_tab1 = LoadURLInNewTab(a_site_ephemeral_storage_url_);
Expand Down Expand Up @@ -263,7 +293,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest, StorageIsPartitioned) {

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
NavigatingClearsEphemeralStorage) {
AllowAllCookies();
BlockThirdPartyCookies();

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
Expand Down Expand Up @@ -303,7 +333,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
ClosingTabClearsEphemeralStorage) {
AllowAllCookies();
BlockThirdPartyCookies();

WebContents* site_a_tab = LoadURLInNewTab(a_site_ephemeral_storage_url_);
EXPECT_EQ(browser()->tab_strip_model()->count(), 2);
Expand Down Expand Up @@ -353,7 +383,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
ReloadDoesNotClearEphemeralStorage) {
AllowAllCookies();
BlockThirdPartyCookies();

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
Expand Down Expand Up @@ -392,7 +422,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
EphemeralStorageDoesNotLeakBetweenProfiles) {
AllowAllCookies();
BlockThirdPartyCookies();

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
Expand Down Expand Up @@ -457,7 +487,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
NavigationCookiesArePartitioned) {
AllowAllCookies();
BlockThirdPartyCookies();

GURL a_site_set_cookie_url = https_server_.GetURL(
"a.com", "/set-cookie?name=acom;path=/;SameSite=None;Secure");
Expand Down Expand Up @@ -510,7 +540,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
FirstPartyNestedInThirdParty) {
AllowAllCookies();
BlockThirdPartyCookies();

auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();

Expand Down Expand Up @@ -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);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest, ThirdPartyCookiesEnabled) {
AllowAllCookies();

ui_test_utils::NavigateToURL(browser(), b_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetValuesInFrames(web_contents, "b.com - first party", "from=b.com");

// The storage in the first-party iframes should still reflect the
// original value that was written in the non-ephemeral storage area.
ValuesFromFrames first_party_values = GetValuesFromFrames(web_contents);
EXPECT_EQ("b.com - first party", first_party_values.main_frame.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.local_storage);

EXPECT_EQ("b.com - first party",
first_party_values.main_frame.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.session_storage);

EXPECT_EQ("from=b.com", first_party_values.main_frame.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_1.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_2.cookies);

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* a_site_content = browser()->tab_strip_model()->GetActiveWebContents();

// If third-party cookies is enabled, site_a_tab should be able to access to
// non-ephemeral sotrages.
ValuesFromFrames site_a_tab_values = GetValuesFromFrames(a_site_content);
EXPECT_EQ(nullptr, site_a_tab_values.main_frame.local_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_1.local_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_2.local_storage);

EXPECT_EQ(nullptr, site_a_tab_values.main_frame.session_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_1.session_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_2.session_storage);

EXPECT_EQ("", site_a_tab_values.main_frame.cookies);
EXPECT_EQ("from=b.com", site_a_tab_values.iframe_1.cookies);
EXPECT_EQ("from=b.com", site_a_tab_values.iframe_2.cookies);
}

IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"

Copy link
Author

Choose a reason for hiding this comment

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

Done, thanks

ThirdPartyCookiesEnabledAndNavigateCookies) {
AllowAllCookies();

GURL b_site_set_cookie_url = https_server_.GetURL(
"b.com", "/set-cookie?name=bcom;path=/;SameSite=None;Secure");

ui_test_utils::NavigateToURL(browser(), b_site_set_cookie_url);
ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);

std::string a_cookie =
content::GetCookies(browser()->profile(), GURL("https://a.com/"));
std::string b_cookie =
content::GetCookies(browser()->profile(), GURL("https://b.com/"));
EXPECT_EQ("", a_cookie);
EXPECT_EQ("name=bcom", b_cookie);

// The third-party iframe should have the b.com cookie for third-party
// cookies is enabled.
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
RenderFrameHost* main_frame = web_contents->GetMainFrame();
RenderFrameHost* iframe_a = content::ChildFrameAt(main_frame, 0);
RenderFrameHost* iframe_b = content::ChildFrameAt(main_frame, 1);
ASSERT_EQ("", GetCookiesInFrame(main_frame));
ASSERT_EQ("name=bcom", GetCookiesInFrame(iframe_a));
ASSERT_EQ("name=bcom", GetCookiesInFrame(iframe_b));
}

class EphemeralStorageDisabledBrowserTest
: public EphemeralStorageBaseBrowserTest {
public:
EphemeralStorageDisabledBrowserTest() {
scoped_feature_list_.InitAndDisableFeature(
net::features::kBraveEphemeralStorage);
}
};

IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have two tests with the same name ThirdPartyCookiesEnabled

Copy link
Author

Choose a reason for hiding this comment

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

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?

ThirdPartyCookiesEnabled) {
AllowAllCookies();

ui_test_utils::NavigateToURL(browser(), b_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetValuesInFrames(web_contents, "b.com - first party", "from=b.com");

ValuesFromFrames first_party_values = GetValuesFromFrames(web_contents);
EXPECT_EQ("b.com - first party", first_party_values.main_frame.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.local_storage);

EXPECT_EQ("b.com - first party",
first_party_values.main_frame.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.session_storage);

EXPECT_EQ("from=b.com", first_party_values.main_frame.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_1.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_2.cookies);

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* a_site_content = browser()->tab_strip_model()->GetActiveWebContents();

// If third-party cookies is enabled, site_a_tab should be able to access to
// non-ephemeral sotrages.
ValuesFromFrames site_a_tab_values = GetValuesFromFrames(a_site_content);
EXPECT_EQ(nullptr, site_a_tab_values.main_frame.local_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_1.local_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_2.local_storage);

EXPECT_EQ(nullptr, site_a_tab_values.main_frame.session_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_1.session_storage);
EXPECT_EQ("b.com - first party", site_a_tab_values.iframe_2.session_storage);

EXPECT_EQ("", site_a_tab_values.main_frame.cookies);
EXPECT_EQ("from=b.com", site_a_tab_values.iframe_1.cookies);
EXPECT_EQ("from=b.com", site_a_tab_values.iframe_2.cookies);
}

IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this also looks like a duplicate test name

ThirdPartyCookiesEnabledAndNavigateCookies) {
AllowAllCookies();

GURL b_site_set_cookie_url = https_server_.GetURL(
"b.com", "/set-cookie?name=bcom;path=/;SameSite=None;Secure");

ui_test_utils::NavigateToURL(browser(), b_site_set_cookie_url);
ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);

std::string a_cookie =
content::GetCookies(browser()->profile(), GURL("https://a.com/"));
std::string b_cookie =
content::GetCookies(browser()->profile(), GURL("https://b.com/"));
EXPECT_EQ("", a_cookie);
EXPECT_EQ("name=bcom", b_cookie);

// The third-party iframe should have the b.com cookie for third-party
// cookies is enabled.
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
RenderFrameHost* main_frame = web_contents->GetMainFrame();
RenderFrameHost* iframe_a = content::ChildFrameAt(main_frame, 0);
RenderFrameHost* iframe_b = content::ChildFrameAt(main_frame, 1);
ASSERT_EQ("", GetCookiesInFrame(main_frame));
ASSERT_EQ("name=bcom", GetCookiesInFrame(iframe_a));
ASSERT_EQ("name=bcom", GetCookiesInFrame(iframe_b));
}

IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,
ThirdPartyCookiesDisabled) {
BlockThirdPartyCookies();

ui_test_utils::NavigateToURL(browser(), b_site_ephemeral_storage_url_);
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
// We set a value in the page where all the frames are first-party.
SetValuesInFrames(web_contents, "b.com - first party", "from=b.com");

ValuesFromFrames first_party_values = GetValuesFromFrames(web_contents);
EXPECT_EQ("b.com - first party", first_party_values.main_frame.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.local_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.local_storage);

EXPECT_EQ("b.com - first party",
first_party_values.main_frame.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_1.session_storage);
EXPECT_EQ("b.com - first party", first_party_values.iframe_2.session_storage);

EXPECT_EQ("from=b.com", first_party_values.main_frame.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_1.cookies);
EXPECT_EQ("from=b.com", first_party_values.iframe_2.cookies);

ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);
auto* a_site_content = browser()->tab_strip_model()->GetActiveWebContents();

// If both ephemeral storage and third-party cookies disabled, third-party
// frames can not access to any dom storage.
AssertEmptyInSubframes(a_site_content);
}

IN_PROC_BROWSER_TEST_F(EphemeralStorageDisabledBrowserTest,
ThirdPartyCookiesDisabledAndNavigateCookies) {
BlockThirdPartyCookies();

GURL b_site_set_cookie_url = https_server_.GetURL(
"b.com", "/set-cookie?name=bcom;path=/;SameSite=None;Secure");

ui_test_utils::NavigateToURL(browser(), b_site_set_cookie_url);
ui_test_utils::NavigateToURL(browser(), a_site_ephemeral_storage_url_);

std::string a_cookie =
content::GetCookies(browser()->profile(), GURL("https://a.com/"));
std::string b_cookie =
content::GetCookies(browser()->profile(), GURL("https://b.com/"));
EXPECT_EQ("", a_cookie);
EXPECT_EQ("name=bcom", b_cookie);

// The third-party iframe should be empty for third-party
// cookies is disabled.
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
RenderFrameHost* main_frame = web_contents->GetMainFrame();
RenderFrameHost* iframe_a = content::ChildFrameAt(main_frame, 0);
RenderFrameHost* iframe_b = content::ChildFrameAt(main_frame, 1);
ASSERT_EQ("", GetCookiesInFrame(main_frame));
ASSERT_EQ("", GetCookiesInFrame(iframe_a));
ASSERT_EQ("", GetCookiesInFrame(iframe_b));
}
14 changes: 14 additions & 0 deletions chromium_src/components/content_settings/core/common/cookie_settings_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "components/content_settings/core/common/content_settings.h"
#include "components/content_settings/core/common/content_settings_pattern.h"
#include "components/content_settings/core/common/features.h"
#include "net/base/features.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "url/gurl.h"
#include "url/origin.h"
Expand Down Expand Up @@ -171,6 +172,19 @@ bool CookieSettingsBase::IsCookieAccessAllowed(
return IsChromiumCookieAccessAllowed(url, site_for_cookies, top_frame_origin);
}

bool CookieSettingsBase::IsCookieAccessOrEphemeralCookiesAccessAllowed(
const GURL& url,
const GURL& site_for_cookies,
const base::Optional<url::Origin>& top_frame_origin) const {
if (IsCookieAccessAllowed(url, site_for_cookies, top_frame_origin))
return true;
if (!base::FeatureList::IsEnabled(net::features::kBraveEphemeralStorage))
return false;
if (!top_frame_origin.has_value())
return false;
return *top_frame_origin != url::Origin::Create(url);
}

} // namespace content_settings

#define IsCookieAccessAllowed IsChromiumCookieAccessAllowed
Expand Down
18 changes: 11 additions & 7 deletions chromium_src/components/content_settings/core/common/cookie_settings_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_CONTENT_SETTINGS_CORE_COMMON_COOKIE_SETTINGS_BASE_H_
#define BRAVE_CHROMIUM_SRC_COMPONENTS_CONTENT_SETTINGS_CORE_COMMON_COOKIE_SETTINGS_BASE_H_

#define BRAVE_COOKIE_SETTINGS_BASE_H \
private: \
bool IsChromiumCookieAccessAllowed(const GURL& url, \
const GURL& first_party_url) const; \
bool IsChromiumCookieAccessAllowed( \
const GURL& url, \
const GURL& site_for_cookies, \
#define BRAVE_COOKIE_SETTINGS_BASE_H \
private: \
bool IsChromiumCookieAccessAllowed(const GURL& url, \
const GURL& first_party_url) const; \
bool IsChromiumCookieAccessAllowed( \
const GURL& url, const GURL& site_for_cookies, \
const base::Optional<url::Origin>& top_frame_origin) const; \
\
public: \
bool IsCookieAccessOrEphemeralCookiesAccessAllowed( \
const GURL& url, const GURL& site_for_cookies, \
const base::Optional<url::Origin>& top_frame_origin) const;

#include "../../../../../../components/content_settings/core/common/cookie_settings_base.h"
Expand Down
Loading