1971488 - "storage.getCookies" can return cookies with "sameSite=none" and "secure=false" on HTTP pages, but this is invalid for "storage.setCookies"

Status: NEW

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the changes on bug 1955685 it is no longer possible to set cookies with the sameSite=none attribute when secure=false. In such a case an error like the following is raised:

Cookie “cookie1” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.

In BiDi tests most likely a cookie is getting retrieved first, then a value changed and the cookie stored again. While the WebDriver BiDi command storage.getCookies doesn't explicitly fail it returns cookies with invalid values for those two attributes:

  pw:browser [pid=71245][out] 1749631851661	RemoteAgent	DEBUG	WebDriverBiDiConnection 714a5e19-8ab3-4bd1-ab8c-9ad339ab32e2 -> {"id":20,"method":"storage.getCookies","params":{"partition":{"type":"storageKey","userContext":"8fda15f9-d5c0-4613-a965-a67f06d194f7"}}} +1ms
  pw:browser [pid=71245][out] 1749631851661	RemoteAgent	TRACE	Received command storage.getCookies for destination ROOT +0ms
  pw:browser [pid=71245][out] 1749631851661	RemoteAgent	DEBUG	WebDriverBiDiConnection 714a5e19-8ab3-4bd1-ab8c-9ad339ab32e2 <- {"type":"success","id":20,"result":{"cookies":[{"domain":"localhost","httpOnly":false,"name":"cookie1","path":"/","sameSite":"none","secure":false,"size":8,"value":{"type":"string","value":"1"}},{"domain":"127.0.0.1","httpOnly":false,"name":"cookie2","path":"/","sameSite":"none","secure":false,"size":8,"value":{"type":"string","value":"2"}}],"partitionKey":{"userContext":"8fda15f9-d5c0-4613-a965-a67f06d194f7"}}} +1ms

The page that is used for this test is http://127.0.0.1:8907/ - so it's not HTTPS. It explains why secure is not true. But can we actually return sameSite=None in such a case?

Andrea, can you please help? Thanks

Comment 1

[Andrea Marchesini [:baku]]

Firefox does not allow sameSite=none without the secure attribute. But, sameSite=none is the default value. So, if you do not set the sameSite attribute, you achieve the same goal. Is this a good solution for you?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for the reply, Andrea. Just to confirm: when retrieving cookies from the store, it’s valid to receive a combination of SameSite=None and Secure=false, even though attempting to set that same combination would be rejected, correct? I assume this behavior is consistent across browsers, and that there’s no reliable way for us to tell whether the cookie was originally set without an explicit SameSite attribute?

Given that, I’m wondering what the best approach would be for WebDriver. Automatically stripping out SameSite=None when Secure=false before returning the cookie to the client might prevent setting errors later, but it introduces somewhat magical behavior. That could be surprising to developers who expect the cookie to reflect exactly what’s stored. But leaving it would always force client code to pre-process cookies before using the storage.setCookie command.

Comment 3

[Andrea Marchesini [:baku]]

Receiving a combination of SameSite=None + Secure=false should not happen (except if you use a profile with that cookie created before bug 1955685). The expected behavior is sameSite=unset and Secure=false.

In bug 1960824, we have introduced nsICookie::SAMESITE_UNSET. This value is used when the sameSite attribute was not explicitly set. Can this new value be used to fix the issue?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So Playwright sends the following cookies:

  pw:browser [pid=46933][out] 1750069871879	RemoteAgent	DEBUG	WebDriverBiDiConnection 6baebc83-aedc-4a29-a346-2dff4cd81387 -> {"id":8,"method":"storage.setCookie","params":{"cookie":{"name":"cookie1","value":{"type":"string","value":"1"},"domain":"localhost","path":"/"},"partition":{"type":"storageKey","userContext":"9af2a2f4-1cbe-49cd-991d-3aafc8f7a6f9"}}} +15ms
  pw:browser [pid=46933][out] 1750069871880	RemoteAgent	TRACE	Received command storage.setCookie for destination ROOT +1ms
  pw:browser [pid=46933][out] 1750069871880	RemoteAgent	DEBUG	WebDriverBiDiConnection 6baebc83-aedc-4a29-a346-2dff4cd81387 <- {"type":"success","id":8,"result":{"partitionKey":{"userContext":"9af2a2f4-1cbe-49cd-991d-3aafc8f7a6f9"}}} +0ms
  pw:browser [pid=46933][out] 1750069871881	RemoteAgent	DEBUG	WebDriverBiDiConnection 6baebc83-aedc-4a29-a346-2dff4cd81387 -> {"id":9,"method":"storage.setCookie","params":{"cookie":{"name":"cookie2","value":{"type":"string","value":"2"},"domain":"127.0.0.1","path":"/"},"partition":{"type":"storageKey","userContext":"9af2a2f4-1cbe-49cd-991d-3aafc8f7a6f9"}}} +1ms
  pw:browser [pid=46933][out] 1750069871881	RemoteAgent	TRACE	Received command storage.setCookie for destination ROOT +0ms
  pw:browser [pid=46933][out] 1750069871881	RemoteAgent	DEBUG	WebDriverBiDiConnection 6baebc83-aedc-4a29-a346-2dff4cd81387 <- {"type":"success","id":9,"result":{"partitionKey":{"userContext":"9af2a2f4-1cbe-49cd-991d-3aafc8f7a6f9"}}}

When I use the Cookie Manager web extension to inspect the cookie I can see that the same-site status is set to Unspecified (default). So that seems to be fine. Maybe there is something wrong in our code when we retrieve the cookie details. Let me check.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Strange. I don't have your changes for #getSameSitePlatformProperty() locally. That clearly explains the problem why I see that but not why it's not there. I'll have to investigate.

Also why has Playwright a problem as well when it uses a Firefox Nightly build? Maybe there is a bug on their side in GitHub actions and they don't download a recent version anymore. I'll ask over there.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so the problem does not occur when using the WebDriver BiDi storage.setCookie command but only when you load a HTTPS page and then evaluate document.cookie = 'foo=bar'. Then the returned cookie looks like:

{'domain': 'not-web-platform.test', 'httpOnly': False, 'name': 'foo', 'path': '/webdriver/tests/support', 'sameSite': 'none', 'secure': False, 'size': 6, 'value': {'type': 'string', 'value': 'bar'}

You can apply the following diff to reproduce the situation:

diff --git a/testing/web-platform/mozilla/tests/webdriver/bidi/storage/get_cookies/partition.py b/testing/web-platform/mozilla/tests/webdriver/bidi/storage/get_cookies/partition.py
index 5503f13224122..6de8f61f57416 100644
--- a/testing/web-platform/mozilla/tests/webdriver/bidi/storage/get_cookies/partition.py
+++ b/testing/web-platform/mozilla/tests/webdriver/bidi/storage/get_cookies/partition.py
@@ -1,17 +1,29 @@
 import pytest
 from tests.bidi import recursive_compare
 from tests.support.helpers import get_origin_from_url
 from webdriver.bidi.modules.storage import BrowsingContextPartitionDescriptor
 
 pytestmark = pytest.mark.asyncio
 
 
-async def test_partition_context(
+async def test_same_site(bidi_session, new_tab, test_page_cross_origin, add_cookie):
+    await bidi_session.browsing_context.navigate(
+        context=new_tab["context"], url=test_page_cross_origin, wait="complete"
+    )
+    await add_cookie(new_tab["context"], "foo", "bar")
+    cookies = await bidi_session.storage.get_cookies(
+        partition=BrowsingContextPartitionDescriptor(new_tab["context"])
+    )
+    print(f"\n\n\n*** Cookies: {cookies} \n\n\n")
+    return
+
+
+async def tst_partition_context(
     bidi_session,
     new_tab,
     test_page,
     domain_value,
     add_cookie,
     top_context,
     test_page_cross_origin,
 ):
@@ -29,16 +41,20 @@ async def test_partition_context(
     cookie_value = "bar"
     await add_cookie(new_tab["context"], cookie_name, cookie_value)
 
     # Check that added cookies are present on the right context.
     cookies = await bidi_session.storage.get_cookies(
         partition=BrowsingContextPartitionDescriptor(new_tab["context"])
     )
 
+    print(f"\n\n **** cookie: {cookies}")
+    await bidi_session.storage.set_cookies(cookies[0])
+    return
+
     recursive_compare(
         {
             "cookies": [
                 {
                     "domain": domain_value(),
                     "httpOnly": False,
                     "name": cookie_name,
                     "path": "/webdriver/tests/support",
@@ -64,17 +80,17 @@ async def test_partition_context(
             "partitionKey": {"sourceOrigin": source_origin_2, "userContext": "default"},
         },
         cookies,
     )
 
 
 # Because of Dynamic First-Party Isolation, adding the cookie with `document.cookie`
 # works only with same-origin iframes.
-async def test_partition_context_same_origin_iframe(
+async def tst_partition_context_same_origin_iframe(
     bidi_session, new_tab, inline, domain_value, add_cookie
 ):
     iframe_url = inline("<div id='in-iframe'>foo</div>")
     source_origin = get_origin_from_url(iframe_url)
     page_url = inline(f"<iframe src='{iframe_url}'></iframe>")
     await bidi_session.browsing_context.navigate(
         context=new_tab["context"], url=page_url, wait="complete"
     )

Now run the following command and scroll up a bit for the print output:

mach wpt --no-install-font --setpref="remote.log.truncate=false" --webdriver-arg=-vv --setpref="remote.events.async.wheel.enabled=true" testing/web-platform/mozilla/tests/webdriver/bidi/storage/get_cookies/partition.py

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

When I use `document.cookie = 'foo=bar;SameSite=None' to set the cookie, it's not getting returned by our API, which seems to be the expected behavior. So omitting this attribute we seem to inappropriately set the cookie value to an invalid state.

Comment 8

[Julian Descottes [:jdescottes]]

Note that the BiDi implementation doesn't really handle unset properly, and we somewhat map unset to none

https://searchfox.org/mozilla-central/rev/b4412cedce6e2900f5553cbdc43c3fa49c4b9adb/remote/webdriver-bidi/modules/root/storage.sys.mjs#69

Comment 9

[Andrea Marchesini [:baku]]

Julian is right. We map SAMESITE_UNSET as "none". We can change this part to return "unset", but I'm unfamiliar with webdriver-bidi to know the level of breakage.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrea Marchesini [:baku] from comment #9)

Julian is right. We map SAMESITE_UNSET as "none". We can change this part to return "unset", but I'm unfamiliar with webdriver-bidi to know the level of breakage.

This implies an interoperability issue. Chrome adheres to the WebDriver BiDi spec and returns the actual cookie values, but still allows setting cookies with sameSite=none and secure=false. But with Firefox the following wdspec test fails these days:

async def test_same_site(bidi_session, new_tab, test_page, add_cookie, set_cookie):
    await bidi_session.browsing_context.navigate(
        context=new_tab["context"], url=test_page, wait="complete"
    )
    await add_cookie(new_tab["context"], "foo", "bar")

    partition = BrowsingContextPartitionDescriptor(new_tab["context"])

    cookies = await bidi_session.storage.get_cookies(partition=partition)

    await set_cookie(cookies["cookies"][0], partition)

Shouldn't we consider making the sameSite field in network.Cookie optional? That way, we wouldn’t serialize the field when it’s unset, and if it is present, we ensure it contains a valid value.

In Playwright we could then as well update the following line to only set the sameSite field if its present and the tests will start passing again:
https://github.com/microsoft/playwright/blob/114c9c045213888179b11f0d5a1e2fb87ebc1bd8/packages/playwright-core/src/server/bidi/bidiBrowser.ts#L284

Comment 11

[Julian Descottes [:jdescottes]]

Maybe we can update the implementation to fix the regression, but I think we need a spec discussion here.

Per the definition of the same-site flag in https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site#section-4.2 I think we should always get either none/lax/strict when reading a cookie from storage.
So even if we allow sameSite to be optional in our spec, in theory we have no way of knowing that the cookie was created without a sameSite value?

Andrea, do you know if the spec for cookies is supposed to be updated to expose the "unset" state? Or maybe I'm misunderstanding the current spec and we can actually tell when sameSite is unset for a cookie?

Comment 12

[Andrea Marchesini [:baku]]

The sameSite attribute is not meant to be exposed via Cookie APIs (CookieStore—at least on Firefox and Safari, document.cookie, Cookie header).
We recently introduced the SAMESITE_UNSET value, which indicates when the sameSite attribute was not passed at creation time. CookieStorage computes the value to use based on the context: currently, if SAMESITE_UNSET, it uses SAMESITE_NONE, but in the future, SAMESITE_LAX.

A few browser components expose the underlying sameSite attribute value: WebExt Cookies API, DevTools, and BiDi. But should they expose the "real value" or the computed one? For DevTools, we decided to reveal the underlying value (you can see when the sameSite attribute was unset). The same is true for the WebExt Cookies API (this matches the Chrome implementation). For BiDi, we have to decide.

I like the idea of making sameSite optional here: https://w3c.github.io/webdriver-bidi/#cddl-type-networksamesite . And for now, we can implement an if-stmt to set it to SAMESITE_UNSET if secure is false. Would that work as a temporary solution?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sasha is going to have a look at this bug soon.

Comment 15

[Alexandra Borovova [:Sasha]]

Alright, looks like we have an agreement that it would be preferable to omit sameSite property in case it's not set. I've created the spec PR to update/discuss it: https://github.com/w3c/webdriver-bidi/pull/942.