Session cookie sameSite option

[ropaolle]

I added a session cookie sameSite option. It may be too early to implement as sameSite still is a Draft. Not sure.

SameSite is an attribute that has not yet been fully standardized, and may change in the future. This also means many clients may ignore this attribute until they understand it.

[changeset-bot]

🦋 Changeset is good to go

Latest commit: fc9f22a

We got this.

This PR includes changesets to release 2 packages

Name	Type
@keystonejs/keystone	Minor
@keystonejs/session	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[timleslie]

I think this PR highlights that we should perhaps have a different approach to doing cookie config. I'd rather not have the Keystone class API need to match every possible cookie option. It would be better if we could pass through a single cookie object and have it spread into the expressSession constructor along with some sane defaults.

[ropaolle]

This will introduce a couple of breaking changes in passport.js, social-login and the documentation. I could do something like this and update related files and documentation.

Is this a good direction or will it cause to much issues for existing projects?

- cookieSecret = 'qwerty',
- sessionStore,
- secureCookies = process.env.NODE_ENV === 'production', // Default to true in production
- cookieMaxAge = 1000 * 60 * 60 * 24 * 30, // 30 days
- cookieSameSite = false,
+ cookie = {
+   sessionStore: null,
+   cookieSecret: 'qwerty',
+   secureCookies: process.env.NODE_ENV === 'production', // Default to true in production
+   cookieMaxAge: 1000 * 60 * 60 * 24 * 30, // 30 days
+   cookieSameSite: false,
+ },

this._sessionManager = new SessionManager({
-  cookieSecret,
-  secureCookies,
-  cookieMaxAge,
-  cookieSameSite,
-  sessionStore,
+  ...cookie,
});

[timleslie]

This will introduce a couple of breaking changes in passport.js, social-login and the documentation. I could do something like this and update related files and documentation.

Is this a good direction or will it cause to much issues for existing projects?

- cookieSecret = 'qwerty',
- sessionStore,
- secureCookies = process.env.NODE_ENV === 'production', // Default to true in production
- cookieMaxAge = 1000 * 60 * 60 * 24 * 30, // 30 days
- cookieSameSite = false,
+ cookie = {
+   sessionStore: null,
+   cookieSecret: 'qwerty',
+   secureCookies: process.env.NODE_ENV === 'production', // Default to true in production
+   cookieMaxAge: 1000 * 60 * 60 * 24 * 30, // 30 days
+   cookieSameSite: false,
+ },

this._sessionManager = new SessionManager({
-  cookieSecret,
-  secureCookies,
-  cookieMaxAge,
-  cookieSameSite,
-  sessionStore,
+  ...cookie,
});

That's more or less it, except it would only be secureCookies and cookieMaxAge (which should be renamed secure and maxAge respectively) so that the cookie object can be spread into the the cookie argument to expressSession at line 61 of session.js.

[ropaolle]

The intention with this PR was to add the sameSite property. However, by passing an object we can also configure other cookie properties domain, expires, httpOnly and path. Should Keystone include default values also for thees or shall we just ignore them?

[ropaolle]

That's more or less it, except it would only be secureCookies and cookieMaxAge (which should be renamed secure and maxAge respectively) so that the cookie object can be spread into the the cookie argument to expressSession at line 61 of session.js.

Not sure what @timleslie mean with "spread into the the cookie argument". I just passed a cookie object and used as is in session.js, please check my implementation.

[timleslie]

Thanks @ropaolle, the new changes look like what I had in mind. I'll review this today and let you know if there are any changes required, but I think this is getting really close now 👍

[timleslie]

Thanks @ropaolle, these changes have made it through in #2861 🎉