Skip to content
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

Jump to bottom

Fix for issue #762: Session middleware fails to add session when request contains absoluteURI #763

Closed
wants to merge 5 commits into from
Closed

Fix for issue #762: Session middleware fails to add session when request contains absoluteURI #763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af84ea2
Fix URL parsing in session.js
Mar 13, 2013
209dac4
Add missing 'var'
Mar 13, 2013
b5a2747
Fix call to parse()
Mar 13, 2013
cb8a8a7
Add a test case to make sure sessions are added with Request-URI is a…
Mar 14, 2013
b8d8a6d
Use req.originalUrl instead of req.url.
Mar 18, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/middleware/session.js
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ var Session = require('./session/session')
, Cookie = require('./session/cookie')
, Store = require('./session/store')
, utils = require('./../utils')
, parse = utils.parseUrl
, parse = require('url').parse
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason for 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.

After 8 months? No idea. It certainly looks like it would be safe to revert this line.

, crc32 = require('buffer-crc32');

// environment
Expand Down Expand Up @@ -218,7 +218,8 @@ function session(options){
if (!storeReady) return debug('store is disconnected'), next();

// pathname mismatch
if (0 != req.originalUrl.indexOf(cookie.path || '/')) return next();
var parsedUrl = parse(req.originalUrl);
if (0 != parsedUrl.path.indexOf(cookie.path || '/')) return next();

// backwards compatibility for signed cookies
// req.secret is passed from the cookie parser middleware
Expand Down
41 changes: 41 additions & 0 deletions test/session.js
View file
Open in desktop
Expand Up @@ -207,6 +207,23 @@ describe('connect.session()', function(){
});
})

it('should work when the Request-URI is an absoluteURI', function(done){
var app = connect()
.use(connect.cookieParser())
.use(connect.session({ secret: 'keyboard cat', cookie: { maxAge: min }}))
.use(function(req, res, next){
// checks that session exists.
var answer = (req.session?'session found':'no session found');
res.end(answer);
});
app.request()
.get('http://test.com/')
.end(function(res){
res.body.should.equal('session found');
done();
});
})

it('should only set-cookie when modified', function(done){
var modify = true;

Expand Down Expand Up @@ -347,6 +364,30 @@ describe('connect.session()', function(){
});
})

it('should work when the Request-URI is an absoluteURI', function(done){
var app = connect()
.use(connect.cookieParser())
.use(connect.session({ secret: 'keyboard cat', cookie: { path: '/admin' }}))
.use(function(req, res, next){
// checks that session exists.
var answer = (req.session?'session found':'no session found');
res.end(answer);
});
app.request()
.get('http://test.com/admin')
.end(function(res){
// Session should exist
res.body.should.equal('session found');
app.request()
.get('http://test.com/no')
.end(function(res){
// Session should not exist
res.body.should.equal('no session found');
done();
});
});
})

it('should Set-Cookie only once for browser-session cookies', function(done){
var app = connect()
.use(connect.cookieParser())
Expand Down