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

req.isAuthenticated occasionally fails - race condition? #306

Closed
benheymink opened this issue Dec 15, 2014 · 15 comments
Closed

req.isAuthenticated occasionally fails - race condition? #306

benheymink opened this issue Dec 15, 2014 · 15 comments

Comments

@benheymink
Copy link

I have set up passport to use a custom authentication scheme, but for arguments sake, imagine it authenticates any user to the app. I have a 'landing' page with a login button that issues a post, handled as follows:

app.post('/:id/landing', function (req, res, next) {
  passport.authenticate('myAuth', function (err, user) {
    if (err || !user) {
      return res.redirect('/' + req.params.id + '/landing' + req.body.redirectTo);
    } else {
      req.logIn(user, function () {
        return res.redirect('/' + req.params.id + '/' + req.body.redirectTo);
      });
    }
  })(req, res, next);
});

later, a route handler matches the redirect:

app.all('/:id/', function (req, res) {
  if (req.isAuthenticated()) {
    res.sendFile('index.html', {root: __dirname + '/../frontend/'});
  } else {
    // Instead of redirecting to /landing, simply render it.
    // this gets round safari issue with losing url fragments during a redirect:
    // https://bugs.webkit.org/show_bug.cgi?id=24175
    //
    res.render('landing', {message: '', previousID: '/' + req.params.id});
  }
});

9 times out of ten, this all works fine, but occasionally req.isAuthenticated will return false. If I put some logging in, I can see that before the redirect I have a valid req.user object, but then in the second route handler following the redirect, req.user is undefined. Sometimes it works, sometimes it doesn't! (When it does work, req.user IS defined in the second route handler) Is this issue the same as others have reported around the user not being serialised correctly?

@benheymink
Copy link
Author

Actually I think my issue may be related to the redirect and my use of express: expressjs/session#69

@ChristopherGS
Copy link

@benheymink was it the redirect issue you mention in your comment? I'm experiencing the same problem.

@benheymink
Copy link
Author

@ChristopherGS The issue was that req.logIn is async, and hadn't finished flushing to the session store by the time the next request had come in for the redirect resource. I got around it by manually saving the season before the redirect:

          req.logIn(user, function () {
            // Manually save session before redirect. See bug https://github.com/expressjs/session/pull/69
            req.session.save(function(){
                res.redirect('/someURL');
            });
          });

Hope this helps!

@ChristopherGS
Copy link

@benheymink thanks for the explanation - will give this a try. Like you say, it's a tricky one because it only shows up about 1 in 10 times.

@smartmouse
Copy link

We also had such issues.
But this issue does not happen on all of our machines, we only see this in some of them.
Do you know what are the conditions that will trigger this?
Could this be some default flags that is uninitialized that causes the behavour?

We ran into the same issues , after a few redirects, the req.user=undefined

currentLng set to: en-US
debug: User is authenticated. Proceeding to the next page
{"req.user":{"password":"$2a$08$kBhANRyFam8eyNt4z8CXw.lNyixXbGVFVFEXNVrLO3agGQfmxy5jS","username":"pure1","_id":"55074542a34c0a9b55fe3a76","__v":0}}
debug: GET /gallery/ 304 162.091 ms - -

currentLng set to: en-US
debug: User is authenticated. Proceeding to the next page
{"req.user":{"password":"$2a$08$kBhANRyFam8eyNt4z8CXw.lNyixXbGVFVFEXNVrLO3agGQfmxy5jS","username":"pure1","_id":"55074542a34c0a9b55fe3a76","__v":0}}
debug: GET /resources 304 164.363 ms - -

currentLng set to: en-US
debug: User is authenticated. Proceeding to the next page
{"req.user":{"password":"$2a$08$kBhANRyFam8eyNt4z8CXw.lNyixXbGVFVFEXNVrLO3agGQfmxy5jS","username":"pure1","_id":"55074542a34c0a9b55fe3a76","__v":0}}
debug: GET /tutorials/ 304 164.499 ms - -

currentLng set to: en-US
debug: User is authenticated. Proceeding to the next page
{"req.user":{"password":"$2a$08$kBhANRyFam8eyNt4z8CXw.lNyixXbGVFVFEXNVrLO3agGQfmxy5jS","username":"pure1","_id":"55074542a34c0a9b55fe3a76","__v":0}}
debug: GET / 304 163.698 ms - -

currentLng set to: en-US
debug: User is authenticated. Proceeding to the next page
{"req.user":{"password":"$2a$08$kBhANRyFam8eyNt4z8CXw.lNyixXbGVFVFEXNVrLO3agGQfmxy5jS","username":"pure1","_id":"55074542a34c0a9b55fe3a76","__v":0}}
debug: GET /tutorials/ 304 165.213 ms - -

currentLng set to: en-US
debug: User failed authentication. Proceeding to the login page
{"auth":false} req.user=undefined
debug: GET /resources 302 241.021 ms - 70

@smartmouse
Copy link

Could it be related to this issue?

#320

That is why I am seeing it happening on some machines consistently and not on other machines.

funny thing is if I run my app with node debug app.js, the issue cannot be reproduced.
Looks like debug changes the timing or memory being loaded.

@benheymink
Copy link
Author

@smartmouse You are probably seeing it on some machines and not others as the saving of the session is ASYNC. When you debug your app you subtly change the timings of your functions, which is perhaps why it now works. If you want to confirm, try the work around I proposed above, manually saving the session before doing any post-authentication redirect:

      req.logIn(user, function () {
        // Manually save session before redirect. See bug https://github.com/expressjs/session/pull/69
        req.session.save(function(){
            res.redirect('/someURL');
        });
      });

The other issue you linked to is unrelated I believe.

@tkalfigo
Copy link

Just spent the better part of the last 3 days trying to figure out what was going on. Exact same issue using Postgres as session store. Intermittently the log in redirect would fail even though the user had authenticated correctly i.e. he would again see the log in screen, but if he typed manually a privileged page, he would see it just fine. All this with [email protected], [email protected], [email protected] and [email protected]

Thanks for the temp solution @benheymink . The forced session.save() workaround seems to work though not sure it resolves the problem 100% of the time. Just stopped seeing failures in a stretch of N consecutive log ins when earlier there were some.

@benheymink
Copy link
Author

@tkalfigo Weird, it should save it 100% of the time. Are you sure you are only finishing the request/performing a redirect AFTER the session.save callback has completed? Doing the following won't work:

req.session.save()
req.redirect()

Make sure you are finishing the request in the callback:

req.session.save(function(){
  req.redirect()
});

@tkalfigo
Copy link

tkalfigo commented Jun 2, 2015

@benheymink I didn't say it doesn't work 100% of the time. I meant that even though I see it working correctly now, I cannot be sure it is full proof. Admittedly, have been working with it the past 2 weeks without problems.

@bradocchs
Copy link

FYI - Just experienced this problem (September 2017) and this fix still works!

@mckenzieja
Copy link

@benheymink how do you prevent the req.logIn method from attempting to save the session twice?

@tilleps
Copy link

tilleps commented Sep 7, 2018

If anyone is using cookie-session, the above/following might not work (as the cookie-session's save() does not support callbacks):

req.session.save(function(){
  req.redirect();
});

Managed to get it to work by setting some value on the session before redirecting:

req.session.timestamp = new Date();
req.redirect();

@matthiasprieth
Copy link

matthiasprieth commented Aug 3, 2022

Hi, unfortunately all the fixes described above did not solve the problem. I ended up using https://github.com/expressjs/cookie-session. It worked out of the box. I did not have to change a single line of code except for the config.

I realized it's a better fit for this thread: #314

@ianormous
Copy link

2022 and redirect as a callback to login fixed this same issue

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

9 participants