Skip to content

Commit

Permalink
Remove old cross-scheme same-site metrics
Browse files Browse the repository at this point in the history 
The UMAs
SameSiteDifferentSchemeResponse
SameSiteDifferentSchemeRequest

and the UKMs
SameSiteDifferentSchemeReqsponse
SameSiteDifferentSchemeRequest

do not capture the complete/correct information relating to cookies
that would be affected by schemeful same-site.

This CL obsoletes and removes the metrics logging code along with the
necessary framework code used to capture the metrics.


Bug: 1066231
Change-Id: Ia4c952626ac8fbdf19e6da348ea9f6ca61ee043e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151481
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Steven Bingler <bingler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760980}
  • Loading branch information
Steven Bingler authored and Commit Bot committed Apr 21, 2020
1 parent fea9d6a commit d4aa743
Show file tree
Hide file tree
Showing 17 changed files with 161 additions and 977 deletions.
37 changes: 1 addition & 36 deletions content/browser/devtools/devtools_instrumentation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -647,42 +647,7 @@ std::vector<blink::mojom::SameSiteCookieWarningReason> BuildWarningReasons(
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteUnspecifiedLaxAllowUnsafe);
}
if (status.HasWarningReason(
net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL)) {
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeSecureUrlMethodUnsafe);
}
if (status.HasWarningReason(net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL)) {
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeSecureUrlLax);
}
if (status.HasWarningReason(
net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL)) {
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeSecureUrlStrict);
}
if (status.HasWarningReason(
net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL)) {
warning_reasons.push_back(
blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeInsecureUrlMethodUnsafe);
}
if (status.HasWarningReason(
net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL)) {
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeInsecureUrlLax);
}
if (status.HasWarningReason(
net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL)) {
warning_reasons.push_back(blink::mojom::SameSiteCookieWarningReason::
WarnSameSiteCrossSchemeInsecureUrlStrict);
}

return warning_reasons;
}
} // namespace
Expand Down
102 changes: 0 additions & 102 deletions content/browser/storage_partition_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,11 @@
#include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "net/base/net_errors.h"
#include "net/cookies/canonical_cookie.h"
#include "net/cookies/cookie_options.h"
#include "net/cookies/cookie_util.h"
#include "net/http/http_auth_preferences.h"
#include "net/ssl/client_cert_store.h"
#include "net/url_request/url_request_context.h"
#include "ppapi/buildflags/buildflags.h"
#include "services/metrics/public/cpp/ukm_builders.h"
#include "services/network/public/cpp/cross_thread_pending_shared_url_loader_factory.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/mojom/cookie_manager.mojom.h"
Expand Down Expand Up @@ -529,71 +527,6 @@ void DeprecateSameSiteCookies(
}
}

int64_t CrossSchemeWarningToContextInt64(
net::CanonicalCookie::CookieInclusionStatus::WarningReason reason) {
// Convert from the status's WarningReason enum to a SameSiteCookieContext
// enum and cast to a int64_t for UKM. The UKMs are using the
// SameSiteCookieContext in order to match up with the UMAs which are
// recording similar information.
// TODO(https://crbug.com/1046456): Remove after deprecated.
net::CookieOptions::SameSiteCookieContext same_site_context;
switch (reason) {
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL:
same_site_context.set_context(
net::CookieOptions::SameSiteCookieContext::ContextType::
SAME_SITE_LAX_METHOD_UNSAFE);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
INSECURE_SECURE);
return same_site_context.ConvertToMetricsValue();
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL:
same_site_context.set_context(net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
INSECURE_SECURE);
return same_site_context.ConvertToMetricsValue();
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL:
same_site_context.set_context(net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_STRICT);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
INSECURE_SECURE);
return same_site_context.ConvertToMetricsValue();
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL:
same_site_context.set_context(
net::CookieOptions::SameSiteCookieContext::ContextType::
SAME_SITE_LAX_METHOD_UNSAFE);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
SECURE_INSECURE);
return same_site_context.ConvertToMetricsValue();
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL:
same_site_context.set_context(net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
SECURE_INSECURE);
return same_site_context.ConvertToMetricsValue();
case net::CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL:
same_site_context.set_context(net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_STRICT);
same_site_context.set_cross_schemeness(
net::CookieOptions::SameSiteCookieContext::CrossSchemeness::
SECURE_INSECURE);
return same_site_context.ConvertToMetricsValue();
default:
// Return invalid value if there is no cross-scheme warning.
return -1;
}
}

void ReportCookiesChangedOnUI(
std::vector<GlobalFrameRoutingId> destinations,
const GURL& url,
Expand Down Expand Up @@ -629,22 +562,6 @@ void ReportCookiesChangedOnUI(
web_contents->OnCookieChange(url, site_for_cookies.RepresentativeUrl(),
cookie_and_status.cookie,
/* blocked_by_policy =*/false);

// TODO(https://crbug.com/1046456): Remove after deprecated.
net::CanonicalCookie::CookieInclusionStatus::WarningReason
cross_scheme_warning;
if (cookie_and_status.status.HasCrossSchemeWarning(
&cross_scheme_warning)) {
ukm::SourceId source_id = static_cast<WebContentsImpl*>(web_contents)
->GetMainFrame()
->GetPageUkmSourceId();

int64_t context =
CrossSchemeWarningToContextInt64(cross_scheme_warning);
ukm::builders::SameSiteDifferentSchemeRequest(source_id)
.SetSameSiteContextWithSchemes(context)
.Record(ukm::UkmRecorder::Get());
}
}
}
}
Expand All @@ -665,15 +582,13 @@ void ReportCookiesReadOnUI(
}

net::CookieList accepted, blocked;
std::vector<net::CanonicalCookie::CookieInclusionStatus> accepted_status;
for (auto& cookie_and_status : cookie_list) {
if (cookie_and_status.status.HasExclusionReason(
net::CanonicalCookie::CookieInclusionStatus::
EXCLUDE_USER_PREFERENCES)) {
blocked.push_back(std::move(cookie_and_status.cookie));
} else if (cookie_and_status.status.IsInclude()) {
accepted.push_back(std::move(cookie_and_status.cookie));
accepted_status.push_back(std::move(cookie_and_status.status));
}
}

Expand All @@ -686,23 +601,6 @@ void ReportCookiesReadOnUI(
web_contents->OnCookiesRead(url, site_for_cookies.RepresentativeUrl(),
accepted,
/* blocked_by_policy =*/false);

// TODO(https://crbug.com/1046456): Remove after deprecated.
for (const auto& status : accepted_status) {
net::CanonicalCookie::CookieInclusionStatus::WarningReason
cross_scheme_warning;
if (status.HasCrossSchemeWarning(&cross_scheme_warning)) {
ukm::SourceId source_id = static_cast<WebContentsImpl*>(web_contents)
->GetMainFrame()
->GetPageUkmSourceId();

int64_t context =
CrossSchemeWarningToContextInt64(cross_scheme_warning);
ukm::builders::SameSiteDifferentSchemeResponse(source_id)
.SetSameSiteContextWithSchemes(context)
.Record(ukm::UkmRecorder::Get());
}
}
}
}

Expand Down
133 changes: 0 additions & 133 deletions net/cookies/canonical_cookie.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,29 +148,6 @@ void ApplySameSiteCookieWarningToStatus(
status->MaybeClearSameSiteWarning();
}

// This function is used to indicate if the same-site context of a cookie should
// recorded for the histograms SameSiteDifferentSchemeRequest and
// SameSiteDifferentSchemeResponse. It returns true if the context is
// cross-scheme but not cross-site and there is an effective same-site. It
// should be removed when the histrograms are removed.
// TODO(https://crbug.com/1066231)
bool ShouldLogCrossSchemeForHistograms(
const CookieOptions::SameSiteCookieContext& context,
const CookieEffectiveSameSite effective_same_site) {
bool correct_context =
context.cross_schemeness() !=
CookieOptions::SameSiteCookieContext::CrossSchemeness::NONE &&
context.context() !=
CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE;

bool correct_effective_same_site =
effective_same_site == CookieEffectiveSameSite::LAX_MODE ||
effective_same_site == CookieEffectiveSameSite::STRICT_MODE ||
effective_same_site == CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE;

return correct_context && correct_effective_same_site;
}

} // namespace

// Keep defaults here in sync with content/public/common/cookie_manager.mojom.
Expand Down Expand Up @@ -580,17 +557,6 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IncludeForRequestURL(
UMA_HISTOGRAM_ENUMERATION("Cookie.IncludedRequestEffectiveSameSite",
effective_same_site,
CookieEffectiveSameSite::COUNT);

if (ShouldLogCrossSchemeForHistograms(options.same_site_cookie_context(),
effective_same_site)) {
// TODO(https://crbug.com/1066231)
UMA_HISTOGRAM_ENUMERATION(
"Cookie.SameSiteDifferentSchemeRequest",
options.same_site_cookie_context().ConvertToMetricsValue(),
CookieOptions::SameSiteCookieContext::MetricCount());
AddSameSiteCrossSchemeWarning(&status,
options.same_site_cookie_context());
}
}

// TODO(chlily): Log metrics.
Expand Down Expand Up @@ -678,18 +644,6 @@ void CanonicalCookie::IsSetPermittedInContext(
UMA_HISTOGRAM_ENUMERATION("Cookie.IncludedResponseEffectiveSameSite",
effective_same_site,
CookieEffectiveSameSite::COUNT);

if (ShouldLogCrossSchemeForHistograms(options.same_site_cookie_context(),
effective_same_site)) {
// TODO(crbug.com/1034014): Change enum to one with less confusing
// phrasing.
// TODO(https://crbug.com/1066231)
UMA_HISTOGRAM_ENUMERATION(
"Cookie.SameSiteDifferentSchemeResponse",
options.same_site_cookie_context().ConvertToMetricsValue(),
CookieOptions::SameSiteCookieContext::MetricCount());
AddSameSiteCrossSchemeWarning(status, options.same_site_cookie_context());
}
}

// TODO(chlily): Log metrics.
Expand Down Expand Up @@ -781,55 +735,6 @@ std::string CanonicalCookie::BuildCookieLine(
return cookie_line;
}

void net::CanonicalCookie::AddSameSiteCrossSchemeWarning(
CookieInclusionStatus* status,
CookieOptions::SameSiteCookieContext same_site_context) const {
if (same_site_context.cross_schemeness() ==
CookieOptions::SameSiteCookieContext::CrossSchemeness::INSECURE_SECURE) {
switch (same_site_context.context()) {
case CookieOptions::SameSiteCookieContext::ContextType::
SAME_SITE_LAX_METHOD_UNSAFE:
status->AddWarningReason(
CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL);
break;
case CookieOptions::SameSiteCookieContext::ContextType::SAME_SITE_LAX:
status->AddWarningReason(
CookieInclusionStatus::WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL);
break;
case CookieOptions::SameSiteCookieContext::ContextType::SAME_SITE_STRICT:
status->AddWarningReason(
CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL);
break;
default:
break;
}
} else if (same_site_context.cross_schemeness() ==
CookieOptions::SameSiteCookieContext::CrossSchemeness::
SECURE_INSECURE) {
switch (same_site_context.context()) {
case CookieOptions::SameSiteCookieContext::ContextType::
SAME_SITE_LAX_METHOD_UNSAFE:
status->AddWarningReason(
CookieInclusionStatus::
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL);
break;
case CookieOptions::SameSiteCookieContext::ContextType::SAME_SITE_LAX:
status->AddWarningReason(
CookieInclusionStatus::WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL);
break;
case CookieOptions::SameSiteCookieContext::ContextType::SAME_SITE_STRICT:
status->AddWarningReason(
CookieInclusionStatus::
WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL);
break;
default:
break;
}
}
}

// static
CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
const std::string& name) {
Expand Down Expand Up @@ -996,29 +901,6 @@ bool CanonicalCookie::CookieInclusionStatus::HasWarningReason(
return warning_reasons_ & GetWarningBitmask(reason);
}

bool net::CanonicalCookie::CookieInclusionStatus::HasCrossSchemeWarning(
CookieInclusionStatus::WarningReason* reason) const {
if (!ShouldWarn())
return false;

const CookieInclusionStatus::WarningReason cross_scheme_warnings[] = {
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL,
WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL,
WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL,
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL,
WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL,
WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL};
for (const auto warning : cross_scheme_warnings) {
if (HasWarningReason(warning)) {
if (reason)
*reason = warning;
return true;
}
}

return false;
}

void CanonicalCookie::CookieInclusionStatus::AddWarningReason(
WarningReason reason) {
warning_reasons_ |= GetWarningBitmask(reason);
Expand Down Expand Up @@ -1080,21 +962,6 @@ std::string CanonicalCookie::CookieInclusionStatus::GetDebugString() const {
base::StrAppend(&out, {"WARN_SAMESITE_NONE_INSECURE, "});
if (HasWarningReason(WARN_SAMESITE_UNSPECIFIED_LAX_ALLOW_UNSAFE))
base::StrAppend(&out, {"WARN_SAMESITE_UNSPECIFIED_LAX_ALLOW_UNSAFE, "});
if (HasWarningReason(WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL))
base::StrAppend(
&out, {"WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_SECURE_URL, "});
if (HasWarningReason(WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL))
base::StrAppend(&out, {"WARN_SAMESITE_LAX_CROSS_SCHEME_SECURE_URL, "});
if (HasWarningReason(WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL))
base::StrAppend(&out, {"WARN_SAMESITE_STRICT_CROSS_SCHEME_SECURE_URL, "});
if (HasWarningReason(
WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL))
base::StrAppend(
&out, {"WARN_SAMESITE_LAX_METHOD_UNSAFE_CROSS_SCHEME_INSECURE_URL, "});
if (HasWarningReason(WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL))
base::StrAppend(&out, {"WARN_SAMESITE_LAX_CROSS_SCHEME_INSECURE_URL, "});
if (HasWarningReason(WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL))
base::StrAppend(&out, {"WARN_SAMESITE_STRICT_CROSS_SCHEME_INSECURE_URL, "});

// Strip trailing comma and space.
out.erase(out.end() - 2, out.end());
Expand Down
Loading

0 comments on commit d4aa743

Please sign in to comment.