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.

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

Feature: auto-renew session expiry before expiry #206

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ Setting this to `false` can save storage space and comply with the EU cookie law
##### rolling (optional)
Forces the session identifier cookie to be set on every response. The expiration is reset to the original maxAge - effectively resetting the cookie lifetime. This is typically used in conjuction with short, non-session-length maxAge values to provide a quick expiration of the session data with reduced potential of session expiration occurring during ongoing server interactions. Defaults to true.

##### refresh (optional)
Automatically refresh ( extend the expiry of ) session before `<refresh>` milliseconds before `expiry`. This is more efficient way than setting `rolling` option.
Copy link
Member

Choose a reason for hiding this comment

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

Does it not clear what we should set - a boolean? a string?

Copy link
Author

Choose a reason for hiding this comment

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

refresh option is an number . it is a duration in milliseconds . I can fix this by editing the Readme once the discussion is complete.

The default value is `0 ms` meaning, this feature is disabled.
Consider `cookie.maxAge` is `60 seconds`. If we set `refresh` = `20 seconds`, then it will auto refresh the session if sent any request after 40 second.
It is recommended to disable `rolling` and `saveUninitialized` options if we set this option.

##### idGenerator(request) (optional)

Function used to generate new session IDs.
Expand Down
5 changes: 4 additions & 1 deletion lib/cookie.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ module.exports = class Cookie {
this._expires = null

if (originalMaxAge) {
if (cookie.expires) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we use originalMaxAge here?

Copy link
Author

Choose a reason for hiding this comment

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

No. Please see my previous comment regarding expiry calculation bug. In this case, if cookie.expiry is set, it will be a string loaded from session store. So, it is converted to proper Date object using this if condition

this.expires = new Date(cookie.expires)
}
this.maxAge = originalMaxAge
} else if (cookie.expires) {
this.expires = new Date(cookie.expires)
Expand Down Expand Up @@ -40,7 +43,7 @@ module.exports = class Cookie {
}

set maxAge (ms) {
this.expires = new Date(Date.now() + ms)
if (!this.expires) { this.expires = new Date(Date.now() + ms) }
Copy link
Member

Choose a reason for hiding this comment

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

We are not setting the maxAge any more here - why should we check it?

Copy link
Author

@harish2704 harish2704 Aug 20, 2023

Choose a reason for hiding this comment

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

This if conduction ensures that this.expiry is only set for newly created session and not for existing session loaded from session store .
For session loaded from session store, the expiry value will be already there ( it is determined at the time of session creation ).

// we force the same originalMaxAge to match old behavior
this.originalMaxAge = ms
}
Expand Down
13 changes: 8 additions & 5 deletions lib/fastifySession.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ function fastifySession (fastify, options, next) {
request,
idGenerator,
cookieOpts,
cookieSigner
cookieSigner,
session
)
done()
} else {
Expand Down Expand Up @@ -92,7 +93,6 @@ function fastifySession (fastify, options, next) {
session,
decryptedSessionId
)

if (restoredSession.cookie.expires && restoredSession.cookie.expires.getTime() <= Date.now()) {
restoredSession.destroy(err => {
if (err) {
Expand Down Expand Up @@ -156,6 +156,7 @@ function fastifySession (fastify, options, next) {
const cookieOpts = options.cookie
const saveUninitializedSession = options.saveUninitialized
const rollingSessions = options.rolling
const refresh = options.refresh

return function saveSession (request, reply, payload, done) {
const session = request.session
Expand All @@ -165,7 +166,7 @@ function fastifySession (fastify, options, next) {
}

const cookieSessionId = getCookieSessionId(request)
const saveSession = shouldSaveSession(request, cookieSessionId, saveUninitializedSession, rollingSessions)
const saveSession = shouldSaveSession(request, cookieSessionId, saveUninitializedSession, rollingSessions, refresh)
const isInsecureConnection = cookieOpts.secure === true && isConnectionSecure(request) === false
const sessionIdWithPrefix = hasCookiePrefix ? `${cookiePrefix}${session.encryptedSessionId}` : session.encryptedSessionId
if (!saveSession || isInsecureConnection) {
Expand All @@ -187,6 +188,7 @@ function fastifySession (fastify, options, next) {
return
}

session.touch()
Copy link
Member

Choose a reason for hiding this comment

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

why do you need it?

Copy link
Author

Choose a reason for hiding this comment

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

I found a bug like case while trying to implement this option.
The issue is this: the session.expiry was always dynamically calculated at the time of incoming request using maxage & Date.now() instead of loading it from session store.

This issue was not causing any problem since rolling option was enabled by default.
But if disable rolling option, session.expiry value will become a useless information because, it will be always unexpired value because it is calculated dynamically for each request.

But in overall, the system was working well due to proper session cleanup taking place in 1 second interval .

I fixed this behavior by loading expiry value from session store.

When that is done, we need to manually call touch() to extend the expiry of session before saving it into session store.

session.save((err) => {
if (err) {
done(err)
Expand Down Expand Up @@ -223,6 +225,7 @@ function fastifySession (fastify, options, next) {
opts.cookie = options.cookie || {}
opts.cookie.secure = option(opts.cookie, 'secure', true)
opts.rolling = option(options, 'rolling', true)
opts.refresh = option(options, 'refresh', 0) // refreshing is disabled
opts.saveUninitialized = option(options, 'saveUninitialized', true)
opts.algorithm = options.algorithm || 'sha256'
opts.signer = typeof options.secret === 'string' || Array.isArray(options.secret)
Expand All @@ -232,10 +235,10 @@ function fastifySession (fastify, options, next) {
return opts
}

function shouldSaveSession (request, cookieId, saveUninitializedSession, rollingSessions) {
function shouldSaveSession (request, cookieId, saveUninitializedSession, rollingSessions, refresh) {
return cookieId !== request.session.encryptedSessionId
? saveUninitializedSession || request.session.isModified()
: rollingSessions || request.session.isModified()
: rollingSessions || shouldRefresh || (Boolean(request.session.cookie.expires) && request.session.isModified())
}

function option (options, key, def) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"@fastify/pre-commit": "^2.0.2",
"@types/node": "^20.1.0",
"connect-mongo": "^5.0.0",
"connect-redis": "^7.0.0",
"connect-redis": "7.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

why did you lock the version?

Copy link
Author

Choose a reason for hiding this comment

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

I locked it because benchmark was not running due to following error

TypeError: redisStoreFactory is not a function

But, when I re-checked it now, I realized that I should have lock this version to ^6 rather than 7.0.0 because the error still happening in 7.0.0 version.

Copy link
Contributor

Choose a reason for hiding this comment

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

The benchmark is fixed in #248 :)

"cronometro": "^1.1.0",
"fastify": "^4.3.0",
"ioredis": "^5.0.5",
Expand Down
59 changes: 59 additions & 0 deletions test/session.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ const Fastify = require('fastify')
const fastifyCookie = require('@fastify/cookie')
const fastifySession = require('..')
const { buildFastify, DEFAULT_OPTIONS, DEFAULT_COOKIE, DEFAULT_SESSION_ID, DEFAULT_SECRET, DEFAULT_COOKIE_VALUE } = require('./util')
const Cookie = require('cookie')

function sleep (ms) {
return new Promise(resolve => setTimeout(resolve, ms))
}

test('should add session object to request', async (t) => {
t.plan(2)
Expand Down Expand Up @@ -401,6 +406,60 @@ test('should bubble up errors with destroy call if session expired', async (t) =
t.equal(JSON.parse(response.body).message, 'No can do')
})

test('should refresh session cookie expiration if refresh is set to nonzero', async (t) => {
t.plan(9)

const fastify = Fastify()

const options = {
secret: DEFAULT_SECRET,
rolling: false,
saveUninitialized: false,
refresh: 1000, // should refresh cookie after 1 second
cookie: { secure: false, maxAge: 2000 }
}
fastify.register(fastifyCookie)
fastify.register(fastifySession, options)

fastify.get('/check', (request, reply) => {
request.session.testSessionId = request.session.sessionId
return reply.send(request.session.testSessionId)
})
await fastify.listen({ port: 0 })
t.teardown(() => { fastify.close() })

let response1 = await fastify.inject({
url: '/check'
})
t.equal(response1.statusCode, 200)
t.ok(response1.headers['set-cookie'])
// we should not get 'set-cookie' header if we sent request
// within <refresh> interval . Here it is 1000 ms
await sleep(500)
let response2 = await fastify.inject({
url: '/check',
headers: { Cookie: response1.headers['set-cookie'] }
})
t.equal(response2.statusCode, 200)
t.notOk(response2.headers['set-cookie'])

response1 = await fastify.inject({
url: '/check'
})
t.equal(response1.statusCode, 200)
t.ok(response1.headers['set-cookie'])
// we should get 'set-cookie' header if we sent request
// after <refresh> interval . Here it is 1000 ms
await sleep(1100)
response2 = await fastify.inject({
url: '/check',
headers: { Cookie: response1.headers['set-cookie'] }
})
t.equal(response2.statusCode, 200)
t.ok(response2.headers['set-cookie'])
t.equal(Cookie.parse(response2.headers['set-cookie']).sessionId, Cookie.parse(response1.headers['set-cookie']).sessionId)
})

test('should not reset session cookie expiration if rolling is false', async (t) => {
t.plan(3)

Expand Down
Loading