Skip to content

Remove session id and encrypted session id round 3 #121

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 5 commits into from
Closed

Remove session id and encrypted session id round 3 #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
799f092
Bugfix: we were not re-signing cookies with new secrets
rclmenezes Aug 12, 2022
f5e9205
Remove addDataToSession. Just causes confusion
rclmenezes Aug 12, 2022
3ffa8fb
Remove sessionId and encryptedSessionId from being stored
rclmenezes Aug 12, 2022
e4bdad1
Move sessionId and encryptedSessionId into symbols
rclmenezes Aug 12, 2022
6cac322
Make sure encryptedSessionId does not get serialized
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
10 changes: 5 additions & 5 deletions lib/cookie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ module.exports = class Cookie {
}

function getExpires (cookieOpts) {
let expires = null
if (cookieOpts.maxAge) {
return new Date(Date.now() + cookieOpts.maxAge)
}
if (cookieOpts.expires) {
expires = cookieOpts.expires
} else if (cookieOpts.maxAge) {
expires = new Date(Date.now() + cookieOpts.maxAge)
return cookieOpts.expires
Comment on lines +39 to +43
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.

maxAge has priority over expires, according to the docs.

However, because Session was calling touch in its constructor, it was still doing the right behavior.

I removed that touch() because it is unnecessary, so I had to fix this

}
return expires
return null
}
9 changes: 6 additions & 3 deletions lib/fastifySession.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ function decryptSession (sessionId, options, request, done) {
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
71 changes: 35 additions & 36 deletions lib/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,36 @@ const { configure: configureStringifier } = require('safe-stable-stringify')

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)) {
// 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.touch()
if (!this.sessionId) {
this.sessionId = this[generateId](this[requestKey])
this.encryptedSessionId = this[sign]()
}

this.cookie = new Cookie(cookieOpts)
this[sessionIdKey] = sessionId
this[encryptedSessionIdKey] = cookieSignature.sign(sessionId, secret)
this[originalHash] = this[hash]()
}

touch () {
if (this[maxAge]) {
this.cookie.expires = new Date(Date.now() + this[maxAge])
if (this[cookieOptsKey].maxAge) {
Comment thread
Uzlopak marked this conversation as resolved.
this.cookie.expires = new Date(Date.now() + this[cookieOptsKey].maxAge)
}
}

Expand Down Expand Up @@ -67,14 +67,6 @@ module.exports = class Session {
}
}

[addDataToSession] (prevSession) {
for (const key in prevSession) {
if (key !== 'cookie') {
this[key] = prevSession[key]
}
}
}

get (key) {
return this[key]
}
Expand All @@ -85,14 +77,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 @@ -107,15 +99,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 @@ -129,12 +121,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 @@ -145,8 +137,12 @@ module.exports = class Session {
}
}

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

get encryptedSessionId() {
return this[encryptedSessionIdKey]
}

[hash] () {
Expand All @@ -170,10 +166,13 @@ 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)
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)
if (prevSession.cookie.expires) {
// Need to parse as Date
restoredCookie.expires = new Date(prevSession.cookie.expires)
}
Comment on lines +172 to +175
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.

This is a bugfix! Otherwise Session.restore() will set expires, even when it's not set.

I discovered this bug as a result of the new test.

restoredSession.cookie = restoredCookie
restoredSession[originalHash] = restoredSession[hash]()
return restoredSession
Expand Down
50 changes: 29 additions & 21 deletions test/base.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -45,15 +46,21 @@ 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, {
test: {}
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.

For accuracy -- sessionId / encryptedSessionId should never be in the session store anymore

}, done)
})
})
Expand All @@ -64,16 +71,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) => {
Expand All @@ -99,20 +117,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({
Expand All @@ -124,15 +133,14 @@ 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) => {
t.plan(2)
const plugin = fastifyPlugin(async (fastify, opts) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set(DEFAULT_SESSION_ID, {
sessionId: DEFAULT_SESSION_ID,
cookie: { secure: true, httpOnly: true, path: '/', expires: Date.now() - 1000 }
}, done)
})
Expand Down Expand Up @@ -188,7 +196,7 @@ test('should set new session cookie if expired', async (t) => {
fastify.addHook('onRequest', (request, reply, done) => {
request.sessionStore.set(DEFAULT_SESSION_ID, {
cookie: {
expires: Date.now() + 1000
expires: Date.now() - 1000
Comment thread
Uzlopak marked this conversation as resolved.
}
}, done)
})
Expand Down
Loading