Support cookie SameSite=None option on nodejs by n2o1988 · Pull Request #8652 · SeleniumHQ/selenium

n2o1988 · 2020-08-27T17:36:28Z

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Allows cookies to be added with SameSite set to "None" in addition to "Lax" and "Strict", as per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite.
This option is only valid if the cookie is Secure (see https://web.dev/samesite-cookies-explained/#samesitenone-must-be-secure and proposal).

Motivation and Context

Follow up to #7901
Recent updates to Chrome 85 (and soon landing on Firefox) changed the default from None to Lax, requiring us to explicitly set sameSite: 'None', which the original implementation does not allow (not sure why, maybe None wasn't an option, back then?).
This also mirrors what the Java bindings support: https://github.com/SeleniumHQ/selenium/blob/trunk/java/client/src/org/openqa/selenium/Cookie.java#L111
Other bindings (such as Python) have the same issue and should be updated too (I'm not a Python dev though).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Allows cookies to be set with SameSite=None in addition to "Lax" and "Strict". As per https://web.dev/samesite-cookies-explained/, this option is only valid if the cookie is set as Secure.

CLAassistant · 2020-08-27T17:36:33Z

All committers have signed the CLA.

n2o1988 · 2020-08-27T17:53:49Z

There's an existing failing test (at least on my machine)

[chrome] can send commands to devtools and get return
Message:
TypeError: driver.sendDevToolsCommandAndGetReturn is not a function

This error occurs in the trunk branch too, so nothing related to this PR.

I'm not sure why the Lint check failed, I use an ESLint extension in VSCode that's supposed to auto format everything according to the repo's rules. The error doesn't seem related to my change.

n2o1988

A couple of clarifications

n2o1988 · 2020-08-27T18:02:27Z

 *
 *
- * @type {(!Date|number|undefined)}
+ * @type {(string|undefined)}


I think this was a copy-paste mistake in the original PR.

n2o1988 · 2020-08-27T18:07:09Z

+        await driver.manage().addCookie(cookie)
+        await assert.doesNotReject(
+          async () => await driver.manage().addCookie(cookie)
+        )


Other test cases for SameSite take a different approach to test this: they set the cookie and read it straight after.
In case of None, the cookie has to be secure, and I found this means the getCookie call would not return any cookie (since the test page in localhost is not served via https I presume?).

I figured the best way to test this is to test that it does not throw an exception. In fact, an invalid combination (see below tests) would cause the underlying driver to raise an exception.
This sounds sufficient to me but let me know if this is not enough.

I did make sure that this works in our real test suites (where the app is served on https).

I think that this is fine

AutomatedTester · 2020-08-27T20:13:02Z

      }
    )
+
+    ignore(env.browsers(Browser.FIREFOX, Browser.IE, Browser.SAFARI)).it(


Shouldn't these tests work in Firefox?

I mirrored the original tests, didn't want to change too much. I'll double-check. Maybe it wasn't an option at the time of the original implementation but it is now.

You're right, updated.

AutomatedTester · 2020-08-27T20:13:18Z

+        await driver.manage().addCookie(cookie)
+        await assert.doesNotReject(
+          async () => await driver.manage().addCookie(cookie)
+        )


I think that this is fine

AutomatedTester · 2020-08-27T20:13:52Z

+      }
+    )
+
+    ignore(env.browsers(Browser.FIREFOX, Browser.IE, Browser.SAFARI)).it(


Shouldn't this work in Firefox?

KevinBon · 2020-08-28T08:04:46Z

@@ -1177,9 +1177,15 @@ class Options {
      expiry = Math.floor(date.getTime() / 1000)
    }



Based on the related webdriver specification

Defaults to None if omitted when adding a cookie.

We should set sameSite cookie by default as "None" when omitted.

Suggested change

if (!sameSite) {

sameSite = 'None'

}

I'm not too sure we should do that given that:

no other Selenium binding is doing that: all seem to leverage on the underlying driver/browser implementation for the default value

I think it's the Browsers' responsibility to define the default behavior. For instance, in Chrome 85 that's exactly what changed: default is now Lax for Chrome and soon for Firefox too. That change was the reason I opened this PR in the first place.

with the new security proposal, sameSite='None' should always be accompanied by Secure=true, so we'd need to take that into account

changing that would be a breaking change (nothing against it, but I'd like the maintainers' opinion)

Interested to hear thoughts from @AutomatedTester

KevinBon · 2020-08-28T08:14:07Z

+
+    if (sameSite === 'None' && !secure) {
+      throw new error.InvalidArgumentError(
+        `Invalid cookie configuration: SameSite=None must be Secure`


It helps further understanding that None is a string and we keep a consistent sameSite formatting

Suggested change

`Invalid cookie configuration: SameSite=None must be Secure`

'Invalid cookie configuration: sameSite="None" must be Secure'

I was following the cookie header format (e.g. SameSite=None; Secure; ).
I can change that if you feel strongly about it, but then it should be secure: true instead of Secure.

n2o1988 · 2020-09-01T15:03:49Z

@AutomatedTester any other thoughts on this one?

AutomatedTester · 2020-09-03T13:37:26Z

This looks like it is about ready to land. There are linting errors. Could you fix those please and I will get it landed.

n2o1988 · 2020-09-04T11:49:50Z

This looks like it is about ready to land. There are linting errors. Could you fix those please and I will get it landed.

@AutomatedTester the linting errors seem unrelated to my change.
I think you the same errors show up in the trunk branch?

It also seems like this "Super-Linter" has conflicting rules with the ones declared in the .eslintrc.js config
Example: prettier wants you to have a trailing comma for arrays on multiple lines.
The "Super-Linter" check on Github says the opposite.

/github/workspace/javascript/node/selenium-webdriver/lib/webdriver.js:45:28: Unexpected trailing comma.

diemol · 2020-09-04T12:05:02Z

@n2o1988 I just ran the linter again on trunk and it is passing. Somehow the changes to the files in this PR are making the linter fail. It'd be nice if you can have a look 🙏

n2o1988 · 2020-09-04T12:07:57Z

@diemol maybe I'm doing this wrong.
How do I run that linter locally?

My linting config says everything is good, locally (at least according to .eslintrc.js).

diemol · 2020-09-04T12:09:37Z

@n2o1988 I did not do it locally, I only ran the GitHub action job again on trunk.

n2o1988 · 2020-09-04T12:14:30Z

@diemol it's kinda hard to spot what's wrong in this case.

If you look at the output of the linter, it shows a whole bunch of things I haven't changed, and the rules it's using are conflicting with the rules specified by ESLint

I never touched those lines.. :|

AutomatedTester · 2020-09-04T12:17:50Z

Let me have a look at the linter stuff since I set it up

n2o1988 · 2020-09-07T15:52:50Z

@AutomatedTester any updates on merging this PR?

n2o1988 · 2020-09-08T11:11:50Z

@AutomatedTester @diemol 🆙

We're sort of in a hurry to get this released, otherwise we'll have to go the way of pinning our own fork (which I'd like to avoid).
Any chance this can get merged & released anytime soon?

n2o1988 · 2020-09-10T16:20:11Z

Up: any updates on merging this PR @AutomatedTester @diemol ?

diemol · 2020-09-10T21:33:07Z

Merging this, thank you for your contribution @n2o1988!

However, I cannot say when the next alpha release will happen.

n2o1988 · 2020-09-11T10:10:46Z

Thanks @diemol !
Is there a release tracker page or a way to get notified when the next alpha is released?

n2o1988 added 2 commits August 27, 2020 19:06

Modify: support cookie SameSite=None (c-nodejs)

14afd4c

Allows cookies to be set with SameSite=None in addition to "Lax" and "Strict". As per https://web.dev/samesite-cookies-explained/, this option is only valid if the cookie is set as Secure.

Fix sameSite doc

d348242

n2o1988 changed the title ~~Nodejs cookie samesite allow none~~ Support cookie SameSite=None option on nodejs bindings Aug 27, 2020

n2o1988 changed the title ~~Support cookie SameSite=None option on nodejs bindings~~ Support cookie SameSite=None option on nodejs Aug 27, 2020

n2o1988 commented Aug 27, 2020

View reviewed changes

AutomatedTester reviewed Aug 27, 2020

View reviewed changes

AutomatedTester added the C-nodejs JavaScript Bindings label Aug 27, 2020

KevinBon reviewed Aug 28, 2020

View reviewed changes

Allow tests on Firefox

3978375

n2o1988 closed this Aug 28, 2020

n2o1988 reopened this Aug 28, 2020

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

aa6d4f2

n2o1988 added 5 commits September 4, 2020 14:25

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

25b7d33

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

907ee9b

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

e8e9e1a

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

d8cc833

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

409d835

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

2b473e2

n2o1988 added 2 commits September 8, 2020 10:02

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

83a4e45

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

fce3da2

n2o1988 added 2 commits September 9, 2020 16:07

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

5140353

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

aceeb24

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

4d6b892

diemol approved these changes Sep 10, 2020

View reviewed changes

Merge branch 'trunk' into nodejs-cookie-samesite-allow-none

9049409

diemol merged commit 63d9869 into SeleniumHQ:trunk Sep 10, 2020

		@@ -1177,9 +1177,15 @@ class Options {
		expiry = Math.floor(date.getTime() / 1000)
		}

+if (!sameSite) {
+ sameSite = 'None'
+}

	`Invalid cookie configuration: SameSite=None must be Secure`
	'Invalid cookie configuration: sameSite="None" must be Secure'

Uh oh!

Uh oh!

Conversation

n2o1988 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

Checklist

Uh oh!

CLAassistant commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n2o1988 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n2o1988 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n2o1988 Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n2o1988 Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n2o1988 commented Sep 1, 2020

Uh oh!

AutomatedTester commented Sep 3, 2020

Uh oh!

n2o1988 commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diemol commented Sep 4, 2020

Uh oh!

n2o1988 commented Sep 4, 2020

Uh oh!

diemol commented Sep 4, 2020

Uh oh!

n2o1988 commented Sep 4, 2020

Uh oh!

AutomatedTester commented Sep 4, 2020

Uh oh!

n2o1988 commented Sep 7, 2020

Uh oh!

n2o1988 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n2o1988 commented Sep 10, 2020

Uh oh!

diemol commented Sep 10, 2020

Uh oh!

n2o1988 commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

n2o1988 commented Aug 27, 2020 •

edited

Loading

CLAassistant commented Aug 27, 2020 •

edited

Loading

n2o1988 commented Aug 27, 2020 •

edited

Loading

n2o1988 Aug 27, 2020 •

edited

Loading

n2o1988 Aug 28, 2020 •

edited

Loading

n2o1988 commented Sep 4, 2020 •

edited

Loading

n2o1988 commented Sep 8, 2020 •

edited

Loading