Skip to content

Implement custom cookie signature #337

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

Implement custom cookie signature #337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01b4edb
Implement custom cookie signature
roblabla Aug 1, 2016
a955d25
Fix documentation about the signature feature
roblabla Aug 16, 2016
3f880c8
Restore compatibility with previous versions
roblabla Aug 16, 2016
c8223e9
100% coverage
roblabla Aug 16, 2016
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
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,13 @@ The default value is `'keep'`.
- `'keep'` The session in the store will be kept, but modifications made during
the request are ignored and not saved.

##### signature

Pass in your own cookie signing object/module here that implements the
`sign(value, secret)` and `unsign(value, secret)` functions.

The default value is the `node-cookie-signature` module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here, as this module uses the cookie-signature module, not node-cookie-signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably include a link to the npm page for the module as well.


### req.session

To store or access session data, simply use the request property `req.session`,
Expand Down
56 changes: 30 additions & 26 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ var defer = typeof setImmediate === 'function'
* @param {Boolean} [options.saveUninitialized] Save uninitialized sessions to the store
* @param {String|Array} [options.secret] Secret for signing session ID
* @param {Object} [options.store=MemoryStore] Session store
* @param {Object} [options.signature] Object that has the same API as node-cookie-signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: node-cookie-signature -> cookie-signature

Also, I'm not sure we should define our API as "being the same as another module". Perhaps just spell out what we want and just say that cookie-signature is what is following our API, instead of the other way around.

if you need to implement your own signing mechanism
* @param {String} [options.unset]
* @return {Function} middleware
* @public
Expand Down Expand Up @@ -131,6 +133,16 @@ function session(options) {
throw new TypeError('unset option must be "destroy" or "keep"');
}

var signer = signature;
if (typeof opts.signature === 'object') {
if (typeof opts.signature.sign !== 'function' ||
typeof opts.signature.unsign !== 'function') {
throw new TypeError('signature option object must have sign and unsign functions');
}

signer = options.signature;
}

// TODO: switch to "destroy" on next major
var unsetDestroy = opts.unset === 'destroy'

Expand Down Expand Up @@ -212,7 +224,7 @@ function session(options) {
req.sessionStore = store;

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

// set-cookie
onHeaders(res, function(){
Expand All @@ -235,7 +247,7 @@ function session(options) {
req.session.touch();

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

// proxy end() to commit the session
Expand Down Expand Up @@ -483,7 +495,7 @@ function generateSessionId(sess) {
* @private
*/

function getcookie(req, name, secrets) {
function getcookie(req, name, secrets, signer) {
var header = req.headers.cookie;
var raw;
var val;
Expand All @@ -495,15 +507,11 @@ function getcookie(req, name, secrets) {
raw = cookies[name];

if (raw) {
if (raw.substr(0, 2) === 's:') {
val = unsigncookie(raw.slice(2), secrets);
val = unsigncookie(raw, secrets, signer);

if (val === false) {
debug('cookie signature invalid');
val = undefined;
}
} else {
debug('cookie unsigned')
if (val === false) {
debug('cookie signature missing or invalid');
val = undefined;
}
}
}
Expand All @@ -522,19 +530,15 @@ function getcookie(req, name, secrets) {
raw = req.cookies[name];

if (raw) {
if (raw.substr(0, 2) === 's:') {
val = unsigncookie(raw.slice(2), secrets);
val = unsigncookie(raw, secrets, signer);

if (val) {
deprecate('cookie should be available in req.headers.cookie');
}
if (val) {
deprecate('cookie should be available in req.headers.cookie');
}

if (val === false) {
debug('cookie signature invalid');
val = undefined;
}
} else {
debug('cookie unsigned')
if (val === false) {
debug('cookie signature missing or invalid');
val = undefined;
}
}
}
Expand Down Expand Up @@ -602,8 +606,8 @@ function issecure(req, trustProxy) {
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not backwards-compatible, just as a FYI. It would also make the cookies emitted from here unreadable by other modules trying to decode them without them changing. What is the purpose of this change?

Copy link
Author

Choose a reason for hiding this comment

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

My purpose here was that I need to have full control over the cookie's content. (My end-game is to make express-session compatible with PHP's session management). That means having a "pass-through" signature function (AKA no signature at all).

I have a patch that should (in theory) turn back compatibility while still allowing me to achieve my goals.

*/

function setcookie(res, name, val, secret, options) {
var signed = 's:' + signature.sign(val, secret);
function setcookie(res, name, val, secret, options, signer) {
var signed = signer.sign(val, secret);
var data = cookie.serialize(name, signed, options);

debug('set-cookie %s', data);
Expand All @@ -624,9 +628,9 @@ function setcookie(res, name, val, secret, options) {
* @returns {String|Boolean}
* @private
*/
function unsigncookie(val, secrets) {
function unsigncookie(val, secrets, signer) {
for (var i = 0; i < secrets.length; i++) {
var result = signature.unsign(val, secrets[i]);
var result = signer.unsign(val, secrets[i]);

if (result !== false) {
return result;
Expand Down
42 changes: 38 additions & 4 deletions test/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ describe('session()', function(){

request(createServer({ genid: genid }))
.get('/')
.expect(shouldSetCookieToValue('connect.sid', 's%3Aapple.D8Y%2BpkTAmeR0PobOhY4G97PRW%2Bj7bUnP%2F5m6%2FOn1MCU'))
.expect(shouldSetCookieToValue('connect.sid', 'apple.D8Y%2BpkTAmeR0PobOhY4G97PRW%2Bj7bUnP%2F5m6%2FOn1MCU'))
.expect(200, done)
});

Expand All @@ -754,7 +754,7 @@ describe('session()', function(){

request(createServer({ genid: genid }))
.get('/')
.expect(shouldSetCookieToValue('connect.sid', 's%3A%25.kzQ6x52kKVdF35Qh62AWk4ZekS28K5XYCXKa%2FOTZ01g'))
.expect(shouldSetCookieToValue('connect.sid', '%25.kzQ6x52kKVdF35Qh62AWk4ZekS28K5XYCXKa%2FOTZ01g'))
.expect(200, done)
});

Expand All @@ -763,7 +763,7 @@ describe('session()', function(){

request(createServer({ genid: genid }))
.get('/foo')
.expect(shouldSetCookieToValue('connect.sid', 's%3A%2Ffoo.paEKBtAHbV5s1IB8B2zPnzAgYmmnRPIqObW4VRYj%2FMQ'))
.expect(shouldSetCookieToValue('connect.sid', '%2Ffoo.paEKBtAHbV5s1IB8B2zPnzAgYmmnRPIqObW4VRYj%2FMQ'))
.expect(200, done)
});
});
Expand Down Expand Up @@ -2179,6 +2179,40 @@ describe('session()', function(){
})
})

it('should use a custom signature object if passed in', function(done){
var app = express()
.use(cookieParser())
.use(function(req, res, next){ req.headers.cookie = 'foo=bar'; next() })
.use(session({
secret: 'keyboard cat',
key: 'sessid',
signature: {
sign: function(val, secret) {
return secret + val + secret;
},
unsign: function(val, secret) {
return val.replace(/secret/g, '')
}
}
}))
.use(function(req, res, next){
req.session.count = req.session.count || 0
req.session.count++
res.end(req.session.count.toString())
})

request(app)
.get('/')
.expect(200, '1', function (err, res) {
if (err) return done(err)
var val = cookie(res).replace(/...\./, '.')
request(app)
.get('/')
.set('Cookie', val)
.expect(200, '1', done)
})
})

it('should read from req.signedCookies', function(done){
var app = express()
.use(cookieParser('keyboard cat'))
Expand Down Expand Up @@ -2301,7 +2335,7 @@ function shouldSetSecureCookie(name) {
}

function sid(res) {
var match = /^[^=]+=s%3A([^;\.]+)[\.;]/.exec(cookie(res))
var match = /^[^=]+=([^;\.]+)[\.;]/.exec(cookie(res))
var val = match ? match[1] : undefined
return val
}
Expand Down