From 799f0928610d3c73113d6290f663cf15d14f485b Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Fri, 12 Aug 2022 10:00:42 -0400 Subject: [PATCH 1/2] Bugfix: we were not re-signing cookies with new secrets --- lib/session.js | 4 ++-- test/base.test.js | 48 ++++++++++++++++++++++++++++------------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/lib/session.js b/lib/session.js index 60953e6..e0f12b6 100644 --- a/lib/session.js +++ b/lib/session.js @@ -30,8 +30,8 @@ module.exports = class Session { this.touch() if (!this.sessionId) { this.sessionId = this[generateId](this[requestKey]) - this.encryptedSessionId = this[sign]() } + this.encryptedSessionId = this[sign]() this[originalHash] = this[hash]() } @@ -69,7 +69,7 @@ module.exports = class Session { [addDataToSession] (prevSession) { for (const key in prevSession) { - if (key !== 'cookie') { + if (!['cookie', 'encryptedSessionId'].includes(key)) { this[key] = prevSession[key] } } diff --git a/test/base.test.js b/test/base.test.js index aba1323..673c6ca 100644 --- a/test/base.test.js +++ b/test/base.test.js @@ -2,6 +2,7 @@ const test = require('tap').test const fastifyPlugin = require('fastify-plugin') +const cookieSignature = require('cookie-signature') const { DEFAULT_OPTIONS, DEFAULT_COOKIE, DEFAULT_SESSION_ID, DEFAULT_SECRET, DEFAULT_ENCRYPTED_SESSION_ID, buildFastify } = require('./util') test('should not set session cookie on post without params', async (t) => { @@ -45,15 +46,22 @@ test('should set session cookie', async (t) => { }) test('should support multiple secrets', async (t) => { - t.plan(2) + t.plan(4) + const newSecret = 'geheim' const options = { - secret: ['geheim', DEFAULT_SECRET] + secret: [newSecret, DEFAULT_SECRET] } + const sessionId = 'aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e' + const sessionIdSignedWithOldSecret = cookieSignature.sign(sessionId, DEFAULT_SECRET) + const sessionIdSignedWithNewSecret = cookieSignature.sign(sessionId, newSecret) + const htmlEncodedSessionIdSignedWithNewSecret = 'aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e.InCp31AuDa7DX%2F8rGBz8RMFiCpmUtjcF%2BS7Aco7tur8' + const plugin = fastifyPlugin(async (fastify, opts) => { fastify.addHook('onRequest', (request, reply, done) => { - request.sessionStore.set('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e', { - expires: Date.now() + 1000 + request.sessionStore.set(sessionId, { + sessionId, + encryptedSessionId: sessionIdSignedWithOldSecret }, done) }) }) @@ -64,16 +72,27 @@ test('should support multiple secrets', async (t) => { const fastify = await buildFastify(handler, options, plugin) t.teardown(() => fastify.close()) - const response = await fastify.inject({ + 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=${sessionIdSignedWithOldSecret}; Path=/; HttpOnly; Secure` } }) - t.equal(response.statusCode, 200) - t.equal(response.headers['set-cookie'].includes('aYb4uTIhdBXCfk_ylik4QN6-u26K0u0e'), false) + t.equal(responseForOldSecret.statusCode, 200) + // It will be replaced with the new secret! + t.equal(responseForOldSecret.headers['set-cookie'].includes(htmlEncodedSessionIdSignedWithNewSecret), true) + + const responseForNewSecret = await fastify.inject({ + url: '/', + headers: { + 'x-forwarded-proto': 'https', + cookie: `sessionId=${sessionIdSignedWithNewSecret}; Path=/; HttpOnly; Secure` + } + }) + t.equal(responseForNewSecret.statusCode, 200) + t.equal(responseForNewSecret.headers['set-cookie'].includes(htmlEncodedSessionIdSignedWithNewSecret), true) }) test('should set session cookie using the specified cookie name', async (t) => { @@ -99,20 +118,11 @@ test('should set session cookie using the specified cookie name', async (t) => { 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(DEFAULT_SESSION_ID, { - expires: Date.now() + 1000, - sessionId: DEFAULT_SESSION_ID, - cookie: { secure: true, httpOnly: true, path: '/' } - }, done) - }) - }) function handler (request, reply) { request.session.test = {} reply.send(200) } - const fastify = await buildFastify(handler, DEFAULT_OPTIONS, plugin) + const fastify = await buildFastify(handler, DEFAULT_OPTIONS) t.teardown(() => fastify.close()) const response = await fastify.inject({ @@ -124,7 +134,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=[\w-]{32}.[\w-%]{43,57}; Path=\/; HttpOnly; Secure/) }) test('should create new session on expired session', async (t) => { From f5e9205ef4b22a39fce2843ecb9ebe3677332257 Mon Sep 17 00:00:00 2001 From: Rodrigo Menezes Date: Fri, 12 Aug 2022 12:19:49 -0400 Subject: [PATCH 2/2] Remove addDataToSession. Just causes confusion --- lib/session.js | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/session.js b/lib/session.js index e0f12b6..a0f958c 100644 --- a/lib/session.js +++ b/lib/session.js @@ -11,7 +11,6 @@ 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') @@ -20,13 +19,18 @@ const hash = Symbol('hash') module.exports = class Session { constructor (request, idGenerator, cookieOpts, secret, prevSession = {}) { + // Copy over values from the previous session + for (const key in prevSession) { + this[key] = prevSession[key] + } + + this[requestKey] = request this[generateId] = idGenerator - this.cookie = new Cookie(cookieOpts) this[cookieOptsKey] = cookieOpts - this[maxAge] = cookieOpts.maxAge this[secretKey] = secret - this[addDataToSession](prevSession) - this[requestKey] = request + + this.cookie = new Cookie(cookieOpts) + this[maxAge] = cookieOpts.maxAge this.touch() if (!this.sessionId) { this.sessionId = this[generateId](this[requestKey]) @@ -67,14 +71,6 @@ module.exports = class Session { } } - [addDataToSession] (prevSession) { - for (const key in prevSession) { - if (!['cookie', 'encryptedSessionId'].includes(key)) { - this[key] = prevSession[key] - } - } - } - get (key) { return this[key] }