Fix a race condition in Express where browsers redirect before session is persisted

[KidkArolis]

As discussed in #121.

Note - this doesn't "double save" session if resave: false is used. Maybe it doesn't double save eve nif resave: true, but I'm not sure. So overally, I don't know of any downsides to this approach - only a big reliability upside.

Without this guard, grant is unreliable in that by calling res.redirect, even if express-session holds back the response from being fully completed, the redirect headers are sent to browsers and some browsers (e.g. Chrome) eagerly request the new page before session has been commited. This results in auth errors where the callback page can not authenticate the user since the session does not contain the oauth payload. I hope that's clear.

I'd argue this is a bug in express-session - that lib should potentially prevent redirect headers from being sent if session is not persisted. But I can see how that's probably more complicated.

[KidkArolis]

Pushed a new commit that handles errors from calling save.

Also - should I update the connect() bits that do redirects directly to also call session.save?

[simov]

Hi @KidkArolis,

The session store you use is not part of Grant.

Grant only uses the API that all session store implementations had agreed upon in order to be compatible with certain HTTP framework.

Here are a few examples using Express.

You are not bound to use session-store specifically, you just have to use a session store. You can also implement your own, or wrap session-store with your fix and use that instead.

Besides, you can also save the session before entering the Grant routes.

So your fix falls out of scope for this module.

Thanks for your effort!

[KidkArolis]

Hi, thanks for taking a look!

I hope you reconsider. This is a real issue and while it's not necessarily clear where it's best fixed, I'll try to convince you one more time that Grant is possibly the best place for that fix.

For context, the application in my case has the following dependency tree (at least virtually, not necesarilly from npm dependencies point of view):

MyApp
  -> Feathers.js
    -> Grant
      -> Express Session (via injection, not a direct dependency)
        -> Express

The issue that this PR is trying to fix is Saving data in session and redirecting in rapid succession can lead to a race condition causing failed Grant authentication. This is because of how express-session, for better or worse, doesn't withhold the redirect headers until the session completely persists. (Note: although I focus on Express case here, this might or might not be an issue with other frameworks, that's slightly besides the point.)

The official recommendation from Doug Wilson is to wrap redirects in session.save(cb). He recommended this back in 2016 and as recently as last month.

Note that people have been running into this issue for years now. Including in OAuth authentication use case specifically, see Passport issues and PRs, many users over the years reported this exact same issue.

Let's see the possible places to fix this.

Fix in express-session

I'd argue this would be probably the best place to fix this, because this way all upstream consumers would benefit. However, there might be good reasons not to do it there. This issue is slightly use case specific. E.g. if you're doing OAuth/auth redirect where next page view strictly needs to access the session data, it's critical. Maybe in other cases it's tolerable and therefore not worth complicating the logic / impacting performance / breaking backwards compatibility. How would you withold redirects? The current express-session implementation doesn't touch res.redirect it only overrides res.end, it doesn't know that redirect headers have already been sent to the browser. Maybe inspecting headers would be a performance hit, redirects are rare, so it's a waste to check for that every time. Maybe it's possibly to explicitly override res.redirect as a solution instead, but that might break backwards compatiblity. On some level, this is not an issue with express-session, but with how it's API can be used incorrectly.

As I mentioned earlier, Doug's recommendation is to do redirects inside a save callback, res.session.save(() => res.redirect(...)). That's the "correct" usage of the sessions.

The save function is part of the express-session API: https://github.com/expressjs/session#sessionsavecallback.
And all of the popular stores correctly implement the store.set(..., cb) signature:

• connect-redis
• connect-mongo
• connect-session-sequelize
• express-mysql-session

Fix in Grant

As pointed out above, express-session recommends doing redirects inside the save callback. And so one could argue grant is using the session API incorrectly (Koa session also provides save btw, but I'm not sure if it's necessary there since Koa's middleware work differently).

The fix in grant would save all downstream users from suffering from this issue. This issue is quite specific to Grant's use case and implementation - passing data from one route to the next via session. Also, Grant is in control of storing data in the session and doing the redirect so it's the perfect place for making the session usage correct.

Fix in Feathers.js

Feathers is the consumer of Grant. Grant passes auth data via the session mechanism, except it doesn't always work.. We could put the fix in here, but that would only make Feather's users happy and everyone else using Grant will either run into this issue as well or will never notice it's happening in production for some users (it is potentially rare, it's a race condition). If the fix was indeed put in Feathers, this would kind of reveal the flawed Grant design - "to use Grant you plug it in as middleware here, oh, but also, you monkey patch it's redirect functions to workaround a session race condition".

Fix in MyApp

This is what I had to do for now in my app:

app.use('/oauth/connect', (req, res, next) => {
  const redirect = res.redirect
  res.redirect = function(...args) {
    req.session.save(err => {
      if (err) return next(err)
      redirect.call(res, ...args)
    })
  }
  next()
})

Does this mean every Grant/Feathers user will have to do this? That doesn't seem great..

In conclusion

Thanks for reading and I hope this clarifies my point of view. Let me know if I'm missing some easier fix / correct usage.

Finally, I understand Grant is trying to be framework and session implementation agnostic. I'm happy to help fix this issue and update this PR in a way that'd be most inline with Grant's values. For example, apply the fix consistently across frameworks if necessary (e.g. Koa, although it might not have this same issue depending on when Koa sends headers wrt to it's middleware) and in a backwards compatible way (e.g. testing for req.session.save existence).

🙏

[KidkArolis]

Just trying to help make Grant / Feathers / Node ecosystem great.. If I'm wrong on this issue and there's a better place to fix it, I'm happy to do that instead, but so far to me it looks like it's an issue with Grant, that's all.

[simov]

Thanks for the detailed bug report, @KidkArolis!

Can you provide a test case that reproduces this behavior reliably! It doesn't have to be a test case in Grant, just a simple Express/Feathers server instead, using Grant, and probably I few HTTP requests to that server?

If that's too hard to implement, can you provide a Feathers app with a detailed steps on how to reproduce the issue?

[KidkArolis]

Sure! Here's a first cut, haven't had time to look at turning it into a grant test yet, but here's a small example reproducing the issue:

const express = require('express')
const session = require('express-session')
const grant = require('grant-express')

const MemoryStore = session.MemoryStore

const config = {
  "defaults": {
    "transport": "session"
  },
  "github": {
    "key": "KEY",
    "secret": "SECRET",
    "scope": ["public_repo"],
    "callback": "/hello"
  }
}

const DELAY = 100

class SlowMemoryStore extends MemoryStore {
  set(sessionId, session, callback) {
    setTimeout(() => {
      this.sessions[sessionId] = JSON.stringify(session)
      callback()
    }, DELAY)
  }
}

express()
  .use(session({
    secret: 'grant',
    saveUninitialized: true,
    resave: true,
    store: new SlowMemoryStore
  }))
  .use(grant(config))
  .get('/hello', (req, res) => {
    res.end(JSON.stringify(req.session, null, 2))
  })
  .listen(3000)

Steps:

1. Create a new GitHub OAuth app: https://github.com/settings/developers.
2. Fill in credentials
3. Start the server
4. Open http://localhost:3000/connect/github in Chrome (haven't tried other browsers)

Expected behavior – see session contents printed out including response with access_token.
What happens instead – session contents are printed out without any response.

Note that even you keep refreshing this page now, the token won't be there anymore since the new page load overwrote the session with an empty grant object!

Also note that delay might need to be adjusted, I've put it at 100ms to reliably reproduce it on my machine, but I can put it as low as 3ms and I still observe the issue! Wrapping the redirect in req.session.save() fixes the issue for me.

[simov]

Thanks, I can definitely see the issue now, but I want to do some more testing first. Will get back to you in a few days.

[simov]

@KidkArolis just pushed a small wrapper around the redirect handling. I'm thinking about improving my test suite a little bit, so I'll have to do that in a separate branch, but then I'll get back to this one and add the tests for the slow session store. I want to have similar test for Koa as well, even though most probably Koa won't need any additional fixes.

[KidkArolis]

Awesome! 2eb66f4 looks good!

[simov]

Improved my test suite just to figure out that there is no easy way to automate the slow session store one, unless I want to somehow simulate the Chrome's behavior during redirect.

I'll probably add an example about it, so that at least it can be tested manually.

I was definitely able to reproduce it on my end. No idea how no one catched that for so long, but even with the explicit save, now set here, it shouldn't affect existing apps in a negative way, since this is done only during authorization, and in that case you are more interested in correct behavior than speed, and speed might be affected only on external stores.