From be78b5413f310f7374e02299d4e3c024baa2a2f5 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Wed, 24 Aug 2022 16:19:45 +0200 Subject: [PATCH 1/8] prevent encryptedSessionId from being stored in the sessionStore --- lib/session.js | 9 ++++++--- test/base.test.js | 5 ++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/session.js b/lib/session.js index 2212a6c..c123727 100644 --- a/lib/session.js +++ b/lib/session.js @@ -15,6 +15,7 @@ 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, cookieSigner, prevSession, sessionId = idGenerator(request)) { @@ -24,6 +25,7 @@ module.exports = class Session { this[cookieSignerKey] = cookieSigner this[requestKey] = request this[sessionIdKey] = sessionId + this[encryptedSessionIdKey] = (prevSession && prevSession[encryptedSessionIdKey]) || cookieSigner.sign(this.sessionId) this.cookie = new Cookie((prevSession && prevSession.cookie) || cookieOpts) if (prevSession) { @@ -34,9 +36,6 @@ module.exports = class Session { } this.touch() - if (!this.encryptedSessionId) { - this.encryptedSessionId = cookieSigner.sign(this.sessionId) - } this[originalHash] = this[hash]() } @@ -146,6 +145,10 @@ module.exports = class Session { return this[sessionIdKey] } + get encryptedSessionId () { + return this[encryptedSessionIdKey] + } + [hash] () { const sess = this const str = stringify(sess, function (key, val) { diff --git a/test/base.test.js b/test/base.test.js index 59d7eaa..b77fcb0 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -22,7 +22,7 @@ test('should not set session cookie on post without params', async (t) => { }) test('should save the session properly', async (t) => { - t.plan(6) + t.plan(5) const store = new Store() const fastify = await buildFastify((request, reply) => { request.session.test = true @@ -36,9 +36,8 @@ test('should save the session properly', async (t) => { const keys = Object.keys(session) // Only storing three keys: cookie, encryptedSessionId and test - t.equal(keys.length, 3) + t.equal(keys.length, 2) t.ok(session.cookie) - t.ok(session.encryptedSessionId) t.equal(session.test, true) }) reply.send() From 9be659ff32e22c8fa89bb619b7fdea701f6c2def Mon Sep 17 00:00:00 2001 From: uzlopak Date: Wed, 24 Aug 2022 16:39:46 +0200 Subject: [PATCH 2/8] add one more condition --- lib/session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/session.js b/lib/session.js index c123727..c44a206 100644 --- a/lib/session.js +++ b/lib/session.js @@ -25,7 +25,7 @@ module.exports = class Session { this[cookieSignerKey] = cookieSigner this[requestKey] = request this[sessionIdKey] = sessionId - this[encryptedSessionIdKey] = (prevSession && prevSession[encryptedSessionIdKey]) || cookieSigner.sign(this.sessionId) + this[encryptedSessionIdKey] = (prevSession && prevSession[sessionIdKey] === sessionId && prevSession[encryptedSessionIdKey]) || cookieSigner.sign(this.sessionId) this.cookie = new Cookie((prevSession && prevSession.cookie) || cookieOpts) if (prevSession) { From 709e29cec65b7e454f5b4e8abaf4abf003487adb Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 25 Aug 2022 19:17:45 +0200 Subject: [PATCH 3/8] improve test --- test/base.test.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index b77fcb0..42ee738 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -22,7 +22,7 @@ test('should not set session cookie on post without params', async (t) => { }) test('should save the session properly', async (t) => { - t.plan(5) + t.plan(11) const store = new Store() const fastify = await buildFastify((request, reply) => { request.session.test = true @@ -35,10 +35,17 @@ test('should save the session properly', async (t) => { const session = [...storeMap.entries()][0][1] const keys = Object.keys(session) - // Only storing three keys: cookie, encryptedSessionId and test + // Only storing three keys: cookie and test t.equal(keys.length, 2) + t.ok(keys.indexOf('cookie') !== -1) + t.ok(keys.indexOf('test') !== -1) + t.ok(keys.indexOf('sessionId') === -1) + t.ok(keys.indexOf('encryptedSessionId') === -1) + t.ok(session.cookie) t.equal(session.test, true) + t.ok(session.sessionId) + t.ok(session.encryptedSessionId) }) reply.send() }, { ...DEFAULT_OPTIONS, store }) From 93423fb5a7e02ad466f95da61673199ac28ba41e Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 25 Aug 2022 19:49:35 +0200 Subject: [PATCH 4/8] add TestStore, fix unit test --- test/TestStore.js | 27 +++++++++++++++++++++++++++ test/base.test.js | 12 ++++++------ test/util.js | 3 ++- 3 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 test/TestStore.js diff --git a/test/TestStore.js b/test/TestStore.js new file mode 100644 index 0000000..154f69b --- /dev/null +++ b/test/TestStore.js @@ -0,0 +1,27 @@ +'use strict' + +const { EventEmitter } = require('events') + +class TestStore extends EventEmitter { + constructor (storeMap = new Map()) { + super() + this.store = storeMap + } + + set (sessionId, session, callback) { + this.store.set(sessionId, JSON.parse(JSON.stringify(session))) + callback() + } + + get (sessionId, callback) { + const session = this.store.get(sessionId) + callback(null, session) + } + + destroy (sessionId, callback) { + this.store.delete(sessionId) + callback() + } +} + +module.exports = TestStore diff --git a/test/base.test.js b/test/base.test.js index 42ee738..338be52 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -4,7 +4,7 @@ const test = require('tap').test const Signer = require('@fastify/cookie').Signer const fastifyPlugin = require('fastify-plugin') const { DEFAULT_OPTIONS, DEFAULT_COOKIE, DEFAULT_SESSION_ID, DEFAULT_SECRET, DEFAULT_ENCRYPTED_SESSION_ID, buildFastify } = require('./util') -const Store = require('../lib/fastifySession').Store +const TestStore = require('./TestStore') test('should not set session cookie on post without params', async (t) => { t.plan(3) @@ -23,7 +23,7 @@ test('should not set session cookie on post without params', async (t) => { test('should save the session properly', async (t) => { t.plan(11) - const store = new Store() + const store = new TestStore() const fastify = await buildFastify((request, reply) => { request.session.test = true @@ -44,8 +44,8 @@ test('should save the session properly', async (t) => { t.ok(session.cookie) t.equal(session.test, true) - t.ok(session.sessionId) - t.ok(session.encryptedSessionId) + t.equal(session.sessionId, undefined) + t.equal(session.encryptedSessionId, undefined) }) reply.send() }, { ...DEFAULT_OPTIONS, store }) @@ -93,7 +93,7 @@ test('should support multiple secrets', async (t) => { const sessionIdSignedWithNewSecret = sign(sessionId, newSecret) const storeMap = new Map() - const store = new Store(storeMap) + const store = new TestStore(storeMap) storeMap.set(sessionId, { test: 0, @@ -135,7 +135,7 @@ test('should support multiple secrets', async (t) => { cookie: `sessionId=${sessionIdSignedWithNewSecret}; Path=/; HttpOnly; Secure` } }) - t.not(storeMap.get(sessionId).sessionId) + t.equal(storeMap.get(sessionId).sessionId, undefined) t.equal(storeMap.get(sessionId).test, 2) t.equal(response2.statusCode, 200) t.equal(response2.headers['set-cookie'].includes(sessionId), true) diff --git a/test/util.js b/test/util.js index ae53565..6e22ec7 100644 --- a/test/util.js +++ b/test/util.js @@ -3,6 +3,7 @@ const Fastify = require('fastify') const fastifyCookie = require('@fastify/cookie') const fastifySession = require('../lib/fastifySession') +const TestStore = require('./TestStore') const DEFAULT_SECRET = 'cNaoPYAwF60HZJzkcNaoPYAwF60HZJzk' const DEFAULT_OPTIONS = { secret: DEFAULT_SECRET } @@ -17,7 +18,7 @@ async function buildFastify (handler, sessionOptions, plugin) { if (plugin) { await fastify.register(plugin) } - await fastify.register(fastifySession, sessionOptions) + await fastify.register(fastifySession, { store: new TestStore(), ...sessionOptions }) fastify.get('/', handler) await fastify.listen({ port: 0 }) From 3ab223b4d6e9234a81e4dc4fdbe0555aa87ca689 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 25 Aug 2022 19:53:19 +0200 Subject: [PATCH 5/8] reformat --- lib/session.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/session.js b/lib/session.js index c44a206..8937ba1 100644 --- a/lib/session.js +++ b/lib/session.js @@ -25,7 +25,11 @@ module.exports = class Session { this[cookieSignerKey] = cookieSigner this[requestKey] = request this[sessionIdKey] = sessionId - this[encryptedSessionIdKey] = (prevSession && prevSession[sessionIdKey] === sessionId && prevSession[encryptedSessionIdKey]) || cookieSigner.sign(this.sessionId) + this[encryptedSessionIdKey] = ( + prevSession && + prevSession[sessionIdKey] === sessionId && + prevSession[encryptedSessionIdKey] + ) || cookieSigner.sign(this.sessionId) this.cookie = new Cookie((prevSession && prevSession.cookie) || cookieOpts) if (prevSession) { From 982fa4e316a5b7ac2fde5d61708fd525b84a33db Mon Sep 17 00:00:00 2001 From: Uzlopak Date: Thu, 25 Aug 2022 23:23:35 +0200 Subject: [PATCH 6/8] Update test/base.test.js Co-authored-by: Simen Bekkhus --- test/base.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/base.test.js b/test/base.test.js index 338be52..8ad2850 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -35,7 +35,7 @@ test('should save the session properly', async (t) => { const session = [...storeMap.entries()][0][1] const keys = Object.keys(session) - // Only storing three keys: cookie and test + // Only storing two keys: cookie and test t.equal(keys.length, 2) t.ok(keys.indexOf('cookie') !== -1) t.ok(keys.indexOf('test') !== -1) From 8cc14c8a62cfc838c6e221cbe7d5a8b8f24a5ce9 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 26 Aug 2022 08:56:00 +0200 Subject: [PATCH 7/8] use includes instead of indexOf --- test/base.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/base.test.js b/test/base.test.js index 8ad2850..8e8bed3 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -37,10 +37,10 @@ test('should save the session properly', async (t) => { // Only storing two keys: cookie and test t.equal(keys.length, 2) - t.ok(keys.indexOf('cookie') !== -1) - t.ok(keys.indexOf('test') !== -1) - t.ok(keys.indexOf('sessionId') === -1) - t.ok(keys.indexOf('encryptedSessionId') === -1) + t.ok(keys.includes('cookie')) + t.ok(keys.includes('test')) + t.not(keys.includes('sessionId')) + t.not(keys.includes('encryptedSessionId')) t.ok(session.cookie) t.equal(session.test, true) From a70627e20a0ab08a4dc93880fab73e378c26e83d Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 26 Aug 2022 11:30:59 +0200 Subject: [PATCH 8/8] simplify TestStore --- test/TestStore.js | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/test/TestStore.js b/test/TestStore.js index 154f69b..d433cc2 100644 --- a/test/TestStore.js +++ b/test/TestStore.js @@ -1,27 +1,12 @@ 'use strict' -const { EventEmitter } = require('events') - -class TestStore extends EventEmitter { - constructor (storeMap = new Map()) { - super() - this.store = storeMap - } +const { MemoryStore } = require('../lib/store') +class TestStore extends MemoryStore { set (sessionId, session, callback) { this.store.set(sessionId, JSON.parse(JSON.stringify(session))) callback() } - - get (sessionId, callback) { - const session = this.store.get(sessionId) - callback(null, session) - } - - destroy (sessionId, callback) { - this.store.delete(sessionId) - callback() - } } module.exports = TestStore