From 28f5c939460feb8ff09112fbdd88e0eed9e7aed1 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Tue, 28 Jun 2022 16:17:32 +0200 Subject: [PATCH 01/16] feat: remove session id from session data --- lib/fastifySession.js | 9 ++++++--- lib/session.js | 45 +++++++++++++++++++++++-------------------- test/base.test.js | 12 +++++++----- test/session.test.js | 5 +++-- 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/lib/fastifySession.js b/lib/fastifySession.js index da15eaa..12828d1 100644 --- a/lib/fastifySession.js +++ b/lib/fastifySession.js @@ -68,7 +68,8 @@ function decryptSession (sessionId, options, request, done) { idGenerator, cookieOpts, secret, - session + session, + decryptedSessionId ) restoredSession.destroy(err => { @@ -87,7 +88,8 @@ function decryptSession (sessionId, options, request, done) { idGenerator, cookieOpts, secret, - session + session, + decryptedSessionId ) } else { request.session = Session.restore( @@ -95,7 +97,8 @@ function decryptSession (sessionId, options, request, done) { idGenerator, cookieOpts, secret, - session + session, + decryptedSessionId ) } done() diff --git a/lib/session.js b/lib/session.js index 59eca9c..dd0debe 100644 --- a/lib/session.js +++ b/lib/session.js @@ -10,16 +10,17 @@ 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) @@ -29,10 +30,8 @@ module.exports = class Session { 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(this[sessionIdKey], secret) this[originalHash] = this[hash]() } @@ -87,14 +86,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) { @@ -109,15 +108,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) @@ -131,12 +130,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 { @@ -147,10 +146,6 @@ module.exports = class Session { } } - [sign] () { - return cookieSignature.sign(this.sessionId, this[secretKey]) - } - [hash] () { const sess = this const str = stringify(sess, function (key, val) { @@ -172,10 +167,18 @@ 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) + restoredCookie.expires = new Date(prevSession.expires) restoredSession.cookie = restoredCookie restoredSession.expires = restoredCookie.expires restoredSession[originalHash] = restoredSession[hash]() diff --git a/test/base.test.js b/test/base.test.js index 22dbf5b..119f177 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -52,8 +52,10 @@ test('should support multiple secrets', async (t) => { const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { + const expires = Date.now() - 1000 request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', { - expires: Date.now() + 1000 + cookie: { expires }, + expires }, done) }) }) @@ -103,7 +105,6 @@ test('should set session cookie using the default cookie name', async (t) => { 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: '/' } }, done) }) @@ -124,7 +125,7 @@ test('should set session cookie using the default cookie name', async (t) => { }) t.equal(response.statusCode, 200) - t.match(response.headers['set-cookie'], /sessionId=undefined; Path=\/; HttpOnly; Secure/) + t.match(response.headers['set-cookie'], /sessionId=.*\..*; Path=\/; HttpOnly; Secure/) }) test('should create new session on expired session', async (t) => { @@ -133,7 +134,6 @@ test('should create new session on expired session', async (t) => { 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: '/' } }, done) }) @@ -193,8 +193,10 @@ test('should set new session cookie if expired', async (t) => { const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { + const expires = Date.now() - 1000 request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { - expires: Date.now() + 1000 + cookie: { expires }, + expires }, done) }) }) diff --git a/test/session.test.js b/test/session.test.js index a1e5158..779f11f 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -42,9 +42,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 fastify = await buildFastify((request, reply) => { t.ok(request.session.encryptedSessionId) + // serialize, then deserialize to make sure it's gone + t.falsy(JSON.parse(JSON.stringify(request.session)).encryptedSessionId) reply.send(200) }, DEFAULT_OPTIONS) t.teardown(() => fastify.close()) @@ -249,7 +251,6 @@ test('should decryptSession with custom request object', async (t) => { 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: '/' } }, done) }) From c07c6c5ee40ac35729c9c573745d53b326b9e1a5 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Wed, 29 Jun 2022 10:13:38 +0200 Subject: [PATCH 02/16] use local variable --- lib/session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/session.js b/lib/session.js index dd0debe..9ca81df 100644 --- a/lib/session.js +++ b/lib/session.js @@ -31,7 +31,7 @@ module.exports = class Session { this[requestKey] = request this.touch() this[sessionIdKey] = sessionId - this[encryptedSessionIdKey] = cookieSignature.sign(this[sessionIdKey], secret) + this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret) this[originalHash] = this[hash]() } From 69c0551726721192d9e0d1f6a17e02b0daa96ae1 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Wed, 29 Jun 2022 10:48:41 +0200 Subject: [PATCH 03/16] do not add session id to session --- lib/session.js | 2 +- test/base.test.js | 2 ++ test/session.test.js | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/session.js b/lib/session.js index 9ca81df..8d3b10c 100644 --- a/lib/session.js +++ b/lib/session.js @@ -70,7 +70,7 @@ module.exports = class Session { [addDataToSession] (prevSession) { for (const key in prevSession) { - if (!['expires', 'cookie'].includes(key)) { + if (!['expires', 'cookie', 'sessionId', 'encryptedSessionId'].includes(key)) { this[key] = prevSession[key] } } diff --git a/test/base.test.js b/test/base.test.js index 119f177..1a713bb 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -105,6 +105,7 @@ test('should set session cookie using the default cookie name', async (t) => { 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: '/' } }, done) }) @@ -134,6 +135,7 @@ test('should create new session on expired session', async (t) => { 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: '/' } }, done) }) diff --git a/test/session.test.js b/test/session.test.js index 779f11f..911b49e 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -251,6 +251,7 @@ test('should decryptSession with custom request object', async (t) => { 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: '/' } }, done) }) From 07ccc0dbc792570fe225bc101a01acb29263484e Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 30 Jun 2022 08:54:53 +0200 Subject: [PATCH 04/16] remove top level expires --- README.md | 2 +- lib/fastifySession.js | 2 +- lib/session.js | 12 +++++------- test/base.test.js | 20 +++++++------------- test/session.test.js | 28 ++++------------------------ test/store.test.js | 2 +- types/types.d.ts | 2 +- 7 files changed, 20 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 7e72c2a..d0d020d 100644 --- a/README.md +++ b/README.md @@ -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) diff --git a/lib/fastifySession.js b/lib/fastifySession.js index 12828d1..c98f7ac 100644 --- a/lib/fastifySession.js +++ b/lib/fastifySession.js @@ -62,7 +62,7 @@ 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( request, idGenerator, diff --git a/lib/session.js b/lib/session.js index 8d3b10c..dd18f43 100644 --- a/lib/session.js +++ b/lib/session.js @@ -22,7 +22,6 @@ const encryptedSessionIdKey = Symbol('encryptedSessionId') module.exports = class Session { 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 @@ -37,8 +36,7 @@ module.exports = class Session { 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]) } } @@ -70,7 +68,7 @@ module.exports = class Session { [addDataToSession] (prevSession) { for (const key in prevSession) { - if (!['expires', 'cookie', 'sessionId', 'encryptedSessionId'].includes(key)) { + if (!['cookie', 'sessionId', 'encryptedSessionId'].includes(key)) { this[key] = prevSession[key] } } @@ -151,7 +149,8 @@ module.exports = class Session { 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 } return val @@ -178,9 +177,8 @@ module.exports = class Session { 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.expires) + restoredCookie.expires = new Date(prevSession.cookie.expires) restoredSession.cookie = restoredCookie - restoredSession.expires = restoredCookie.expires restoredSession[originalHash] = restoredSession[hash]() return restoredSession } diff --git a/test/base.test.js b/test/base.test.js index 1a713bb..c0304c2 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -52,10 +52,8 @@ test('should support multiple secrets', async (t) => { const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - const expires = Date.now() - 1000 request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', { - cookie: { expires }, - expires + cookie: { expires: Date.now() - 1000 } }, done) }) }) @@ -104,9 +102,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', - cookie: { secure: true, httpOnly: true, path: '/' } + cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' } }, done) }) }) @@ -134,9 +131,8 @@ test('should create new session on expired 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, sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', - cookie: { secure: true, httpOnly: true, path: '/' } + cookie: { expires: Date.now() - 1000, secure: true, httpOnly: true, path: '/' } }, done) }) }) @@ -171,12 +167,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.ok(request.session.expires) + t.ok(request.session.cookie.expires) reply.send(200) } const fastify = await buildFastify(handler, options, plugin) @@ -195,10 +191,8 @@ test('should set new session cookie if expired', async (t) => { const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - const expires = Date.now() - 1000 request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { - cookie: { expires }, - expires + cookie: { expires: Date.now() - 1000 } }, done) }) }) @@ -272,7 +266,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) }) }) diff --git a/test/session.test.js b/test/session.test.js index 911b49e..2f870a0 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -73,25 +73,6 @@ test('should add session.cookie object to request', async (t) => { t.equal(response.statusCode, 200) }) -test('should add session.expires object to request', async (t) => { - t.plan(2) - const options = { - secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', - cookie: { maxAge: 42 } - } - const fastify = await buildFastify((request, reply) => { - t.ok(request.session.expires) - reply.send(200) - }, options) - t.teardown(() => fastify.close()) - - const response = await fastify.inject({ - url: '/' - }) - - t.equal(response.statusCode, 200) -}) - test('should add session.sessionId object to request', async (t) => { t.plan(2) const fastify = await buildFastify((request, reply) => { @@ -250,9 +231,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) }) @@ -311,7 +291,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')) } } @@ -352,7 +332,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() }) @@ -389,7 +369,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() }) diff --git a/test/store.test.js b/test/store.test.js index 770986c..b61c528 100644 --- a/test/store.test.js +++ b/test/store.test.js @@ -101,7 +101,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) }) }) diff --git a/types/types.d.ts b/types/types.d.ts index c5c2ebf..66aca95 100644 --- a/types/types.d.ts +++ b/types/types.d.ts @@ -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; /** From 6ea82220def62d231cebc2fc71d3c1fbb6d81586 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 11:49:57 -0400 Subject: [PATCH 05/16] Run lint --- test/store.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/store.test.js b/test/store.test.js index b61c528..1117164 100644 --- a/test/store.test.js +++ b/test/store.test.js @@ -101,7 +101,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', { - cookie: {expires: Date.now() - 1000} + cookie: { expires: Date.now() - 1000 } }, done) }) }) From 732b08933d7c9c90c8c9aa5ce020f5dad2c2fe9e Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 12:01:29 -0400 Subject: [PATCH 06/16] Remove sessionId within sessions of test setups --- test/base.test.js | 1 - test/session.test.js | 1 - 2 files changed, 2 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index c0304c2..6d3160a 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -102,7 +102,6 @@ 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', { - sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' } }, done) }) diff --git a/test/session.test.js b/test/session.test.js index 2f870a0..100e222 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -231,7 +231,6 @@ 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', - sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' } }, done) }) From ebb0a72e55ac4bc7f48704926a0f0a0e16b8e664 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 13:19:00 -0400 Subject: [PATCH 07/16] Fix falsy -> notOk --- test/session.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/session.test.js b/test/session.test.js index 100e222..a23dc3b 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -46,7 +46,7 @@ test('should add session.encryptedSessionId object to request', async (t) => { const fastify = await buildFastify((request, reply) => { t.ok(request.session.encryptedSessionId) // serialize, then deserialize to make sure it's gone - t.falsy(JSON.parse(JSON.stringify(request.session)).encryptedSessionId) + t.notOk(JSON.parse(JSON.stringify(request.session)).encryptedSessionId) reply.send(200) }, DEFAULT_OPTIONS) t.teardown(() => fastify.close()) From 4d79c0a1e60691fbd2f932495607ad8f1d483a07 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 13:26:08 -0400 Subject: [PATCH 08/16] Rm unnecessary session modification in test This meant that the test passed even though the session was not actually expired --- test/base.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/base.test.js b/test/base.test.js index 6d3160a..b041a7e 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -196,7 +196,6 @@ test('should set new session cookie if expired', async (t) => { }) }) function handler (request, reply) { - request.session.test = {} reply.send(200) } const fastify = await buildFastify(handler, DEFAULT_OPTIONS, plugin) From ea68b95a1dd3ce6e9f93fc6195c24d7e51ef760b Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 13:38:05 -0400 Subject: [PATCH 09/16] Fix tests for multiple secrets --- test/base.test.js | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index b041a7e..9f7659f 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -45,7 +45,7 @@ test('should set session cookie', async (t) => { }) test('should support multiple secrets', async (t) => { - t.plan(2) + t.plan(4) const options = { secret: ['geheim', 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk'] } @@ -53,7 +53,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', { - cookie: { expires: Date.now() - 1000 } + cookie: { expires: Date.now() + 1000 } }, done) }) }) @@ -64,16 +64,29 @@ test('should support multiple secrets', async (t) => { const fastify = await buildFastify(handler, options, plugin) t.teardown(() => fastify.close()) - const response = await fastify.inject({ + const sessionIdEncryptedWithOldSecret = "aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.eiVu2YbrcqbTUYTYaANks%2Fjn%2Bjta7QgpsxLO%2BOLN%2F4U" + const sessionIdEncryptedWithNewSecret = "aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.InCp31AuDa7DX%2F8rGBz8RMFiCpmUtjcF%2BS7Aco7tur8" + + const responseForOldSecret = await fastify.inject({ url: '/', headers: { 'x-forwarded-proto': 'https', - cookie: 'sessionId=aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.eiVu2YbrcqbTUYTYaANks%2Fjn%2Bjta7QgpsxLO%2BOLN%2F4U; Path=/; HttpOnly; Secure' + cookie: `sessionId=${sessionIdEncryptedWithOldSecret}; Path=/; HttpOnly; Secure` } }) + t.equal(responseForOldSecret.statusCode, 200) + // It will be replaced with the new secret! + t.equal(responseForOldSecret.headers['set-cookie'].includes(sessionIdEncryptedWithNewSecret), true) - t.equal(response.statusCode, 200) - t.equal(response.headers['set-cookie'].includes('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e'), false) + const responseForNewSecret = await fastify.inject({ + url: '/', + headers: { + 'x-forwarded-proto': 'https', + cookie: `sessionId=${sessionIdEncryptedWithNewSecret}; Path=/; HttpOnly; Secure` + } + }) + t.equal(responseForNewSecret.statusCode, 200) + t.equal(responseForNewSecret.headers['set-cookie'].includes(sessionIdEncryptedWithNewSecret), true) }) test('should set session cookie using the specified cookie name', async (t) => { @@ -274,7 +287,7 @@ test('should create new session if cookie contains invalid session', async (t) = const response = await fastify.inject({ url: '/', headers: { - cookie: 'sessionId=Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN.B7fUDYXx9fXF9pNuL3qm4NVmSduLJ6kzCOPh5JhHGoE; Path=/; HttpOnly; Secure', + cookie: 'sessionId=Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN.ohnoinvalidnooooL3qm4NVmSduLJ6kzCOPh5JhHGoE; Path=/; HttpOnly; Secure', 'x-forwarded-proto': 'https' } }) From b73c0ab54a3bcf9fcaecce51e0215dc416a732ec Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 17:55:15 -0400 Subject: [PATCH 10/16] Fix a bug where maxAge was not setting a new cookie - 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 --- lib/fastifySession.js | 2 +- lib/session.js | 4 ++-- test/base.test.js | 43 ++++++++++++++++++++++++++++++++++--------- test/session.test.js | 4 ++-- test/store.test.js | 2 +- 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/lib/fastifySession.js b/lib/fastifySession.js index c98f7ac..dfa6e6d 100644 --- a/lib/fastifySession.js +++ b/lib/fastifySession.js @@ -62,7 +62,7 @@ function decryptSession (sessionId, options, request, done) { newSession(secret, request, cookieOpts, idGenerator, done) return } - if (session?.cookie?.expires && session.cookie.expires <= Date.now()) { + if (session?.cookie?.expires && session.cookie.expires <= new Date()) { const restoredSession = Session.restore( request, idGenerator, diff --git a/lib/session.js b/lib/session.js index dd18f43..5bea33f 100644 --- a/lib/session.js +++ b/lib/session.js @@ -28,10 +28,10 @@ module.exports = class Session { this[secretKey] = secret this[addDataToSession](prevSession) this[requestKey] = request - this.touch() this[sessionIdKey] = sessionId this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret) this[originalHash] = this[hash]() + this.touch() } touch () { @@ -150,7 +150,7 @@ module.exports = class Session { // ignore sess.cookie property if (this === sess && key === 'cookie') { // we want `touch` to affect the hash of the session - return sess.cookie.expires + return sess.cookie.expires?.toISOString() } return val diff --git a/test/base.test.js b/test/base.test.js index 9f7659f..0180688 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -53,7 +53,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', { - cookie: { expires: Date.now() + 1000 } + test: {} }, done) }) }) @@ -115,7 +115,7 @@ 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', { - cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' } + cookie: { secure: true, httpOnly: true, path: '/' } }, done) }) }) @@ -144,7 +144,7 @@ test('should create new session on expired session', async (t) => { fastify.addHook('onRequest', (request, reply, done) => { request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', - cookie: { expires: Date.now() - 1000, secure: true, httpOnly: true, path: '/' } + cookie: { expires: new Date(Date.now() - 1000), secure: true, httpOnly: true, path: '/' } }, done) }) }) @@ -170,8 +170,8 @@ test('should create new session on expired session', async (t) => { t.match(response.headers['set-cookie'], /sessionId=.*\..*; Path=\/; Expires=.*; HttpOnly; Secure/) }) -test('should set session.expires if maxAge', async (t) => { - t.plan(2) +test('should set session.cookie.expires if maxAge', async (t) => { + t.plan(3) const options = { secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', cookie: { maxAge: 42 } @@ -179,7 +179,7 @@ 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', { - cookie: { expires: Date.now() + 1000 } + test: {} }, done) }) }) @@ -192,10 +192,12 @@ test('should set session.expires if maxAge', async (t) => { const response = await fastify.inject({ url: '/', - headers: { cookie: DEFAULT_COOKIE } + headers: { cookie: DEFAULT_COOKIE, 'x-forwarded-proto': 'https'}, }) t.equal(response.statusCode, 200) + // Should be sending cookie as well + t.match(response.headers['set-cookie'], /sessionId=.*\..*; Path=\/; Expires=.*; HttpOnly; Secure/) }) test('should set new session cookie if expired', async (t) => { @@ -204,7 +206,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', { - cookie: { expires: Date.now() - 1000 } + cookie: { expires: new Date(Date.now() - 1000) } }, done) }) }) @@ -277,7 +279,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', { - cookie: { expires: Date.now() + 1000 } + test: {} }, done) }) }) @@ -314,3 +316,26 @@ test('should not set session cookie if no data in session and saveUninitialized t.equal(response.statusCode, 200) t.ok(response.headers['set-cookie'] === undefined) }) + +test('should set session cookie if saveUninitialized is false and data not modified but maxAge is on', async (t) => { + t.plan(2) + const options = { + cookie: { + maxAge: 42, + expires: new Date(Date.now() + 1000) + }, + secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', + saveUninitialized: false + } + const fastify = await buildFastify((request, reply) => reply.send(200), options) + t.teardown(() => fastify.close()) + + const response = await fastify.inject({ + url: '/', + headers: { 'x-forwarded-proto': 'https' } + }) + + t.equal(response.statusCode, 200) + t.ok(response.headers['set-cookie']) +}) + diff --git a/test/session.test.js b/test/session.test.js index a23dc3b..58fecfe 100644 --- a/test/session.test.js +++ b/test/session.test.js @@ -231,7 +231,7 @@ 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', - cookie: { expires: Date.now() + 1000, secure: true, httpOnly: true, path: '/' } + cookie: { secure: true, httpOnly: true, path: '/' } }, done) }) @@ -290,7 +290,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, { cookie: { expires: Date.now() - 1000 } }) + cb(null, { cookie: { expires: new Date(Date.now() - 1000) } }) }, destroy (id, cb) { cb(new Error('No can do')) } } diff --git a/test/store.test.js b/test/store.test.js index 1117164..1c4af10 100644 --- a/test/store.test.js +++ b/test/store.test.js @@ -101,7 +101,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', { - cookie: { expires: Date.now() - 1000 } + cookie: { expires: new Date(Date.now() - 1000) } }, done) }) }) From 0dcc54460380e958444e1f643d369f29d74a47f6 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 18:00:02 -0400 Subject: [PATCH 11/16] Some linting --- test/base.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index 0180688..5a1565d 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -64,8 +64,8 @@ test('should support multiple secrets', async (t) => { const fastify = await buildFastify(handler, options, plugin) t.teardown(() => fastify.close()) - const sessionIdEncryptedWithOldSecret = "aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.eiVu2YbrcqbTUYTYaANks%2Fjn%2Bjta7QgpsxLO%2BOLN%2F4U" - const sessionIdEncryptedWithNewSecret = "aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.InCp31AuDa7DX%2F8rGBz8RMFiCpmUtjcF%2BS7Aco7tur8" + const sessionIdEncryptedWithOldSecret = 'aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.eiVu2YbrcqbTUYTYaANks%2Fjn%2Bjta7QgpsxLO%2BOLN%2F4U' + const sessionIdEncryptedWithNewSecret = 'aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.InCp31AuDa7DX%2F8rGBz8RMFiCpmUtjcF%2BS7Aco7tur8' const responseForOldSecret = await fastify.inject({ url: '/', @@ -192,7 +192,7 @@ test('should set session.cookie.expires if maxAge', async (t) => { const response = await fastify.inject({ url: '/', - headers: { cookie: DEFAULT_COOKIE, 'x-forwarded-proto': 'https'}, + headers: { cookie: DEFAULT_COOKIE, 'x-forwarded-proto': 'https' } }) t.equal(response.statusCode, 200) @@ -329,7 +329,7 @@ test('should set session cookie if saveUninitialized is false and data not modif } const fastify = await buildFastify((request, reply) => reply.send(200), options) t.teardown(() => fastify.close()) - + const response = await fastify.inject({ url: '/', headers: { 'x-forwarded-proto': 'https' } @@ -338,4 +338,3 @@ test('should set session cookie if saveUninitialized is false and data not modif t.equal(response.statusCode, 200) t.ok(response.headers['set-cookie']) }) - From 96c180c1c42a489747c33850ec1f514e7a762c63 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Wed, 10 Aug 2022 18:43:08 -0400 Subject: [PATCH 12/16] Clean up some of the uninitialized tests --- lib/session.js | 2 +- test/base.test.js | 42 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/session.js b/lib/session.js index 5bea33f..c9eac77 100644 --- a/lib/session.js +++ b/lib/session.js @@ -31,7 +31,7 @@ module.exports = class Session { this[sessionIdKey] = sessionId this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret) this[originalHash] = this[hash]() - this.touch() + this.touch() // Needs to happen after originalHash is set, in case maxAge forces an update } touch () { diff --git a/test/base.test.js b/test/base.test.js index 5a1565d..591e2b3 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -299,7 +299,7 @@ test('should create new session if cookie contains invalid session', async (t) = t.match(response.headers['set-cookie'], /sessionId=[\w-]{32}.[\w-%]{43,57}; Path=\/; HttpOnly; Secure/) }) -test('should not set session cookie if no data in session and saveUninitialized is false', async (t) => { +test('should not set session cookie if saveUninitialized is false and no data in session', async (t) => { t.plan(2) const options = { secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', @@ -317,17 +317,49 @@ test('should not set session cookie if no data in session and saveUninitialized t.ok(response.headers['set-cookie'] === undefined) }) -test('should set session cookie if saveUninitialized is false and data not modified but maxAge is on', async (t) => { +test('should not set session cookie if saveUninitialized is false and data in session is unmodified', async (t) => { + t.plan(2) + const options = { + secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', + saveUninitialized: false + } + const plugin = fastifyPlugin(async (fastify, opts) => { + fastify.addHook('onRequest', (request, reply, done) => { + request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + test: {} + }, done) + }) + }) + const fastify = await buildFastify((request, reply) => reply.send(200), options, plugin) + t.teardown(() => fastify.close()) + + const response = await fastify.inject({ + url: '/', + headers: { 'x-forwarded-proto': 'https' } + }) + + t.equal(response.statusCode, 200) + t.ok(response.headers['set-cookie'] === undefined) +}) + +test('should set session cookie if saveUninitialized is false and maxAge is on', async (t) => { t.plan(2) const options = { cookie: { - maxAge: 42, - expires: new Date(Date.now() + 1000) + maxAge: 42 }, secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk', saveUninitialized: false } - const fastify = await buildFastify((request, reply) => reply.send(200), options) + const plugin = fastifyPlugin(async (fastify, opts) => { + fastify.addHook('onRequest', (request, reply, done) => { + request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + // In this scenario, maxAge would have set expires in a previous request + expires: new Date(Date.now() + 1000) + }, done) + }) + }) + const fastify = await buildFastify((request, reply) => reply.send(200), options, plugin) t.teardown(() => fastify.close()) const response = await fastify.inject({ From 5abe7431f99e20f6a0e3f58606e8da51ea132474 Mon Sep 17 00:00:00 2001 From: Uzlopak Date: Thu, 11 Aug 2022 18:05:49 +0200 Subject: [PATCH 13/16] Update lib/fastifySession.js --- lib/fastifySession.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fastifySession.js b/lib/fastifySession.js index dfa6e6d..b4607b3 100644 --- a/lib/fastifySession.js +++ b/lib/fastifySession.js @@ -62,7 +62,7 @@ function decryptSession (sessionId, options, request, done) { newSession(secret, request, cookieOpts, idGenerator, done) return } - if (session?.cookie?.expires && session.cookie.expires <= new Date()) { + if (session.cookie?.expires && session.cookie.expires <= Date.now()) { const restoredSession = Session.restore( request, idGenerator, From 8efbe87d202c954e44cecfc8353357c229f8d515 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Thu, 11 Aug 2022 17:41:16 -0400 Subject: [PATCH 14/16] Test was because I wasn't setting the cookie... ugh. Adding some constants to make reading a little easier --- test/base.test.js | 25 ++++++++++++------------- test/util.js | 6 ++++-- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index 591e2b3..2264658 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -2,7 +2,7 @@ const test = require('tap').test const fastifyPlugin = require('fastify-plugin') -const { DEFAULT_OPTIONS, DEFAULT_COOKIE, buildFastify } = require('./util') +const { DEFAULT_OPTIONS, DEFAULT_COOKIE, DEFAULT_COOKIE_VALUE, DEFAULT_SESSION_ID, buildFastify } = require('./util') test('should not set session cookie on post without params', async (t) => { t.plan(3) @@ -114,7 +114,7 @@ test('should set session cookie using the default cookie name', async (t) => { t.plan(2) const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + request.sessionStore.set(DEFAULT_SESSION_ID, { cookie: { secure: true, httpOnly: true, path: '/' } }, done) }) @@ -142,8 +142,7 @@ 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', { - sessionId: 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', + request.sessionStore.set(DEFAULT_SESSION_ID, { cookie: { expires: new Date(Date.now() - 1000), secure: true, httpOnly: true, path: '/' } }, done) }) @@ -178,7 +177,7 @@ test('should set session.cookie.expires if maxAge', async (t) => { } const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + request.sessionStore.set(DEFAULT_SESSION_ID, { test: {} }, done) }) @@ -205,7 +204,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', { + request.sessionStore.set(DEFAULT_SESSION_ID, { cookie: { expires: new Date(Date.now() - 1000) } }, done) }) @@ -224,7 +223,7 @@ test('should set new session cookie if expired', async (t) => { } }) - t.equal(response.headers['set-cookie'].includes('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN.B7fUDYXU9fXF9pNuL3qm4NVmSduLJ6kzCOPh5JhHGoE'), false) + t.equal(response.headers['set-cookie'].includes(DEFAULT_COOKIE_VALUE), false) t.match(response.headers['set-cookie'], /sessionId=[\w-]{32}.[\w-%]{43,57}; Path=\/; HttpOnly; Secure/) t.equal(response.statusCode, 200) }) @@ -247,7 +246,7 @@ test('should return new session cookie if does not exist in store', async (t) => }) t.equal(response.statusCode, 200) - t.equal(response.headers['set-cookie'].includes('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN.B7fUDYXU9fXF9pNuL3qm4NVmSduLJ6kzCOPh5JhHGoE'), false) + t.equal(response.headers['set-cookie'].includes(DEFAULT_COOKIE_VALUE), false) t.match(response.headers['set-cookie'], /sessionId=[\w-]{32}.[\w-%]{43,57}; Path=\/; HttpOnly; Secure/) }) @@ -278,7 +277,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', { + request.sessionStore.set(DEFAULT_SESSION_ID, { test: {} }, done) }) @@ -325,7 +324,7 @@ test('should not set session cookie if saveUninitialized is false and data in se } const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + request.sessionStore.set(DEFAULT_SESSION_ID, { test: {} }, done) }) @@ -335,7 +334,7 @@ test('should not set session cookie if saveUninitialized is false and data in se const response = await fastify.inject({ url: '/', - headers: { 'x-forwarded-proto': 'https' } + headers: { cookie: DEFAULT_COOKIE, 'x-forwarded-proto': 'https' } }) t.equal(response.statusCode, 200) @@ -353,7 +352,7 @@ test('should set session cookie if saveUninitialized is false and maxAge is on', } const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - request.sessionStore.set('Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN', { + request.sessionStore.set(DEFAULT_SESSION_ID, { // In this scenario, maxAge would have set expires in a previous request expires: new Date(Date.now() + 1000) }, done) @@ -364,7 +363,7 @@ test('should set session cookie if saveUninitialized is false and maxAge is on', const response = await fastify.inject({ url: '/', - headers: { 'x-forwarded-proto': 'https' } + headers: { cookie: DEFAULT_COOKIE, 'x-forwarded-proto': 'https' } }) t.equal(response.statusCode, 200) diff --git a/test/util.js b/test/util.js index 9088b1f..591e700 100644 --- a/test/util.js +++ b/test/util.js @@ -5,7 +5,8 @@ const fastifyCookie = require('@fastify/cookie') const fastifySession = require('../lib/fastifySession') const DEFAULT_OPTIONS = { secret: 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk' } -const DEFAULT_COOKIE_VALUE = 'sessionId=Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN.B7fUDYXU9fXF9pNuL3qm4NVmSduLJ6kzCOPh5JhHGoE' +const DEFAULT_SESSION_ID = 'Qk_XT2K7-clT-x1tVvoY6tIQ83iP72KN' +const DEFAULT_COOKIE_VALUE = `sessionId=${DEFAULT_SESSION_ID}.B7fUDYXU9fXF9pNuL3qm4NVmSduLJ6kzCOPh5JhHGoE` const DEFAULT_COOKIE = `${DEFAULT_COOKIE_VALUE}; Path=/; HttpOnly; Secure` async function buildFastify (handler, sessionOptions, plugin) { @@ -25,5 +26,6 @@ module.exports = { buildFastify, DEFAULT_COOKIE_VALUE, DEFAULT_COOKIE, - DEFAULT_OPTIONS + DEFAULT_OPTIONS, + DEFAULT_SESSION_ID } From 65712d2ee322e26733b15506a2a6026a3ea2664f Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Fri, 12 Aug 2022 06:53:19 -0400 Subject: [PATCH 15/16] Fix issue where maxAge was not triggering a session modification --- lib/session.js | 8 +++++++- test/base.test.js | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/session.js b/lib/session.js index c9eac77..ff9ec46 100644 --- a/lib/session.js +++ b/lib/session.js @@ -163,7 +163,13 @@ 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] + ) } get sessionId () { diff --git a/test/base.test.js b/test/base.test.js index 2264658..f9db942 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -354,7 +354,9 @@ test('should set session cookie if saveUninitialized is false and maxAge is on', fastify.addHook('onRequest', (request, reply, done) => { request.sessionStore.set(DEFAULT_SESSION_ID, { // In this scenario, maxAge would have set expires in a previous request - expires: new Date(Date.now() + 1000) + cookie: { + expires: new Date(Date.now() + 1000) + } }, done) }) }) From 517f6d0093a7f491fe757730ccf3f9be7476c198 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Fri, 12 Aug 2022 06:56:50 -0400 Subject: [PATCH 16/16] Use getTime() instead of toISOString() in session hash Co-authored-by: Uzlopak --- lib/session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/session.js b/lib/session.js index ff9ec46..c280112 100644 --- a/lib/session.js +++ b/lib/session.js @@ -150,7 +150,7 @@ module.exports = class Session { // ignore sess.cookie property if (this === sess && key === 'cookie') { // we want `touch` to affect the hash of the session - return sess.cookie.expires?.toISOString() + return sess.cookie.expires?.getTime() } return val