[NSP] IsolationInfo method cleanup
Also fixes a few typos and moves two TODO references to a new, open bug.
Bug: None
Change-Id: Ie49df08547767f66598291107da21a9a7fa94931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598358
Commit-Queue: Andrew Williams <awillia@chromium.org>
Reviewed-by: mmenke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1314042}
diff --git a/net/base/isolation_info.cc b/net/base/isolation_info.cc
index 9361e41..f241823 100644
--- a/net/base/isolation_info.cc
+++ b/net/base/isolation_info.cc
@@ -218,11 +218,6 @@
return frame_origin_;
}
-const std::optional<url::Origin>& IsolationInfo::frame_origin_for_testing()
- const {
- return frame_origin_;
-}
-
bool IsolationInfo::IsEqualForTesting(const IsolationInfo& other) const {
return (request_type_ == other.request_type_ &&
top_frame_origin_ == other.top_frame_origin_ &&
@@ -300,21 +295,6 @@
return s;
}
-NetworkAnonymizationKey
-IsolationInfo::CreateNetworkAnonymizationKeyForIsolationInfo(
- const std::optional<url::Origin>& top_frame_origin,
- const std::optional<url::Origin>& frame_origin,
- const std::optional<base::UnguessableToken>& nonce) const {
- if (!top_frame_origin) {
- return NetworkAnonymizationKey();
- }
- SchemefulSite top_frame_site(*top_frame_origin);
- SchemefulSite frame_site(*frame_origin);
-
- return NetworkAnonymizationKey::CreateFromFrameSite(top_frame_site,
- frame_site, nonce);
-}
-
IsolationInfo::IsolationInfo(RequestType request_type,
const std::optional<url::Origin>& top_frame_origin,
const std::optional<url::Origin>& frame_origin,
@@ -330,9 +310,11 @@
SchemefulSite(*frame_origin),
nonce)),
network_anonymization_key_(
- CreateNetworkAnonymizationKeyForIsolationInfo(top_frame_origin,
- frame_origin,
- nonce)),
+ !top_frame_origin ? NetworkAnonymizationKey()
+ : NetworkAnonymizationKey::CreateFromFrameSite(
+ SchemefulSite(*top_frame_origin),
+ SchemefulSite(*frame_origin),
+ nonce)),
site_for_cookies_(site_for_cookies),
nonce_(nonce) {
DCHECK(IsConsistent(request_type_, top_frame_origin_, frame_origin_,
diff --git a/net/base/isolation_info.h b/net/base/isolation_info.h
index fb50541..569c3a4 100644
--- a/net/base/isolation_info.h
+++ b/net/base/isolation_info.h
@@ -28,17 +28,18 @@
namespace net {
// Class to store information about network stack requests based on the context
-// in which they are made. It provides NetworkIsolationKeys, used to shard
-// storage, and SiteForCookies, used determine when to send same site cookies.
-// The IsolationInfo is typically the same for all subresource requests made in
-// the context of the same frame, but may be different for different frames
-// within a page. The IsolationInfo associated with requests for frames may
-// change as redirects are followed, and this class also contains the logic on
-// how to do that.
+// in which they are made. It provides NetworkIsolationKeys, used to shard the
+// HTTP cache, NetworkAnonymizationKeys, used to shard other network state, and
+// SiteForCookies, used determine when to send same site cookies. The
+// IsolationInfo is typically the same for all subresource requests made in the
+// context of the same frame, but may be different for different frames within a
+// page. The IsolationInfo associated with requests for frames may change as
+// redirects are followed, and this class also contains the logic on how to do
+// that.
//
-// The SiteForCookies logic in this class is currently unused, but will
-// eventually replace the logic in URLRequest/RedirectInfo for tracking and
-// updating that value.
+// TODO(crbug.com/40093296): The SiteForCookies logic in this class is currently
+// unused, but will eventually replace the logic in URLRequest/RedirectInfo for
+// tracking and updating that value.
class NET_EXPORT IsolationInfo {
public:
// The update-on-redirect patterns.
@@ -123,8 +124,8 @@
const SiteForCookies& site_for_cookies,
const std::optional<base::UnguessableToken>& nonce = std::nullopt);
- // TODO(crbug.com/40871266): Remove this and create a safer way to ensure NIKs
- // created from NAKs aren't used by accident.
+ // TODO(crbug.com/344943210): Remove this and create a safer way to ensure
+ // NIKs created from NAKs aren't used by accident.
static IsolationInfo DoNotUseCreatePartialFromNak(
const net::NetworkAnonymizationKey& network_anonymization_key);
@@ -182,16 +183,8 @@
// policy. It MUST NEVER be used for any kind of SECURITY check.
const SiteForCookies& site_for_cookies() const { return site_for_cookies_; }
- // Do not use outside of testing. Returns the `frame_origin_`.
- const std::optional<url::Origin>& frame_origin_for_testing() const;
-
bool IsEqualForTesting(const IsolationInfo& other) const;
- NetworkAnonymizationKey CreateNetworkAnonymizationKeyForIsolationInfo(
- const std::optional<url::Origin>& top_frame_origin,
- const std::optional<url::Origin>& frame_origin,
- const std::optional<base::UnguessableToken>& nonce) const;
-
// Serialize the `IsolationInfo` into a string. Fails if transient, returning
// an empty string.
std::string Serialize() const;
diff --git a/net/base/isolation_info_unittest.cc b/net/base/isolation_info_unittest.cc
index 9c1d9e7..7231dee4 100644
--- a/net/base/isolation_info_unittest.cc
+++ b/net/base/isolation_info_unittest.cc
@@ -118,58 +118,6 @@
EXPECT_EQ(isolation_info.DebugString(), base::StrCat(parts));
}
-TEST_P(IsolationInfoTest, CreateNetworkAnonymizationKeyForIsolationInfo) {
- IsolationInfo isolation_info = IsolationInfo::Create(
- IsolationInfo::RequestType::kMainFrame, kOrigin1, kOrigin2,
- SiteForCookies::FromOrigin(kOrigin1), kNonce1);
- NetworkAnonymizationKey nak =
- isolation_info.CreateNetworkAnonymizationKeyForIsolationInfo(
- kOrigin1, kOrigin2, kNonce1);
-
- IsolationInfo same_site_isolation_info = IsolationInfo::Create(
- IsolationInfo::RequestType::kMainFrame, kOrigin1, kOrigin1,
- SiteForCookies::FromOrigin(kOrigin1), kNonce1);
-
- // Top frame should be populated regardless of scheme.
- EXPECT_EQ(nak.GetTopFrameSite(), SchemefulSite(kOrigin1));
- EXPECT_EQ(isolation_info.top_frame_origin(), kOrigin1);
- EXPECT_EQ(isolation_info.network_anonymization_key().GetTopFrameSite(),
- SchemefulSite(kOrigin1));
-
- // Nonce should be empty regardless of scheme
- EXPECT_EQ(nak.GetNonce().value(), kNonce1);
- EXPECT_EQ(isolation_info.network_anonymization_key().GetNonce().value(),
- kNonce1);
- EXPECT_EQ(isolation_info.nonce().value(), kNonce1);
-
- // Triple-keyed IsolationInfo + double-keyed + cross site bit
- // NetworkAnonymizationKey case.
- EXPECT_EQ(isolation_info.frame_origin(), kOrigin2);
- EXPECT_TRUE(isolation_info.network_anonymization_key().IsCrossSite());
- EXPECT_TRUE(
- same_site_isolation_info.network_anonymization_key().IsSameSite());
-}
-
-// A 2.5-keyed NAK created with two identical opaque origins should be
-// same-site.
-TEST_P(IsolationInfoTest, CreateNetworkAnonymizationKeyForIsolationInfoOpaque) {
- url::Origin opaque;
- IsolationInfo isolation_info = IsolationInfo::Create(
- IsolationInfo::RequestType::kMainFrame, opaque, opaque,
- SiteForCookies::FromOrigin(opaque), kNonce1);
- NetworkAnonymizationKey nak =
- isolation_info.CreateNetworkAnonymizationKeyForIsolationInfo(
- opaque, opaque, kNonce1);
-
- EXPECT_TRUE(nak.IsSameSite());
-
- url::Origin opaque2;
- nak = isolation_info.CreateNetworkAnonymizationKeyForIsolationInfo(
- opaque, opaque2, kNonce1);
-
- EXPECT_TRUE(nak.IsCrossSite());
-}
-
TEST_P(IsolationInfoTest, RequestTypeMainFrame) {
IsolationInfo isolation_info =
IsolationInfo::Create(IsolationInfo::RequestType::kMainFrame, kOrigin1,
diff --git a/net/base/network_anonymization_key_unittest.cc b/net/base/network_anonymization_key_unittest.cc
index edc24b7..33213cf 100644
--- a/net/base/network_anonymization_key_unittest.cc
+++ b/net/base/network_anonymization_key_unittest.cc
@@ -160,7 +160,8 @@
TEST_F(NetworkAnonymizationKeyTest, CreateFromFrameSite) {
SchemefulSite site_a = SchemefulSite(GURL("http://a.test/"));
SchemefulSite site_b = SchemefulSite(GURL("http://b.test/"));
- SchemefulSite opaque = SchemefulSite(url::Origin());
+ SchemefulSite opaque_1 = SchemefulSite(url::Origin());
+ SchemefulSite opaque_2 = SchemefulSite(url::Origin());
base::UnguessableToken nonce = base::UnguessableToken::Create();
NetworkAnonymizationKey nak_from_same_site =
@@ -168,26 +169,32 @@
NetworkAnonymizationKey nak_from_cross_site =
NetworkAnonymizationKey::CreateFromFrameSite(site_a, site_b, nonce);
NetworkAnonymizationKey nak_from_same_site_opaque =
- NetworkAnonymizationKey::CreateFromFrameSite(opaque, opaque, nonce);
+ NetworkAnonymizationKey::CreateFromFrameSite(opaque_1, opaque_1, nonce);
+ NetworkAnonymizationKey nak_from_cross_site_opaque =
+ NetworkAnonymizationKey::CreateFromFrameSite(opaque_1, opaque_2, nonce);
// Top site should be populated correctly.
EXPECT_EQ(nak_from_same_site.GetTopFrameSite(), site_a);
EXPECT_EQ(nak_from_cross_site.GetTopFrameSite(), site_a);
- EXPECT_EQ(nak_from_same_site_opaque.GetTopFrameSite(), opaque);
+ EXPECT_EQ(nak_from_same_site_opaque.GetTopFrameSite(), opaque_1);
+ EXPECT_EQ(nak_from_cross_site_opaque.GetTopFrameSite(), opaque_1);
// Nonce should be populated correctly.
EXPECT_EQ(nak_from_same_site.GetNonce(), nonce);
EXPECT_EQ(nak_from_cross_site.GetNonce(), nonce);
EXPECT_EQ(nak_from_same_site_opaque.GetNonce(), nonce);
+ EXPECT_EQ(nak_from_cross_site_opaque.GetNonce(), nonce);
// Is cross site boolean should be populated correctly.
EXPECT_TRUE(nak_from_same_site.IsSameSite());
EXPECT_TRUE(nak_from_cross_site.IsCrossSite());
EXPECT_TRUE(nak_from_same_site_opaque.IsSameSite());
+ EXPECT_TRUE(nak_from_cross_site_opaque.IsCrossSite());
- // Double-keyed + cross site bit NAKs created from different third party
- // cross site contexts should be the different.
- EXPECT_FALSE(nak_from_same_site == nak_from_cross_site);
+ // NAKs created from different third party cross site contexts should be
+ // different.
+ EXPECT_NE(nak_from_same_site, nak_from_cross_site);
+ EXPECT_NE(nak_from_same_site_opaque, nak_from_cross_site_opaque);
}
TEST_F(NetworkAnonymizationKeyTest, IsEmpty) {
diff --git a/net/base/network_isolation_key.h b/net/base/network_isolation_key.h
index c73302f1..2ce8f9f 100644
--- a/net/base/network_isolation_key.h
+++ b/net/base/network_isolation_key.h
@@ -28,7 +28,7 @@
namespace net {
-// NetworkIsolationKey (NIK) is used to partition shared netork state based on
+// NetworkIsolationKey (NIK) is used to partition shared network state based on
// the context in which the requests were made. It is used to divide the HTTP
// cache, while the NetworkAnonymizationKey is used for most other network
// state.
diff --git a/net/reporting/reporting_cache_impl.cc b/net/reporting/reporting_cache_impl.cc
index 18964dd..bfa43ba 100644
--- a/net/reporting/reporting_cache_impl.cc
+++ b/net/reporting/reporting_cache_impl.cc
@@ -961,7 +961,7 @@
const ReportingEndpoint& endpoint) const {
// V0 endpoint groups do not support credentials.
if (!endpoint.group_key.reporting_source.has_value()) {
- // TODO(crbug.com/40871266): Remove this and have a better way to get an
+ // TODO(crbug.com/344943210): Remove this and have a better way to get a
// correct IsolationInfo here.
return IsolationInfo::DoNotUseCreatePartialFromNak(
endpoint.group_key.network_anonymization_key);
diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
index d2785138..8f7d474 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -287,7 +287,7 @@
// changed before Start() is called. Setting this value causes the
// cookie_partition_key_ to be recalculated.
//
- // TODO(crbug.com/40122112): This isn't actually used yet for SameSite
+ // TODO(crbug.com/40093296): This isn't actually used yet for SameSite
// cookies. Update consumers and fix that.
void set_isolation_info(const IsolationInfo& isolation_info);