feat: remove session id from session data #101

SimenB · 2022-06-28T14:30:01Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Fixes #52
Closes #70

I'm not 100% sure about the test changes, would like a second pair of eyes

lib/session.js

SimenB · 2022-06-28T14:31:52Z

lib/session.js

    const restoredCookie = new Cookie(cookieOpts)
-    restoredCookie.expires = new Date(prevSession.cookie.expires)


we don't check cookie before calling restore (

session/lib/fastifySession.js

Line 66 in 76459c2

if (session && session.expires && session.expires <= Date.now()) {

), only expires. So this could be a TypeError, depending on what's in the store

what should be done about this?

either works I think, but I think it's weird to look at the expiry of the cookie and not the expiry itself.

That said, express-session only looks at the cookie data, and have not hoisted expires up into its own top level thing. So we could do the same here and just get rid of the top level expires

Go for that!

SimenB · 2022-06-28T14:32:39Z

test/base.test.js

      request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', {
-        expires: Date.now() + 1000


I don't understand this test, so this change is probably wrong

what would you like to do about this?

I need to dig into the test to understand what it's supposed to test.

I think the intent of this test was to verify that the old secret still works. The expiration should be + 1000 to make sure the cookie hasn't expired yet.

I did some spelunkling. Why did this test pass earlier?

The session was modified

sessionId and encryptedSessionId were NOT in the session store

This meant that a new session was created to contain the modifications

Thanks to your changes, sessionid / encryptedSessionId don't have to be in the test prep. This means the existing session will be used!

SimenB · 2022-06-28T14:33:21Z

test/base.test.js

      request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
-        expires: Date.now() + 1000


this test is explicitly "if expired", not sure why expiry was set in the future?

Yup, this test only passed because of request.session.test = {} in the handler modified the session.

I'm starting forking off your branch, rebasing and removing...

test/session.test.js

test/base.test.js

jsardev · 2022-07-10T14:32:54Z

@SimenB Hey! Can I help somehow to finish this PR? I'd need this change as well 🙏

SimenB · 2022-07-21T10:24:24Z

@sarneeh sure, go for it!

We tried rolling this out at work, and had to revert due to some sessions seemingly not updating correctly. Unfortunately I haven't found the time yet to dig into it, which also means this PR is on the backburner for the moment (and now I'm on vacation, so won't get to it for at least a few more weeks). However, feel free to pick this up in the meantime!

rclmenezes

Rebased + fixed some issues in: #114

rclmenezes · 2022-08-10T17:24:36Z

test/base.test.js

      request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
-        expires: Date.now() + 1000


Yup, this test only passed because of request.session.test = {} in the handler modified the session.

I'm starting forking off your branch, rebasing and removing...

rclmenezes · 2022-08-10T17:47:34Z

test/base.test.js

      request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', {
-        expires: Date.now() + 1000


I think the intent of this test was to verify that the old secret still works. The expiration should be + 1000 to make sure the cookie hasn't expired yet.

rclmenezes · 2022-08-10T18:36:31Z

test/base.test.js

      request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', {
-        expires: Date.now() + 1000


I did some spelunkling. Why did this test pass earlier?

The session was modified

sessionId and encryptedSessionId were NOT in the session store

This meant that a new session was created to contain the modifications

Thanks to your changes, sessionid / encryptedSessionId don't have to be in the test prep. This means the existing session will be used!

rclmenezes · 2022-08-10T19:26:24Z

lib/session.js

  [hash] () {
    const sess = this
    const str = stringify(sess, function (key, val) {
      // ignore sess.cookie property
      if (this === sess && key === 'cookie') {
-        return
+        // we want `touch` to affect the hash of the session
+        return sess.cookie.expires


This needs to be sess.cookie.expires.toISOString() or something. The Date object gets transformed into {} by stringify

e.g. str becomes something like {"cookie":{},"csrfSecret":"5iGefd13a29MxLBIMvAT5hrZ","passport":{"user":7}}

Uzlopak · 2022-08-11T06:38:14Z

closing this in favor of #114

feat: remove session id from session data

a111700

SimenB commented Jun 28, 2022

View reviewed changes

SimenB mentioned this pull request Jun 28, 2022

Do not leak encryptedSessionId to session #70

Closed

4 tasks

SimenB commented Jun 28, 2022

View reviewed changes

test/base.test.js Show resolved Hide resolved

use local variable

86ac8fc

SimenB force-pushed the remove-session-id-and-encrypted-session-id-from-session-data branch from bd44a82 to a371c36 Compare June 29, 2022 08:49

do not add session id to session

bd4706f

SimenB force-pushed the remove-session-id-and-encrypted-session-id-from-session-data branch from a371c36 to bd4706f Compare June 29, 2022 08:50

remove top level expires

9e0819b

This was referenced Aug 10, 2022

Add cookiePrefix as an option to allow for compatibility with express-session #113

Merged

Remove session id and encrypted session id from session data #114

Closed

rclmenezes reviewed Aug 10, 2022

View reviewed changes

Uzlopak closed this Aug 11, 2022

rclmenezes mentioned this pull request Aug 12, 2022

Move expires from session to session.cookies #120

Merged

4 tasks

rclmenezes mentioned this pull request Aug 22, 2022

Remove session id round 4 #134

Merged

4 tasks

SimenB deleted the remove-session-id-and-encrypted-session-id-from-session-data branch August 25, 2022 08:37

SimenB mentioned this pull request Aug 25, 2022

@fastify/session v10.0.0 or What is missing for the next version? #146

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove session id from session data #101

feat: remove session id from session data #101

SimenB commented Jun 28, 2022

SimenB Jun 28, 2022

mcollina Jun 29, 2022

SimenB Jun 29, 2022

mcollina Jun 29, 2022

SimenB Jun 28, 2022

mcollina Jun 29, 2022

SimenB Jun 29, 2022

rclmenezes Aug 10, 2022

rclmenezes Aug 10, 2022

SimenB Jun 28, 2022

rclmenezes Aug 10, 2022

jsardev commented Jul 10, 2022 •

edited

Loading

SimenB commented Jul 21, 2022

rclmenezes left a comment

rclmenezes Aug 10, 2022

rclmenezes Aug 10, 2022

rclmenezes Aug 10, 2022

rclmenezes Aug 10, 2022

Uzlopak commented Aug 11, 2022

		const restoredCookie = new Cookie(cookieOpts)
		restoredCookie.expires = new Date(prevSession.cookie.expires)

		request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', {
		expires: Date.now() + 1000

		request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
		expires: Date.now() + 1000

feat: remove session id from session data #101

feat: remove session id from session data #101

Conversation

SimenB commented Jun 28, 2022

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsardev commented Jul 10, 2022 • edited Loading

SimenB commented Jul 21, 2022

rclmenezes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented Aug 11, 2022

jsardev commented Jul 10, 2022 •

edited

Loading