Skip to content

Add support for Secret function #214

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

Open
wants to merge 3 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
27 changes: 25 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,32 @@ it to be saved.
**Required option**

This is the secret used to sign the session ID cookie. This can be either a string
for a single secret, or an array of multiple secrets. If an array of secrets is
for a single secret, an array of multiple secrets, or a function. If an array of secrets is
provided, only the first element will be used to sign the session ID cookie, while
all the elements will be considered when verifying the signature in requests.
all the elements will be considered when verifying the signature in requests. If a function
is provided then it will be executed with `req` as the first parameter for each request.
The function should return a string or array of strings to be used as the secret for
signing the cookie.

```js
var rotatingSecretKey;
function rotateKey() {
rotatingSecretKey = Math.random();
}
// Initial rotation.
rotateKey();

setInterval(rotateKey, 60 * 60 * 1000) // Rotate the key at least once an hour.
app.use(function(req, res, next) {
var subdomain = subdomain[0];
});

app.use(session({
secret: function () {
return rotatingSecretKey;
}
}))
```

##### store

Expand Down
29 changes: 23 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,20 @@ function session(options){
// TODO: switch to "destroy" on next major
var unsetDestroy = options.unset === 'destroy';

if (Array.isArray(secret) && secret.length === 0) {
if (Array.isArray(secret)) {
if (secret.length === 0) {
throw new TypeError('secret option array must contain one or more strings');
}
// Make sure that we have a string for each item in the array.
for (var i = 0; i < secret.length; i++) {
if (typeof secret[i] !== 'string') {
throw new TypeError('secret option array must only contain strings');
}
}
} else if(typeof secret !== 'function' && (secret && typeof secret !== 'string')) {
throw new TypeError('secret option array must contain one or more strings');
}

if (secret && !Array.isArray(secret)) {
secret = [secret];
}

if (!secret) {
deprecate('req.secret; provide secret option');
}
Expand Down Expand Up @@ -166,7 +172,18 @@ function session(options){

// backwards compatibility for signed cookies
// req.secret is passed from the cookie parser middleware
var secrets = secret || [req.secret];
var secrets = typeof secret === 'function' ? secret(req) : (secret || [req.secret]);

if (!Array.isArray(secrets)) {
secrets = [secrets];
}

for (var i = 0; i < secrets.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The error "secret option array must contain one or more strings" is now misleading. Would you be willing to make the following changes:

  1. Do not accept req.secret to be a function.
  2. Only accept a single function for the secret instead of letting any element in the array be a function? The function can return an array if it wants to provide multiple.

Copy link
Author

Choose a reason for hiding this comment

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

Both sound good to me. I only added support for the function in an array to try and be consistent.

if (typeof secrets[i] !== 'string') {
next(new Error('secret must be a string'));
return;
}
}

var originalHash;
var originalId;
Expand Down
210 changes: 205 additions & 5 deletions test/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -1011,11 +1011,27 @@ describe('session()', function(){
});

describe('secret option', function () {
it('shouldn\'t reject string', function () {
assert.doesNotThrow(createServer.bind(null, { secret: 'keyboard cat' }));
});

it('should reject empty arrays', function () {
assert.throws(createServer.bind(null, { secret: [] }), /secret option array/);
})
assert.throws(createServer.bind(null, { secret: [] }), /secret option array must contain one or more strings/);
});

it('should reject object secret', function () {
assert.throws(createServer.bind(null, { secret: {} }), /secret option array must contain one or more strings/);
});

describe('when an array', function () {
it('should reject array with function', function () {
assert.throws(createServer.bind(null, { secret: ['keyboard cat', function() {} ] }), /secret option array must only contain strings/);
});

it('should reject array with object', function () {
assert.throws(createServer.bind(null, { secret: ['keyboard cat', {} ] }), /secret option array must only contain strings/);
});

it('should sign cookies', function (done) {
var server = createServer({ secret: ['keyboard cat', 'nyan cat'] }, function (req, res) {
req.session.user = 'bob';
Expand All @@ -1026,7 +1042,7 @@ describe('session()', function(){
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', done);
})
});

it('should sign cookies with first element', function (done) {
var store = new session.MemoryStore();
Expand Down Expand Up @@ -1075,8 +1091,192 @@ describe('session()', function(){
.expect(200, 'bob', done);
});
});
})
})
});

describe('when a function', function () {
var rotatingSecretKey;

function rotateKey() {
rotatingSecretKey = Math.random() + '';
}

rotateKey();
var rotateIntervalId = setInterval(rotateKey, 5000);

after(function() {
clearInterval(rotateIntervalId);
});

it('should sign cookie with secret from function', function (done) {
var server = createServer({ secret: function() {
return rotatingSecretKey;
}}, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', done);
});

it('should load session from cookie sid', function (done) {
var count = 0;
var server = createServer({ secret: function() {
return rotatingSecretKey;
}}, function (req, res) {
req.session.num = req.session.num || ++count;
res.end('session ' + req.session.num)
});

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'session 1', function (err, res) {
if (err) return done(err);
request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(200, 'session 1', done)
})
});

it('should not load session from cookie sid when secret is different', function (done) {
var count = 0;
var server = createServer({ secret: function() {
return rotatingSecretKey;
}}, function (req, res) {
req.session.num = req.session.num || ++count;
res.end('session ' + req.session.num)
});

request(server)
.get('/')
.set('Host', 'test1.doamin.com')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'session 1', function (err, res) {
if (err) return done(err);

rotateKey(); // Rotate the key so the old session is now invalid.

request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(200, 'session 2', done)
})
});

it('should sign cookie with secret from function', function (done) {
var server = createServer({ secret: function() {
return rotatingSecretKey;
}}, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', done);
});

it('should sign cookies with different session ids', function (done) {
var server = createServer({ secret: function() {
return rotatingSecretKey;
} }, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

request(server)
.get('/test1')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', function (err, test1Res) {
if (err) {
return done(err);
}

rotateKey(); // Rotate the key so we generate a session id with a new signature.
request(server)
.get('/test2')
.set('Cookie', cookie(test1Res))
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', function (err, test2Res) {
if (err) {
return done(err);
}
var test1Sid = sid(test1Res);
var test2Sid = sid(test2Res);
assert.ok(test1Sid !== test2Sid, 'session ids should not be equal for different secrets');
done();
});
});
});

it('shouldn\'t sign when function doesn\'t return string', function (done) {
var server = createServer({ secret: function() {
return {};
}}, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

request(server)
.get('/')
.expect(shouldNotHaveHeader('cookie'))
.expect(500, /secret must be a string/, done);
});

it('should sign cookies with first element', function (done) {
var store = new session.MemoryStore();

var server1 = createServer({ secret: function() { return [ rotatingSecretKey, 'keyboard cat' ] }, store: store }, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

var server2 = createServer({ secret: 'keyboard cat', store: store }, function (req, res) {
res.end(String(req.session.user));
});

request(server1)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', function (err, res) {
if (err) return done(err);
request(server2)
.get('/')
.set('Cookie', cookie(res))
.expect(200, 'undefined', done);
});
});

it('should read cookies using all elements', function (done) {
var store = new session.MemoryStore();

var server1 = createServer({ secret: 'nyan cat', store: store }, function (req, res) {
req.session.user = 'bob';
res.end(req.session.user);
});

var server2 = createServer({ secret: function() { return [ rotatingSecretKey, 'nyan cat' ] }, store: store }, function (req, res) {
res.end(String(req.session.user));
});

request(server1)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, 'bob', function (err, res) {
if (err) return done(err);
request(server2)
.get('/')
.set('Cookie', cookie(res))
.expect(200, 'bob', done);
});
});
});
});

describe('unset option', function () {
it('should reject unknown values', function(){
Expand Down