Cookies

[lolaodelola]

WIP

Issue https://github.com/w3c/webdriver-bidi/issues/287

---

Preview | Diff

[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]

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.

I don't know how I missed that. Fixed.

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 :)

You got it. I've included an HTML comment which reminds contributors to keep these representations in sync (the CDDL build script ignores HTML comments inside the spec's definition elements). I've also introduced a named type for the sameSite properties to reduce the noise a little.

[jugglinmike]

We just today updated BiDi to use "expiry" instead of "expires" to better align with WebDriver Classic and allow reuse (thanks again to @sadym-chromium for calling out the feasibility of that change). I've merged that change into this branch so that the new types also use "expiry" and so that we can rely on WebDriver Classic's "table for cookie conversion" instead of defining a new one.

[jugglinmike]

@jgraham I have two questions about your earlier feedback, here and here.

[jugglinmike]

Thanks for the input, @OrKoN and @jgraham! I've responded here on the main thread to promote visibility (my latest changes have replaced the changes upon which the original feedback was based).

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).

Got it. I've made the text require one of those three pieces of information. I've also persisted the map named "default values for storage partition key attributes" and its associated semantics because they seem necessary to support implementation-defined extensions to the partition key.

Yes, browsing contexts (navigables).

Okay. The latest iteration replaces the concept of a "partition key spec" (which was a nullable map describing zero or more entries of a partition key) with a more generic "partition spec" (which is a nullable value that's either a map as before, or a browsing context id). At the protocol level, I've suggested referring to this value simply as "partition".

Although @jgraham mentioned that browsing contexts are implicitly associated with a specific partition, I believe this patch needs to make that association explicit. In order to do that, I've also added formal definitions for "storage partition" and "storage partition key."

(Separately, I've also corrected the return type for the "get the cookie store" algorithm. In order to be used with WebDriver's "try", it has to return either a "success" value or an "error" value.)

[lolaodelola]

Hi @gsnedders,

We're putting finishing touches on this piece of work & would like a rep from Apple to review and make sure it's up to scratch for you folks too as Mozilla & Google have given their input.

[sadym-chromium]

UPD: looks like the problem is in my local setup, and the check is passing https://github.com/w3c/webdriver-bidi/actions/runs/6938331389/job/18873878728?pr=501#step:7:1. Nevermind. Note: please be aware that running ./scripts/test.sh is failing:

error: parser errors
    ┌─ input:854:3
    │
854 │   storage.DeleteCookiesResult
    │   ^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing definition for rule storage.DeleteCookiesResult
    ·
857 │ storage.PartitionKey {
    │ ^^^^^^^^^^^^^^^^^^^^ expected assignment token '=', '/=' or '//=' after rule identifier
    ·
865 │   partitionKey: storage.partitionKey,
    │                 ^^^^^^^^^^^^^^^^^^^^ missing definition for rule storage.partitionKey
    ·
869 │   partitionKey: storage.partitionKey
    │                 ^^^^^^^^^^^^^^^^^^^^ missing definition for rule storage.partitionKey

Error: "incremental parsing error"

[jugglinmike]

@sadym-chromium @OrKoN sorry to take your time with such obvious mistakes as syntax validation errors. I've been working without the benefit of running this project's test script locally because I couldn't properly install its dependencies. I initially assumed this was an inconsequential problem with my system and decided to rely on the project's continuous integration environment. Now, though, I'm starting to wonder if the problem exists there, as well.

[sadym-chromium]

Fix for CDDL: https://github.com/bocoup/webdriver-bidi/pull/2 And please merge from main: https://github.com/bocoup/webdriver-bidi/pull/3

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed Clarify `script.addPreloadScript` behavior with a duplicated `context` in `contexts`..

The full IRC log of that discussion

<jgraham> Topic: Clarify `script.addPreloadScript` behavior with a duplicated `context` in `contexts`.
<jgraham> github: https://github.com/w3c/webdriver-bidi/issues/617
<jgraham> Topics: Cookies status
<jgraham> github: https://github.com/w3c/webdriver-bidi/pull/501
<jdescottes> jgraham (IRC): What is the status of cookies PR
<jrandolf> q+
<jgraham> ack next
<MaksimSadym> q+
<jrandolf> q-
<jdescottes> Lola: PR in a good place. Mike has implemented feedback on the issues. Now, looking for non-mozilla, non-google feedback. Someone from Apple should have a look.
<jdescottes> Sam Sneddon [:gsnedders]: Will take a look or will find someone
<jgraham> ack next
<jgraham> q+
<jdescottes> Maksim Sadym: Partition key: do we get the actual cookie partition or do we just get the filter provided.
<jgraham> ack next
<jdescottes> jgraham (IRC): All cookies should be in the same partition of the cookie store. But if chrome works differently we should change that
<jgraham> q+
<jdescottes> Sam Sneddon [:gsnedders]: Not opposed to returning the partition key but might end up in a situation where browsers return what was passed
<jgraham> ack next
<jdescottes> jgraham (IRC): will not always return what was passed in because you pass a context. In short term, maybe easiest to return it for each cookie. It will add protocol overhead. Could have a base partition key, potentially updated by each cookie if needed. Pushes complexity on the client
<gsnedders> s/Not opposed to returning the partition key but might end up in a situation where browsers return what was passed/CHIPS, as Chrome is currently planning on shipping (or now shipping?), has different cookies having different partitions (or rather: some partitioned and some not). But we're probably moving towards all browsers having all cookies either partitioned or blocked, so we'd end up with every browser basically just returning what was
<gsnedders> passed as an argument./
<jdescottes> Maksim Sadym: can proceed with current approach. No critical issue from implementation perspective
<jgraham> q+
<jdescottes> Sam Sneddon [:gsnedders]: other question: in cases where cookies are not sent to 3rd party contexts (eg currently Safari), people will want to know if cookies where sent to frames.
<jgraham> ack next
<gsnedders> s/Safari/Safari, depending on the Storage Access API/
<jdescottes> jgraham (IRC): easiest way to tell is to intercept request and see where cookies are sent. Not clear if we need to add a feature to support this use case.
<jdescottes> Could have isStorageAccess key as part of the partition key. Should comment on the issue
<jdescottes> Sam Sneddon [:gsnedders]: need to come to a conclusion whether what we want to do storage api should block the cookies api
<jdescottes> jgraham (IRC): Someone from apple should look at it and say if the basic design is good enough
<jdescottes> jgraham (IRC): Maksim should do a concrete proposal for different partition keys
<jdescottes> Maksim Sadym: not a blocker, we can proceed
<jgraham> q?
<jgraham> RRSAgent: make logs public
<RRSAgent> I have made the request, jgraham
<jgraham> RRSAgent: make minutes
<jgraham> zakim, bye
<Zakim> leaving. As of this point the attendees have been jrandolf, JimEvans, MaksimSadym, jgraham, whimboo, lola_, lightning00blade, sasha, jdescottes, cb
<RRSAgent> I have made the request to generate https://www.w3.org/2023/12/13-webdriver-minutes.html jgraham
<jgraham> RRSAgent, bye
<RRSAgent> I see no action items
<JimEvans> Maksim Sadym: I mentioned this a couple of weeks back, but I didn't open an issue until yesterday. https://github.com/GoogleChromeLabs/chromium-bidi/issues/1618

[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.

[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/w3c/webdriver-bidi/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