[DIPS] Only report stateful redirectors in inspector issues
This CL filters the sites reported by DIPS after a bounce happens to
only include those who also accessed cookies during the bounce (i.e.,
statefully bounced).
Bug: 1416548
Change-Id: Ia3e9488e8aedd9252411f43db76c9604460a4bdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4428292
Reviewed-by: Ryan Tarpine <rtarpine@chromium.org>
Commit-Queue: Joshua Hood <jdh@chromium.org>
Reviewed-by: Jonathan Njeunje <njeunje@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1140179}
diff --git a/chrome/browser/dips/dips_bounce_detector.cc b/chrome/browser/dips/dips_bounce_detector.cc
index 8b09c513..532c8c5 100644
--- a/chrome/browser/dips/dips_bounce_detector.cc
+++ b/chrome/browser/dips/dips_bounce_detector.cc
@@ -84,8 +84,8 @@
detector_(this,
base::DefaultTickClock::GetInstance(),
base::DefaultClock::GetInstance()) {
- issue_callback_ = base::BindRepeating(&DIPSWebContentsObserver::EmitDIPSIssue,
- weak_factory_.GetWeakPtr());
+ issue_reporting_callback_ = base::BindRepeating(
+ &DIPSWebContentsObserver::EmitDIPSIssue, weak_factory_.GetWeakPtr());
}
DIPSWebContentsObserver::~DIPSWebContentsObserver() = default;
@@ -105,6 +105,9 @@
redirect_context_(
base::BindRepeating(&DIPSBounceDetectorDelegate::HandleRedirectChain,
base::Unretained(delegate)),
+ base::BindRepeating(
+ &DIPSBounceDetectorDelegate::ReportRedirectorsWithoutInteraction,
+ base::Unretained(delegate)),
/*initial_url=*/GURL::EmptyGURL()),
client_bounce_detection_timer_(
FROM_HERE,
@@ -126,8 +129,11 @@
}
DIPSRedirectContext::DIPSRedirectContext(DIPSRedirectChainHandler handler,
+ DIPSIssueHandler issue_handler,
const GURL& initial_url)
- : handler_(handler), initial_url_(initial_url) {}
+ : handler_(handler),
+ issue_handler_(issue_handler),
+ initial_url_(initial_url) {}
DIPSRedirectContext::~DIPSRedirectContext() = default;
@@ -137,6 +143,9 @@
if (client_redirect->access_type > CookieAccessType::kNone) {
update_offset_ = redirects_.size();
}
+ if (client_redirect->access_type > CookieAccessType::kRead) {
+ redirectors_.insert(GetSiteForDIPS(client_redirect->url));
+ }
redirects_.push_back(std::move(client_redirect));
}
@@ -147,10 +156,29 @@
if (redirect->access_type > CookieAccessType::kNone) {
update_offset_ = redirects_.size();
}
+ if (redirect->access_type > CookieAccessType::kRead) {
+ redirectors_.insert(GetSiteForDIPS(redirect->url));
+ }
redirects_.push_back(std::move(redirect));
}
}
+void DIPSRedirectContext::ReportIssue(const GURL& final_url) {
+ // Since redirectors that are the same as the start or final page won't be
+ // acted on, we don't report on them.
+ //
+ // NOTE: This is not exactly right since the end of this navigation may not
+ // necessarily be the end of the chain, if a client redirect happens. However,
+ // this is better for developer experience than waiting until then, since
+ // notifications come faster.
+ redirectors_.erase(GetSiteForDIPS(initial_url_));
+ redirectors_.erase(GetSiteForDIPS(final_url));
+
+ issue_handler_.Run(std::move(redirectors_));
+
+ redirectors_.clear();
+}
+
void DIPSRedirectContext::HandleUncommitted(
DIPSNavigationStart navigation_start,
std::vector<DIPSRedirectInfoPtr> server_redirects,
@@ -163,9 +191,11 @@
// `DIPSRedirectContext`'s in-progress chain within the temp
// `DIPSRedirectContext`, whilst leaving *this*
// `DIPSRedirectContext`'s in-progress chain unchanged.
- DIPSRedirectContext temp_context(handler_, initial_url_);
+ DIPSRedirectContext temp_context(handler_, issue_handler_,
+ initial_url_);
temp_context.AppendClientRedirect(std::move(client_redirect));
temp_context.AppendServerRedirects(std::move(server_redirects));
+ temp_context.ReportIssue(final_url);
temp_context.EndChain(std::move(final_url));
},
[&](GURL previous_nav_last_committed_url) {
@@ -173,9 +203,10 @@
// a new redirect chain within a new `DIPSRedirectContext` and
// process it immediately (the in-progress chain in *this*
// `DIPSRedirectContext` is irrelevant).
- DIPSRedirectContext temp_context(handler_,
+ DIPSRedirectContext temp_context(handler_, issue_handler_,
previous_nav_last_committed_url);
temp_context.AppendServerRedirects(std::move(server_redirects));
+ temp_context.ReportIssue(final_url);
temp_context.EndChain(std::move(final_url));
},
},
@@ -184,7 +215,8 @@
void DIPSRedirectContext::AppendCommitted(
DIPSNavigationStart navigation_start,
- std::vector<DIPSRedirectInfoPtr> server_redirects) {
+ std::vector<DIPSRedirectInfoPtr> server_redirects,
+ const GURL& final_url) {
// If there was a client-side redirect before
// `DIPSBounceDetector::client_bounce_detection_timer_` timedout, grow the
// chain. Otherwise, end it.
@@ -205,6 +237,7 @@
// Server-side redirects always grow the chain.
AppendServerRedirects(std::move(server_redirects));
+ ReportIssue(final_url);
}
void DIPSRedirectContext::EndChain(GURL final_url) {
@@ -229,6 +262,12 @@
if (redirects_[update_offset_]->url == url) {
redirects_[update_offset_]->access_type =
redirects_[update_offset_]->access_type | ToCookieAccessType(op);
+
+ // This cookie access might indicate a stateful bounce and ideally we'd
+ // report an issue to notify the user, but the navigation already
+ // committed and any relevant notifications were already emitted, so it's
+ // too late.
+
return true;
}
@@ -271,7 +310,7 @@
dips_service_->storage()
->AsyncCall(&DIPSStorage::FilterSitesWithoutInteraction)
.WithArgs(sites)
- .Then(issue_callback_);
+ .Then(issue_reporting_callback_);
}
void DIPSWebContentsObserver::RecordEvent(DIPSRecordedEvent event,
@@ -519,12 +558,10 @@
/*time=*/clock_->Now()));
}
- delegate_->ReportRedirectorsWithoutInteraction(
- GetRedirectors(server_state->navigation_start, navigation_handle));
-
if (navigation_handle->HasCommitted()) {
redirect_context_.AppendCommitted(std::move(server_state->navigation_start),
- std::move(redirects));
+ std::move(redirects),
+ navigation_handle->GetURL());
} else {
redirect_context_.HandleUncommitted(
std::move(server_state->navigation_start), std::move(redirects),
@@ -580,47 +617,6 @@
(now - last_time.value()) >= kTimestampUpdateInterval);
}
-std::set<std::string> DIPSBounceDetector::GetRedirectors(
- const DIPSNavigationStart& navigation_start,
- DIPSNavigationHandle* navigation_handle) {
- std::set<std::string> redirectors;
- std::string initial_site;
-
- absl::visit( //
- base::Overloaded{
- [&](const DIPSRedirectInfoPtr& client_redirect) {
- initial_site = GetSiteForDIPS(redirect_context_.GetInitialURL());
- // If the navigation started with a client redirect,
- // `navigation_start` will be a DIPSRedirectInfoPtr and
- // we'll include that redirector as well.
- redirectors.insert(GetSiteForDIPS(client_redirect->url));
- },
- [&](const GURL& client_url) {
- initial_site = GetSiteForDIPS(client_url);
- },
- },
- navigation_start);
-
- const auto& redirect_chain = navigation_handle->GetRedirectChain();
- // The last site in the chain is the destination page, so it is ignored here.
- for (auto it = redirect_chain.begin(); it != (redirect_chain.end() - 1);
- it++) {
- redirectors.insert(GetSiteForDIPS(*it));
- }
-
- // Since redirectors that are the same as the start or final page won't be
- // acted on, we don't report on them.
- //
- // NOTE: This is not exactly right since the end of this navigation may not
- // necessarily be the end of the chain, if a client redirect happens. However,
- // this is better for developer experience than waiting until then, since
- // notifications come faster.
- redirectors.erase(initial_site);
- redirectors.erase(GetSiteForDIPS(redirect_chain.back()));
-
- return redirectors;
-}
-
void DIPSWebContentsObserver::WebContentsDestroyed() {
detector_.BeforeDestruction();
}
diff --git a/chrome/browser/dips/dips_bounce_detector.h b/chrome/browser/dips/dips_bounce_detector.h
index 163e542..ca62741 100644
--- a/chrome/browser/dips/dips_bounce_detector.h
+++ b/chrome/browser/dips/dips_bounce_detector.h
@@ -33,6 +33,11 @@
class TickClock;
} // namespace base
+using DIPSIssueHandler =
+ base::RepeatingCallback<void(const std::set<std::string>& sites)>;
+using DIPSIssueReportingCallback =
+ base::RepeatingCallback<void(const std::set<std::string>& sites)>;
+
// ClientBounceDetectionState is owned by the DIPSBounceDetector and stores
// data needed to detect stateful client-side redirects.
class ClientBounceDetectionState {
@@ -62,6 +67,7 @@
class DIPSRedirectContext {
public:
DIPSRedirectContext(DIPSRedirectChainHandler handler,
+ DIPSIssueHandler issue_handler,
const GURL& initial_url);
~DIPSRedirectContext();
@@ -76,7 +82,8 @@
// start of a new one, or extends it, according to the value of
// `navigation_start`.
void AppendCommitted(DIPSNavigationStart navigation_start,
- std::vector<DIPSRedirectInfoPtr> server_redirects);
+ std::vector<DIPSRedirectInfoPtr> server_redirects,
+ const GURL& final_url);
// Terminates the in-progress redirect chain, ending it with `final_url`, and
// parsing it to the `DIPSRedirectChainHandler` iff the chain is valid. It
@@ -85,6 +92,8 @@
// NOTE: A chain is valid if it has a non-empty `initial_url_`.
void EndChain(GURL final_url);
+ void ReportIssue(const GURL& final_url);
+
[[nodiscard]] bool AddLateCookieAccess(GURL url, CookieOperation op);
size_t size() const { return redirects_.size(); }
@@ -100,18 +109,17 @@
void AppendServerRedirects(std::vector<DIPSRedirectInfoPtr> server_redirects);
DIPSRedirectChainHandler handler_;
+ DIPSIssueHandler issue_handler_;
// Represents the start of a chain and also indicates the presence of a valid
// chain.
GURL initial_url_;
std::vector<DIPSRedirectInfoPtr> redirects_;
+ std::set<std::string> redirectors_;
// The index of the last redirect to have a known cookie access. When adding
// late cookie accesses, we only consider redirects from this offset onwards.
size_t update_offset_ = 0;
};
-using DIPSIssueCallback =
- base::RepeatingCallback<void(const std::set<std::string>& sites)>;
-
// A simplified interface to WebContents and DIPSService that can be faked in
// tests. Needed to allow unit testing DIPSBounceDetector.
class DIPSBounceDetectorDelegate {
@@ -221,13 +229,6 @@
bool ShouldUpdateTimestamp(base::optional_ref<const base::Time> last_time,
base::Time now);
- // Returns the set of sites in the current (server) redirect chain. If the
- // navigation started with a client redirect, that site is also included.
- // Redirectors matching the initial or end site are omitted.
- std::set<std::string> GetRedirectors(
- const DIPSNavigationStart& navigation_start,
- DIPSNavigationHandle* navigation_handle);
-
raw_ptr<const base::TickClock> tick_clock_;
raw_ptr<const base::Clock> clock_;
raw_ptr<DIPSBounceDetectorDelegate> delegate_;
@@ -251,8 +252,9 @@
}
// Use the passed handler instead of DIPSWebContentsObserver::EmitDIPSIssue().
- void SetIssueReportingCallbackForTesting(DIPSIssueCallback callback) {
- issue_callback_ = callback;
+ void SetIssueReportingCallbackForTesting(
+ DIPSIssueReportingCallback callback) {
+ issue_reporting_callback_ = callback;
}
void SetClockForTesting(base::Clock* clock) {
@@ -300,7 +302,7 @@
// DIPSWebContentsObserver is observing.
raw_ptr<DIPSService> dips_service_;
DIPSBounceDetector detector_;
- DIPSIssueCallback issue_callback_;
+ DIPSIssueReportingCallback issue_reporting_callback_;
base::WeakPtrFactory<DIPSWebContentsObserver> weak_factory_{this};
diff --git a/chrome/browser/dips/dips_bounce_detector_browsertest.cc b/chrome/browser/dips/dips_bounce_detector_browsertest.cc
index 5ce7fec0..0c5befb 100644
--- a/chrome/browser/dips/dips_bounce_detector_browsertest.cc
+++ b/chrome/browser/dips/dips_bounce_detector_browsertest.cc
@@ -40,6 +40,7 @@
#endif
using base::Bucket;
+using content::CookieAccessDetails;
using content::NavigationHandle;
using content::WebContents;
using testing::ElementsAre;
@@ -133,8 +134,7 @@
log_.push_back(base::StringPrintf(
"OnCookiesAccessed(RenderFrameHost, %s: %s)",
- details.type == content::CookieAccessDetails::Type::kChange ? "Change"
- : "Read",
+ details.type == CookieOperation::kChange ? "Change" : "Read",
FormatURL(details.url).c_str()));
}
@@ -143,8 +143,7 @@
const content::CookieAccessDetails& details) {
log_.push_back(base::StringPrintf(
"OnCookiesAccessed(NavigationHandle, %s: %s)",
- details.type == content::CookieAccessDetails::Type::kChange ? "Change"
- : "Read",
+ details.type == CookieOperation::kChange ? "Change" : "Read",
FormatURL(details.url).c_str()));
}
@@ -245,7 +244,7 @@
const auto url =
embedded_test_server()->GetURL(host, "/set-cookie?name=value");
URLCookieAccessObserver observer(web_contents, url,
- URLCookieAccessObserver::Type::kChange);
+ CookieOperation::kChange);
bool success = content::NavigateToURL(web_contents, url);
if (success) {
observer.Wait();
@@ -256,7 +255,7 @@
void CreateImageAndWaitForCookieAccess(const GURL& image_url) {
WebContents* web_contents = GetActiveWebContents();
URLCookieAccessObserver observer(web_contents, image_url,
- URLCookieAccessObserver::Type::kRead);
+ CookieOperation::kRead);
ASSERT_TRUE(content::ExecJs(web_contents,
content::JsReplace(
R"(
@@ -323,7 +322,7 @@
// Visit the redirect.
URLCookieAccessObserver observer(web_contents, final_url,
- URLCookieAccessObserver::Type::kChange);
+ CookieOperation::kChange);
ASSERT_TRUE(content::NavigateToURL(web_contents, redirect_url, final_url));
observer.Wait();
@@ -458,24 +457,42 @@
ASSERT_TRUE(content::NavigateToURL(
web_contents, embedded_test_server()->GetURL("a.test", "/title1.html")));
- // Navigate with a click (not a redirect) to b.test, which S-redirects to
- // c.test.
+ // Navigate with a click (not a redirect) to b.test, which statefully
+ // S-redirects to c.test.
ASSERT_TRUE(content::NavigateToURLFromRenderer(
web_contents,
- embedded_test_server()->GetURL("b.test",
- "/cross-site/c.test/title1.html"),
+ embedded_test_server()->GetURL(
+ "b.test", "/cross-site-with-cookie/c.test/title1.html"),
embedded_test_server()->GetURL("c.test", "/title1.html")));
+ // Write a cookie via JS on c.test.
+ content::RenderFrameHost* frame = web_contents->GetPrimaryMainFrame();
+ FrameCookieAccessObserver c_cookie_observer(web_contents, frame,
+ CookieOperation::kChange);
+ ASSERT_TRUE(content::ExecJs(frame, "document.cookie = 'foo=bar';",
+ content::EXECUTE_SCRIPT_NO_USER_GESTURE));
+ c_cookie_observer.Wait();
+
// Navigate without a click (i.e. by C-redirecting) to d.test.
ASSERT_TRUE(content::NavigateToURLFromRendererWithoutUserGesture(
web_contents, embedded_test_server()->GetURL("d.test", "/title1.html")));
+ // Write a cookie via JS on d.test.
+ frame = web_contents->GetPrimaryMainFrame();
+ FrameCookieAccessObserver d_cookie_observer(web_contents, frame,
+ CookieOperation::kChange);
+ ASSERT_TRUE(content::ExecJs(frame, "document.cookie = 'foo=bar';",
+ content::EXECUTE_SCRIPT_NO_USER_GESTURE));
+ d_cookie_observer.Wait();
+
// Navigate without a click (i.e. by C-redirecting) to e.test, which
- // S-redirects to f.test, which S-redirects to g.test.
+ // statefully S-redirects to f.test, which statefully S-redirects to g.test.
ASSERT_TRUE(content::NavigateToURLFromRendererWithoutUserGesture(
web_contents,
embedded_test_server()->GetURL(
- "e.test", "/cross-site/f.test/cross-site/g.test/title1.html"),
+ "e.test",
+ "/cross-site-with-cookie/f.test/cross-site-with-cookie/g.test/"
+ "title1.html"),
embedded_test_server()->GetURL("g.test", "/title1.html")));
EndRedirectChain();
BlockUntilHelperProcessesPendingRequests();
@@ -753,11 +770,19 @@
// c.test.
ASSERT_TRUE(content::NavigateToURLFromRenderer(
web_contents,
- embedded_test_server()->GetURL("b.test",
- "/cross-site/c.test/title1.html"),
+ embedded_test_server()->GetURL(
+ "b.test", "/cross-site-with-cookie/c.test/title1.html"),
embedded_test_server()->GetURL("c.test", "/title1.html")));
WaitForIssueAndCheckTrackingSites({"b.test"});
+ // Write a cookie via JS on c.test.
+ content::RenderFrameHost* frame = web_contents->GetPrimaryMainFrame();
+ FrameCookieAccessObserver cookie_observer(web_contents, frame,
+ CookieOperation::kChange);
+ ASSERT_TRUE(content::ExecJs(frame, "document.cookie = 'foo=bar';",
+ content::EXECUTE_SCRIPT_NO_USER_GESTURE));
+ cookie_observer.Wait();
+
// Navigate without a click (i.e. by C-redirecting) to d.test.
ASSERT_TRUE(content::NavigateToURLFromRendererWithoutUserGesture(
web_contents, embedded_test_server()->GetURL("d.test", "/title1.html")));
@@ -768,7 +793,11 @@
ASSERT_TRUE(content::NavigateToURLFromRendererWithoutUserGesture(
web_contents,
embedded_test_server()->GetURL(
- "e.test", "/cross-site/f.test/cross-site/g.test/title1.html"),
+ "e.test",
+ "/cross-site-with-cookie/f.test/cross-site-with-cookie/g.test/"
+ "title1.html"),
embedded_test_server()->GetURL("g.test", "/title1.html")));
- WaitForIssueAndCheckTrackingSites({"d.test", "e.test", "f.test"});
+ // Note d.test is not listed as a potentially tracking site since it did not
+ // write cookies before bouncing the user.
+ WaitForIssueAndCheckTrackingSites({"e.test", "f.test"});
}
diff --git a/chrome/browser/dips/dips_bounce_detector_unittest.cc b/chrome/browser/dips/dips_bounce_detector_unittest.cc
index 08c372ca..63cbfc2 100644
--- a/chrome/browser/dips/dips_bounce_detector_unittest.cc
+++ b/chrome/browser/dips/dips_bounce_detector_unittest.cc
@@ -7,6 +7,7 @@
#include <tuple>
#include "base/functional/bind.h"
+#include "base/functional/callback_helpers.h"
#include "base/strings/strcat.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/simple_test_clock.h"
@@ -587,23 +588,29 @@
TEST_F(DIPSBounceDetectorTest,
ReportRedirectorsInChain_OnEachFinishedNavigation) {
- // Visit initial page on a.test
+ // Visit initial page on a.test and access cookies via JS.
NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
// Navigate with a click (not a redirect) to b.test, which S-redirects to
- // c.test
+ // c.test.
StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
- // Navigate without a click (i.e. by C-redirecting) to d.test
+ // Navigate without a click (i.e. by C-redirecting) to d.test.
NavigateTo("http://d.test", kNoUserGesture);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test", "c.test"));
+ // Access cookies on d.test.
+ AccessClientCookie(CookieOperation::kChange);
// Navigate without a click (i.e. by C-redirecting) to e.test, which
- // S-redirects to f.test
+ // S-redirects to f.test.
StartNavigation("http://e.test", kNoUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://f.test")
.Finish(true);
EXPECT_THAT(GetReportedSites(),
@@ -612,50 +619,93 @@
TEST_F(DIPSBounceDetectorTest,
ReportRedirectorsInChain_IncludingUncommittedNavigations) {
- // Visit initial page on a.test
+ // Visit initial page on a.test and access cookies via JS.
NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
// Start a redirect chain that doesn't commit.
StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://d.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(false);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test, c.test"));
// Because the previous navigation didn't commit, the following chain still
// starts from http://a.test/.
StartNavigation("http://e.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://f.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(),
testing::ElementsAre("b.test, c.test", "e.test"));
}
+TEST_F(DIPSBounceDetectorTest,
+ ReportRedirectorsInChain_OmitNonStatefulRedirects) {
+ // Visit initial page on a.test and access cookies via JS.
+ NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
+
+ // Navigate with a click (not a redirect) to b.test, which S-redirects to
+ // c.test (which doesn't access cookies).
+ StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
+ .RedirectTo("http://c.test")
+ .Finish(true);
+ EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
+
+ // Navigate without a click (i.e. by C-redirecting) to d.test (which doesn't
+ // access cookies).
+ NavigateTo("http://d.test", kNoUserGesture);
+ EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
+
+ // Navigate without a click (i.e. by C-redirecting) to e.test, which
+ // S-redirects to f.test.
+ StartNavigation("http://e.test", kNoUserGesture)
+ .AccessCookie(CookieOperation::kChange)
+ .RedirectTo("http://f.test")
+ .Finish(true);
+ EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test", "e.test"));
+}
+
// This test verifies that sites in a redirect chain that are the same as the
// starting site (i.e., last site before the redirect chain started) are not
// reported.
TEST_F(DIPSBounceDetectorTest,
ReportRedirectorsInChain_OmitSitesMatchingStartSite) {
- // Visit initial page on a.test.
+ // Visit initial page on a.test and access cookies via JS.
NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
// Navigate with a click (not a redirect) to b.test, which S-redirects to
// a.test, which S-redirects to c.test.
StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://a.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
// Navigate without a click (i.e. by C-redirecting) to a.test.
NavigateTo("http://a.test", kNoUserGesture);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test", "c.test"));
+ // Access cookies via JS on a.test.
+ AccessClientCookie(CookieOperation::kChange);
// Navigate without a click (i.e. by C-redirecting) to d.test, which
// S-redirects to e.test, which S-redirects to f.test.
StartNavigation("http://d.test", kNoUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://e.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://f.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(),
testing::ElementsAre("b.test", "c.test", "d.test, e.test"));
@@ -665,26 +715,35 @@
// as the ending site of a navigation are not reported.
TEST_F(DIPSBounceDetectorTest,
ReportRedirectorsInChain_OmitSitesMatchingEndSite) {
- // Visit initial page on a.test.
+ // Visit initial page on a.test and access cookies via JS.
NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
// Navigate with a click (not a redirect) to b.test, which S-redirects to
// c.test, which S-redirects to c.test.
StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
// Navigate without a click (i.e. by C-redirecting) to d.test.
NavigateTo("http://d.test", kNoUserGesture);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test", "c.test"));
+ // Access cookies via JS on d.test.
+ AccessClientCookie(CookieOperation::kChange);
// Navigate without a click (i.e. by C-redirecting) to e.test, which
// S-redirects to f.test, which S-redirects to e.test.
StartNavigation("http://e.test", kNoUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://f.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://e.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(true);
EXPECT_THAT(GetReportedSites(),
testing::ElementsAre("b.test", "c.test", "d.test, f.test"));
@@ -692,14 +751,18 @@
TEST_F(DIPSBounceDetectorTest,
ReportRedirectorsInChain_OmitSitesMatchingEndSite_Uncommitted) {
- // Visit initial page on a.test.
+ // Visit initial page on a.test and access cookies via JS.
NavigateTo("http://a.test", kWithUserGesture);
+ AccessClientCookie(CookieOperation::kChange);
// Navigate with a click (not a redirect) to b.test, which S-redirects to
// c.test, which S-redirects to c.test.
StartNavigation("http://b.test", kWithUserGesture)
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.RedirectTo("http://c.test")
+ .AccessCookie(CookieOperation::kChange)
.Finish(false);
EXPECT_THAT(GetReportedSites(), testing::ElementsAre("b.test"));
@@ -1000,11 +1063,13 @@
TEST(DIPSRedirectContextTest, OneAppend) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
GURL("http://a.test/"),
- MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}));
+ MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}),
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
context.EndChain(GURL("http://d.test/"));
@@ -1019,14 +1084,17 @@
TEST(DIPSRedirectContextTest, TwoAppends_NoClientRedirect) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
GURL("http://a.test/"),
- MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}));
+ MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}),
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(GURL("http://d.test/"),
- MakeServerRedirects(0, {"http://e.test/"}));
+ MakeServerRedirects(0, {"http://e.test/"}),
+ GURL("http://f.test/"));
ASSERT_EQ(chains.size(), 1u);
context.EndChain(GURL("http://f.test/"));
@@ -1046,15 +1114,18 @@
TEST(DIPSRedirectContextTest, TwoAppends_WithClientRedirect) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
GURL("http://a.test/"),
- MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}));
+ MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}),
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
MakeClientRedirect(2, "http://d.test/"),
- MakeServerRedirects(3, {"http://e.test/", "http://f.test/"}));
+ MakeServerRedirects(3, {"http://e.test/", "http://f.test/"}),
+ GURL("http://g.test/"));
ASSERT_EQ(chains.size(), 0u);
context.EndChain(GURL("http://g.test/"));
@@ -1078,13 +1149,16 @@
TEST(DIPSRedirectContextTest, OnlyClientRedirects) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
- context.AppendCommitted(GURL("http://a.test/"), {});
+ context.AppendCommitted(GURL("http://a.test/"), {}, GURL("http://b.test/"));
ASSERT_EQ(chains.size(), 0u);
- context.AppendCommitted(MakeClientRedirect(0, "http://b.test/"), {});
+ context.AppendCommitted(MakeClientRedirect(0, "http://b.test/"), {},
+ GURL("http://c.test/"));
ASSERT_EQ(chains.size(), 0u);
- context.AppendCommitted(MakeClientRedirect(1, "http://c.test/"), {});
+ context.AppendCommitted(MakeClientRedirect(1, "http://c.test/"), {},
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
context.EndChain(GURL("http://d.test"));
@@ -1099,11 +1173,13 @@
TEST(DIPSRedirectContextTest, Uncommitted_NoClientRedirects) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
GURL("http://a.test/"),
- MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}));
+ MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}),
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
context.HandleUncommitted(
GURL("http://d.test/"),
@@ -1111,7 +1187,8 @@
GURL("http://g.test/"));
ASSERT_EQ(chains.size(), 1u);
context.AppendCommitted(GURL("http://h.test/"),
- MakeServerRedirects(0, {"http://i.test/"}));
+ MakeServerRedirects(0, {"http://i.test/"}),
+ GURL("http://j.test/"));
ASSERT_EQ(chains.size(), 2u);
context.EndChain(GURL("http://j.test/"));
@@ -1138,11 +1215,13 @@
TEST(DIPSRedirectContextTest, Uncommitted_IncludingClientRedirects) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
context.AppendCommitted(
GURL("http://a.test/"),
- MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}));
+ MakeServerRedirects(0, {"http://b.test/", "http://c.test/"}),
+ GURL("http://d.test/"));
ASSERT_EQ(chains.size(), 0u);
// Uncommitted navigation:
context.HandleUncommitted(
@@ -1151,7 +1230,8 @@
GURL("http://g.test/"));
ASSERT_EQ(chains.size(), 1u);
context.AppendCommitted(MakeClientRedirect(2, "http://h.test/"),
- MakeServerRedirects(3, {"http://i.test/"}));
+ MakeServerRedirects(3, {"http://i.test/"}),
+ GURL("http://j.test/"));
ASSERT_EQ(chains.size(), 1u);
context.EndChain(GURL("http://j.test/"));
@@ -1179,13 +1259,14 @@
TEST(DIPSRedirectContextTest, NoRedirects) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
ASSERT_EQ(chains.size(), 0u);
- context.AppendCommitted(GURL("http://a.test/"), {});
+ context.AppendCommitted(GURL("http://a.test/"), {}, GURL("http://b.test/"));
ASSERT_EQ(chains.size(), 0u);
- context.AppendCommitted(GURL("http://b.test/"), {});
+ context.AppendCommitted(GURL("http://b.test/"), {}, GURL("http://c.test/"));
ASSERT_EQ(chains.size(), 1u);
context.HandleUncommitted(GURL("http://c.test/"), {}, GURL("http://d.test/"));
@@ -1213,13 +1294,15 @@
TEST(DIPSRedirectContextTest, AddLateCookieAccess) {
std::vector<ChainPair> chains;
DIPSRedirectContext context(
- base::BindRepeating(AppendChainPair, std::ref(chains)), GURL());
+ base::BindRepeating(AppendChainPair, std::ref(chains)), base::DoNothing(),
+ GURL());
context.AppendCommitted(
GURL("http://a.test/"),
MakeServerRedirects(
0, {"http://b.test/", "http://c.test/", "http://d.test/"},
- CookieAccessType::kNone));
+ CookieAccessType::kNone),
+ GURL("http://e.test/"));
EXPECT_TRUE(context.AddLateCookieAccess(GURL("http://b.test/"),
CookieOperation::kChange));
@@ -1239,7 +1322,8 @@
context.AppendCommitted(
MakeClientRedirect(3, "http://e.test/", CookieAccessType::kNone),
MakeServerRedirects(4, {"http://f.test/", "http://g.test/"},
- CookieAccessType::kRead));
+ CookieAccessType::kRead),
+ GURL("http://h.test/"));
// This late "write" will be merged with the "read" already recorded.
EXPECT_TRUE(context.AddLateCookieAccess(GURL("http://g.test/"),
@@ -1247,7 +1331,8 @@
context.AppendCommitted(
MakeClientRedirect(6, "http://h.test/", CookieAccessType::kNone),
- MakeServerRedirects(7, {"http://i.test/"}, CookieAccessType::kRead));
+ MakeServerRedirects(7, {"http://i.test/"}, CookieAccessType::kRead),
+ GURL("http://j.test/"));
// Can't modify h.test since i.test already has a known cookie access.
EXPECT_FALSE(context.AddLateCookieAccess(GURL("http://h.test/"),
diff --git a/chrome/browser/dips/dips_helper_browsertest.cc b/chrome/browser/dips/dips_helper_browsertest.cc
index f73a2481..d2c3b3d 100644
--- a/chrome/browser/dips/dips_helper_browsertest.cc
+++ b/chrome/browser/dips/dips_helper_browsertest.cc
@@ -53,29 +53,6 @@
namespace {
-class FrameCookieAccessObserver : public content::WebContentsObserver {
- public:
- explicit FrameCookieAccessObserver(WebContents* web_contents,
- RenderFrameHost* render_frame_host)
- : WebContentsObserver(web_contents),
- render_frame_host_(render_frame_host) {}
-
- // Wait until the frame accesses cookies.
- void Wait() { run_loop_.Run(); }
-
- // WebContentsObserver override
- void OnCookiesAccessed(content::RenderFrameHost* render_frame_host,
- const content::CookieAccessDetails& details) override {
- if (render_frame_host_ == render_frame_host) {
- run_loop_.Quit();
- }
- }
-
- private:
- const raw_ptr<content::RenderFrameHost> render_frame_host_;
- base::RunLoop run_loop_;
-};
-
// Histogram names
constexpr char kTimeToInteraction[] =
"Privacy.DIPS.TimeFromStorageToInteraction.Standard";
@@ -339,7 +316,8 @@
// Write a cookie in the b.test iframe.
SetDIPSTime(time);
- FrameCookieAccessObserver observer(web_contents, iframe);
+ FrameCookieAccessObserver observer(web_contents, iframe,
+ CookieAccessDetails::Type::kChange);
ASSERT_TRUE(content::ExecJs(
iframe, "document.cookie = 'foo=bar; SameSite=None; Secure';",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
@@ -385,7 +363,8 @@
// b.test.
SetDIPSTime(time + base::Seconds(10));
FrameCookieAccessObserver observer(web_contents,
- web_contents->GetPrimaryMainFrame());
+ web_contents->GetPrimaryMainFrame(),
+ CookieAccessDetails::Type::kRead);
ASSERT_TRUE(content::ExecJs(web_contents,
content::JsReplace(
R"(
@@ -527,7 +506,8 @@
// Write a cookie now that the click has been handled.
SetDIPSTime(time + base::Seconds(10));
- FrameCookieAccessObserver cookie_observer(web_contents, frame);
+ FrameCookieAccessObserver cookie_observer(web_contents, frame,
+ CookieAccessDetails::Type::kChange);
ASSERT_TRUE(content::ExecJs(frame, "document.cookie = 'foo=bar';",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
cookie_observer.Wait();
@@ -629,7 +609,8 @@
// Write a cookie now that both clicks have been handled.
SetDIPSTime(time + DIPSBounceDetector::kTimestampUpdateInterval +
base::Seconds(10));
- FrameCookieAccessObserver cookie_observer(web_contents, frame);
+ FrameCookieAccessObserver cookie_observer(web_contents, frame,
+ CookieAccessDetails::Type::kChange);
ASSERT_TRUE(content::ExecJs(frame, "document.cookie = 'foo=bar';",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
cookie_observer.Wait();
diff --git a/chrome/browser/dips/dips_test_utils.cc b/chrome/browser/dips/dips_test_utils.cc
index 66a96ca..680ed0ef 100644
--- a/chrome/browser/dips/dips_test_utils.cc
+++ b/chrome/browser/dips/dips_test_utils.cc
@@ -17,7 +17,7 @@
URLCookieAccessObserver::URLCookieAccessObserver(WebContents* web_contents,
const GURL& url,
- Type access_type)
+ CookieOperation access_type)
: WebContentsObserver(web_contents), url_(url), access_type_(access_type) {}
void URLCookieAccessObserver::Wait() {
@@ -40,6 +40,26 @@
}
}
+FrameCookieAccessObserver::FrameCookieAccessObserver(
+ WebContents* web_contents,
+ RenderFrameHost* render_frame_host,
+ CookieOperation access_type)
+ : WebContentsObserver(web_contents),
+ render_frame_host_(render_frame_host),
+ access_type_(access_type) {}
+
+void FrameCookieAccessObserver::Wait() {
+ run_loop_.Run();
+}
+
+void FrameCookieAccessObserver::OnCookiesAccessed(
+ content::RenderFrameHost* render_frame_host,
+ const content::CookieAccessDetails& details) {
+ if (details.type == access_type_ && render_frame_host_ == render_frame_host) {
+ run_loop_.Quit();
+ }
+}
+
RedirectChainObserver::RedirectChainObserver(DIPSService* service,
GURL final_url)
: final_url_(std::move(final_url)) {
diff --git a/chrome/browser/dips/dips_test_utils.h b/chrome/browser/dips/dips_test_utils.h
index 120f7af..1d83b608 100644
--- a/chrome/browser/dips/dips_test_utils.h
+++ b/chrome/browser/dips/dips_test_utils.h
@@ -14,6 +14,7 @@
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/dips/dips_redirect_info.h"
#include "chrome/browser/dips/dips_service.h"
+#include "chrome/browser/dips/dips_utils.h"
#include "chrome/browser/profiles/profile_test_util.h"
#include "components/ukm/test_ukm_recorder.h"
#include "content/public/browser/cookie_access_details.h"
@@ -28,10 +29,9 @@
class URLCookieAccessObserver : public content::WebContentsObserver {
public:
- using Type = content::CookieAccessDetails::Type;
URLCookieAccessObserver(content::WebContents* web_contents,
const GURL& url,
- Type access_type);
+ CookieOperation access_type);
void Wait();
@@ -43,7 +43,27 @@
const content::CookieAccessDetails& details) override;
GURL url_;
- Type access_type_;
+ CookieOperation access_type_;
+ base::RunLoop run_loop_;
+};
+
+class FrameCookieAccessObserver : public content::WebContentsObserver {
+ public:
+ explicit FrameCookieAccessObserver(
+ content::WebContents* web_contents,
+ content::RenderFrameHost* render_frame_host,
+ CookieOperation access_type);
+
+ // Wait until the frame accesses cookies.
+ void Wait();
+
+ // WebContentsObserver override
+ void OnCookiesAccessed(content::RenderFrameHost* render_frame_host,
+ const content::CookieAccessDetails& details) override;
+
+ private:
+ const raw_ptr<content::RenderFrameHost> render_frame_host_;
+ CookieOperation access_type_;
base::RunLoop run_loop_;
};
