Cookies

[lolaodelola]

WIP

Issue #287

---

Preview | Diff

[OrKoN]

we probably want cookies outside of the browsing context module (except for the command to get a partition key or other commands that require the context as input)? I am thinking a new domain called storage would be good? cc @jgraham

[lolaodelola]

That sounds fine to me. Currently I haven't included a command to get a cookie's partition key, are you thinking something like broswsingContext.getCookiePartitionKey for that?

[jgraham]

Yes to a storage domain.

For the partition key, I think one question is whether it's sufficient to be able to get the key only for already loaded contexts. That is roughly equivalent to not exposing the key at all, but only being able to set cookies for a currently loaded page, because you always need to load a page, get the key, and then use that to set cookies.

I slightly wondered if we should define some partition key fields and have something in the capabilities response that returns the list of keys recognised by the current browser. So we might define sourceOrigin as a key. Then a browser with only SOP would just return an empty key set like:

storagePartitionKeys: {}

A browser that required keying on sourceOrigin and couldn't provide any defaults would return something like:

storagePartitionKeys: {sourceOrigin: {required: true}}

And then something like current gecko with containers could provide a custom key like moz:userContext like so:

storagePartitionKeys: {sourceOrigin: {required: true}, "moz:userContext": {default: null}}

So then clients/users could know that to set storage properties you'd have to provide a value for the sourceOrigin key and that moz:userContext is another part of the key, but if you don't provide it you'll get some default value that hopefully does the right thing for most cases. This doesn't totally solve the forwards compatibility problem, but at least makes it possible for clients to implement a good UX (by telling you exactly what you need to change if there's a change, and by allowing users to provide a set of properties that's a superset of what the client expects).

[lolaodelola]

I just want to make a clarification here, when we're talking about partition key for cookies, are we talking about partitioned cookies as defined in CHIPS or are we talking about cookies defined within Mozilla's containers for different user contexts?

[jgraham]

Both. Or rather CHIPS is a Chromium API that allows sites to opt in to source-partitioned cookies. Other browsers have source partitioned cookies everywhere. Containers are another part of the partition key in Gecko today but it could be adopted by other browsers (I think maybe WebKit has something similar? I'm not sure). It also seems conceivable that browsers will add additional partitioning in the future if there are additional UX/privacy benefits. So I don't think we want to start from the position that there's a known set of attributes that partition storage state.

[jugglinmike]

I am thinking a new domain called storage would be good?

I believe "domain" is a term from Chrome DevTools Protocol which roughly corresponds to the BiDi draft's "module." Please let me know if I'm mistaken!

It's my understanding that there's no interest in extending the original WebDriver specification with implementation-defined cookie partitioning. That's why I've wrapped WebDriver's "process capabilities" with a new algorithm which adds partition information. To reduce the risk of confusion, I've referenced the original algorithm with the word "classic" and used "BiDi" in the name of the new algorithm (though either one of those alone would be sufficient to disambiguate).

@jgraham's recent comment describes a scenario involving multiple partition keys, so I've changed the type in storage.Cookie to an optional extensible object (rather than string|undefined).

The draft currently defines a type named network.Cookie. Is there a need for that type to differ from storage.Cookie? I believe the organization of types is not observable, so would it be possible to extend network.Cookie and use it from the new storage module?

[jgraham]

Yes, reusing network.Cookie is the right thing to do here.

On the more general design here, I think we agreed at TPAC to drop the capabilities part for now.

Instead, in the meantime, we can have something like

storage.PartitionKey = {
  ?container: text,
  ?sourceOrigin: text,  
}

Then when you set a cookie you have something like:

storage.SetCookieParameters = {
  cookies: [ +network.Cookie ],
  ? partitionKey: storage.PartitionKey,
}

and storage.SetCookieResult which will look like

storage.SetCookieResult = {
   partitionKey: partitionKey
}

In the return value all partition keys that were used must be set to the value that was used, and any that were not used must be unset. For sourceOrigin the default must be the same as the Cookie's destination origin (there might be some detail here because cookies are not precisely keyed on origin). For container the default must be the default container (which we might call null or empty string, I'm not sure).

[jugglinmike]

Hi folks, I've completed a first draft; I'll share a few specific thoughts as in-line comments.

[jugglinmike]

These commands can't use BiDi's network.Cookie type directly because in these contexts, many of the entries are optional. In the interest of limiting duplication, I've attempted to specify the storage.CookieFilter and storage.PartialCookie types in terms of network.Cookie, but I'm not certain the CDDL syntax I've applied is appropriate. (You can find more detail in the message I sent to the CBOR mailing list.)

[jgraham]

I'm quite in favour of just duplicating the definition with explicit markers for optionality, rather than trying to use clever CDDL which readers are unlikely to figure out the meaning of without being CDDL experts :)

[jugglinmike]

This could be expressed without an algorithm like "match cookie", but doing so would involve labeling a loop in order to continue from within a nested loop. Factoring the operation into two simpler algorithms seems preferable.

[jugglinmike]

If we want to re-use the corresponding table in WebDriver Classic, we'll have to special-case the "Cookie expiry time" concept because WebDriver classic uses "expiry" as the JSON key, but BiDi's network.Cookie uses "expires".

[sadym-chromium]

I believe it's not too late for changing the BiDi's network.Cookie "expires" to "expiry" to align with WebDriver Classic.

[jugglinmike]

Oh, that's great! Here's a patch for that.

[jgraham]

At a high level this is looking pretty good to me.

One formatting comment: in the rest of the spec we leave a blank line between algorithm steps in order to make it easier to read.

[jgraham]

I think in #570 we are likely to rename "container" to "userContext". Since we seem to be heading in the direction of just making user contexts a mandatory feature, we could even consider making this a top-level property outside the partition key, whilst still keeping the partition key construct in case we want to key storage on more than just souceOrigin in the future (however that's a point for discussion).

[jugglinmike]

I've renamed the key, but I'll hold off on restructuring until there's a stronger signal about whether it's mandatory.

[jgraham]

I'm quite in favour of just duplicating the definition with explicit markers for optionality, rather than trying to use clever CDDL which readers are unlikely to figure out the meaning of without being CDDL experts :)

[jgraham]

We should also return the value of the partition key that we finally used.

[jugglinmike]

Sounds good.

[jgraham]

We should definitely change the CDDL instead of having this, but also in general we don't raise errors for additional fields.

[jugglinmike]

Okay, I've made Storage.CookieFilter and Storage.PartialCookie extensible.

[jgraham]

Again we should return the expanded partition key.

[jugglinmike]

Got it. This makes Storage.SetCookieResult and Storage.DeleteCookiesResult identical, but I've defined them as distinct types, anyway.

[jgraham]

I don't think a fixed default works. For sourceOrigin in particular the right default is to set it as a first-party cookie i.e. with the source origin matching the destination.

[jugglinmike]

Does this mean that we need to require the domain key in storage.getCookies and storage.deleteCookies (just as we already do for storage.setCookie)? Or maybe require at least one, e.g. either:

getCookies({domain: 'zombo.com'})

or

getCookies({}, {partitionKey: {sourceOrigin: 'zombo.com'}})

?

Or could a wildcard value actually work for "get" and "delete" operations? Unlike "set" (where implementations may not be able to create cookies for all domains a piori), it seems like "get" and "delete" can describe meaningful operations in the absence of any particular domain, e.g. "retrieve matching cookies from all source origins."

[jgraham]

I think it's theoretically possible to get all cookies in a partition (at least in gecko, c.f. https://searchfox.org/mozilla-central/source/netwerk/cookie/nsICookieManager.idl#232-245; note that "origin attributes" here is basically the same as "partition").

But I don't think it's possible to get all cookies from all partitions at once.

So I think you either need to exactly specify which partition you want or you need to specify the domain so that we can compute a default partition (or specify a navigable / context which is implicitly associated with a specific partition).

[jgraham]

Should we also allow passing in a context to use as the default for the partition key? e.g if you already have example.org loaded in some context, then the default values of the partition key will be set as if the cookie was being set by example.org loaded in that specific context.

[jugglinmike]

Could you clarify what you mean by "context" in this case? I believe the existing "browsing context" and the proposed "user context" can both have associated domains, so it seems like either could fit in your example.

[OrKoN]

I believe both could be applicable here. But I guess we could start with the browsing context since the user contexts are not fully defined yet.

[jgraham]

Yes, browsing contexts (navigables).

(I don't think a user context has an associated domain? It's "just" a storage partition key. One can imagine having different domains load in different user contexts by default or something, and certainly each user context might at any one time have multiple navigables, all with documents that have origins covering multiple domains, but I don't see how there would be a useful 1:1 mapping).

[jgraham]

This feels really close to landing; I think we should just clarify how "required" and "default" partition keys relate to each other, but at that point we're good to go.

[jgraham]

So this means that you're treating "required" and "default" as mutually exclusive? i.e. the full key will be all the required keys plus all the keys with a default value? I was imagining that "required" would be everything and "default" would be the subset for which a known default exists. Either way works, but I found this quite surprising, so we at least need to be clearer in the definitions what's expected.

[jugglinmike]

Thanks, @jgraham! I've personally been considering this "requirement" from the perspective of the local end, where the attributes with default values are implicitly not required. I believe your expectation comes from the remote end's perspective, where the requirement stands regardless of whether each property was explicitly specified.

It seems like your recommendation to have guidance applies in either case--that is, whether the list SHOULD or SHOULD NOT include property values from the map. I've added some text which supports the "mutually exclusive"/"local end" framing since that seems like the lowest-friction solution at this late stage of the design process.

This has me wondering, though: do we really need two data structures here? If we only allowed implementers to specify which attributes are optional (and treat all other attributes are required), then would that limit them in any meaningful way?

If not, then we could avoid confusion (and simplify the spec generally) by eliminating this avenue of implementer choice. If, instead of "required partition key attributes", we had a definition like:

A remote end has a set of <dfn>recognized storage partition key attributes</dfn>
which is the union of the attributes in the [=table of standard storage
partition key attributes=] and the [=remote end=]'s [=extension storage
partition key attributes=].

Note: The [=local end=] must specify any partition key attribute that a
[=remote end=] [=recognized storage partition key attributes|recognizes=] but
does not define a [default values for storage partition key
attributes|default value=].

I believe that would be sufficient to express the desired semantics of the "expand" algorithm.

[jgraham]

Writing the spec from the perspective of the local end isn't going to work out well; in practice the spec is specifying what happens in remote ends, and local ends are essentially free to do whatever they like to achieve their desired result. Of course by constraining the behaviour of remote ends the spec also de-facto constrains the things that local ends can do to achieve their goals. But note that, for example, we don't have a local end testsuite, only a remote end one. That reflects the fact that the normative criteria in the spec are all on the remote end.

I've made some suggestions for small improvements that I think clarify things for now. With those I'm basically happy for this to land, although as with all other commands I expect we'll need some later cleanup as we implement and as the interaction between this feature and other features in the spec is worked out.

[jugglinmike]

Thanks, @jgraham. I'm still interested in learning if the simplification I proposed is feasible, but like you, I'm satisfied with where we've landed for now.

[whimboo]

Thanks for all the updates @jugglinmike! I did a overall check only for now given that there are some issues to fix.

[whimboo]

nit: Lets sort alphabetical, which applies to the following definitions as well.

[jugglinmike]

This document doesn't follow a convention concerning if/how optionality impacts sort order (or concerning sorting in general). Would you like to sort with or without regard for optionality? Or would you prefer to introduce the new convention atomically?

[whimboo]

Generally we apply this sorting in our CDDL definitions. Usually we have the mandatory group first, and then the optional one, whereby within each group we sort alphabetically. I checked quite a few definitions and that is true, only some do not follow. With introducing complete new commands and types we should follow that.

[whimboo]

We can do the sorting in a follow-up PR. It's not blocking.

My comment shouldn't block. Leaving review to others.

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed Cookies.

The full IRC log of that discussion

<jgraham> Topic: Cookies
<jgraham> github: https://github.com//pull/501
<MaksimSadym> q+
<jgraham> jgraham: The Cookies PR seems nearly ready to land. Is there more that needs to be done here?
<AutomatedTester> q?
<jgraham> ack next
<jgraham> MaksimSadym: I tried to implement it and found a few possible extensions we'd like to make. That's fine, but we want to add some Chrome-specific data to the cookies.
<jgraham> jgraham: I think adding extra data is fine in general, but you're expected to use a prefix on the key like goog:
<jgraham> jgraham: If it's really Chrome-specific data then that's fine, if it's actually relevant to multiple browsers we should standardise it
<jgraham> MaksimSadym: I think it's Chrome-specific extensions
<jgraham> whimboo: The comment from me hasn't been addressed yet. I don't know when Mike is back, but this doesn't need to block; we can do it as a follow up.
<jgraham> jgraham: Great. Sounds like we should merge the current PR and address any further issues in follow ups.

[whimboo]

We have full review from Mozilla and Google. So lets get it merged! 🥇

[lutien]

I've picked up the work on storage.getCookies command in the scope of this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1854580.
@sadym-chromium, let me know if you started with the wpt tests already. If not, I'm going to add them with the implementation.

[sadym-chromium]

I've picked up the work on storage.getCookies command in the scope of this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1854580. @sadym-chromium, let me know if you started with the wpt tests already. If not, I'm going to add them with the implementation.

I started working on implementation, but not on WPT. We can split the work on WPT

[lutien]

I started working on implementation, but not on WPT. We can split the work on WPT

Sounds good to split :)
What do you think what would be the best way to do it? I could only think about that I take tests for getCookies and you for setCookies, but I'm open to suggestions