Skip to content

Conversation

@Venefilyn
Copy link

@Venefilyn Venefilyn commented Jun 15, 2020

Fixes #55

This could be considered a breaking change since session is now accessed through req.session rather than req[cookieName]. This can probably be fixed with a Proxy but not sure if that's ideal

This dependency change also goes away from encryption and uses verification instead

@Venefilyn
Copy link
Author

@Rhysjc could we get this reviewed?

@hamishhossack
Copy link
Contributor

This could be considered a breaking change since session is now accessed through req.session rather than req[cookieName]. This can probably be fixed with a Proxy but not sure if that's ideal

I think that the req[opts.sessionName] -> req.session is a valid change - this would rarely be used (if at all) externally since the session would be accessed via the cookie not request.

LGTM, but agree it's breaking the api so a major version would be required. @samtgarson thoughts?

@samtgarson
Copy link
Contributor

Agreed, this looks great. Thanks!

Quick question though @spytec, I'm wondering why you chose lax for the SameSite policy, and if this should be strict given this is an authentication session, or at least user configurable (with a sensible default).

@Venefilyn
Copy link
Author

strict would definitely be more secure, but that would definitely invoke a breaking change. lax is the default in browsers and has always been. Making it user configurable makes sense to me, I can get on that if you want

@samtgarson
Copy link
Contributor

samtgarson commented Aug 21, 2020

@spytec makes sense.

For now we'd like to try and keep the session encrypted, @hamishhossack has an alternative fix for #55 that will hopefully allow us to achieve a win win—we'll get back to you next week. Ignore this, he's already pushed #63 and replied on #55.

@Venefilyn Venefilyn closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace client-sessions to avoid nuxt-oauth breaking

4 participants