Skip to content

Commit 5b2f289

Browse files
committed
fix(auth): enhance OAuth flow and improve account security
- Prevent OAuth redirect loops by implementing safe returnTo paths and better handling for duplicate accounts - Standardize email normalization across OAuth and manual signup - Fix Twitch OAuth email extraction - Implement secure account discovery flow with email notifications and mitigate user enumeration vulnerabilities in signup/password flows - Reject disposable emails during signup and profile updates - Additional unit tests
1 parent c59e9f0 commit 5b2f289

File tree

6 files changed

+432
-131
lines changed

6 files changed

+432
-131
lines changed

app.js

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,9 @@ app.use((req, res, next) => {
138138
res.locals.user = req.user;
139139
next();
140140
});
141+
// Function to validate if the URL is a safe relative path
142+
const isSafeRedirect = (url) => /^\/[a-zA-Z0-9/_-]*$/.test(url);
141143
app.use((req, res, next) => {
142-
// Function to validate if the URL is a safe relative path
143-
const isSafeRedirect = (url) => /^\/[a-zA-Z0-9/]*$/.test(url);
144-
145144
// After successful login, redirect back to the intended page
146145
if (!req.user && req.path !== '/login' && req.path !== '/signup' && !req.path.match(/^\/auth/) && !req.path.match(/\./)) {
147146
const returnTo = req.originalUrl;
@@ -235,56 +234,80 @@ app.post('/api/openai-moderation', apiController.postOpenAIModeration);
235234
app.get('/api/togetherai-classifier', apiController.getTogetherAIClassifier);
236235
app.post('/api/togetherai-classifier', apiController.postTogetherAIClassifier);
237236

237+
/**
238+
* OAuth authentication failure handler (common for all providers)
239+
* passport.js requires a static route for failureRedirect.
240+
* With this auth failure handler, we can decide where to redirect the user
241+
* and avoid infinite loops in cases when they navigate to a route
242+
* protected by isAuthorized and the user is not authorized.
243+
*/
244+
app.get('/auth/failure', (req, res) => {
245+
// Check if a flash message for 'errors' already exists in the session (do not consume it)
246+
const hasErrorFlash = req.session && req.session.flash && req.session.flash.errors && req.session.flash.errors.length > 0;
247+
248+
if (!hasErrorFlash) {
249+
req.flash('errors', { msg: 'Authentication failed or provider account is already linked.' });
250+
}
251+
const { returnTo } = req.session;
252+
req.session.returnTo = undefined;
253+
// Prevent infinite loop: if returnTo is the current URL or an /auth/ route, redirect to /
254+
if (!returnTo || !isSafeRedirect(returnTo) || returnTo === req.originalUrl || /^\/auth\//.test(returnTo)) {
255+
res.redirect('/');
256+
} else {
257+
res.redirect(returnTo);
258+
}
259+
});
260+
238261
/**
239262
* OAuth authentication routes. (Sign in)
240263
*/
241264
app.get('/auth/facebook', passport.authenticate('facebook'));
242-
app.get('/auth/facebook/callback', passport.authenticate('facebook', { failureRedirect: '/login' }), (req, res) => {
265+
app.get('/auth/facebook/callback', passport.authenticate('facebook', { failureRedirect: '/auth/failure' }), (req, res) => {
243266
res.redirect(req.session.returnTo || '/');
244267
});
245268
app.get('/auth/github', passport.authenticate('github'));
246-
app.get('/auth/github/callback', passport.authenticate('github', { failureRedirect: '/login' }), (req, res) => {
269+
app.get('/auth/github/callback', passport.authenticate('github', { failureRedirect: '/auth/failure' }), (req, res) => {
247270
res.redirect(req.session.returnTo || '/');
248271
});
249272
app.get('/auth/google', passport.authenticate('google'));
250-
app.get('/auth/google/callback', passport.authenticate('google', { failureRedirect: '/login' }), (req, res) => {
273+
app.get('/auth/google/callback', passport.authenticate('google', { failureRedirect: '/auth/failure' }), (req, res) => {
251274
res.redirect(req.session.returnTo || '/');
252275
});
253276
app.get('/auth/x', passport.authenticate('X'));
254-
app.get('/auth/x/callback', passport.authenticate('X', { failureRedirect: '/login' }), (req, res) => {
277+
app.get('/auth/x/callback', passport.authenticate('X', { failureRedirect: '/auth/failure' }), (req, res) => {
255278
res.redirect(req.session.returnTo || '/');
256279
});
257280
app.get('/auth/linkedin', passport.authenticate('linkedin'));
258-
app.get('/auth/linkedin/callback', passport.authenticate('linkedin', { failureRedirect: '/login' }), (req, res) => {
281+
app.get('/auth/linkedin/callback', passport.authenticate('linkedin', { failureRedirect: '/auth/failure' }), (req, res) => {
259282
res.redirect(req.session.returnTo || '/');
260283
});
261284
app.get('/auth/twitch', passport.authenticate('twitch'));
262-
app.get('/auth/twitch/callback', passport.authenticate('twitch', { failureRedirect: '/login' }), (req, res) => {
285+
app.get('/auth/twitch/callback', passport.authenticate('twitch', { failureRedirect: '/auth/failure' }), (req, res) => {
263286
res.redirect(req.session.returnTo || '/');
264287
});
265288
app.get('/auth/discord', passport.authenticate('discord'));
266-
app.get('/auth/discord/callback', passport.authenticate('discord', { failureRedirect: '/login' }), (req, res) => {
289+
app.get('/auth/discord/callback', passport.authenticate('discord', { failureRedirect: '/auth/failure' }), (req, res) => {
267290
res.redirect(req.session.returnTo || '/');
268291
});
269292

270293
/**
271294
* OAuth authorization routes. (API examples)
272295
*/
273296
app.get('/auth/tumblr', passport.authorize('tumblr'));
274-
app.get('/auth/tumblr/callback', passport.authorize('tumblr', { failureRedirect: '/api' }), (req, res) => {
275-
res.redirect(req.session.returnTo);
297+
app.get('/auth/tumblr/callback', passport.authorize('tumblr', { failureRedirect: '/auth/failure' }), (req, res) => {
298+
res.redirect(req.session.returnTo || '/');
276299
});
277300
app.get('/auth/steam', passport.authorize('steam-openid'));
278-
app.get('/auth/steam/callback', passport.authorize('steam-openid', { failureRedirect: '/api' }), (req, res) => {
279-
res.redirect(req.session.returnTo);
301+
app.get('/auth/steam/callback', passport.authorize('steam-openid', { failureRedirect: '/auth/failure' }), (req, res) => {
302+
res.redirect(req.session.returnTo || '/');
280303
});
281304
app.get('/auth/trakt', passport.authorize('trakt'));
282-
app.get('/auth/trakt/callback', passport.authorize('trakt', { failureRedirect: '/login' }), (req, res) => {
283-
res.redirect(req.session.returnTo);
305+
app.get('/auth/trakt/callback', passport.authorize('trakt', { failureRedirect: '/auth/failure' }), (req, res) => {
306+
res.redirect(req.session.returnTo || '/');
284307
});
285308
app.get('/auth/quickbooks', passport.authorize('quickbooks'));
286-
app.get('/auth/quickbooks/callback', passport.authorize('quickbooks', { failureRedirect: '/login' }), (req, res) => {
287-
res.redirect(req.session.returnTo);
309+
app.get('/auth/quickbooks/callback', passport.authorize('quickbooks', { failureRedirect: '/auth/failure' }), (req, res) => {
310+
res.redirect(req.session.returnTo || '/');
288311
});
289312

290313
/**

0 commit comments

Comments
 (0)