[webkit-changes] [WebKit/WebKit] 4413e2: [Cookie Store API] Ensure subdomains can set/get c...

[Rupin Mittal]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 4413e221bc3419c996bc44b737d6af540908a0ca
      https://github.com/WebKit/WebKit/commit/4413e221bc3419c996bc44b737d6af540908a0ca
  Author: Rupin Mittal <rupin at apple.com>
  Date:   2025-01-15 (Wed, 15 Jan 2025)

  Changed paths:
    M LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_delete_arguments.https.any.js
    M LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any-expected.txt
    M LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any.js
    M LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any.serviceworker-expected.txt
    M Source/WebCore/Modules/cookie-store/CookieListItem.h
    M Source/WebCore/Modules/cookie-store/CookieStore.cpp
    M Tools/TestWebKitAPI/SourcesCocoa.txt
    M Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
    A Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieStoreAPI.mm

  Log Message:
  -----------
  [Cookie Store API] Ensure subdomains can set/get cookies for higher-level domains
https://bugs.webkit.org/show_bug.cgi?id=285890
rdar://142427907

Reviewed by Sihui Liu.

Step 9 in the Cookie Store API spec for setting a cookie (https://wicg.github.io/cookie-store/#set-a-cookie)
says that if a non-null domain is passed in, then:

1. If domain starts with U+002E (.), then return failure.
2. If host does not equal domain and host does not end with U+002E (.)
   followed by domain, then return failure.

This means that on the test website (which has a host of static-safari.apple.com),
setting a cookie with the domain "apple.com" should work.

But currently, it does not.

More generally speaking, this means subdomains should be able to set/get cookies for higher level domains,
but currently are unable to.

The issue is that we're not following all the steps of setting a cookie. After the pre-processing steps
in the #set-a-cookie part of the spec linked above, the spec says to follow the steps here:
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-14#name-storage-model

Step 10 of this says:
If the domain-attribute is non-empty:
    If the canonicalized request-host does not domain-match the domain-attribute:
        Abort these steps and ignore the cookie entirely.
    Otherwise:
        Set the cookie's host-only-flag to false.
        Set the cookie's domain to the domain-attribute.
Otherwise:
    Set the cookie's host-only-flag to true.
    Set the cookie's domain to the canonicalized request-host.

A host-only cookie is one accessible only by the exact host/domain it was set by. (If you don't
specify a domain, the host will be used as the domain, so they'll be same).

The way host-only only works is a CFNetwork quirk and is explained in a comment in the
normalizeCookieProperties function in CFHTTPCookie.mm:
- If domain begins with a '.', host-only is false, and so the cookie is accessible by subdomains
- If not, it's considered a host-only cookie (not accessible by subdomains).

Currently, we're setting all cookies without the '.' (so they are host-only). This patch
updates CookieStore::set to follow step 10. If the user passes in a non-null domain which
domain-matches the host, we will prepend a '.' to the domain (which sets host-only to false).

This allows subdomains to set/get cookies for higher level domains.

As noted at the bottom of the spec's section about querying cookies:
https://wicg.github.io/cookie-store/#query-cookies-algorithm, the host-only property is not
exposed to script. And CFNetwork's quirk with the '.' should not be exposed to script either.
So this '.' is stripped when the cookie is returned by any getter of the Cookie Store API. How: get
operations always return a CookieListItem constructed from the Cookie. So upon construction from a
Cookie, we force CookieListItem to strip the beginning '.' from the domain if if exists.

Testing:
- Checking that subdomains can set/get cookies for higher-level domains (checking host-only is set
to false) is done by the new API test CookieStoreSetCookieForHigherLevelDomain in CookieStoreAPI.mm

- Checking that returning a cookie will not have a prepended '.' is already done by existing layout tests.
One of the tests in this file below sets a cookie by specifying a domain and checks the returned domain:
https://github.com/web-platform-tests/wpt/blob/master/cookie-store/cookieListItem_attributes.https.any.js

Changes to WPT layout tests:
There is a test in cookieStore_delete_arguments.https.any.js that sets a cookie, gets the cookie, and
passes the result of the get into delete. This test was passing before this change but now fails to
delete the cookie. The issue is that the result of the get is a CookieListItem (which contains a
domain). When we set the cookie, we don't specify a domain, so the domain is set to "localhost".
The returned result (which contains "localhost") is passed to delete. Since the delete call recieves
a passed-in domain, it adds the '.' and tries to delete the cookie with domain ".localhost". Of course,
there is no such cookie and so our original cookie doesn't get deleted.

But CookieListItem shouldn't contain domain at all--we don't want to expose this property. CookieListItem
should contain only name and value. So we change the test to rely only on the properties we actually
expose to script--so the test continues to pass now. Similar change in the other changed test. We'll
upstream these changes to WPT soon.

* LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_delete_arguments.https.any.js:
* LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any.js:
* LayoutTests/imported/w3c/web-platform-tests/cookie-store/cookieStore_set_arguments.https.any.serviceworker-expected.txt:
* Source/WebCore/Modules/cookie-store/CookieListItem.h:
(WebCore::CookieListItem::CookieListItem):
* Source/WebCore/Modules/cookie-store/CookieStore.cpp:
(WebCore::CookieStore::set):
* Tools/TestWebKitAPI/SourcesCocoa.txt:
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieStoreAPI.mm: Added.
(TestWebKitAPI::TEST(WebKit, CookieStoreSetCookieForHigherLevelDomain)):

Canonical link: https://commits.webkit.org/288983@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications