OAuth redirect lost sometimes due to session store race

[mterrel]

Steps to reproduce

1. Initiate an OAuth login with the redirect query parameter set, for example to https://example.com/oauth/github?redirect=dashboard
2. Go through the OAuth flow to authorize the user.
3. At the end of the flow, the browser should eventually be redirected to https://example.com/dashboard#access_token=xyz123. This happens correctly most of the time. But occasionally, the browser is redirected to https://example.com#access_token=xyz123 instead.

Note that this behavior is difficult to reproduce and appears to require specific timing of external resources (the session store needs to be slow) to trigger the failure.

Expected behavior

At the end of the OAuth flow, the browser should get redirected to the path requested by the initial redirect query parameter 100% of the time.

Actual behavior

At the end of the OAuth flow, the browser gets redirected to the root of the site about 5% of the time (about 1 of every 20 requests redirects incorrectly).

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working):

Feathers 4.5.x:

├─ @feathersjs/adapter-commons@4.5.2
├─ @feathersjs/authentication-client@4.5.8
├─ @feathersjs/authentication-local@4.5.4
├─ @feathersjs/authentication-oauth@4.5.4
├─ @feathersjs/authentication@4.5.8
├─ @feathersjs/commons@4.5.11
├─ @feathersjs/configuration@4.5.3
├─ @feathersjs/errors@4.5.11
├─ @feathersjs/express@4.5.4
├─ @feathersjs/feathers@4.5.8
├─ @feathersjs/rest-client@4.5.11
├─ @feathersjs/socketio-client@4.5.11
├─ @feathersjs/socketio@4.5.11
├─ @feathersjs/transport-commons@4.5.11
├─ feathers-hooks-common@5.0.3
└─ feathers-sequelize@6.2.0
├─ connect-session-sequelize@7.1.1
└─ express-session@1.17.2

NodeJS version:

14.18

Operating System:

Linux

Browser Version:

Chrome 96.0.4664.110 / Win10

React Native Version:
N/A

Module Loader:
None

Analysis

I've looked carefully at this issue by turning on all the Feathers-related debugs and looking closely at the network requests in the browser. I think the issue is due to a race condition with Express sessions in the Feathers OAuth code. Note that we happen to be using connect-session-sequelize and a Postgres DB for our Express session store, but I believe this can happen with any session store if the store is sufficiently slow.

Here's what I found. Please correct me if I've gotten anything wrong.

Feathers and Grant both use the same Express session to store information about the OAuth transaction. Feathers uses it to store the redirect query parameter and the remaining other query parameters as well.

Successful case

In the successful OAuth case, Feathers creates the Express session during the request to /oauth/github, setting the session cookie ID to some ID we'll call "A". Feathers code owns this Express handler (see /packages/authentication-oauth/src/express.ts:49). Feathers then generates a redirect response to /oauth/connect/github and Express session stores the session (in our case, into Postgres with connect-session-sequelize) as part of standard Express flow at the end of a request. The browser now contacts /oauth/connect/github, which is handled by Grant. Express looks for an existing Express session using ID "A", finds the session in Postgres and uses it, passing the session to the Grant handler. A few steps later, when the client requests /oauth/github/authenticate, Feathers again looks up session ID "A", finds it, and successfully retrieves the redirect property from the session, which eventually gets passed back to the browser in the final redirect of the flow.

The browser dev tools network panel in this case shows a single consistent cookie connect.sid value across all of the requests and responses.

Failing case

In the failing case, Feathers creates the Express session during the request /oauth/github, setting the session ID to "A". And Express session begins saving the session to Postgres, BUT this does not complete prior to Feathers sending the redirect to and the client accessing /oauth/connect/github. When the browser does access /oauth/connect/github, the session cookie with session ID "A" is set, so Express looks for an existing Express session using ID "A" in Postgres, does not find it in the database (yet) and creates a new Express session with ID "B", overwriting the session cookie ID in its response. Session "A", which has the original redirect parameter stored is orphaned and never used again. Later, when the client requests /oauth/github/authenticate, Express looks up session ID "B" and finds it, but the redirect property in session "B" was never set and is undefined, so cannot be passed back in the final redirect to the browser.

The browser dev tools network panel in the failing case shows two different connect.sid values. The connect.sid response to /oauth/github and the request to /oauth/connect/github use one ID and all the subsequent responses and requests use a different connect.sid. This is consistent with the explanation above.

Suggested fix

Because Feathers issues a redirect and expects the Express session to be saved before the browser initiates the next request, Feathers should explicitly save the Express session and wait for completion prior to issuing the initial redirect.

This is consistent with guidance from Express session's docs:

Session.save(callback)
Save the session back to the store, replacing the contents on the store with the contents in memory (though a store may do something else--consult the store's documentation for exact behavior).
This method is automatically called at the end of the HTTP response if the session data has been altered (though this behavior can be altered with various options in the middleware constructor). Because of this, typically this method does not need to be called.
There are some cases where it is useful to call this method, for example, redirects, long-lived requests or in WebSockets.

Also see issues like https://github.com/expressjs/session/issues/660

So I think the fix is to modify /packages/authentication-oauth/src/express.ts:60 roughly like this:

-     res.redirect(`${path}/connect/${name}?${qs.stringify(query as any)}`);
+     req.session.save((err) => {
+       if (err) res.status(500).send(`Error storing session: ${err}`);
+       else res.redirect(`${path}/connect/${name}?${qs.stringify(query as any)}`);
+     });

I've made a PR (#2515) to address this issue.

[daffl]

This has been published via #2515. Thank you again for digging into this!