Remove session id and encrypted session id round 3#121
Remove session id and encrypted session id round 3#121rclmenezes wants to merge 5 commits intofastify:masterfrom
Conversation
rclmenezes
force-pushed
the
remove-session-id-and-encrypted-session-id-round-3
branch
4 times, most recently
from
b2b84c3 to
4cca4b9
Compare
| if (cookieOpts.maxAge) { | ||
| return new Date(Date.now() + cookieOpts.maxAge) | ||
| } | ||
| if (cookieOpts.expires) { | ||
| expires = cookieOpts.expires | ||
| } else if (cookieOpts.maxAge) { | ||
| expires = new Date(Date.now() + cookieOpts.maxAge) | ||
| return cookieOpts.expires |
There was a problem hiding this comment.
maxAge has priority over expires, according to the docs.
However, because Session was calling touch in its constructor, it was still doing the right behavior.
I removed that touch() because it is unnecessary, so I had to fix this
| this.sessionId = sessionId | ||
| this.encryptedSessionId = cookieSignature.sign(sessionId, secret) |
There was a problem hiding this comment.
I didn't make a sessionId or encryptedSessionid symbol because fastifySession.ts needs access to it anyway.
| if (prevSession.cookie.expires) { | ||
| // Need to parse as Date | ||
| restoredCookie.expires = new Date(prevSession.cookie.expires) | ||
| } |
There was a problem hiding this comment.
This is a bugfix! Otherwise Session.restore() will set expires, even when it's not set.
I discovered this bug as a result of the new test.
| request.sessionStore.set(sessionId, { | ||
| sessionId, | ||
| encryptedSessionId: sessionIdSignedWithOldSecret | ||
| test: {} |
There was a problem hiding this comment.
For accuracy -- sessionId / encryptedSessionId should never be in the session store anymore
rclmenezes
force-pushed
the
remove-session-id-and-encrypted-session-id-round-3
branch
from
b71415b to
6cac322
Compare
|
@Uzlopak -- do you want me to fix this up for merging? Or are you still doing other work that you want to merge first? |
|
Can you first create a PR which removes the sessionId from the stored session + unit tests? There is imho currently no tests for what is actually got stored. So I would really like to have atleast the sessionId issue solved with unit tests. Then we can assess how we can remove the encryptedSessionId issue. |
2 participants
Replaces: #114
Depends on a few other PRs being merged.
Checklist
npm run testandnpm run benchmarkand the Code of conduct