Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Added: cookieSession() only set-cookie on change. Clos #442

  • Loading branch information...
commit b028b4f20f4737e52a1a455af63aaff828b345a2 1 parent 13dc2be
@tj tj authored
View
4 lib/middleware/cookieParser.js
@@ -48,7 +48,9 @@ module.exports = function cookieParser(secret){
req.cookies = utils.parseCookie(cookie);
if (secret) {
req.signedCookies = utils.parseSignedCookies(req.cookies, secret);
- req.signedCookies = utils.parseJSONCookies(req.signedCookies);
+ var obj = utils.parseJSONCookies(req.signedCookies);

does it really make sense to hash every cookie? What defines a json cookie vs a non-json cookie? My gut feeling is that the cookie parser should be doing complex operations with cookies, maybe this is a wrong feeling?

@tj Owner
tj added a note

ideally just the sess one, we could move it out and hash the signed value, but then we have to do the same on the other end so we would have the signing overhead. The JSON cookie is used for cookieSession(), and in Express so you can do res.cookie({ complex: 'obj' }). It could be removed but then we would need another middleware just for that. Sometimes implementation elegance < usability IMO

does res.cookie({ complex: 'obj' }); make a new cookie called "complex" with value obj? If that is the case, I would think that the res.cookie method could do the proper breakdown. I am not strictly opposed, it was more of an observation on keeping functionality separate for debug-ability, testing, etc. Just seems "cleaner" to avoid doing things that aren't needed and "coupling" middleware too much. Certainly one might not be able to get around it in some cases but this one seemed simple enough. Just my random code feedback :)

@tj Owner
tj added a note

sorry my example was not even valid hahaha res.cookie('some-name', { complex: 'obj' }). Otherwise it would be much cleaner to move that logic into the two session middleware, though they're coupled to this one already. I could add a separate middleware for Express, but then you have to explain oh if you want to send JSON.stringified cookies you have to add this other middleware after cookieParser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ req.signedCookies = obj.cookies;
+ req.cookieHashes = obj.hashes;
}
req.cookies = utils.parseJSONCookies(req.cookies);
} catch (err) {
View
14 lib/middleware/cookieSession.js
@@ -11,7 +11,8 @@
var utils = require('./../utils')
, Cookie = require('./session/cookie')
- , debug = require('debug')('connect:cookieSession');
+ , debug = require('debug')('connect:cookieSession')
+ , crc16 = require('crc').crc16;
// environment
@@ -69,12 +70,19 @@ module.exports = function cookieSession(options){
// only send secure cookies via https
if (cookie.secure && !secured) return debug('not secured');
- // set cookie
+ // serialize
debug('serializing %j', req.session);
var val = 'j:' + JSON.stringify(req.session);
+
+ // compare hashes
+ var originalHash = req.cookieHashes && req.cookieHashes[key];
+ var hash = crc16(val);
+ if (originalHash == hash) return debug('unmodified session');
+
+ // set-cookie
val = utils.sign(val, req.secret);
val = utils.serializeCookie(key, val, cookie);
- debug('cookie %j', cookie);
+ debug('set-cookie %j', cookie);
res.setHeader('Set-Cookie', val);
});
View
10 lib/utils.js
@@ -12,6 +12,7 @@
var http = require('http')
, crypto = require('crypto')
+ , crc16 = require('crc').crc16
, Path = require('path')
, fs = require('fs');
@@ -188,17 +189,24 @@ exports.parseSignedCookies = function(obj, secret){
*/
exports.parseJSONCookies = function(obj){
+ var hashes = {};
+
Object.keys(obj).forEach(function(key){
var val = obj[key];
if (0 == val.indexOf('j:')) {
try {
+ hashes[key] = crc16(val); // only crc json cookies for now
obj[key] = JSON.parse(val.slice(2));
} catch (err) {
// nothing
}
}
});
- return obj;
+
+ return {
+ cookies: obj,
+ hashes: hashes
+ };
};
/**
View
5 package.json
@@ -9,7 +9,12 @@
"dependencies": {
"qs": "0.4.2",
"mime": "1.2.4",
+<<<<<<< HEAD
"formidable": "1.0.9",
+=======
+ "formidable": "1.0.x",
+ "crc": "0.1.0",
+>>>>>>> crc-sessions
"debug": "*"
},
"devDependencies": {
View
71 test/cookieSession.js
@@ -145,6 +145,77 @@ describe('connect.cookieSession()', function(){
})
})
+ describe('when modified', function(){
+ it('should set-cookie', function(done){
+ var n = 0;
+ var app = connect()
+ .use(connect.cookieParser('keyboard cat'))
+ .use(connect.cookieSession())
+ .use(function(req, res, next){
+ req.session.foo = ++n;
+ res.end();
+ });
+
+ app.request()
+ .get('/')
+ .end(function(res){
+ res.headers.should.have.property('set-cookie');
+
+ app.request()
+ .get('/')
+ .set('Cookie', sess(res))
+ .end(function(res){
+ res.headers.should.have.property('set-cookie');
+ done();
+ });
+ });
+ })
+ })
+
+ describe('when un-modified', function(){
+ it('should set-cookie only the initial time', function(done){
+ var modify;
+
+ var app = connect()
+ .use(connect.cookieParser('keyboard cat'))
+ .use(connect.cookieSession())
+ .use(function(req, res, next){
+ if (modify) req.session.foo = 'bar';
+ res.end();
+ });
+
+ app.request()
+ .get('/')
+ .end(function(res){
+ res.headers.should.have.property('set-cookie');
+ var cookie = sess(res);
+
+ app.request()
+ .get('/')
+ .set('Cookie', cookie)
+ .end(function(res){
+ res.headers.should.not.have.property('set-cookie');
+
+ app.request()
+ .get('/')
+ .set('Cookie', cookie)
+ .end(function(res){
+ res.headers.should.not.have.property('set-cookie');
+ modify = true;
+
+ app.request()
+ .get('/')
+ .set('Cookie', cookie)
+ .end(function(res){
+ res.headers.should.have.property('set-cookie');
+ done();
+ });
+ });
+ });
+ });
+ })
+ })
+
describe('.secure', function(){
it('should not set-cookie when insecure', function(done){
var app = connect()
Please sign in to comment.
Something went wrong with that request. Please try again.