-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
base: main
Are you sure you want to change the base?
Changes from all commits
f69da5d
bde997c
02034ac
8e57228
0ae8b06
40ef1c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,9 @@ module.exports = class Cookie { | |
this._expires = null | ||
|
||
if (originalMaxAge) { | ||
if (cookie.expires) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
this.expires = new Date(cookie.expires) | ||
} | ||
this.maxAge = originalMaxAge | ||
} else if (cookie.expires) { | ||
this.expires = new Date(cookie.expires) | ||
|
@@ -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) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This if conduction ensures that |
||
// we force the same originalMaxAge to match old behavior | ||
this.originalMaxAge = ms | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,8 @@ function fastifySession (fastify, options, next) { | |
request, | ||
idGenerator, | ||
cookieOpts, | ||
cookieSigner | ||
cookieSigner, | ||
session | ||
) | ||
done() | ||
} else { | ||
|
@@ -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) { | ||
|
@@ -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 | ||
|
@@ -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) { | ||
|
@@ -187,6 +188,7 @@ function fastifySession (fastify, options, next) { | |
return | ||
} | ||
|
||
session.touch() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a bug like case while trying to implement this option. This issue was not causing any problem since 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 When that is done, we need to manually call |
||
session.save((err) => { | ||
if (err) { | ||
done(err) | ||
|
@@ -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) | ||
|
@@ -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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did you lock the version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I locked it because benchmark was not running due to following error
But, when I re-checked it now, I realized that I should have lock this version to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.