Tie state to agent clusters. Fixes #28.

[hober]

No description provided.

[annevk]

This looks to be heading in the right direction, but as I understand from a related issue Safari has a slightly different model in that requestStorageAccess() still needs to be invoked from a user gesture before the global state is copied into the agent cluster. Unless you all are planning on changing that we probably need to allow for it?

[hober]

This looks to be heading in the right direction

Great, and thanks so much for all of the suggested improvements. I think I've taken care of all of them.

but as I understand from a related issue Safari has a slightly different model in that requestStorageAccess() still needs to be invoked from a user gesture before the global state is copied into the agent cluster. Unless you all are planning on changing that we probably need to allow for it?

@johnwilander, what do you think?

[hober]

Cleaned things up a bit, caught a bug, and adopted Infra's implementation-defined. Please take another look.

[Brandr0id]

Thanks for adding the addition of a global state to represent the access grants. Now that we have a concept of a map storing the storage access flags does it make sense to just integrate with permissions that are keyed to the top-level and embedder origins rather than introduce a storage access map that's unique to this spec?

I could see the [=storage access map=] being a permission for Storage Access set with the [=partitioned storage key=] properties and the state "granted" or "denied" representing a success/failure of the API. The presence of the permissions "prompt" state and the concept of potentially prompting the user for access if no implicit grant is made may also tie nicely into this.

@annevk is it possible for two instances of a top-level origin to be in separate agent clusters? This part of the spec change had me curious about the ability to implement:

When an [=agent cluster=] is created, its [=agent cluster/storage access map=] is initialized with a [=map/clone=] of the [=global storage access map=].

Taking cookies being set or retrieved alongside a network request there would be context as to the [=partitioned storage key=] and the Storage Partition may have an up to date copy of the [=global storage access map=] but it may be problematic to have the Storage Partition know of multiple point-in-time copies of the [=global storage access map=] if two instances of a [=partitioned storage key=] may belong to two separate agent clusters and potentially require semantically different branches of the [=global storage access map=].

[hober]

Thanks for adding the addition of a global state to represent the access grants. Now that we have a concept of a map storing the storage access flags does it make sense to just integrate with permissions that are keyed to the top-level and embedder origins rather than introduce a storage access map that's unique to this spec?

I've filed #32 to consider this.

[hober]

The way I see it you'd almost never use the global map directly. You only use that for pushing a change in policy and when minting new agent clusters.

Right, that's what I've attempted to define.

[hober]

Okay, I've addressed all of @annevk's second batch of comments. Please take another look.

[johnwilander]

See comments. None of them blockers.

[johnwilander]

I believe this covers when an iframe is loaded from the same site as the top frame but is sandboxed without the allow-same-origin token.

[annevk]

So yeah, that's the case under discussion at whatwg/html#5491. Per @shivanigithub (and I tend to agree) that ought to change the top-level origin. So if that is the reality the "Otherwise" branch above would be sufficient.

I would be okay with merging and making this an open issue though. Firefox also doesn't deal with sandboxed content in the most ideal way.

[hober]

@annevk please do file such a followup.

[Brandr0id]

Certainly. If you have two browsing context groups they'll always have distinct agent clusters. The way I see it you'd almost never use the global map directly. You only use that for pushing a change in policy and when minting new agent clusters.

Perhaps the resolution of #31 may help sort this out, but I'm wondering if the agent cluster is the right granularity for keeping the copy of the [=global storage access map=] that's meant to be accessed to unblock storage access. This may also be diving too deep into the impl details but in a world where both the storage access and networking are performed from a different process than the renderer I see two potential pain points:

• For storage access ultimately coming back to the renderer like localStorage or document.cookie there is a point in time the agent cluster could be checked to unblock access; however, this may not be at the point of storage access in the associated StoragePartition which in itself could be shared by multiple agent clusters representing the same [=partitioned storage key=]. Since a block decision could be made lower down in the StoragePartition being shared by multiple agent clusters and then reversed at a higher layer that has context about the agent cluster accessing it there may exist some time where the result is inconsistent. The StoragePartition thought it blocked access but the agent cluster requesting access unblocked it vs enlightening the StoragePartion about the access map and making the right decision to start with.
• For storage access that is self-contained and not directly linked to an agent cluster such as a https://fetch.spec.whatwg.org/#http-network-or-cache-fetch occurring in a separate networking process the above issue becomes a bit more challenging. Once we get sufficiently far into the spec to start retrieving (or setting) cookies for a given request object we would be accessing the associated StoragePartition without context about the agent cluster in question and a decision to allow/block this access needs to be made.

Clarifying the actual intersection with Fetch and other specs as to how the storage access on the agent cluster is then propagated and used would be helpful in resolving these issues and concerns.

[annevk]

If you have A1 which embeds B1 and A2 which embeds B2 and As are same-site, Bs are same-site, and A and B are cross-site, you get (A, A) and (A, B) as keys. If B1 gets storage access it gets the key (B, B). But that doesn't mean B2 changes from (A, B). The parent process ought to keep track of these keys and attach them to state lookups as appropriate.

[annevk]

I'm okay with merging this if we add some caveats and probably new issues for first-party + sandboxing and storage access map caching details. Does that seem reasonable?

[annevk]

So yeah, that's the case under discussion at whatwg/html#5491. Per @shivanigithub (and I tend to agree) that ought to change the top-level origin. So if that is the reality the "Otherwise" branch above would be sufficient.

I would be okay with merging and making this an open issue though. Firefox also doesn't deal with sandboxed content in the most ideal way.

[annevk]

The one weird thing here is that the map goes from site to flags, but an agent cluster is already keyed on a site so it would really only need the flags.

Another alternative would be to put the cache on the browsing context group or some such. I probably need a more detailed discussion with @bakulf to see what would be good here.

[hober]

@annevk: The global storage access map uses (site, site) keys, and it seemed easiest for the agent cluster ones to use keys of the same type. I'd be okay with changing that, though, maybe in a followup?

[annevk]

Yeah, I think this needs some further consideration. Another oddity is that agent clusters are subject to GC so if a browsing context group could not contain an agent cluster for a while (site was navigated away from, frame got removed) and then it's needed again it would depend on GC whether that agent cluster has the cached state.

[Brandr0id]

Is there a risk of returning the wrong result by checking for an expressly denied flag value here? Since the flag is checked in [=determine if a site has storage access=] can we skip these two lines here?

If the result of [=determine if a site has storage access=] can vary by implementation, by user setting to block third party storage or other mechanisms, would it be possible to enter here once while the impl logic and lack of "grant" cause hasStorageAccess to return false. As a result requestStorageAccess is called which is then expressly denied. Then the result of the impl logic to grant storage access changes and future calls to hasStorageAccess would still return false despite access actually being present and no grant being required.

[hober]

I think I see what you mean. The [=was expressly denied storage access flag=] needs to get reset somewhere.

[hober]

Does 8cde662 address your concern, @Brandr0id?

[Brandr0id]

Does 8cde662 address your concern

That actually looks like a similar but different instance of the same class of bug :).

In this case I think the nuance is in the implementer defined logic inside of [=determine if a site has storage access=]

1. Let |has storage access| (a [=boolean=]) be the result of running an [=implementation-defined=] set of steps to determine if |key|'s [=partitioned storage key/embedded site=] has access to its [=first party data=] on |key|'s [=partitioned storage key/top-level site=].

A nice simple example would be the block all third party cookies user option/preference. If the |implementation-defined| logic can vary during the lifetime of the page we run into this issue. I would say that a user and UX should probably prompt/lead to the refresh of already opened tabs but in practice that may not occur. The implementation logic could also just as easily by something that is by nature dynamic like tracking prevention features or other user experiences.

Using the third party cookie option we could have:

1. User has "block all third party cookies" set.
2. Visits a page with embedded 3p content that has storage access blocked so uses document.requestStorageAccess()
   1. Access is blocked, no implicit grant is given due to impl logic
   2. A prompt is shown and rejected leading to explicit denial and a flag being set to prevent future use of requestStorageAccess() on this document.
3. Subsequent calls to document.hasStorageAccess() are correct and return false
4. User preference for blocking third party cookies is unset, no reload occurs.
5. Subsequent calls to document.hasStorageAccess() are incorrect as flag is set but implementation logic no longer blocks storage access so false is returned but access to storage past this point would work.

[hober]

@Brandr0id thanks, it's clear now, and I've tried to address this in 6c1e4b4.

[Brandr0id]

The change adds good clarity to the spec but I don't think it addresses this issue. The issue is the potentially inaccurate response of hasStorageAccess and there is no guarantee a new requestStorageAccess call has been made that might update the flags/map.

Specifically I think I would propose just removing steps 2-6 inclusively (note step 3 appears to be a duplicate of step 11):

Since when we get execute step 12, we'll take the flag value into consideration along with any implementation specific logic to determine if access is present or not:

Actually looking closer here too, I'm not sure if caching the "has storage access flag" or referencing it here doesn't cause potential issues either.

Since the hasStorageAccess method is trying to reflect the current state of storage access and ultimately gets this from an implementation-defined set of steps I think we may need to always check the implementation-defined steps. Perhaps any early short circuit in the method that avoids the implementation-defined logic either has an implicit guarantee that the implementation-defined steps don't change throughout the lifetime of the document or risks the cached flag/response being inaccurate.

[hober]

@Brandr0id wrote:

Clarifying the actual intersection with Fetch and other specs as to how the storage access on the agent cluster is then propagated and used would be helpful in resolving these issues and concerns.

I think "clarified" is understating it here; this bit is totally undefined at the moment. #31 is tracking the task to do this.

Do you think this PR should wait until we've resolved #31?

[Brandr0id]

@Brandr0id wrote:

Clarifying the actual intersection with Fetch and other specs as to how the storage access on the agent cluster is then propagated and used would be helpful in resolving these issues and concerns.

I think "clarified" is understating it here; this bit is totally undefined at the moment. #31 is tracking the task to do this.

Do you think this PR should wait until we've resolved #31?

I don't think you should necessarily block on this. However depending how we clarify interactions with different types of storage it may make sense to change where the map or copy/cache of the map lives.

For example Fetch itself doesn't necessarily have an implicit relationship with the Agent Cluster that I'm aware of. I'd like to see spec language identify how we go from Agent Cluster map -> Fetch -> Storage Access in a manner that doesn't rely on specific arch nuances of any one implementation. Ideally this would rely on primitives available in Fetch if the state is to be passed in, or the map/state would be retrieved from somewhere other than the agent cluster if meant to be on dealt with inside the Storage Partition.

[annevk]

FWIW, currently fetch (at least when invoked) very much has access to the agent cluster through request's client. (That's not to say I don't think you have a point about some of this potentially being problematic depending on how you go about implementing it, but it's not as simple as that.)

[hober]

@Brandr0id wrote:

Do you think this PR should wait until we've resolved #31?

I don't think you should necessarily block on this. However depending how we clarify interactions with different types of storage it may make sense to change where the map or copy/cache of the map lives.

Oh, yeah, for sure. I expect the current text to drastically change as we start to spec out the interactions with different storage types.

[hober]

I've re-requested review from you, @Brandr0id, now that 6c1e4b4 is in place. I'd like to land this before the F2F if possible.

[Brandr0id]

I've re-requested review from you, @Brandr0id, now that 6c1e4b4 is in place. I'd like to land this before the F2F if possible.

Added a comment about the update. Please feel free to merge as-is and we can always address with a follow up change; there are a bunch of other great changes worth getting in here.

[hober]

Okay, I'm going to go ahead and merge this then. @Brandr0id, please file a followup! :)