Remove session id and encrypted session id from session data by rclmenezes · Pull Request #114 · fastify/session

rclmenezes · 2022-08-10T22:16:11Z

Picking up where @SimenB left off here: #101

First, I rebased feat: remove session id from session data #101 on main. Needed to fix some Ava -> Tap stuff
Next, I cleaned up the tests a little bit. There are some tests that modify a session or set expiration when that isn't the part of the test. It's best to keep tests more focused because otherwise bugs and slip through, as I'll talk about later.
@SimenB had some questions about the "multiple secrets" tests. The expiration was a red herring. The real issue was that sessionId/expiredSessionId wasn't in the session store, so the test always reset the sessionId. I made it cleaner.
Finally, I found a bug in the original PR where saveUninitialized: true and maxAge wouldn't set a new cookie. It was because the hashing mechanism for dates converted all of them to the string "{}" by accident.

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

Benchmarks

Running locally. Before:

$ node benchmark/bench.js
╔══════════════╤═════════╤═══════════════╤═══════════╤═════════════════════════╗
║ Slower tests │ Samples │        Result │ Tolerance │ Difference with slowest ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ file         │      25 │  32.68 op/sec │ ±  7.20 % │                         ║
║ redis        │      25 │ 139.43 op/sec │ ± 18.75 % │ + 326.64 %              ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ Fastest test │ Samples │        Result │ Tolerance │ Difference with slowest ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ memory       │      25 │ 760.16 op/sec │ ± 38.32 % │ + 2225.96 %             ║
╚══════════════╧═════════╧═══════════════╧═══════════╧═════════════════════════╝

✨  Done in 27.04s.

After:

$ node benchmark/bench.js
╔══════════════╤═════════╤═══════════════╤═══════════╤═════════════════════════╗
║ Slower tests │ Samples │        Result │ Tolerance │ Difference with slowest ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ file         │      25 │  27.58 op/sec │ ± 15.34 % │                         ║
║ redis        │      25 │  80.42 op/sec │ ± 25.62 % │ + 191.56 %              ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ Fastest test │ Samples │        Result │ Tolerance │ Difference with slowest ║
╟──────────────┼─────────┼───────────────┼───────────┼─────────────────────────╢
║ memory       │      25 │ 781.85 op/sec │ ± 37.90 % │ + 2734.57 %             ║
╚══════════════╧═════════╧═══════════════╧═══════════╧═════════════════════════╝

✨  Done in 31.01s.

This meant that the test passed even though the session was not actually expired

- Need saveUnitialized: false and https for this condition to happen. - Problem was that we need to hash dates correctly. - Test revealed other problem: dates were not actually Date objects many times. Made all tests use a Date object for expires or removed expires entirely if the test did not need it

rclmenezes · 2022-08-10T22:19:37Z

      }
-      if (session?.expires && session.expires <= Date.now()) {
+      if (session?.cookie?.expires && session.cookie.expires <= new Date()) {
        const restoredSession = Session.restore(


We should re-evaluate:

Whether or not to use object params. Once we get to like 8 it gets pretty confusing

Whether Session.restore should exist at all. At this point, it's mostly a wrapper around prevSession

rclmenezes · 2022-08-10T22:45:33Z

+    this[sessionIdKey] = sessionId
+    this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret)
    this[originalHash] = this[hash]()
+    this.touch() // Needs to happen after originalHash is set, in case maxAge forces an update


This was a bug in the original PR. Added a test against it

rclmenezes · 2022-08-10T22:54:14Z

    set (id, data, cb) { cb(null) },
    get (id, cb) {
-      cb(null, { expires: Date.now() - 1000, cookie: { expires: Date.now() - 1000 } })
+      cb(null, { cookie: { expires: new Date(Date.now() - 1000) } })


As I explain elsewhere, expires is supposed to be a Date, not a number

SimenB

yay, thanks for picking this up!

SimenB · 2022-08-11T09:12:56Z

+    this[sessionIdKey] = sessionId
+    this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret)
    this[originalHash] = this[hash]()
+    this.touch() // Needs to happen after originalHash is set, in case maxAge forces an update


Uzlopak

I want to make a proper review. So dont merge it

Uzlopak · 2022-08-11T16:22:43Z

@rclmenezes

What is up with the failing unit test?

rclmenezes · 2022-08-11T17:52:32Z

Interesting -- failing test works locally. It appears to be a race condition somewhere.

My guess is that touch() is running so quickly in CI that session.isModified() is returning true. One moment...

…tants to make reading a little easier

rclmenezes · 2022-08-11T21:44:59Z

Ugh, I'm dumb. The cookie wasn't in fastify.inject so the test was wrong 🤦‍♂️

So it was making a new session. This would have a fresh cookies.expires value. Then .touch() would be called and either:

.touch() would happen quickly enough where the fresh Date.now() was the same. Nothing modified so the cookie wouldn't change. This happened in CI.
.touch() would take too long and the Date.now() was different, resulting in session.isModified() and triggering a set-cookie header. This happened locally and was what the test expected.

Uzlopak · 2022-08-11T21:49:51Z

To be honest this PR is hard to digest. Can we please fix one issue at a time?

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

rclmenezes · 2022-08-12T10:59:24Z

@Uzlopak happily! This PR is hard for me to digest too.

There were some tests that didn't do what they advertised, which caused a lot of grief for me. I can split it up into a PR that cleans up tests and a PR that does this change, if that helps?

SimenB · 2022-08-12T11:23:23Z

+    return Boolean(
+      this[originalHash] !== this[hash]() ||
+      // If maxAge is set, the session is modified.
+      // This is necessary because we don't read the cookie's expiration from the cookie itself.
+      // session.cookie.expires is initially set from Cookie(cookieOpts), so we cannot detect if it changed from maxAge alone.
+      this[maxAge]
+    )


Suggested change

return Boolean(

this[originalHash] !== this[hash]() ||

// If maxAge is set, the session is modified.

// This is necessary because we don't read the cookie's expiration from the cookie itself.

// session.cookie.expires is initially set from Cookie(cookieOpts), so we cannot detect if it changed from maxAge alone.

this[maxAge]

)

if (this[originalHash] !== this[hash]()) {

return true

}

// If maxAge is set, the session is modified.

// This is necessary because we don't read the cookie's expiration from the cookie itself.

// session.cookie.expires is initially set from Cookie(cookieOpts), so we cannot detect if it changed from maxAge alone.

return this[maxAge] != null

However, this seems wrong. Just setting a max age should never make isModified return true every single time

This is necessary because we don't read the cookie's expiration from the cookie itself.

We should probably do that, and isn't that what happens with your return sess.cookie.expires?.getTime() in the hash function?

and isn't that what happens with your... hash function?

I wish!

cookie.expires is set using maxAge upon initialization anyway:
https://github.com/fastify/session/blob/master/lib/cookie.js#L42-L44

But you're right, maxAge shouldn't auto-trigger a modification. I realized that if maxAge is set but rolling is false, we want it to stay.

Also, we probably want to eventually support session stores that implement the touch method. In this case, the session would not be modified, just the TTL would.

Reverting this but I'm not quite sure how to implement this otherwise... need to think about it

Ah -- I'm mistaken. This code is confusing :)

If rolling is false, Session.restore will be called, which addresses this.

Uzlopak · 2022-08-12T12:59:42Z

Guys... really split this PR in some logical parts. I am investigating some time on having some reliable benchmark as the current benchmark sucks currently. But yeah. Please lets split it.If you like we could first agree which are the parts and who wants to implement this or that feature/bug fix.

rclmenezes · 2022-08-12T13:02:55Z

Okay I'm going to split this PR up into a bunch of parts:

I'm gonna clean up the tests. Add some constants, etc. Also I'll add the missing documentation for rolling.
touch should be implemented differently. First, it should not be called in the Session constructor; sessions should not be touched if rolling is false or maxAge is not set. I'll make isTouched a boolean property of Session and if it's true, the session will get saved (for now). Eventually, if isTouched && !isModified fastifySession can invoke sessionStore.touch to update TTL instead of sessionStore.save
I'll remove expires, sessionid and encryptedSessionid from the session state, like in the original PR.

Sound good?

Uzlopak · 2022-08-12T13:11:20Z

Maybe simpler. would say following parts:

Add documentation for rolling
clean up tests, maybe another PR for for adding some constans
touch
expires are Date
sessionId?
expiredSessionId?

Uzlopak · 2022-08-12T13:13:09Z

Just get the PRs as small as possible keep them coming.

rclmenezes · 2022-08-12T14:23:33Z

Sounds great. Will update this comments as the PRs come in

Docs: Add missing documentation for README.md #116
Test clean up: Clean up tests #117
Re-signing multiple secrets bugfix: Bugfix: when there are multiple secrets, we should re-sign with latest #118
Removing expires from being stored outside the cookie in the session.
Bugfix for multiple secrets
Removing sessionid and encryptedSessionId from the session

rclmenezes · 2022-08-16T18:43:41Z

Closing in favor of:
#121

SimenB and others added 12 commits August 10, 2022 12:05

feat: remove session id from session data

28f5c93

use local variable

c07c6c5

do not add session id to session

69c0551

remove top level expires

07ccc0d

Run lint

6ea8222

Remove sessionId within sessions of test setups

732b089

Fix falsy -> notOk

ebb0a72

Rm unnecessary session modification in test

4d79c0a

This meant that the test passed even though the session was not actually expired

Fix tests for multiple secrets

ea68b95

Some linting

0dcc544

Clean up some of the uninitialized tests

96c180c

rclmenezes force-pushed the remove-session-id-and-encrypted-session-id-from-session-data branch from c123ed6 to 96c180c Compare August 10, 2022 22:44

rclmenezes commented Aug 10, 2022

View reviewed changes

Uzlopak mentioned this pull request Aug 11, 2022

feat: remove session id from session data #101

Closed

4 tasks

SimenB reviewed Aug 11, 2022

View reviewed changes

Update lib/fastifySession.js

5abe743

Uzlopak requested changes Aug 11, 2022

View reviewed changes

Test was because I wasn't setting the cookie... ugh. Adding some cons…

8efbe87

…tants to make reading a little easier

rclmenezes and others added 2 commits August 12, 2022 06:53

Fix issue where maxAge was not triggering a session modification

65712d2

Use getTime() instead of toISOString() in session hash

517f6d0

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

SimenB reviewed Aug 12, 2022

View reviewed changes

rclmenezes mentioned this pull request Aug 12, 2022

Remove session id and encrypted session id round 3 #121

Closed

4 tasks

rclmenezes closed this Aug 16, 2022

Uh oh!

Conversation

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

Uh oh!

Checklist

Benchmarks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Aug 11, 2022

Uh oh!

rclmenezes commented Aug 11, 2022

Uh oh!

rclmenezes commented Aug 11, 2022

Uh oh!

Uzlopak commented Aug 11, 2022

Uh oh!

rclmenezes commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rclmenezes Aug 12, 2022 • 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!

Uzlopak commented Aug 12, 2022

Uh oh!

rclmenezes commented Aug 12, 2022

Uh oh!

Uzlopak commented Aug 12, 2022

Uh oh!

Uzlopak commented Aug 12, 2022

Uh oh!

rclmenezes commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rclmenezes commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rclmenezes commented Aug 10, 2022 •

edited

Loading

rclmenezes Aug 12, 2022 •

edited

Loading

rclmenezes commented Aug 12, 2022 •

edited

Loading