Skip to content

Save session before the location header is sent to the browser #157

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

Closed
patriksimek wants to merge 6 commits into from
Closed

Save session before the location header is sent to the browser #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b13c837
Save session before the `location` header is sent to the browser
patriksimek May 15, 2015
8b50592
Fix arguments + check if `res.redirect` exists before proxing it
patriksimek May 15, 2015
ff6bc94
Option to buffer the response when `loation` header is set
patriksimek May 15, 2015
8b2267d
Update tests
patriksimek May 15, 2015
5423b10
Add more tests
patriksimek May 15, 2015
6a4dc7b
Set statusCode immediately in fake writeHead method
patriksimek Jun 19, 2015
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
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ Force a cookie to be set on every response. This resets the expiration date.

The default value is `false`.

##### saveBeforeRedirect

Tells the module to completly buffer the response when `location` header is set.
This is because the browser is performing redirection immediately after the `location`
header is parsed without waiting for the store to finish saving.

The default value is `false`.

##### saveUninitialized

Forces a session that is "uninitialized" to be saved to the store. A session is
Expand Down
74 changes: 68 additions & 6 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ function session(options){
, cookie = options.cookie || {}
, trustProxy = options.proxy
, storeReady = true
, rollingSessions = options.rolling || false;
, rollingSessions = options.rolling || false
, saveBeforeLocationHeader = options.saveBeforeRedirect || false;
var resaveSession = options.resave;
var saveUninitializedSession = options.saveUninitialized;
var secret = options.secret;
Expand Down Expand Up @@ -171,6 +172,9 @@ function session(options){
var originalHash;
var originalId;
var savedHash;
var buffered;
var bufferedChunks;
var writeHeadLater;

// expose store
req.sessionStore = store;
Expand Down Expand Up @@ -199,10 +203,63 @@ function session(options){

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

var _write = res.write;

if (saveBeforeLocationHeader) {
var _writeHead = res.writeHead;
var headWritten = false;
res.writeHead = function writeHead(statusCode, reason, obj) {
if (headWritten) return;
headWritten = true;

// search for location header only for 3xx status codes
if (statusCode < 300 || statusCode >= 400) {

@dougwilson dougwilson Jun 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this not work if the user calls the following, since you are not looking at the already-set response code?

res.statusCode = 301
res.setHeader('Location', '/')
res.end()

@patriksimek patriksimek Jun 19, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It should be fine, implicit writeHead is always called with already-set response code.

https://github.com/nodejs/io.js/blob/3777f415625ce538de5edbb19f7330356da190a8/lib/_http_server.js#L150

This case is also covered by added test should buffer the response #2.

return _writeHead.call(res, statusCode, reason, obj);
}

this.statusCode = statusCode;

if ('string' != typeof(reason)) {
obj = reason;
reason = undefined;
}

if (obj) {
var keys = Object.keys(obj);
for (var i = 0, l = keys.length; i < l; i++) {
var k = keys[i];
if (k) res.setHeader(k, obj[k]);
}
}

// we have a `location` header so we must buffer all writes
if (res.getHeader('location') != null) {
debug('redirect found, buffering response')
buffered = true;
bufferedChunks = new Buffer(0);
writeHeadLater = function() { _writeHead.call(res, statusCode, reason); };

@dougwilson dougwilson May 22, 2015 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The contract with .writeHead is that we must call it sync right away; we cannot actually delay it's call, sadly. This is a limitation with injecting ourselves directly into the http response object without an abstraction.

The reason is that once code calls res.writeHead(), then Node.js guarantees that the side-effects have occurred sync, i.e the following must work, but this PR breaks it:

res.writeHead(500)
assert(res.statusCode === 500)

} else {
_writeHead.call(res, statusCode, reason);
}
};

var __write = _write;
res.write = _write = function write(chunk, encoding, callback) {
if (!headWritten) res.writeHead(this.statusCode);

if (buffered) {

@dougwilson dougwilson Jun 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this code is assuming that calling res.writeHead will invoke your res.writeHead sync, but we are purposely violating this, so in the spirit of that, we need to assume that someone else replaced res.writeHead that is also not sync (for example, what if they are using two express-session modules?).

bufferedChunks = Buffer.concat([bufferedChunks, !Buffer.isBuffer(chunk) ? new Buffer(chunk, encoding) : chunk]);

@dougwilson dougwilson Jun 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Over the long haul, Buffer.concat can get extremely slow. Why not just make bufferedChunks an array and store them up?

if ('function' === typeof(callback)) setImmediate(callback);

@dougwilson dougwilson Jun 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The callback should only be called once we actually performed the write, so these need to be buffered up and called once we write out our buffer.

return true;

@dougwilson dougwilson Jun 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Return should be false, which means "hey, stop writing to me", which is the case here, as we're bufferring.

} else {
return __write.apply(res, arguments);
}
};
};

// proxy end() to commit the session
var _end = res.end;
var _write = res.write;
var ended = false;
res.end = function end(chunk, encoding) {
if (ended) {
Expand All @@ -216,12 +273,17 @@ function session(options){

function writeend() {
if (sync) {
ret = _end.call(res, chunk, encoding);
if (chunk) _write.call(res, chunk, encoding);
sync = false;
return;
}

if (buffered) {
if (writeHeadLater) writeHeadLater();
__write.call(res, bufferedChunks);
}

_end.call(res);
ret = _end.call(res);
return ret;
}

function writetop() {
Expand Down Expand Up @@ -305,7 +367,7 @@ function session(options){

return writetop();
}

return _end.call(res, chunk, encoding);
};

Expand Down
153 changes: 153 additions & 0 deletions test/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,159 @@ describe('session()', function(){
})
})
})

describe('when location header is set', function () {
it('should buffer the response #1', function (done) {
var saved = false
var success = false
var store = new session.MemoryStore()
var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
req.session.hit = true
res.writeHead(308, {location: 'http://xxx.com'});
res.end('custom body');
})

var _set = store.set
store.set = function set(sid, sess, callback) {
setTimeout(function () {
_set.call(store, sid, sess, function (err) {
saved = true
callback(err)
})
}, 200)
}

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect('location', 'http://xxx.com')
.expect(308, 'custom body', function (err) {
if (err) return done(err)
assert.ok(success)
done()
})
.req.on('response', function() {
if (saved) success = true;
})
})

it('should buffer the response #2', function (done) {
var saved = false
var success = false
var store = new session.MemoryStore()
var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
req.session.hit = true
res.setHeader('Location', 'http://xxx.com')
res.statusCode = 308
res.write('a');
res.write('b');
res.end('c');
})

var _set = store.set
store.set = function set(sid, sess, callback) {
setTimeout(function () {
_set.call(store, sid, sess, function (err) {
saved = true
callback(err)
})
}, 200)
}

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect('location', 'http://xxx.com')
.expect(308, 'abc', function (err) {
if (err) return done(err)
assert.ok(success)
done()
})
.req.on('response', function() {
if (saved) success = true;
})
})

it('should buffer the response #3 (synchronous store)', function(done){
var store = new SyncStore()
var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
res.setHeader('Location', 'http://xxx.com')
res.statusCode = 308
res.end('response')
})

request(server)
.get('/')
.expect(308, 'response', done)
})

it('should not buffer the response #1', function(done){
var saved = false
var success = true
var store = new session.MemoryStore()
var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
req.session.hit = true
res.writeHead(200, {location: 'http://xxx.com'});
res.end('custom body');
})

var _set = store.set
store.set = function set(sid, sess, callback) {
setTimeout(function () {
_set.call(store, sid, sess, function (err) {
saved = true
callback(err)
})
}, 200)
}

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect('location', 'http://xxx.com')
.expect(200, 'custom body', function (err) {
if (err) return done(err)
assert.ok(success)
done()
})
.req.on('response', function() {
if (saved) success = false;
})
})

it('should not buffer the response #2', function(done){
var saved = false
var success = true
var store = new session.MemoryStore()
var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
req.session.hit = true
res.writeHead(302);
res.end('custom body');
})

var _set = store.set
store.set = function set(sid, sess, callback) {
setTimeout(function () {
_set.call(store, sid, sess, function (err) {
saved = true
callback(err)
})
}, 200)
}

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(302, 'custom body', function (err) {
if (err) return done(err)
assert.ok(success)
done()
})
.req.on('response', function() {
if (saved) success = false;
})
})
})

describe('when sid not in store', function () {
it('should create a new session', function (done) {
Expand Down