[StorageKey] Allow (de)serialization of opaque top_level_site [chromium/src : main]
Ari Chivukula (Gerrit)
Attention is currently required from: Steven Bingler.
Ari Chivukula would like Steven Bingler to review this change.
[StorageKey] Allow (de)serialization of opaque top_level_site
It's possible, for example on a data url with an iframe, to have a
non-opaque origin and an opaque top_level_site. In such instances we
need to be able to serialize the opaque top_level_site so that storage
access is possible. In future, we may want to remove this capability but
with a target launch of M112 we cannot take on more near term
deprecations.
Bug: 1407243
Change-Id: I0698e0b209884571e9c24b0f3b54490de90d3a2b
---
M net/base/schemeful_site.cc
M net/base/schemeful_site.h
M net/base/schemeful_site_unittest.cc
M third_party/blink/common/storage_key/storage_key.cc
M third_party/blink/common/storage_key/storage_key_mojom_traits_unittest.cc
M third_party/blink/common/storage_key/storage_key_unittest.cc
M third_party/blink/public/common/storage_key/storage_key.h
M third_party/blink/renderer/platform/network/blink_schemeful_site.h
M third_party/blink/renderer/platform/network/blink_schemeful_site_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_mojom_traits_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_test.cc
M url/origin.h
13 files changed, 422 insertions(+), 75 deletions(-)
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Steven Bingler.
Patch set 1:Auto-Submit +1Commit-Queue +1
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng, Steven Bingler, Victor Vasiliev, Yoav Weiss.
Ari Chivukula would like Daniel Cheng, Victor Vasiliev and Yoav Weiss to review this change.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng, Steven Bingler, Victor Vasiliev, Yoav Weiss.
1 comment:
Patchset:
@Victor Vasiliev Just looking for SchemefulSite
@Yoav Weiss Just looking for BlinkSchemefulSite
@bingler and @dcheng looking for holistic review
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Steven Bingler, Victor Vasiliev, Yoav Weiss.
Ari Chivukula would like Alex Moshchuk to review this change.
[StorageKey] Allow (de)serialization of opaque top_level_site
It's possible, for example on a data url with an iframe, to have a
non-opaque origin and an opaque top_level_site. In such instances we
need to be able to serialize the opaque top_level_site so that storage
access is possible. In future, we may want to remove this capability but
with a target launch of M112 we cannot take on more near term
deprecations.
Bug: 1407243
Change-Id: I0698e0b209884571e9c24b0f3b54490de90d3a2b
---
M content/browser/renderer_host/render_frame_host_impl.cc
M net/base/schemeful_site.cc
M net/base/schemeful_site.h
M net/base/schemeful_site_fuzzer.cc
M net/base/schemeful_site_unittest.cc
M third_party/blink/common/storage_key/storage_key.cc
M third_party/blink/common/storage_key/storage_key_mojom_traits_unittest.cc
M third_party/blink/common/storage_key/storage_key_unittest.cc
M third_party/blink/public/common/storage_key/storage_key.h
M third_party/blink/renderer/platform/network/blink_schemeful_site.h
M third_party/blink/renderer/platform/network/blink_schemeful_site_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_mojom_traits_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_test.cc
M url/origin.h
15 files changed, 441 insertions(+), 85 deletions(-)
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Steven Bingler, Victor Vasiliev, Yoav Weiss.
Patch set 2:Auto-Submit +1
1 comment:
Patchset:
@Alex for render frame host only
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Steven Bingler, Victor Vasiliev, Yoav Weiss.
Patch set 4:Auto-Submit +1
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Josh Karlin, Steven Bingler, Yoav Weiss.
Ari Chivukula would like Josh Karlin to review this change.
Ari Chivukula removed Victor Vasiliev from this change.
[StorageKey] Allow (de)serialization of opaque top_level_site
It's possible, for example on a data url with an iframe, to have a
non-opaque origin and an opaque top_level_site. In such instances we
need to be able to serialize the opaque top_level_site so that storage
access is possible. In future, we may want to remove this capability but
with a target launch of M112 we cannot take on more near term
deprecations.
Bug: 1407243
Change-Id: I0698e0b209884571e9c24b0f3b54490de90d3a2b
---
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/render_frame_host_impl.cc
M net/base/schemeful_site.h
M net/base/schemeful_site_unittest.cc
M third_party/blink/common/storage_key/storage_key.cc
M third_party/blink/common/storage_key/storage_key_mojom_traits_unittest.cc
M third_party/blink/common/storage_key/storage_key_unittest.cc
M third_party/blink/public/common/storage_key/storage_key.h
M third_party/blink/renderer/platform/network/blink_schemeful_site.h
M third_party/blink/renderer/platform/network/blink_schemeful_site_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_mojom_traits_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_test.cc
M url/origin.h
14 files changed, 427 insertions(+), 54 deletions(-)
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Josh Karlin, Steven Bingler, Yoav Weiss.
Patch set 6:Auto-Submit +1
Josh Karlin (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
Josh Karlin would like Matt Menke to review this change authored by Ari Chivukula.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Josh Karlin (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
1 comment:
Patchset:
Switching to Matt to review this as he knows much more about the storage keys at this point.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Steven Bingler, Yoav Weiss.
2 comments:
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),Why is top_level_site_.SerializeWithNonce() insufficient? Does this need to be ASCII text?
File third_party/blink/public/common/storage_key/storage_key.h:
Patch Set #6, Line 146:
// Do not call if `this` is opaque.Does this still apply?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
Patch set 7:Auto-Submit +1Commit-Queue +1
2 comments:
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),
Why is top_level_site_. […]
We don't want to depend on other serialization paths here generally, we want to be in as close to absolute control as possible to ensure no unexpected issues
File third_party/blink/public/common/storage_key/storage_key.h:
Patch Set #6, Line 146:
// Do not call if `this` is opaque.Does this still apply?
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Steven Bingler, Yoav Weiss.
1 comment:
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),
We don't want to depend on other serialization paths here generally, we want to be in as close to absolute control as possible to ensure no unexpected issues
Why? Per docs, these will not be reused across restarts. This seems like re-inventing the wheel to me. Using its built-in-functions provides more protection against changes in internal representation.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
Ari Chivukula has uploaded this change for review.
14 files changed, 429 insertions(+), 56 deletions(-)
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
1 comment:
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),
> We don't want to depend on other serialization paths here generally, we want to be in as close to […]
You mean because the top_level_site is opaque? That's true, the storage session shouldn't be resumed, but the code is written in a way that assumes it could be and I want to keep the serialization intact for that reason. If we weren't already doing custom nonce and origin serialization in this file I wouldn't be pushing back, but we are so I want to follow the trend. @mek for perspective
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Marijn Kruisselbrink (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Matt Menke, Steven Bingler, Yoav Weiss.
1 comment:
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),
You mean because the top_level_site is opaque? That's true, the storage session shouldn't be resumed […]
SerializeWithNonce does not seem to currently be part of SchemefulSite's public API? Although that can of course be changed.
A problem with reusing the existing serialization API though is that we have to make sure the data does not contain ^, as that would mess up our parsing. As far as I can tell, SerializeWithNonce can't provide that guarantee, since Pickle.WriteUInt64 just copies the provided bits verbatim. So if we wanted to be able to re-use arbitrary serialization methods for sub fields we would have had to use a serialization format for storage keys that does not rely just on separators to separate the individual fields. Since that's not what we have today, I don't think currently we can rely on at least something like SerializeWithNonce.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Marijn Kruisselbrink, Steven Bingler, Yoav Weiss.
3 comments:
File net/base/schemeful_site.h:
Patch Set #6, Line 210:
nonceLet's not expose nonce() through this API. If the consumer needs it, and it needs internal_origin as well, have it get the nonce through internal_origin(). As-is, this class has no dependency on the details of nonce(), and I don't think we want to introduce one just for one consumer that's opinionated on the issue.
Patch Set #6, Line 214:
internal_originLet's not call this origin, because it's not actually an origin. It's a scheme+eTLD+1 packing into a url::Origin *or* an opaque origin. We could consider making it a variant or something, but since scheme+eTLD+1 can be packing into a url::Origin, I'm not sure it's worth the investment.
I'm open to ideas, but here are some options:
- internal_value()
- as_bogus_origin()
- not_actually_an_origin()
With a scary comment about what it really is, and that demands consumers users pinky swear they know what they're doing before using it. internal_value(base::PinkySwear(<ldap>)) (no, not really).
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),
SerializeWithNonce does not seem to currently be part of SchemefulSite's public API? Although that can of course be changed.
It is not, for the same reason the nonce itself is not part of url::Origin's public API (which is what SerializeWithNonce wraps).
A problem with reusing the existing serialization API though is that we have to make sure the data does not contain ^, as that would mess up our parsing. As far as I can tell, SerializeWithNonce can't provide that guarantee, since Pickle.WriteUInt64 just copies the provided bits verbatim. So if we wanted to be able to re-use arbitrary serialization methods for sub fields we would have had to use a serialization format for storage keys that does not rely just on separators to separate the individual fields. Since that's not what we have today, I don't think currently we can rely on at least something like SerializeWithNonce.
If that's really the only issue, this code could just base-64-encode the serialization of the site.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Marijn Kruisselbrink, Steven Bingler.
1 comment:
File third_party/blink/renderer/platform/storage/blink_storage_key.cc:
Patch Set #6, Line 62:
if (top_level_site.IsOpaque()) {Can you surround all the constructor code with `#ifdef DCHECK_IS_ON`?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Marijn Kruisselbrink, Steven Bingler.
3 comments:
File third_party/blink/common/storage_key/storage_key.cc:
Patch Set #7, Line 239:
std::string high_digits = static_cast<std::string>(We don't need to materialize a string here. StringToUint64 is happy to work with StringPieces.
Patch Set #7, Line 258:
base::UnguessableToken::Deserialize(nonce_high, nonce_low);We're working on updating how this deserialize method works (it can return nullopt now, if both high and low are 0).
Do you proactively using Deserialize2() here? And I guess we'd want to return nullopt to indicate failure if we get nullopt from Deserialize2.
(see https://chromium-review.googlesource.com/c/chromium/src/+/4135540 for some context)
These comments apply to the above (pre-existing) code too.
File url/origin.h:
Patch Set #7, Line 45:
FORWARD_DECLARE_TEST(StorageKeyTest, SerializeDeserializeOpaqueTopLevelSite);Can we just give this test a test fixture and friend the test fixture instead? Same below.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Josh Karlin (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Marijn Kruisselbrink, Steven Bingler.
1 comment:
File net/base/schemeful_site.h:
Patch Set #6, Line 210:
nonce
I think this is called twice in one location. Is there a need for the private method? Same for internal_origin() below.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Josh Karlin, Marijn Kruisselbrink, Steven Bingler.
1 comment:
File net/base/schemeful_site.h:
Patch Set #6, Line 210:
nonceI think this is called twice in one location. Is there a need for the private method? Same for internal_origin() below.
In general, I don't think we should have code depend on private member variables of unrelated classes. If we have to friend across such boundaries, I think it's much better to provide private accessors. Otherwise, it's not clear what internal state external classes are depending on. Yes, there's only on field here (for now), but still think it's better this way.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Josh Karlin, Marijn Kruisselbrink, Matt Menke, Steven Bingler, Yoav Weiss.
Patch set 8:Auto-Submit +1Commit-Queue +1
7 comments:
File net/base/schemeful_site.h:
Patch Set #6, Line 210:
nonce
Let's not expose nonce() through this API. […]
Done
Patch Set #6, Line 210:
nonce
> I think this is called twice in one location. […]
Done
Patch Set #6, Line 214:
internal_origin
Let's not call this origin, because it's not actually an origin. […]
Done
File third_party/blink/common/storage_key/storage_key.cc:
Patch Set #7, Line 239:
std::string high_digits = static_cast<std::string>(We don't need to materialize a string here. StringToUint64 is happy to work with StringPieces.
Done
Patch Set #7, Line 258:
base::UnguessableToken::Deserialize(nonce_high, nonce_low);
We're working on updating how this deserialize method works (it can return nullopt now, if both high […]
Done
File third_party/blink/renderer/platform/storage/blink_storage_key.cc:
Patch Set #6, Line 62:
if (top_level_site.IsOpaque()) {Can you surround all the constructor code with `#ifdef DCHECK_IS_ON`?
Done
File url/origin.h:
Patch Set #7, Line 45:
FORWARD_DECLARE_TEST(StorageKeyTest, SerializeDeserializeOpaqueTopLevelSite);Can we just give this test a test fixture and friend the test fixture instead? Same below.
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Daniel Cheng, Josh Karlin, Marijn Kruisselbrink, Steven Bingler, Yoav Weiss.
1 comment:
File net/base/schemeful_site.h:
it's a scheme+eTLD+1 packed into a
// (possibly opaque) OriginIt's actually on opaque origin or a scheme+eTLD+1 packing into a non-opaque origin. In the opaque origin case, we leave the precusor stuff alone, as that's pretty niche, and not exposed by the API.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Daniel Cheng, Josh Karlin, Marijn Kruisselbrink, Matt Menke, Steven Bingler, Yoav Weiss.
Patch set 9:Auto-Submit +1Commit-Queue +1
1 comment:
File net/base/schemeful_site.h:
it's a scheme+eTLD+1 packed into a
// (possibly opaque) Origin
It's actually on opaque origin or a scheme+eTLD+1 packing into a non-opaque origin. […]
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Josh Karlin, Marijn Kruisselbrink, Matt Menke, Steven Bingler, Yoav Weiss.
Patch set 9:Code-Review +1
3 comments:
Patchset:
LGTM w/nit
File net/base/schemeful_site.h:
Nit: "an"
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #9, Line 37:
getOpaqueSiteNit: GetOpaqueSite
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Josh Karlin, Marijn Kruisselbrink, Steven Bingler, Yoav Weiss.
Patch set 9:Code-Review +1
1 comment:
Patchset:
LGTM. Still skeptical of poking at the internals of origin, but not going to block on it.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Josh Karlin, Marijn Kruisselbrink, Steven Bingler, Yoav Weiss.
Patch set 10:Auto-Submit +1Commit-Queue +1
3 comments:
File net/base/schemeful_site.h:
Nit: "an"
Done
File third_party/blink/common/storage_key/storage_key.cc:
base::NumberToString(
top_level_site_.nonce()->GetHighForSerialization()),> SerializeWithNonce does not seem to currently be part of SchemefulSite's public API? Although that […]
Done
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #9, Line 37:
getOpaqueSiteNit: GetOpaqueSite
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Steven Bingler (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Yoav Weiss.
8 comments:
File third_party/blink/common/storage_key/storage_key.cc:
<StorageKey `key`.origin> + "/" + [ ^4" + <StorageKey
// `key`.top_level_site.nonce.High64Bits> + "^5" + <StorageKey
// `key`.top_level_site.nonce.Low64Bits> ]Can you add the precursor here?
Patch Set #9, Line 102:
attributesnit: "encoded attributes". It's confusing yes, but "encoded attributes" refers to the separator + uint8_t value pair. Saying "attributes" on its own makes me think that only 2 things are encoded in the string when it's actually 3 (origin, highbits, lowbits)
The whole "encoded attributes" things could probably be renamed to something better in the future.
Patch Set #9, Line 286:
url::SchemeHostPort(site_precursor)Can you construct this around line 275 and then call `IsValid()`?
Patch Set #9, Line 426:
ancestor_chain_bit must be kSameSite,Must it? Is a `data: -> a.com -> b.com` frame tree not possible?
Patch Set #9, Line 574:
StorageKey::ShouldSkipKeyDueToPartitioning(You'll also want to update this to skip opaque TLS StorageKeys.
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #10, Line 538:
DCHECK`ASSERT()`?
Patch Set #10, Line 589:
"https://example.com/^10^20",These also get tested in `StorageKeyTest, Deserialize`. I won't block on it, but if it's not too much work can you move or the other so they're all in the same place?
Patch Set #10, Line 976:
(void)What is this for?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Steven Bingler, Yoav Weiss.
Patch set 11:Auto-Submit +1Commit-Queue +1
8 comments:
File third_party/blink/common/storage_key/storage_key.cc:
<StorageKey `key`.origin> + "/" + [ ^4" + <StorageKey
// `key`.top_level_site.nonce.High64Bits> + "^5" + <StorageKey
// `key`.top_level_site.nonce.Low64Bits> ]Can you add the precursor here?
Done
Patch Set #9, Line 102:
attributes
nit: "encoded attributes". […]
Done
Patch Set #9, Line 286:
url::SchemeHostPort(site_precursor)Can you construct this around line 275 and then call `IsValid()`?
Done
Patch Set #9, Line 426:
ancestor_chain_bit must be kSameSite,
Must it? Is a `data: -> a.com -> b. […]
So here's the thing, since ancestor_chain_bit really only cares if the frame's site_for_cookies is first party with (almost) all ancestors . . .
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=4113
. . . and the purpose is to separate the partitions between a first party and an embedded third party . . .
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/storage_key/storage_key.cc;drc=c07db020568971a1d4a94a093aae7c4a8c6252a5;l=133
. . . I think the ancestor_chain_bit is superfluous here the same way it is in the storage key with nonce . . .
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/storage_key/storage_key.cc;drc=c07db020568971a1d4a94a093aae7c4a8c6252a5;l=202
. . . and since we have no reason to encode or decode the value the default seems a safe fallback.Further, I'm not sure it ever makes sense to serialize ancestor_chain_bit. In the case with a non-opaque origin and site where the nonce is empty, we can distinguish between the three states to set the bit in the StorageKey without needing to take up space:
(1) Only an origin was serialized -> {origin, origin_as_site, SameSite}
(2) An origin and an origin_as_site were serialized -> {origin, origin_as_site, CrossSite}
(3) An origin and a non-matching site were serialized -> {origin, origin_as_site, SameSite} [this could also be CrossSite, it's just a question of setting a convention).
Patch Set #9, Line 574:
StorageKey::ShouldSkipKeyDueToPartitioning(You'll also want to update this to skip opaque TLS StorageKeys.
Done
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #10, Line 538:
DCHECK`ASSERT()`?
Done
Patch Set #10, Line 589:
"https://example.com/^10^20",These also get tested in `StorageKeyTest, Deserialize`. […]
I don't think it's a straightforward merge because that one is aimed at nonce testing specifically. I think it's better to have this test focused on the high == low == 0 case.
Patch Set #10, Line 976:
(void)What is this for?
To prevent it getting angry about not storing return values.
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Steven Bingler, Yoav Weiss.
1 comment:
File third_party/blink/common/storage_key/storage_key.cc:
Patch Set #9, Line 426:
ancestor_chain_bit must be kSameSite,So here's the thing, since ancestor_chain_bit really only cares if the frame's site_for_cookies is f […]
Sorry that third line should read:
(3) An origin and a non-matching site were serialized -> {origin, site, SameSite} [this could also be CrossSite, it's just a question of setting a convention).
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Steven Bingler (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Yoav Weiss.
3 comments:
File third_party/blink/common/storage_key/storage_key.cc:
Patch Set #9, Line 426:
ancestor_chain_bit must be kSameSite,
Sorry that third line should read: […]
Ok, since the A -> B -> A case isn't possible here that seems fine by me.
Can you please document the decision to make the ACB irrelevant for opaque TLSs in StorageKey.h?
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #10, Line 976:
(void)
To prevent it getting angry about not storing return values.
Gotcha, I think `std::ignore` is preferred here
https://groups.google.com/a/chromium.org/g/cxx/c/0ltEx0s01MY/m/GigLS0sBAwAJ
File third_party/blink/renderer/platform/network/blink_schemeful_site.h:
Patch Set #10, Line 76:
IsOpaqueAny reason you didn't match `SchemefulSite::opaque()`?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Steven Bingler, Yoav Weiss.
Patch set 12:Auto-Submit +1Commit-Queue +1
3 comments:
File third_party/blink/common/storage_key/storage_key.cc:
Patch Set #9, Line 426:
ancestor_chain_bit must be kSameSite,
Ok, since the A -> B -> A case isn't possible here that seems fine by me. […]
Done
File third_party/blink/common/storage_key/storage_key_unittest.cc:
Patch Set #10, Line 976:
(void)
Gotcha, I think `std::ignore` is preferred here […]
Done
File third_party/blink/renderer/platform/network/blink_schemeful_site.h:
Patch Set #10, Line 76:
IsOpaqueAny reason you didn't match `SchemefulSite::opaque()`?
I wanted to stick with the conventions of local origin representation and that's what SecurityOrigin does
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Steven Bingler (Gerrit)
Attention is currently required from: Alex Moshchuk, Ari Chivukula, Yoav Weiss.
Patch set 12:Code-Review +1
2 comments:
Patchset:
LGTM % request
File third_party/blink/public/common/storage_key/storage_key.h:
// A class used by Storage APIs as a key for storage. An entity with a given
// storage key may not access data keyed with any other storage key.
//
// When third party storage partitioning is disabled, a StorageKey is equivalent
// to an origin, which is how storage has historically been partitioned.
//
// When third party storage partitioning is enabled, a storage key additionally
// contains a top-level site and an ancestor chain bit (see below). This
// achieves partitioning of an origin by the top-level site that it is embedded
// in. For example, https://chat.example.net embedded in
// https://social-example.org is a distinct key from https://chat.example.net
// embedded in https://news-example.org.
//
// A key is a third-party key if its origin is not in its top-level site (or if
// its ancestor chain bit is `kCrossSite`; see below); otherwise it is a
// first-party key.
//
// A corner-case is a first-party origin embedded in a third-party origin, such
// as https://a.com embedded in https://b.com in https://a.com. The inner
// `a.com` frame can be controlled by `b.com`, and is thus considered
// third-party. The ancestor chain bit tracks this status.
//
// Storage keys can also optionally have a nonce. Keys with different nonces are
// considered distinct, and distinct from a key with no nonce. This is used to
// implement iframe credentialless and other forms of storage partitioning.
// Keys with a nonce disregard the top level site and ancestor chain bit. For
// consistency we set them to the origin's site and `kSameSite` respectively.Can you add a note about opaque TLSs and ignoring the ACB up here too?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Alex Moshchuk, Yoav Weiss.
Patch set 13:Auto-Submit +1Commit-Queue +1
1 comment:
File third_party/blink/public/common/storage_key/storage_key.h:
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Attention is currently required from: Ari Chivukula, Yoav Weiss.
Patch set 14:Code-Review +1
2 comments:
Patchset:
content/ LGTM, though I'm not sure I understand why the ancestor bit is kSameSite for opaque top-level frames.
File content/browser/renderer_host/render_frame_host_impl.cc:
If `top_level_site` is opaque the bit must be
// kSameSite.FWIW, without any other context, this seems confusing to me on first glance. The comment on https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/storage_key/ancestor_chain_bit.mojom;l=7;drc=fef0781f2b4b0694ba56a0d688802c3c47f12dc9 says that the ancestor bit represents whether any frame on the ancestor chain is cross-site with the current frame, and in your example of non-data URL iframe on a data URL main frame, aren't these two frames cross-site? Would we want to distinguish that from a case where the data URL frame embeds and scripts an about:blank iframe which inherits the opaque origin and the two frames are actually same-origin? Or does that not matter in practice? Even if the value of the bit doesn't matter, it seems like kCrossSite may be a safer default.
In any case, can you update the comment here to explain the rationale for this?
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Yoav Weiss.
Patch set 15:Auto-Submit +1
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
If `top_level_site` is opaque the bit must be
// kSameSite.
FWIW, without any other context, this seems confusing to me on first glance. […]
Done
To view, visit change 4189064. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 15:Code-Review +1Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[StorageKey] Allow (de)serialization of opaque top_level_site
It's possible, for example on a data url with an iframe, to have a
non-opaque origin and an opaque top_level_site. In such instances we
need to be able to serialize the opaque top_level_site so that storage
access is possible. In future, we may want to remove this capability but
with a target launch of M112 we cannot take on more near term
deprecations.
Bug: 1407243
Change-Id: I0698e0b209884571e9c24b0f3b54490de90d3a2b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189064
Reviewed-by: Matt Menke <mme...@chromium.org>
Reviewed-by: Yoav Weiss <yoav...@chromium.org>
Commit-Queue: Yoav Weiss <yoav...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Auto-Submit: Ari Chivukula <ari...@chromium.org>
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Reviewed-by: Steven Bingler <bin...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1096631}
---
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/render_frame_host_impl.cc
M net/base/schemeful_site.h
M net/base/schemeful_site_unittest.cc
M third_party/blink/common/storage_key/storage_key.cc
M third_party/blink/common/storage_key/storage_key_mojom_traits_unittest.cc
M third_party/blink/common/storage_key/storage_key_unittest.cc
M third_party/blink/public/common/storage_key/storage_key.h
M third_party/blink/renderer/platform/network/blink_schemeful_site.h
M third_party/blink/renderer/platform/network/blink_schemeful_site_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_mojom_traits_test.cc
M third_party/blink/renderer/platform/storage/blink_storage_key_test.cc
M url/origin.h
14 files changed, 580 insertions(+), 86 deletions(-)