Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rclmenezes wants to merge 16 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
28f5c93
feat: remove session id from session data
SimenB Jun 28, 2022
c07c6c5
use local variable
SimenB Jun 29, 2022
69c0551
do not add session id to session
SimenB Jun 29, 2022
07ccc0d
remove top level expires
SimenB Jun 30, 2022
6ea8222
Run lint
rclmenezes Aug 10, 2022
732b089
Remove sessionId within sessions of test setups
rclmenezes Aug 10, 2022
ebb0a72
Fix falsy -> notOk
rclmenezes Aug 10, 2022
4d79c0a
Rm unnecessary session modification in test
rclmenezes Aug 10, 2022
ea68b95
Fix tests for multiple secrets
rclmenezes Aug 10, 2022
b73c0ab
Fix a bug where maxAge was not setting a new cookie
rclmenezes Aug 10, 2022
0dcc544
Some linting
rclmenezes Aug 10, 2022
96c180c
Clean up some of the uninitialized tests
rclmenezes Aug 10, 2022
5abe743
Update lib/fastifySession.js
Uzlopak Aug 11, 2022
8efbe87
Test was because I wasn't setting the cookie... ugh. Adding some cons…
rclmenezes Aug 11, 2022
65712d2
Fix issue where maxAge was not triggering a session modification
rclmenezes Aug 12, 2022
517f6d0
Use getTime() instead of toISOString() in session hash
rclmenezes Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ Allows to destroy the session in the store. If you do not pass a callback, a Pro

#### Session#touch()

Updates the `expires` property of the session.
Updates the `expires` property of the session's cookie.

#### Session#regenerate(callback)

Expand Down
11 changes: 7 additions & 4 deletions lib/fastifySession.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ function decryptSession (sessionId, options, request, done) {
newSession(secret, request, cookieOpts, idGenerator, done)
return
}
if (session?.expires && session.expires <= Date.now()) {
if (session.cookie?.expires && session.cookie.expires <= Date.now()) {
const restoredSession = Session.restore(
Copy link
Copy Markdown
Contributor Author

@rclmenezes rclmenezes Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

request,
idGenerator,
cookieOpts,
secret,
session
session,
decryptedSessionId
)

restoredSession.destroy(err => {
Expand All @@ -87,15 +88,17 @@ function decryptSession (sessionId, options, request, done) {
idGenerator,
cookieOpts,
secret,
session
session,
decryptedSessionId
)
} else {
request.session = Session.restore(
request,
idGenerator,
cookieOpts,
secret,
session
session,
decryptedSessionId
)
}
done()
Expand Down
63 changes: 35 additions & 28 deletions lib/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,36 +10,33 @@ const stringify = configureStringifier({ bigint: false })

const maxAge = Symbol('maxAge')
const secretKey = Symbol('secretKey')
const sign = Symbol('sign')
const addDataToSession = Symbol('addDataToSession')
const generateId = Symbol('generateId')
const requestKey = Symbol('request')
const cookieOptsKey = Symbol('cookieOpts')
const originalHash = Symbol('originalHash')
const hash = Symbol('hash')
const sessionIdKey = Symbol('sessionId')
const encryptedSessionIdKey = Symbol('encryptedSessionId')

module.exports = class Session {
constructor (request, idGenerator, cookieOpts, secret, prevSession = {}) {
constructor (request, idGenerator, cookieOpts, secret, prevSession = {}, sessionId = idGenerator(request)) {
this[generateId] = idGenerator
this.expires = null
this.cookie = new Cookie(cookieOpts)
this[cookieOptsKey] = cookieOpts
this[maxAge] = cookieOpts.maxAge
this[secretKey] = secret
this[addDataToSession](prevSession)
this[requestKey] = request
this.touch()
if (!this.sessionId) {
this.sessionId = this[generateId](this[requestKey])
this.encryptedSessionId = this[sign]()
}
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
Copy link
Copy Markdown
Contributor Author

@rclmenezes rclmenezes Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@SimenB SimenB Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

}

touch () {
if (this[maxAge]) {
this.expires = new Date(Date.now() + this[maxAge])
this.cookie.expires = this.expires
this.cookie.expires = new Date(Date.now() + this[maxAge])
}
}

Expand Down Expand Up @@ -71,7 +68,7 @@ module.exports = class Session {

[addDataToSession] (prevSession) {
for (const key in prevSession) {
if (!['expires', 'cookie'].includes(key)) {
if (!['cookie', 'sessionId', 'encryptedSessionId'].includes(key)) {
this[key] = prevSession[key]
}
}
Expand All @@ -87,14 +84,14 @@ module.exports = class Session {

destroy (callback) {
if (callback) {
this[requestKey].sessionStore.destroy(this.sessionId, error => {
this[requestKey].sessionStore.destroy(this[sessionIdKey], error => {
this[requestKey].session = null

callback(error)
})
} else {
return new Promise((resolve, reject) => {
this[requestKey].sessionStore.destroy(this.sessionId, error => {
this[requestKey].sessionStore.destroy(this[sessionIdKey], error => {
this[requestKey].session = null

if (error) {
Expand All @@ -109,15 +106,15 @@ module.exports = class Session {

reload (callback) {
if (callback) {
this[requestKey].sessionStore.get(this.sessionId, (error, session) => {
this[requestKey].session = new Session(this[requestKey], this[generateId], this[cookieOptsKey], this[secretKey], session)
this[requestKey].sessionStore.get(this[sessionIdKey], (error, session) => {
this[requestKey].session = new Session(this[requestKey], this[generateId], this[cookieOptsKey], this[secretKey], session, this[sessionIdKey])

callback(error)
})
} else {
return new Promise((resolve, reject) => {
this[requestKey].sessionStore.get(this.sessionId, (error, session) => {
this[requestKey].session = new Session(this[requestKey], this[generateId], this[cookieOptsKey], this[secretKey], session)
this[requestKey].sessionStore.get(this[sessionIdKey], (error, session) => {
this[requestKey].session = new Session(this[requestKey], this[generateId], this[cookieOptsKey], this[secretKey], session, this[sessionIdKey])

if (error) {
reject(error)
Expand All @@ -131,12 +128,12 @@ module.exports = class Session {

save (callback) {
if (callback) {
this[requestKey].sessionStore.set(this.sessionId, this, error => {
this[requestKey].sessionStore.set(this[sessionIdKey], this, error => {
callback(error)
})
} else {
return new Promise((resolve, reject) => {
this[requestKey].sessionStore.set(this.sessionId, this, error => {
this[requestKey].sessionStore.set(this[sessionIdKey], this, error => {
if (error) {
reject(error)
} else {
Expand All @@ -147,16 +144,13 @@ module.exports = class Session {
}
}

[sign] () {
return cookieSignature.sign(this.sessionId, this[secretKey])
}

[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?.getTime()
}

return val
Expand All @@ -169,15 +163,28 @@ module.exports = class Session {
}

isModified () {
return this[originalHash] !== this[hash]()
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]
)
Comment on lines +166 to +172
Copy link
Copy Markdown
Member

@SimenB SimenB Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@SimenB SimenB Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@rclmenezes rclmenezes Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@rclmenezes rclmenezes Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

get sessionId () {
return this[sessionIdKey]
}

get encryptedSessionId () {
return this[encryptedSessionIdKey]
}

static restore (request, idGenerator, cookieOpts, secret, prevSession) {
const restoredSession = new Session(request, idGenerator, cookieOpts, secret, prevSession)
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)
restoredSession.cookie = restoredCookie
restoredSession.expires = restoredCookie.expires
restoredSession[originalHash] = restoredSession[hash]()
return restoredSession
}
Expand Down
Loading