Skip to content

Use a nonce for cookie regeneration #627

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 4 commits into
base: master
Choose a base branch
from
Open

Use a nonce for cookie regeneration #627

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7de61d3
use a nonce for cookie regeneration
mkawalec Dec 18, 2018
f3a2133
linter fixes
mkawalec Dec 18, 2018
357aaf8
tests don't use promises
mkawalec Dec 18, 2018
5288295
removed debug cruft
mkawalec Mar 8, 2019
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
27 changes: 24 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ var defer = typeof setImmediate === 'function'
* @param {Function} [options.genid]
* @param {String} [options.name=connect.sid] Session ID cookie name
* @param {Boolean} [options.proxy]
* @param {Boolean} [options.regenerate] Regenerate the session id on every request
* @param {String} [options.regenerateCookieName='sidnonce'] Name of the nonce cookie for session id regeneration
* @param {Boolean} [options.resave] Resave unmodified sessions back to the store
* @param {Boolean} [options.rolling] Enable/disable rolling session expiration
* @param {Boolean} [options.saveUninitialized] Save uninitialized sessions to the store
Expand All @@ -100,7 +102,13 @@ function session(options) {
var store = opts.store || new MemoryStore()

// get the trust proxy setting
var trustProxy = opts.proxy
var trustProxy = opts.proxy;

// get the regenerate session option
var regenerateSessionId = opts.regenerate;

// get the regenerate cookie name option
var regenerateCookieName = opts.regenerateCookieName || 'connect.sidnonce';

// get the resave session option
var resaveSession = opts.resave;
Expand Down Expand Up @@ -215,6 +223,7 @@ function session(options) {

// get the session ID from the cookie
var cookieId = req.sessionID = getcookie(req, name, secrets);
var sessionNonce = getcookie(req, regenerateCookieName, secrets);

// set-cookie
onHeaders(res, function(){
Expand All @@ -241,6 +250,10 @@ function session(options) {

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

if (regenerateSessionId) {
setcookie(res, regenerateCookieName, sessionNonce, secrets[0], req.session.cookie.data);
}
});

// proxy end() to commit the session
Expand Down Expand Up @@ -327,6 +340,11 @@ function session(options) {
touched = true
}

if (regenerateSessionId) {
sessionNonce = generateId(req);
req.session.nonce = sessionNonce;
}

if (shouldSave(req)) {
req.session.save(function onsave(err) {
if (err) {
Expand Down Expand Up @@ -445,7 +463,7 @@ function session(options) {

return cookieId != req.sessionID
? saveUninitializedSession || isModified(req.session)
: rollingSessions || req.session.cookie.expires != null && isModified(req.session);
: regenerateSessionId || rollingSessions || req.session.cookie.expires != null && isModified(req.session);
}

// generate a session if the browser doesn't send a sessionID
Expand All @@ -458,7 +476,7 @@ function session(options) {

// generate the session object
debug('fetching %s', req.sessionID);
store.get(req.sessionID, function(err, sess){
store.get(req.sessionID, function(err, sess) {
// error handling
if (err) {
debug('error %j', err);
Expand All @@ -474,6 +492,9 @@ function session(options) {
debug('no session found');
generate();
// populate req.session
} else if (regenerateSessionId && sess.nonce !== sessionNonce) {
debug('nonce mismatch, session invalid');
next(new Error('Nonce mismatch, session invalid'));
} else {
debug('session found');
store.createSession(req, sess);
Expand Down
114 changes: 109 additions & 5 deletions test/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var Cookie = require('../session/cookie')
var min = 60 * 1000;

describe('session()', function(){
it('should export constructors', function(){
it('should export varructors', function(){
assert.strictEqual(typeof session.Session, 'function')
assert.strictEqual(typeof session.Store, 'function')
assert.strictEqual(typeof session.MemoryStore, 'function')
Expand Down Expand Up @@ -896,6 +896,81 @@ describe('session()', function(){
})
})

describe('regenerate option', function () {
it('should set the session nonce', function (done) {
request(createServer({ regenerate: true }))
.get('/')
.expect(shouldSetCookie('connect.sidnonce'))
.expect(shouldSetCookie('connect.sid'))
.expect(200, done)
})

it('sets new nonce on a second request', function (done) {
var server = createServer({ regenerate: true });

console.log('blah');
request(server)
.get('/')
.expect(200)
.end(function (err, res) {
if (err) return done(err);

var firstNonce = getCookie(res, 'connect.sidnonce');
request(server)
.get('/')
.expect(200)
.end(function (err, res) {
if (err) return done(err);
var secondNonce = getCookie(res, 'connect.sidnonce')
assert.notStrictEqual(firstNonce.value, secondNonce.value)
done();
})
})
})

it('rejects a request with an old nonce', function (done) {
var server = createServer({ regenerate: true });

request(server)
.get('/')
.expect(200)
.end(function (err, res) {
if (err) return done(err);
var nonce = getCookie(res, 'connect.sidnonce');
var sid = getCookie(res, 'connect.sid');
var cookieHeader = 'connect.sidnonce=' + nonce.value + ';connect.sid=' + sid.value

request(server)
.get('/')
.set('Cookie', cookieHeader)
.expect(200)
.end(function (err, res) {
if (err) return done(err);
request(server)
.get('/')
.set('Cookie', cookieHeader)
.end(function (err, res) {
if (err) return done(err);
assert.strictEqual(res.error.text, 'Nonce mismatch, session invalid');
done();
});
});
})
})

it('uses the specified cookie name', function (done) {
request(createServer({ regenerate: true, regenerateCookieName: 'supercustom' }))
.get('/')
.expect(shouldSetCookie('supercustom'))
.expect(function (res) {
var oldName = getCookie(res, 'connect.sidnonce')
assert.equal(oldName, null)
})
.expect(shouldSetCookie('connect.sid'))
.expect(200, done)
})
})

describe('rolling option', function(){
it('should default to false', function(done){
var server = createServer(null, function (req, res) {
Expand Down Expand Up @@ -2125,6 +2200,26 @@ function cookie(res) {
return (setCookie && setCookie[0]) || undefined;
}

function cookies(res) {
return res.headers['set-cookie'];
}

function getCookie(res, name) {
var header = cookies(res);
return header.reduce(function(found, cookie) {
if (found) {
return found;
}

var parsed = parseSetCookie(cookie);
if (parsed.name === name) {
return parsed;
} else {
return null;
}
}, null);
}

function createServer (options, respond) {
var fn = respond
var opts = options
Expand Down Expand Up @@ -2238,10 +2333,19 @@ function shouldNotSetSessionInStore(store) {

function shouldSetCookie (name) {
return function (res) {
var header = cookie(res)
var data = header && parseSetCookie(header)
assert.ok(header, 'should have a cookie header')
assert.strictEqual(data.name, name, 'should set cookie ' + name)
var header = cookies(res)
assert.ok(header.length, 'should have a cookie header')

var foundCookie = header.reduce(function (found, header) {
if (found) {
return found;
}

var data = header && parseSetCookie(header);
return data.name === name;
}, false);

assert.ok(foundCookie, 'should set cookie ' + name)
}
}

Expand Down