tj / connect-redis

Redis session store for Connect
MIT License
2.8k stars 342 forks source link

req.session.cookie.expires value does not match with the browser or the expiry of the key in redis #289

Closed slidenerd closed 4 years ago

slidenerd commented 4 years ago

HERE is the full blown sandbox containing the error, you may have to run it on your machine through File => Export in the sandbox as that sandbox doesnt have redis installed

MY issue seems to be related to #259 #251 #264 from what I have searched so far before asking this question.

Very simple setup

My server/app.js file in case the sandbox is down


const express = require('express')
const session = require('express-session')
const RedisStore = require('connect-redis')(session)
const debug = require('debug')('auth:/server/app')
const client = require('./client')

const store = new RedisStore({ client })
// Transform req & res to have the same API as express
// So we can use res.status() & res.json()
const app = express()

app.use(express.json())
app.use(express.urlencoded({ extended: false }))
app.use(
  session({
    secret: 'super-secret-key',
    resave: false,
    rolling: true,
    saveUninitialized: false,
    cookie: { maxAge: 30 * 1000 },
    store
  })
)

// Create express router
const router = express.Router()

router.use((req, res, next) => {
  Object.setPrototypeOf(req, app.request)
  Object.setPrototypeOf(res, app.response)
  req.res = res
  res.req = req
  next()
})

app.use((req, res, next) => {
  debug(
    req.session.cookie,
    'straight from the source',
    req.session.cookie.expires,
    req.session.cookie.maxAge
  )
  next()
})

// Add POST - /api/login
router.post('/login', (req, res) => {
  if (req.body.username === 'demo' && req.body.password === 'demo') {
    req.session.authUser = { username: 'demo' }
    return res.json({ username: 'demo' })
  }
  res.status(401).json({ message: 'Bad credentials' })
})

// Add POST - /api/logout
router.post('/logout', (req, res) => {
  req.session.destroy((err) => {
    if (err) {
      debug(err)
    }
  })
  res.json({ ok: true })
})

app.use('/auth', router)
module.exports = app

The expiry time of the cookie shown under storage in Firefox does not match with the value of req.session.cookie.expires in the middleware or if you open the sandbox, in the nuxtServerInit method which gets called on the express server

I lose my rolling session ability if I do disableTouch as per #251 If I set resave to true, I get that error mentioned in #259 The expires property of the cookie stored inside redis is not updated either

How to reproduce

Why does this matter

The irony

knoxcard2 commented 4 years ago

This seems unnecessary...

router.use((req, res, next) => {
  Object.setPrototypeOf(req, app.request)
  Object.setPrototypeOf(res, app.response)
  req.res = res
  res.req = req
  next()
})
slidenerd commented 4 years ago

You are right, removed the bit and retested the same setup, seems like we still have the issue Login with demo demo Check the ttl of the key in redis, it will be something under 30 Refresh page Check ttl, it will keep updating but when you print req.session from any middleware, the cookie expires does not change


› Rendering url /                                                                                                                                                                                                                                                                                                                                      12:01:28
Session {
  cookie:
   { path: '/',
     _expires: 2019-11-19T06:31:51.290Z,
     originalMaxAge: 30000,
     httpOnly: true },
  authUser: { username: 'demo' } }
Data fetching /: 1ms
› Rendering url /                                                                                                                                                                                                                                                                                                                                      12:01:44
Session {
  cookie:
   { path: '/',
     _expires: 2019-11-19T06:31:51.290Z,
     originalMaxAge: 30000,
     httpOnly: true },
  authUser: { username: 'demo' } }
Data fetching /: 0ms
› Rendering url /                                                                                                                                                                                                                                                                                                                                      12:01:52
Session {
  cookie:
   { path: '/',
     _expires: 2019-11-19T06:31:51.290Z,
     originalMaxAge: 30000,
     httpOnly: true },
  authUser: { username: 'demo' } }
Data fetching /: 0ms
› Rendering url /                                                                                                                                                                                                                                                                                                                                      12:01:59
Session {
  cookie:
   { path: '/',
     _expires: 2019-11-19T06:31:51.290Z,
     originalMaxAge: 30000,
     httpOnly: true },
  authUser: { username: 'demo' } }
Data fetching /: 1ms
› Rendering url /                                                                                                                                                                                                                                                                                                                                      12:02:06
Session {
  cookie:
   { path: '/',
     _expires: 2019-11-19T06:31:51.290Z,
     originalMaxAge: 30000,
     httpOnly: true },
  authUser: { username: 'demo' } }
Data fetching /: 1ms

The content of the cookie inside redis also does not update which is why we are getting this in the first place I guess

My guess is that shouldnt the expected output be such that the expiry of the cookie updates each time you refresh the page?

wavded commented 4 years ago

We have went back and forth on this, including a "fix" and removing it due to performance concerns. If you want to read the rationale, see here:

https://github.com/tj/connect-redis/pull/285

In short, the cookie object is NOT updated whenever express-session when a touch call.

slidenerd commented 4 years ago
knoxcard2 commented 4 years ago

How are your ajax calls setup? Don't forget to include xhrFields -> xhrCredentials. This passes the session cookie, without it, no dice!

$.ajax({
    url: "yoururl",
    type: "GET" or "POST",
    dataType: 'json',
    xhrFields: {
         withCredentials: true
    }
})
knoxcard2 commented 4 years ago

Try this...

{
    secret: process.env.session_secret,
    name: process.env.session_name,
    store: new RedisSession({
      client
    }),
    rolling: true,
    saveUninitialized: true,
    unset: 'destroy',
    resave: true,
    proxy: true,
    logErrors: process.env.debug === 'true',
    cookie: {
      path: '/',
      domain: '.' + process.env.app_domain,
      secure: true,
      sameSite: true,
      httpOnly: true,
      expires: false,
      maxAge: 60000 * process.env.session_exp_mins,
    }
  }
knoxcard2 commented 4 years ago

Boom! Cash money! Damn I am good, lol

All I ask in return is that you take the time, whenever to possible, to help others in need of answers. It makes you a stellar coder and accelerates repo development, once everyone gets the hang of it!

slidenerd commented 4 years ago

I closed the issue as I did not want to bother you guys, you are maintainers of a project after all, as I mentioned above resave: true does not work, what is unset: 'destroy' it is not mentioned in the docs

slidenerd commented 4 years ago

On refreshing the page with the above settings, the user is logged out immediately Before Refreshing

  auth:passport passport:LocalStrategy session Session {
  auth:passport   cookie:
  auth:passport    { path: '/',
  auth:passport      _expires: 2019-11-20T05:43:59.297Z,
  auth:passport      originalMaxAge: 60000,
  auth:passport      httpOnly: true,
  auth:passport      domain: '.localhost',
  auth:passport      secure: true,
  auth:passport      sameSite: true } } email demo password demo +1m
  auth:passport passport:serializeUser() user { email: 'demo' } +0ms
  auth:user user:postLogin() authenticated user found, user { email: 'demo' }, info 'Success' session Session {
  auth:user   cookie:
  auth:user    { path: '/',
  auth:user      _expires: 2019-11-20T05:43:59.297Z,
  auth:user      originalMaxAge: 60000,
  auth:user      httpOnly: true,
  auth:user      domain: '.localhost',
  auth:user      secure: true,
  auth:user      sameSite: true },
  auth:user   passport: { user: 'demo' } }, req.user { email: 'demo' } +1m
› Rendering url /     

11:13:08

After refreshing

Session {
  cookie:
   { path: '/',
     _expires: 2019-11-20T05:44:08.109Z,
     originalMaxAge: 60000,
     httpOnly: true,
     domain: '.localhost',
     secure: true,
     sameSite: true } }
Data fetching /: 1ms

Interesting, it is creating many sessions for the same user

1) "sess:oXb-t9IGNaT6zS46Z6DjPwXPPzfhZ97r" 2) "sess:Q2hoaZA5HxP3LrctsTjMquk5GQRiFgCV" 3) "sess:4tMWV9FSpAfKuZtUgomOIvKQUNKfqrHB" 4) "sess:WG1qcNf8iptxWGaFal9f0IXKWzI-M7xr" 5) "sess:VcKA0trBknU5tYrLaBqqGb61Do4eGbrW" 6) "sess:KHRo516pPwNRBtKx_Is-KsVB54sxoyt-" 7) "sess:AT7JtA4Yy0bJXHpwNMBWY9WBZgT0qDCe" 8) "sess:DJlhDyZk2cIbjfQP5Ye6oR3VosQumpU9" 9) "sess:0sSTqtH9-BWL1ezCU6smw6QMHkZmvP5s" 10) "sess:jRx25eFrrfpFOnoh9Mw9QBbWX4hIqLwT" 11) "sess:TxLZprw-Bgmxt9wTBxXATekYUBihp2zs" 12) "sess:928VQweST-75qJjtHaGS_Pw_jt5H0JyU" 13) "sess:3_AxO8rw6qkNNHE7ELVe_9OxtWTa56Uk"

What is even more interesting is that there are no cookies whatsoever in the storage tab under inspect element even after logging in

But on a sidenote, this could in itself be a nice joke on r/ProgrammerHumor, you get an error, you try something and you get an even worse error 😂😂😂

Update Running again with your settings after a restart

"sess:RfwPamM2w7h-BJ2Q3h1aebm0X8KxqfN7" 2) "sess:C7d46bn7oIXAZgUSYkb8Th4rr2otm4_e" 3) "sess:thxorXreE1QJl3y2rLPEyeOpLvtobWhD" 4) "sess:0XSLMlrkFDRUCBDiRlubGdZoJYSIer6E" 5) "sess:ritJHeAcquu3GH1zGQt6oYYuVJumEihS" 6) "sess:rGQobQEzFvPbs0ulHNUSvcOUW2PKKL5Z" 7) "sess:L_EBv8hInmsxLOp6XbH67PNYII4CwcX5" 8) "sess:QtzEwmQ5WC9iWsysZ2BUKB598dV_6Wam" 9) "sess:_GlgjGSY2pkjJjsbogy5vp_YRs2T42L0" 10) "sess:pKtu_i9o1eMwqwdy7KYkYKjGBFbdVZy5" 11) "sess:en4FQescqOeY9P3BeAYay6eP1LMjIJYO" 12) "sess:iZKnRdvVrV_dW3GD9Pcl0wNSXIBndfam"

No cookies set on the browser whatsoever, let me update the sandbox link here, i added passport to my setup which was not there in the previous link

UPDATE

wavded commented 4 years ago

Couple thoughts:

  1. I think rolling shouldn't be used unless you have a good use case for it. This forces updates to a session continually even if no session data was changed. Given the nature of the application this can cause unwanted races.

  2. You have a race here:

router.post('/logout', (req, res) => {
  req.session.destroy((err) => { // <-- session will destroy after you've returned a response
    if (err) {
      debug(err)
    }
  })
  res.json({ ok: true })
})

Would be better to:

router.post('/logout', (req, res) => {
  req.session.destroy((err) => {
    if (err) {
      debug(err)
    }
    res.json({ ok: true }) // <-- guaranteed response after session has been removed
  })
})
slidenerd commented 4 years ago

@wavded even if rolling is false, dont we have the TTL automatically being incremented as if the session was rolling, so why do we have the rolling option at all, i observed it in the other run

wavded commented 4 years ago

TTL is not automatically incremented when rolling: false, it only updates when the actual session data changes. The rolling: true enables the use of the touch call in storage providers like this one.

I don't see a huge need to use the rolling option but again there probably are cases where it's warranted.