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

Feature request - handle regenerate race condition #923

Open
htadeusiak opened this issue Dec 6, 2022 · 0 comments
Open

Feature request - handle regenerate race condition #923

htadeusiak opened this issue Dec 6, 2022 · 0 comments

Comments

@htadeusiak
Copy link

So it's kind of a weird bug and I'm not 100% confident I know whats going on, but I have an idea and hoping you guys can help me confirm it.

Problem Statement:

I want to regenerate SID every hour while keeping the same underlying data in the Redis store. However, I also fire 2/3 requests off at the same time when I want to regenerate the SID (which is why I'm having issues).

Note: my implementation is a middleware that just checks if SID has been alive for a certain amount of time on every request. Thus if I fire off 2/3 requests and it is expired, it will try to regenerate each one.

Examples of logs with firing 2 requests off at the same time:

  1. req_1 & req_2 fired off about the same time from browser, both should have the cookie with sessionID sid_0
  2. req_1 arrives at my isExpired function first and because it is expired, it calls req.session.regenerate()
  3. req_1 destroys session sid_0 and moves old data into new session sid_1
  4. req_2 arrives at my isExpired with a never before seen sessionID(lets say sid_2). Because its a brand new session cookie is not considered as expired and thus regenerate is not called
  5. req_1 returns and sets sid cookie sid_1 (which references moved data in redis)
  6. req_2 returns 401 and doesn't set cookie in the browser to be sid_2
  7. because all future requests will use sid_1, all requests start working again

So the first issue here is at step 4. I would think req_2 should arrive with sid_0. But it instead has a completely new SID which has no data tied to it in my store (of course). My only thought would be because I called "regenerate" just before req_2 gets through the express-session middleware. Thus the middleware thinks that it is expired/doesn't exists and tries generating a new session. (This is the part where I'm most confused and those who know the library better can help out and explain if thats how it actually works under the hood)

The second thing that comes to mind is why req_2 does not set the session sid_2 in the browser (step 6)? I'm pretty sure this is an issue on my side with my implementation, so don't bother too much here. I think it's because when the request errors out it calls res.sendStatus(401) right after without a next thus res.cookie() would never be called?

Third is the race problem, even if req_2 arrived with the same sessionId (sid_0) as req_1 did. The store data would have already been deleted from the regenerate call in req_1. Thus req_2 would have no data in its session and would fail. How should we handle these race cases? Would you guys be open to some sort of flag on regenerate that lets the store data persist longer? Or even giving us access to "generate"?

let me know if you would like me to explain anything further. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant