expressjs / session

Simple session middleware for Express
MIT License
6.25k stars 977 forks source link

Sharing session store between multiple session instances doesn't work #521

Open tvainika opened 6 years ago

tvainika commented 6 years ago

In my code I've two session instances with different cookie options (domain, path). Those two express-session instances share same RedisStore with different cookie options. However it does not seem to matter which express-session instance is used, the later instantiated cookie options will be applied.

To be concrete, I wrote test case to show what I expected the results would be, but currently this test case fails:

    it('should use own session opts with shared store', function(done){

      // The beef: sharing same store instance. Replace with null and the test will pass
      var store = new session.MemoryStore()

      var app = express()
          .use('/path1', createSession({store, cookie: { path: '/path1' }}))
          .use('/path2', createSession({store, cookie: { path: '/path2' }}))
          .use(function(req, res, next){
            res.end()
          })

      request(app)
        .get('/path2')
        .expect(shouldSetCookieWithAttributeAndValue('connect.sid', 'Path', '/path2'))
        .expect(200, function (err, res) {
          if (err) return done(err)
          request(app)
            .get('/path1')
            .expect(shouldSetCookieWithAttributeAndValue('connect.sid', 'Path', '/path1'))
            .expect(200, done)
        })
    })

Should it work if one store is shared between multiple session/cookie options? Or is this just wrong usage?

dougwilson commented 6 years ago

AFAIK this isn't a use case that was ever considered so it likely doesn't work correctly as you're experiencing. We could add this as a feature, of course. Any desire to put together a PR?

tvainika commented 6 years ago

I did read some code here. The problem here is caused by session constructor adding generate method to given store in /index.js When shared store is given as parameter to multiple sessions, the generate method gets overwritten.

The generate method is called from two places outside. Session's generate method and Store's public api regenerate method.

The fix needs some refactoring. Mostly it means that all Session objects will have reference to some kind of session context instead of only to session store. Does this sound sensible?

joewagner commented 6 years ago

@tvainika I'm curious, why not use two store instances?

tvainika commented 6 years ago

@joewagner Probably I'll do that, because it seems to be easier fix for us. Having two redis connections parallel doesn't cost that much.

However in this case express-session does not produce any error, but just gives wrong results. Therefore I believe something should be done here also, either prevent this use case, or fix it to work.

joewagner commented 6 years ago

@tvainika are you using connect-redis? Regardless, some stores will allow you to share the same connection with all of your different Store instances.

For connect-redis I believe the option is called client https://github.com/tj/connect-redis#options

knoxcard commented 6 years ago

You do not need two Redis instances. I have it working flawlessly on my site after many months of trial and error and consuming Stackoverflow for breakfast, lunch and dinner. Threw in an extra bonus too, websockets can access the same session as well!

var app = require('express')()
  , Session = require('express-session')
  , redis = require('redis')
  , colors = require('colors')
  , nconf = require('nconf')

var connectRedis = function() {
  return function(callback) {
    const RedisStore = require('connect-redis')(Session)
    app.store = new RedisStore({
      /* configure redisstore options here */
     /* ttl, port, domain, etc */
    /* default options work fine, you can most likely just leave this blank */
    })
    app.store.on('connect', function() {
      console.log('[PASS]'.green + ' Redis Connected')
      // console.log(Object.keys(store))
      callback()
    }).on('disconnect', function() {
      console.log('[FAIL]'.red + ' Redis Connection Failed')
      process.exit()
    })
  }
}
var loadApp = function() {
  return function(callback) {
    var cookie_duration = 33600000
    var sess = {
      name: 'session_name'
      , secret : '******************************'
      , proxy: true
      , secure: false
      , ephemeral: false
      , duration: cookie_duration
      , activeDuration: cookie_duration
      , resave: false
      , saveUninitialized: true
      , store: app.store
    }
    sess.cookie = {
      path: '/',
      httpOnly: true,
      maxAge: cookie_duration,
      rolling: true,
      signed: true
    }
    if(app.nconf.get('ENV') == 'LIVE') {
      sess.secure = true
      sess.cookie.secure = true
    }
    app.sessionMiddleware = Session(sess)
    app.use(app.sessionMiddleware)
    app.cookieParser = require('cookie-parser')
    app.use(app.cookieParser())
    app.csrfProtection = csrf({
      cookie: true
    })
    app.use(app.csrfProtection)
    app.compression = require('compression')
    app.use(app.compression({
      filter: shouldCompress
    }))
    function shouldCompress(req, res) {
      if(req.headers['x-no-compression'])
        return false
      return app.compression.filter(req, res)
    }
    callback()
  }
}

require('async').waterfall([
  connectRedis(),
  loadApp()
], function(error, success) {
  var cookie_duration = 33600000
  var sess = {
    name: 'name_session'
    , secret : '**************************'
    , proxy: true
    , secure: false
    , ephemeral: false
    , duration: cookie_duration
    , activeDuration: cookie_duration
    , resave: false
    , saveUninitialized: true
    , store: app.store
  }
  sess.cookie = {
    path: '/',
    httpOnly: true,
    maxAge: cookie_duration,
    rolling: true,
    signed: true
  }
  if(app.nconf.get('ENV') == 'LIVE') {
    sess.secure = true
    sess.cookie.secure = true
  }
  app.sessionMiddleware = Session(sess)
  app.use(app.sessionMiddleware)

  if(app.nconf.get('ENV') == 'LIVE') {
    const SSL_PATH = ''
    app.server = require('http2').createServer({
      key: require('fs').readFileSync(SSL_PATH + '/privkey.pem', 'utf-8'),
      cert: require('fs').readFileSync(SSL_PATH + '/fullchain.pem', 'utf-8')
    }, app)
    process.setgid('www-data')
    process.setuid('www-data')
  }
  else
    app.server = require('http').Server(app)

 // Now lets connect websockets, all using the same Express session!
 // Pure session harmony!
 app.sio = require('socket.io').listen(app.server)
  app.sio.use(function(socket, next) {
    app.sessionMiddleware(socket.request, socket.request.res, next)
  })
  app.sio.sockets.on('connection', function(socket) {
    socket.on('disconnect', () => {
      socket.request.session = null
      console.info(`Client Gone [id=${socket.id}]`.red)
    })
    console.log(`Client Connected [id=${socket.id}]`.blue)
    var socket_sess = socket.request.session
    var user_id = socket_sess.user_id
    var socket_id = socket.id
  })
})
HarshithaKP commented 4 years ago

@tvainika Is the solution provided by @knoxcard is acceptable as a workaround ??

Tapppi commented 4 years ago

Hey, I had to come back to this in order to complete multi-domain support in our system. Not sure what the @knoxcard example is really about, I don't see multiple domains set up there, only one.. Ended up just using the same client for separate middleware and RedisStore instances and manually matching host header to regex to pick the correct session middleware, see below.

IMO the init should throw an error if it tries to overwrite an existing generate function on the store, this looks like it would work but just doesn't. Just throwing an error should be fine, since RedisStore takes in a Redis client anyways nowadays, you just need to instantiate multiple stores and intuitively you would use the same client.

EDIT: there is something funky with the example below, sessions are not getting stored at all with our implementation based on that. We have pushed off fixing this, but maybe someone will get ideas from the example..

const redisStoreOpts = {
    client: new redis.createClient({/* Your redis options here */}),
    prefix: process.env.SESSION_PREFIX
};

// The actual session middleware initialized with the session options
const sessionMiddleware = [];

// Construct session middleware from uris
for (const domainPath of process.env.SESSION_HOST_DOMAIN_PATHS.split(',')) {
    const hostRegex = domainPath
        .split('/')
        .splice(0, 1)
        .join('');
    const domain = domainPath
        .split('/')
        .splice(1, 1)
        .join('');
    const path = `/${domainPath
        .split('/')
        .splice(2)
        .join('')}`;

    debug(`setting up sessions for host: ${hostRegex} domain: ${domain} path: ${path}`);

    const sessionStore = new RedisStore(redisStoreOpts);

    sessionMiddleware.push({
        regex: new RegExp(hostRegex, 'i'),
        mw: session({
            name: process.env.SESSION_NAME,
            saveUninitialized: false,
            rolling: false,
            resave: false,
            store: sessionStore,
            secret: process.env.SESSION_SECRET,
            cookie: {
                httpOnly: true,
                domain,
                path,
                maxAge: sessionMaxAge,
                secure: !isDev
            }
        })
    });
}

Simple matching the session middleware to use, we also implement retries and stuff here but not relevant to the issue at hand:

app.use((req, res, next) => {
    const {mw, regex} = 
        sessionMiddleware.find(mw => req.get('host').match(mw.regex)) || {};
    if (!mw) {
        return next(new Error('Do not know how to serve sessions for the requested host: ${req.get('host')}');
    }
    return mw(req, res, next);
});
KenEucker commented 3 years ago

I am trying to accomplish this as well, using multiple session strategies alongside multiple authentication strategies, in order to propagate authentication from a main host (mydomain.com) to all configured subdomains underneath that host (x.mydomain.com, y.mydomain.com). I want to provide a single login destination (login.mydomain.com or mydomain.com/login) and then have user permissions defined for each subdomain to determine the level of access to be awarded to the user upon login. My plan is to have cookies store the user's session in the browser, and for Redis to store the logged in users' session information (and JWT) for all of the subdomains. Then, when a user visits x.mydomain.com after having logged into login.mydomain.com they will have a cookie set storing their session for x.mydomain.com.

I have isolated my redis session store to use the path /sessions and for cookies to operate on all other routes, but I am still not able to achieve the desired functionality.

I see that people are still trying to work around this, but is there any intention to patch this behavior in express-session? @joewagner @dougwilson

williamdarkocode commented 3 years ago

Wow I have a similar question and I've been looking for answer. So essentially, I'm working with a microservices architecture, where user sessions are instantiated in one service, but need to be accessed constantly across other services. Yes, I could have all services (which have different host names) share the same store, but, because some services read the session store more often than others, I wanted to create two session store, one for the frequent reads, and the other for the less frequent reads and writes. Any ideas on this? I was initially thinking a pubsub between two session stores? How would this work? Would it work?