Skip to content
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

feat: remove session id from session data #101

Closed
wants to merge 4 commits into from
Closed

feat: remove session id from session data #101

Show file tree
Hide file tree
Changes from 1 commit
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
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 @@ -69,7 +69,8 @@ function decryptSession (sessionId, options, request, done) {
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
45 changes: 24 additions & 21 deletions lib/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -29,10 +30,8 @@ module.exports = class Session {
this[addDataToSession](prevSession)
this[requestKey] = request
this.touch()
if (!this.sessionId) {
mcollina marked this conversation as resolved.
Show resolved Hide resolved
this.sessionId = this[generateId](this[requestKey])
this.encryptedSessionId = this[sign]()
}
this[sessionIdKey] = sessionId
this[encryptedSessionIdKey] = cookieSignature.sign(this[sessionIdKey], secret)
mcollina marked this conversation as resolved.
Show resolved Hide resolved
this[originalHash] = this[hash]()
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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) {
Expand All @@ -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)
Copy link
Member Author

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
Member

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
Member Author

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
Member

Choose a reason for hiding this comment

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

Go for that!

restoredCookie.expires = new Date(prevSession.expires)
restoredSession.cookie = restoredCookie
restoredSession.expires = restoredCookie.expires
restoredSession[originalHash] = restoredSession[hash]()
Expand Down
12 changes: 7 additions & 5 deletions test/base.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,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
Copy link
Member Author

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
Member

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
Member Author

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
Contributor

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
Contributor

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 },
expires
}, done)
})
})
Expand Down Expand Up @@ -99,7 +101,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',
mcollina marked this conversation as resolved.
Show resolved Hide resolved
cookie: { secure: true, httpOnly: true, path: '/' }
}, done)
})
Expand All @@ -119,7 +120,7 @@ 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) => {
Expand All @@ -128,7 +129,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)
})
Expand Down Expand Up @@ -186,8 +186,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
Copy link
Member Author

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
Contributor

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 },
expires
}, done)
})
})
Expand Down
5 changes: 3 additions & 2 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)
mcollina marked this conversation as resolved.
Show resolved Hide resolved
reply.send(200)
}, DEFAULT_OPTIONS)

Expand Down Expand Up @@ -224,7 +226,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)
})
Expand Down