oidc-middleware: session not properly stored before redirect

[zypA13510]

Issue description

When visiting an URL protected by oidcUtil.ensureAuthenticated, the redirect happened prematurely before the session is properly saved in the session store, resulting in a new session being created at the URL specified in routes.login.path. And when the user is authenticated, the callback cannot redirect the user back to the URL protected by oidcUtil.ensureAuthenticated, because the original session is lost.

For more information, refers to mweibel/connect-session-sequelize#39 and mweibel/connect-session-sequelize#54. It seems that connect-couchdb, connect-mongo and connect-session-sequelize are all affected by this issue, judging from the issues referencing to mweibel/connect-session-sequelize#20.

Possible solution

I have traced the issue to a specific middleware used by oidc-middleware, it's connect-ensure-login. And I have created a pull request here jaredhanson/connect-ensure-login#28. But the repo seems to be no longer maintained. I would suggest that you fork connect-ensure-login to maintain usability.

Steps to reproduce

1. Create a new project folder via npm init, and index.js with the following content:

const express = require('express');
const session = require('express-session');
const { ExpressOIDC } = require('@okta/oidc-middleware');

var app = express();

const Sequelize = require(`sequelize`);
const SequelizeStore = require(`connect-session-sequelize`)(session.Store);
var sequelize = new Sequelize(`db_name`, `db_user`, `db_password`, {
	host: `db_host`,
	dialect: `mysql`,
	operatorsAliases: false,
});

const Session = sequelize.define(`Session`, {
/* table definition omitted*/
});

var sessionStore = new SequelizeStore({
	db: sequelize,
	table: `Session`,
});

app.use(session({
	secret: 'keyboard cat',
	store: sessionStore,
	resave: false,
	saveUninitialized: false,
}));

var oidc = new ExpressOIDC({
	issuer: '***',
	client_id: '***',
	client_secret: '***',
	redirect_uri: 'http://localhost:3000/authorization-code/callback',
	scope: 'openid profile',
});

app.use(oidc.router);

app.get('/test', oidc.ensureAuthenticated({
	redirectTo: '/login',
}), (req, res) => {
	res.send(req.session.passport);
});

oidc.on('ready', () => {
	app.listen(3000);
});

oidc.on('error', err => {
	console.log('Unable to configure ExpressOIDC', err);
});

2. npm install dependencies, and run it in node.js
3. Visit http://localhost:3000/test in your browser

Expected result

The user is first redirected to http://localhost:3000/login, then to Okta, then after successful authentication, back to http://localhost:3000/authorization-code/callback, and to http://localhost:3000/test

Actual result

The user is first redirected to http://localhost:3000/login, in Chrome Dev Tools, I can see that a new session cookie is set (despite the request to /login carries a cookie already). Then the user is redirected to Okta. After successful authentication, the user is redirected back to http://localhost:3000/authorization-code/callback, and then to http://localhost:3000/
In the SQL database, I can see two sessions being created. The first one is the one from /test (first time) response, the session is never authenticated nor updated later, and it carries a returnTo:"/test" attribute. The second one is from /login response, the session is later authenticated, but does not carry a returnTo:"/test" at the time authentication is being made.

[robertjd]

Thanks @zypA13510 , I'm able to see the two connection sessions that you talk about in your reproduction steps. I'll want to spend some more time with this to make sure that I fully understand the problem/solution, but I'll have to defer that for a bit.

[zypA13510]

@robertjd actually I have fixed this for my case already. See zypA13510/oidc-okta/fix-upstream. You can review it and apply it back to the main repo if you would like. (For the sake of easier npm install, I have split the monorepo to just oidc-middleware, but which also means that GitHub diff won't work anymore)

[zypA13510]

FYI I just fixed an error in the previous fix. It's now working fine for me. Please let me know if you would like me to open a PR or provide more info.
P.S. most lines added are copied and refactored from passport codebase directly.

[jmelberg-okta]

Hi @zypA13510 - Sorry for the delay.

Just to clarify, is the expectation that the session cookie will remain constant regardless of the user's authenticated or unauthenticated state? If so, saveUninitialized might be a viable workaround option.

Or - are you suggesting that no session cookie should be created until the user enters an authenticated state?

[zypA13510]

Hi @jmelberg-okta, the issue is not caused by saveUninitialized, because it also happens after calling /authorization-code/callback sometimes, thus redirecting back to e.g. /protected will cause another round of login process. Clearly, it's because the session is not saved in time, before the redirect happens, causing a race condition.

The documentation on express-session specifically stated that:

Session.save(callback)

...
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, please see jaredhanson/passport#680 and the bunch of issues linked from there. I believe you will agree with me once you read through all those people's issues. Unfortunately, the maintainer of the repo is not responding.

[zypA13510]

And btw,

saveUninitialized

Forces a session that is "uninitialized" to be saved to the store. A session is uninitialized when it is new but not modified.

Here, obviously it is modified:

req.session.returnTo = req.originalUrl || req.url;

[zypA13510]

@jmelberg-okta Is there any update on this issue?

[jtrebilcockstash]

I can confirm this issue is also happening in the connect-pg-simple package

[swiftone]

Internal ref: OKTA-255316