Skip to content

feat: remove session id from session data #101

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
SimenB wants to merge 4 commits into from
Closed

feat: remove session id from session data #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a111700
feat: remove session id from session data
SimenB Jun 28, 2022
86ac8fc
use local variable
SimenB Jun 29, 2022
bd4706f
do not add session id to session
SimenB Jun 29, 2022
9e0819b
remove top level expires
SimenB Jun 30, 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 @@ -63,13 +63,14 @@ function decryptSession (sessionId, options, request, done) {
newSession(secret, request, cookieOpts, idGenerator, done)
return
}
if (session && session.expires && session.expires <= Date.now()) {
if (session && session.cookie && session.cookie.expires && session.cookie.expires <= Date.now()) {
const restoredSession = Session.restore(
request,
idGenerator,
cookieOpts,
secret,
session
session,
decryptedSessionId
)

restoredSession.destroy(err => {
Expand All @@ -88,15 +89,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
53 changes: 27 additions & 26 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) {
Comment thread
mcollina marked this conversation as resolved.
this.sessionId = this[generateId](this[requestKey])
this.encryptedSessionId = this[sign]()
}
this[sessionIdKey] = sessionId
this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret)
this[originalHash] = this[hash]()
}

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
Copy link
Copy Markdown
Contributor

@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 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}}

}

return val
Expand All @@ -172,12 +166,19 @@ module.exports = class Session {
return this[originalHash] !== this[hash]()
}

static restore (request, idGenerator, cookieOpts, secret, prevSession) {
const restoredSession = new Session(request, idGenerator, cookieOpts, secret, prevSession)
get sessionId () {
return this[sessionIdKey]
}

get encryptedSessionId () {
return this[encryptedSessionIdKey]
}

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)
Copy link
Copy Markdown
Member Author

@SimenB SimenB Jun 28, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@mcollina mcollina Jun 29, 2022

Choose a reason for hiding this comment

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

what should be done about this?

Copy link
Copy Markdown
Member Author

@SimenB SimenB Jun 29, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@mcollina mcollina Jun 29, 2022

Choose a reason for hiding this comment

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

Go for that!

restoredSession.cookie = restoredCookie
restoredSession.expires = restoredCookie.expires
restoredSession[originalHash] = restoredSession[hash]()
return restoredSession
}
Expand Down
18 changes: 8 additions & 10 deletions test/base.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ test('should support multiple secrets', async (t) => {
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', {
expires: Date.now() + 1000
Copy link
Copy Markdown
Member Author

@SimenB SimenB Jun 28, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@mcollina mcollina Jun 29, 2022

Choose a reason for hiding this comment

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

what would you like to do about this?

Copy link
Copy Markdown
Member Author

@SimenB SimenB Jun 29, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@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.

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.

Copy link
Copy Markdown
Contributor

@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.

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!

cookie: { expires: Date.now() - 1000 }
}, done)
})
})
Expand Down Expand Up @@ -98,9 +98,8 @@ test('should set session cookie using the default cookie name', async (t) => {
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() + 1000,
sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN',
Comment thread
mcollina marked this conversation as resolved.
cookie: { secure: true, httpOnly: true, path: '/' }
cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' }
}, done)
})
})
Expand All @@ -119,17 +118,16 @@ test('should set session cookie using the default cookie name', async (t) => {
})

t.is(statusCode, 200)
t.regex(cookie, /sessionId=undefined; Path=\/; HttpOnly; Secure/)
t.regex(cookie, /sessionId=.*\..*; Path=\/; HttpOnly; Secure/)
})

test('should create new session on expired session', async (t) => {
t.plan(2)
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() - 1000,
sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN',
cookie: { secure: true, httpOnly: true, path: '/' }
cookie: { expires: Date.now() - 1000, secure: true, httpOnly: true, path: '/' }
}, done)
})
})
Expand Down Expand Up @@ -163,12 +161,12 @@ test('should set session.expires if maxAge', async (t) => {
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() + 1000
cookie: { expires: Date.now() + 1000 }
}, done)
})
})
function handler (request, reply) {
t.truthy(request.session.expires)
t.truthy(request.session.cookie.expires)
reply.send(200)
}
const port = await testServer(handler, options, plugin)
Expand All @@ -187,7 +185,7 @@ test('should set new session cookie if expired', async (t) => {
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() + 1000
Copy link
Copy Markdown
Member Author

@SimenB SimenB Jun 28, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@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.

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...

cookie: { expires: Date.now() - 1000 }
}, done)
})
})
Expand Down Expand Up @@ -258,7 +256,7 @@ test('should create new session if cookie contains invalid session', async (t) =
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() + 1000
cookie: { expires: Date.now() + 1000 }
}, done)
})
})
Expand Down
29 changes: 7 additions & 22 deletions test/session.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ test('should destroy the session', async (t) => {
})

test('should add session.encryptedSessionId object to request', async (t) => {
t.plan(2)
t.plan(3)
const port = await testServer((request, reply) => {
t.truthy(request.session.encryptedSessionId)
// serialize, then deserialize to make sure it's gone
t.falsy(JSON.parse(JSON.stringify(request.session)).encryptedSessionId)
Comment thread
mcollina marked this conversation as resolved.
reply.send(200)
}, DEFAULT_OPTIONS)

Expand All @@ -59,22 +61,6 @@ test('should add session.cookie object to request', async (t) => {
t.is(statusCode, 200)
})

test('should add session.expires object to request', async (t) => {
t.plan(2)
const options = {
secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk',
cookie: { maxAge: 42 }
}
const port = await testServer((request, reply) => {
t.truthy(request.session.expires)
reply.send(200)
}, options)

const { statusCode } = await request(`http://localhost:${port}`)

t.is(statusCode, 200)
})

test('should add session.sessionId object to request', async (t) => {
t.plan(2)
const port = await testServer((request, reply) => {
Expand Down Expand Up @@ -223,9 +209,8 @@ test('should decryptSession with custom request object', async (t) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
testData: 'this is a test',
expires: Date.now() + 1000,
sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN',
cookie: { secure: true, httpOnly: true, path: '/' }
cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' }
}, done)
})

Expand Down Expand Up @@ -284,7 +269,7 @@ test('should bubble up errors with destroy call if session expired', async (t) =
const store = {
set (id, data, cb) { cb(null) },
get (id, cb) {
cb(null, { expires: Date.now() - 1000, cookie: { expires: Date.now() - 1000 } })
cb(null, { cookie: { expires: Date.now() - 1000 } })
},
destroy (id, cb) { cb(new Error('No can do')) }
}
Expand Down Expand Up @@ -325,7 +310,7 @@ test('should not reset session cookie expiration if rolling is false', async (t)
fastify.register(fastifyCookie)
fastify.register(fastifySession, options)
fastify.addHook('onRequest', (request, reply, done) => {
reply.send(request.session.expires)
reply.send(request.session.cookie.expires)
done()
})

Expand Down Expand Up @@ -362,7 +347,7 @@ test('should update the expires property of the session using Session#touch() ev
fastify.register(fastifySession, options)
fastify.addHook('onRequest', (request, reply, done) => {
request.session.touch()
reply.send(request.session.expires)
reply.send(request.session.cookie.expires)
done()
})

Expand Down
2 changes: 1 addition & 1 deletion test/store.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ test('should set new session cookie if expired', async (t) => {
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', {
expires: Date.now() - 1000
cookie: {expires: Date.now() - 1000}
}, done)
})
})
Expand Down
2 changes: 1 addition & 1 deletion types/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface SessionData extends ExpressSessionData {

encryptedSessionId: string;

/** Updates the `expires` property of the session. */
/** Updates the `expires` property of the session's cookie. */
touch(): void;

/**
Expand Down