Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 5 commits into from

3 participants

@jwalton

No description provided.

@jwalton

This is a fix for #762.

@fengmk2

Any test cases to reappear absoluteURI problem?

@coolaj86 coolaj86 referenced this pull request in ciaranj/connect-auth
Closed

session not kept between http callbacks #119

@jonathanong jonathanong commented on the diff
lib/middleware/session.js
@@ -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

what's the reason for this change?

@jwalton
jwalton added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonathanong

should be fine, just needs a rebase and squash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 13, 2013
  1. @jwalton

    Fix URL parsing in session.js

    jwalton authored
  2. @jwalton

    Add missing 'var'

    jwalton authored
  3. @jwalton

    Fix call to parse()

    jwalton authored
Commits on Mar 14, 2013
  1. @jwalton

    Add a test case to make sure sessions are added with Request-URI is a…

    jwalton authored
    …n absoluteURI (as per HTTP 1.1 section 5.1.2)
Commits on Mar 18, 2013
  1. @jwalton
This page is out of date. Refresh to see the latest.
Showing with 44 additions and 2 deletions.
  1. +3 −2 lib/middleware/session.js
  2. +41 −0 test/session.js
View
5 lib/middleware/session.js
@@ -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

what's the reason for this change?

@jwalton
jwalton added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
, crc32 = require('buffer-crc32');
// environment
@@ -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
View
41 test/session.js
@@ -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;
@@ -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())
Something went wrong with that request. Please try again.