From c37d6cab91f06db8d8a993eecd8dfc321614f3ee Mon Sep 17 00:00:00 2001 From: Shuran Huang Date: Wed, 5 Jul 2023 20:35:12 +0000 Subject: [PATCH] [Merge 116] Include unpartitioned cookie availability check in hasStorageAccess() This change is based on spec PR https://github.com/privacycg/storage-access/pull/174. (cherry picked from commit 120b35b8439d54fdc942184845285860dbc5d315) Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611289 Reviewed-by: Mason Freed Commit-Queue: Shuran Huang Reviewed-by: Chris Fredrickson Cr-Original-Commit-Position: refs/heads/main@{#1161766} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4665808 Auto-Submit: Shuran Huang Reviewed-by: Dominic Farolino Commit-Queue: Dominic Farolino Cr-Commit-Position: refs/branch-heads/5845@{#322} Cr-Branched-From: 5a5dff63a4a4c63b9b18589819bebb2566c85443-refs/heads/main@{#1160321} --- .../storage_access_api/api_browsertest.cc | 77 ++++++++++++++++--- .../blink/renderer/core/dom/document.cc | 5 +- .../hasStorageAccess.sub.https.window.js | 3 + .../hasStorageAccess-ABA-iframe.https.html | 2 + ...orageAccess-ABA-iframe.sub.https.window.js | 2 + .../hasStorageAccess-iframe.https.html | 2 + 6 files changed, 78 insertions(+), 13 deletions(-) diff --git a/chrome/browser/storage_access_api/api_browsertest.cc b/chrome/browser/storage_access_api/api_browsertest.cc index 62f043e40520cb..04147fa22c66c1 100644 --- a/chrome/browser/storage_access_api/api_browsertest.cc +++ b/chrome/browser/storage_access_api/api_browsertest.cc @@ -38,6 +38,7 @@ #include "components/policy/policy_constants.h" #include "components/prefs/pref_service.h" #include "content/public/browser/browser_context.h" +#include "content/public/browser/navigation_handle.h" #include "content/public/browser/storage_partition.h" #include "content/public/common/content_paths.h" #include "content/public/test/browser_task_environment.h" @@ -622,13 +623,12 @@ IN_PROC_BROWSER_TEST_F( EXPECT_EQ(ReadCookiesAndContent(GetFrame(), kHostB), CookieBundleWithContent("cross-site=b.test")); - // TODO(https://crbug.com/1441133): We should either make sure there is way to - // let developer check whether they need to call rSA(), or no prompt is shown - // when 3p cookie is allowed. - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); + EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame())); prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); EXPECT_TRUE(storage::test::RequestAndCheckStorageAccessForFrame(GetFrame())); + // TODO(https://crbug.com/1441133): No prompt should be shown when 3p cookie + // is allowed. EXPECT_EQ(1, prompt_factory()->TotalRequestCount()); } @@ -835,7 +835,10 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest, EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - EXPECT_TRUE(storage::test::RequestAndCheckStorageAccessForFrame(GetFrame())); + // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected + // when 3p cookie is blocked by user explicitly. + EXPECT_TRUE(content::ExecJs(GetFrame(), "document.requestStorageAccess()")); + EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); EXPECT_EQ(ReadCookies(GetFrame(), kHostB), NoCookies()); } @@ -857,9 +860,11 @@ IN_PROC_BROWSER_TEST_F( prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - + // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected + // when 3p cookie is blocked by user explicitly. EXPECT_TRUE( - storage::test::RequestAndCheckStorageAccessForFrame(GetNestedFrame())); + content::ExecJs(GetNestedFrame(), "document.requestStorageAccess()")); + EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame())); EXPECT_EQ(ReadCookies(GetNestedFrame(), kHostB), NoCookies()); } @@ -883,9 +888,11 @@ IN_PROC_BROWSER_TEST_F( prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - + // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected + // when 3p cookie is blocked by user explicitly. EXPECT_TRUE( - storage::test::RequestAndCheckStorageAccessForFrame(GetNestedFrame())); + content::ExecJs(GetNestedFrame(), "document.requestStorageAccess()")); + EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame())); EXPECT_EQ(ReadCookies(GetNestedFrame(), kHostC), NoCookies()); } @@ -1533,7 +1540,7 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIStorageBrowserTest, MultiTabTest) { storage::test::ExpectCrossTabInfoForFrame(GetFrame(), false); storage::test::SetCrossTabInfoForFrame(GetFrame()); storage::test::ExpectCrossTabInfoForFrame(GetFrame(), true); - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); + EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame())); // Create a second tab to test communication between tabs. NavigateToNewTabWithFrame(kHostA); @@ -1545,7 +1552,7 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIStorageBrowserTest, MultiTabTest) { permissions::PermissionRequestManager::ACCEPT_ALL); storage::test::ExpectCrossTabInfoForFrame(GetFrame(), true); - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); + EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame())); SetBlockThirdPartyCookies(true); @@ -1879,6 +1886,7 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIEnterprisePolicyBrowserTest, IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest, EnsureOnePromptDenialSuffices) { + SetBlockThirdPartyCookies(true); NavigateToPageWithFrame(kHostA); NavigateFrameTo(EchoCookiesURL(kHostB)); @@ -1912,6 +1920,7 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest, IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest, DismissalAllowsFuturePrompts) { + SetBlockThirdPartyCookies(true); NavigateToPageWithFrame(kHostA); NavigateFrameTo(EchoCookiesURL(kHostB)); @@ -2096,4 +2105,50 @@ IN_PROC_BROWSER_TEST_F( EXPECT_EQ(ReadCookies(GetFrame(), kHostA), CookieBundle("cross-site=a.test")); } +// Tests to verify that whether 3p cookie is already accessible is checked in +// hasStorageAccess. +class StorageAccessAPIWith3PCEnabledBrowserTest + : public StorageAccessAPIBaseBrowserTest { + public: + StorageAccessAPIWith3PCEnabledBrowserTest() + : StorageAccessAPIBaseBrowserTest(/*is_storage_partitioned=*/false) {} +}; + +IN_PROC_BROWSER_TEST_F(StorageAccessAPIWith3PCEnabledBrowserTest, + AllowedWhenUnblocked) { + SetBlockThirdPartyCookies(false); + + NavigateToPageWithFrame(kHostA); + NavigateFrameTo(EchoCookiesURL(kHostB)); + + EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame())); + + EXPECT_EQ(ReadCookiesAndContent(GetFrame(), kHostB), + CookieBundleWithContent("cross-site=b.test")); +} + +IN_PROC_BROWSER_TEST_F(StorageAccessAPIWith3PCEnabledBrowserTest, + AllowedByUserBypass) { + SetBlockThirdPartyCookies(true); + + NavigateToPageWithFrame(kHostA); + NavigateFrameTo(EchoCookiesURL(kHostB)); + EXPECT_EQ(ReadCookiesAndContent(GetFrame(), kHostB), NoCookiesWithContent()); + + EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); + + // Enable UserBypass on hostA as top-level. + CookieSettingsFactory::GetForProfile(browser()->profile()) + ->SetCookieSettingForUserBypass(GetURL(kHostA)); + + EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame())); + + NavigateToPageWithFrame(kHostA); + NavigateFrameTo(EchoCookiesURL(kHostB)); + EXPECT_EQ(ReadCookiesAndContent(GetFrame(), kHostB), + CookieBundleWithContent("cross-site=b.test")); +} + +// TODO(crbug.com/1448957): Add test cases of 3PC enabled by other mechanisms. + } // namespace diff --git a/third_party/blink/renderer/core/dom/document.cc b/third_party/blink/renderer/core/dom/document.cc index b3f3b88d764922..5e47f5d8e6e590 100644 --- a/third_party/blink/renderer/core/dom/document.cc +++ b/third_party/blink/renderer/core/dom/document.cc @@ -6272,8 +6272,9 @@ ScriptPromise Document::hasStorageAccess(ScriptState* script_state) { return true; } - // #9: return global's `has storage access`. - return dom_window_->HasStorageAccess(); + // #9 & #10: checks unpartitioned cookie availability with global's `has + // storage access`. + return CookiesEnabled(); }()); return promise; } diff --git a/third_party/blink/web_tests/external/wpt/storage-access-api/hasStorageAccess.sub.https.window.js b/third_party/blink/web_tests/external/wpt/storage-access-api/hasStorageAccess.sub.https.window.js index d7ed42baa0da9e..0efc687199a92c 100644 --- a/third_party/blink/web_tests/external/wpt/storage-access-api/hasStorageAccess.sub.https.window.js +++ b/third_party/blink/web_tests/external/wpt/storage-access-api/hasStorageAccess.sub.https.window.js @@ -1,4 +1,6 @@ // META: script=helpers.js +// META: script=/resources/testdriver.js +// META: script=/resources/testdriver-vendor.js 'use strict'; const {testPrefix, topLevelDocument} = processQueryParams(); @@ -9,6 +11,7 @@ promise_test(async () => { }, "[" + testPrefix + "] document.hasStorageAccess() should exist on the document interface"); promise_test(async () => { + await MaybeSetStorageAccess("*", "*", "blocked"); const hasAccess = await document.hasStorageAccess(); if (topLevelDocument || testPrefix.includes('same-origin')) { assert_true(hasAccess, "Access should be granted in top-level frame or iframe that is in first-party context by default."); diff --git a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.https.html b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.https.html index c9f23f02ac2e4f..fdceefc0ab83be 100644 --- a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.https.html +++ b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.https.html @@ -2,6 +2,8 @@ + + \ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.sub.https.window.js b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.sub.https.window.js index 126ae002010fa8..d6227ee47eba27 100644 --- a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.sub.https.window.js +++ b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-ABA-iframe.sub.https.window.js @@ -1,4 +1,6 @@ // META: script=../helpers.js +// META: script=/resources/testdriver.js +// META: script=/resources/testdriver-vendor.js 'use strict'; // This expects to be run in an iframe that is cross-site to the top-level frame. diff --git a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-iframe.https.html b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-iframe.https.html index 95169503c2a846..46194adcf80410 100644 --- a/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-iframe.https.html +++ b/third_party/blink/web_tests/external/wpt/storage-access-api/resources/hasStorageAccess-iframe.https.html @@ -2,6 +2,8 @@ + +