Skip to content

Do not leak encryptedSessionId to session #70

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
m0dch3n wants to merge 1 commit into from
Closed

Do not leak encryptedSessionId to session #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions lib/fastifySession.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ function onRequest (options) {
}

function onSend (options) {
const secret = options.secret[0]
return function saveSession (request, reply, payload, done) {
const session = request.session
if (!session || !session.sessionId || !shouldSaveSession(request, options.cookie, options.saveUninitialized)) {
Expand All @@ -123,9 +124,10 @@ function onSend (options) {
done(err)
return
}
const encryptedSessionId = cookieSignature.sign(session.sessionId, secret)
reply.setCookie(
options.cookieName,
session.encryptedSessionId,
encryptedSessionId,
session.cookie.options(isConnectionSecure(request))
)
done()
Expand Down Expand Up @@ -218,7 +220,7 @@ function shouldSaveSession (request, cookieOpts, saveUninitialized) {
}

function isSessionModified (session) {
return (Object.keys(session).length !== 4)
return (Object.keys(session).length !== 3)
Copy link
Copy Markdown
Member

@jsumners jsumners Mar 24, 2022

Choose a reason for hiding this comment

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

Session is not a plain JavaScript object. It is an instance of

session/lib/session.js

Line 12 in 73a4675

module.exports = class Session {

That object should provide a .keys() method to get this information correctly.

}

function option (options, key, def) {
Expand Down
7 changes: 0 additions & 7 deletions lib/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
'use strict'

const Cookie = require('./cookie')
const cookieSignature = require('cookie-signature')

const maxAge = Symbol('maxAge')
const secretKey = Symbol('secretKey')
const sign = Symbol('sign')
const addDataToSession = Symbol('addDataToSession')
const generateId = Symbol('generateId')

Expand All @@ -32,7 +30,6 @@ module.exports = class Session {

regenerate (request) {
this.sessionId = this[generateId](request)
this.encryptedSessionId = this[sign]()
Copy link
Copy Markdown
Member

@SimenB SimenB Mar 30, 2022

Choose a reason for hiding this comment

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

instead of removing it from Session, what about just ensuring it's not saved to the store? I.e. defining a custom toJSON or something

}

[addDataToSession] (prevSession) {
Expand All @@ -51,10 +48,6 @@ module.exports = class Session {
this[key] = value
}

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

static restore (request, idGenerator, cookieOpts, secret, prevSession) {
const restoredSession = new Session(request, idGenerator, cookieOpts, secret, prevSession)
const restoredCookie = new Cookie(cookieOpts)
Expand Down
2 changes: 1 addition & 1 deletion test/base.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,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 Down
4 changes: 2 additions & 2 deletions test/session.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ test('should destroy the session', async (t) => {
t.is(response.statusCode, 200)
})

test('should add session.encryptedSessionId object to request', async (t) => {
test('should not add session.encryptedSessionId object to request', async (t) => {
t.plan(2)
const port = await testServer((request, reply) => {
t.truthy(request.session.encryptedSessionId)
t.falsy(request.session.encryptedSessionId)
reply.send(200)
}, DEFAULT_OPTIONS)

Expand Down
3 changes: 1 addition & 2 deletions types/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ declare module 'fastify' {
interface SessionData extends ExpressSessionData {
sessionId: string;

encryptedSessionId: string;

/** Updates the `expires` property of the session. */
touch(): void;

Expand Down Expand Up @@ -76,6 +74,7 @@ declare namespace FastifySessionPlugin {

/** The name of the session cookie. Defaults to `sessionId`. */
cookieName?: string;

/**
* The options object used to generate the `Set-Cookie` header of the session cookie.
*
Expand Down