feat: remove session id from session data by SimenB · Pull Request #101 · fastify/session

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

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

+  static restore (request, idGenerator, cookieOpts, secret, prevSession, sessionId) {
+    const restoredSession = new Session(request, idGenerator, cookieOpts, secret, prevSession, sessionId)
    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

    fastify.addHook('onRequest', (request, reply, done) => {
+      const expires = Date.now() - 1000
      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

    fastify.addHook('onRequest', (request, reply, done) => {
+      const expires = Date.now() - 1000
      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...

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

    fastify.addHook('onRequest', (request, reply, done) => {
+      const expires = Date.now() - 1000
      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

    fastify.addHook('onRequest', (request, reply, done) => {
+      const expires = Date.now() - 1000
      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

    fastify.addHook('onRequest', (request, reply, done) => {
+      const expires = Date.now() - 1000
      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

      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

Comment thread test/base.test.js

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

Uh oh!

Conversation

SimenB commented Jun 28, 2022

Checklist

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

jsardev commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Jul 21, 2022

Uh oh!

rclmenezes left a comment

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!

Uzlopak commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jsardev commented Jul 10, 2022 •

edited

Loading