Skip to content

Expire cookie immediately upon destroying session #242

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Expire cookie immediately upon destroying session #242

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
75 changes: 59 additions & 16 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,7 @@ function session(options) {
store.generate = function(req){
req.sessionID = generateId(req);
req.session = new Session(req);
req.session.cookie = new Cookie(cookieOptions);

if (cookieOptions.secure === 'auto') {
req.session.cookie.secure = issecure(req, trustProxy);
}
req.session.cookie = createCookie(cookieOptions, req, trustProxy);
};

var storeImplementsTouch = typeof store.touch === 'function';
Expand Down Expand Up @@ -217,29 +213,47 @@ function session(options) {

// set-cookie
onHeaders(res, function(){

// Is this an existing session that is being destroyed?
var isDestroying = isBeingDestroyed(req);

if (!req.session) {
debug('no session');
return;
if (!isDestroying || isInitialSession(req)) {
return;
}
}

if (!shouldSetCookie(req)) {
return;
var cookie = req.session ? req.session.cookie : undefined;
if (isDestroying) {
if (cookie == null) {
debug('creating expired cookie');
cookie = createCookie(cookieOptions, req, trustProxy);
}

// Set the cookie to immediately expire on the client
cookie.maxAge = 0;
}

// only send secure cookies via https
if (req.session.cookie.secure && !issecure(req, trustProxy)) {
if (cookie.secure && !issecure(req, trustProxy)) {
debug('not secured');
return;
}

if (!touched) {
// touch session
req.session.touch()
touched = true
if (!isDestroying) {
if (!shouldSetCookie(req)) {
return;
}
else if (!touched) {
// touch session
req.session.touch();
touched = true;
}
}

// set cookie
setcookie(res, name, req.sessionID, secrets[0], req.session.cookie.data);
setcookie(res, name, req.sessionID, secrets[0], cookie.data);
});

// proxy end() to commit the session
Expand Down Expand Up @@ -396,6 +410,16 @@ function session(options) {
});
}

// check if session has been or is being destroyed
function isBeingDestroyed(req) {
return req.sessionID && req.session == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this check that unsetDestroy is true here? That option is what makes the unsetting of req.session actually mean the session is to be destroyed, otherwise it does nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if I add that check here, the tests don't pass any longer, but if I don't add the check, then req.session = null will destroy the cookie even if the user doesn't want req.session = null to destroy the session...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, sounds like I must need another unit test for that last condition you mentioned. I'll take a peek when I get a chance.

}

// check if session is the initial session
function isInitialSession(req) {
return req.sessionID && cookieId != req.sessionID;
}

// check if session has been modified
function isModified(sess) {
return originalId !== sess.id || originalHash !== hash(sess);
Expand Down Expand Up @@ -437,8 +461,8 @@ function session(options) {

// determine if cookie should be set on response
function shouldSetCookie(req) {
// cannot set cookie without a session ID
if (typeof req.sessionID !== 'string') {
// cannot set cookie without a session ID and a session
if (typeof req.sessionID !== 'string' || !req.session) {
return false;
}

Expand Down Expand Up @@ -491,6 +515,25 @@ function session(options) {
};
};

/**
* Create a new Cookie for this request.
*
* @param {Object} cookieOptions
* @param {Object} req
* @param {Boolean} [trustProxy]
* @return {Cookie}
* @private
*/
function createCookie(cookieOptions, req, trustProxy) {
var cookieOpts = cookieOptions || {};

var cookie = new Cookie(cookieOpts);
if (cookieOpts.secure === 'auto') {
cookie.secure = issecure(req, trustProxy);
}
return cookie;
}

/**
* Generate a session ID for a new session.
*
Expand Down
Loading