playwright icon indicating copy to clipboard operation
playwright copied to clipboard

[Bug]: Playwright unit tests are setting cross-site cookies with "SameSite=None" but miss "Secure" to be set

Open whimboo opened this issue 7 months ago • 7 comments

Version

latest main

Steps to reproduce

Note that several Playwright tests currently set cross-site cookies with the SameSite=None attribute but miss to as well set the Secure attribute. Due to a recent change in Firefox Nightly this is not allowed anymore and as such those tests are failing.

One example can be found in browsercontext-clearcookies.spec.ts:92:4 › should remove cookies by domain.

Note that I discovered that issue by our BiDi jobs but I'm sure this as well affects non-BiDi.

As per https://web.dev/articles/samesite-cookies-explained#samesitenone_must_be_secure and other sites the secure attribute is required for such cookies.

@yury-s could someone please take a look at this? Thanks.

Expected behavior

The tests need to pass.

Actual behavior

The tests are failing with error messages like Invalid cookie: Cookie “%name%” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.

Additional context

No response

Environment

As run in GitHub actions

whimboo avatar Jun 06 '25 09:06 whimboo

The tests are failing with error messages like Invalid cookie: Cookie “%name%” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.

Can you be more specific, which tests are failing and in which environment?

I'm looking at browsercontext-clearcookies.spec.ts:92 that you mentioned and it currently passes. It does not set SameSite/Secure attributes explicitly, including the case of Bidi. In that case browser defaults should be applied. In our Firefox build we default to SameSite=None, Secure=false which is arguably wrong, but in Firefox code it's also not a clear-cut:

NS_IMETHODIMP Cookie::GetSameSite(int32_t* aSameSite) {
  if (StaticPrefs::network_cookie_sameSite_laxByDefault()) {
    *aSameSite = SameSite();
  } else {
    *aSameSite = RawSameSite();
  }
  return NS_OK;
}

Which basically means None if the pref is off.

There are some tests that explicitly set SameSite=None and we can update the to include Secure attribute. I'm not sure about the default behavior though (when SameSite is omitted), it varies across browsers. What would you suggest there?

yury-s avatar Jun 06 '25 19:06 yury-s

@yury-s the Firefox build that you are using is based on release, right? There was a recent change in Firefox Nightly (141) which triggered this issue. As such the test that I mentioned will not pass with a recent Firefox Nightly but fails with the above mentioned error. I cannot reference the bug given that it's a secure one you won't have access to.

The behavior applies to BiDi as well. When I check the Puppeteer debug logs I can se the following payload for this test:

  pw:browser [pid=80847][out] 1749240303910	RemoteAgent	DEBUG	WebDriverBiDiConnection 2f444c24-d6ca-4676-bca5-c1e482e8ae97 -> {"id":22,"method":"storage.setCookie","params":{"cookie":{"name":"cookie1","value":{"type":"string","value":"1"},"domain":"localhost","path":"/","httpOnly":false,"secure":false,"sameSite":"none"},"partition":{"type":"storageKey","userContext":"bc5e5ad3-407d-44ca-b00e-57f4e4a41dc6"}}} +0ms
  pw:browser [pid=80847][out] 1749240303910	RemoteAgent	TRACE	Received command storage.setCookie for destination ROOT +0ms
  pw:browser [pid=80847][out] 1749240303910	RemoteAgent	DEBUG	WebDriverBiDiConnection 2f444c24-d6ca-4676-bca5-c1e482e8ae97 <- {"type":"error","id":22,"error":"unable to set cookie","message":"Invalid cookie: Cookie “cookie1” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.","stacktrace":"RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8\nWebDriverError@chrome://remote/content/shared/webdr

It sets the sameSite property to none, which then triggers the problem. It's not really omitted by default.

whimboo avatar Jun 06 '25 20:06 whimboo

I see. I assume our tests will start failing once we roll Firefox past the change you mentioned.

It looks like the behavior did change in Firefox around v131

Image

so it makes sense to set it to set Secure=true if we are adding SameSite=None in Firefox.

cc @aslushnikov

yury-s avatar Jun 06 '25 21:06 yury-s

This is what I see with the latest Firefox nightly (141.0a1), we set cookies without explicit sameSite/secure attributes and the commands succeed:

  pw:protocol SEND ► {"id":8,"method":"storage.setCookie","params":{"cookie":{"name":"cookie1","value":{"type":"string","value":"1"},"domain":"localhost","path":"/"},"partition":{"type":"storageKey","userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +19ms
  pw:protocol SEND ► {"id":9,"method":"storage.setCookie","params":{"cookie":{"name":"cookie2","value":{"type":"string","value":"2"},"domain":"127.0.0.1","path":"/"},"partition":{"type":"storageKey","userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +1ms
  pw:protocol ◀ RECV {"type":"success","id":8,"result":{"partitionKey":{"userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +1ms
  pw:protocol ◀ RECV {"type":"success","id":9,"result":{"partitionKey":{"userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +0ms
cookies set

If I read the cookies right after, they both have sameSite=none, secure=false:

  pw:protocol SEND ► {"id":10,"method":"storage.getCookies","params":{"partition":{"type":"storageKey","userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +1ms
  pw:protocol ◀ RECV {"type":"success","id":10,"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":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +1ms

However, later on, when setting the cookie cookie1 with exact same options as we just received from the browser, the browser returns an error:

...
  pw:protocol SEND ► {"id":22,"method":"storage.deleteCookies","params":{"partition":{"type":"storageKey","userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +0ms
  pw:protocol ◀ RECV {"type":"success","id":22,"result":{"partitionKey":{"userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +1ms
  pw:protocol SEND ► {"id":23,"method":"storage.setCookie","params":{"cookie":{"name":"cookie1","value":{"type":"string","value":"1"},"domain":"localhost","path":"/","httpOnly":false,"secure":false,"sameSite":"none"},"partition":{"type":"storageKey","userContext":"b4ecad63-499c-4b05-8155-a2621002d5ce"}}} +0ms
  pw:protocol ◀ RECV {"type":"error","id":23,"error":"unable to set cookie","message":"Invalid cookie: Cookie “cookie1” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.","stacktrace":"RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8\nWebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:199:5\nUnableToSetCookieError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:828:5\nsetCookie@chrome://remote/content/webdriver-bidi/modules/root/storage.sys.mjs:326:13\nhandleCommand@chrome://remote/content/shared/messagehandler/MessageHandler.sys.mjs:260:33\nexecute@chrome://remote/content/shared/webdriver/Session.sys.mjs:387:32\nonPacket@chrome://remote/content/webdriver-bidi/WebDriverBiDiConnection.sys.mjs:236:37\nonMessage@chrome://remote/content/server/WebSocketTransport.sys.mjs:127:18\nhandleEvent@chrome://remote/content/server/WebSocketTransport.sys.mjs:109:14\n"} +1ms

@whimboo Shouldn't Firefox in this case convert missing sameSite attribute into SameSite=None and Secure=true or do what it does today when receives such Set-Cookie header from the server? If cookies with missing SameSite attribute in Set-Cookie header are now dropped on the floor, then storage.setCookie should also throw on an attempt to set such cookie to make it clear that it's not gonna work.

yury-s avatar Jun 09 '25 17:06 yury-s

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1971488 on our side to figure out the correct behavior.

whimboo avatar Jun 11 '25 09:06 whimboo

@yury-s I did some investigation over on https://bugzilla.mozilla.org/show_bug.cgi?id=1971488#c5 and noticed that Playwright might fail here because you are no longer using the latest version of Firefox Nightly for BiDi tests. Can you please check that? I have the feeling that it is from before June 3rd - maybe still the first Nightly from the 141 nightly train?

whimboo avatar Jun 16 '25 11:06 whimboo

Please drop my last comment. It seems to be indeed a platform issue on our side.

whimboo avatar Jun 16 '25 13:06 whimboo

A WebDriver BiDi spec discussion is happening at https://github.com/w3c/webdriver-bidi/pull/942.

whimboo avatar Jun 27 '25 07:06 whimboo

The spec changes are merged, and Firefox changes have landed. So what we did is we added support for a new default value for sameSite argument. Which is set in case sameSite is not set, with e.g. document.cookie. The last thing left to do here is to add support for the "default" value to Playwright.

lutien avatar Jul 07 '25 08:07 lutien

I think the "default" value should also be exposed in the Playwright API (#36877).

hbenl avatar Jul 31 '25 14:07 hbenl

FWIW I think it's wrong that Playwright Firefox and Safari currently report cookies with an unspecified SameSite attribute to have SameSite=None. This is expected behavior according to the tests but I don't think it's how these browsers actually work but (if I understand correctly) was introduced by the Playwright patches (Firefox, Safari)

hbenl avatar Jul 31 '25 14:07 hbenl