From 5781a2dc651521fa153936112280d529b70be01d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bar=C4=B1=C5=9F=20Soner=20U=C5=9Fakl=C4=B1?= Date: Wed, 27 May 2020 12:15:02 -0400 Subject: [PATCH] feat: fix session mismatch errors by clearing cookie on logout (#8338) * feat: fix session mismatch errors by clearing cookie on logout * feat: remove app.upateHeader ported from 2.0 * feat: handle if user doesn't click button and just refreshes page --- public/src/admin/admin.js | 4 +- public/src/app.js | 131 +++++------------------------- public/src/client/login.js | 15 +--- src/controllers/api.js | 1 - src/controllers/authentication.js | 85 ++++++------------- src/middleware/header.js | 1 + src/middleware/headers.js | 7 -- src/routes/authentication.js | 3 +- src/webserver.js | 2 +- 9 files changed, 54 insertions(+), 195 deletions(-) diff --git a/public/src/admin/admin.js b/public/src/admin/admin.js index ee2432fa5c..0b0bfc86a0 100644 --- a/public/src/admin/admin.js +++ b/public/src/admin/admin.js @@ -37,7 +37,9 @@ }); } - $('[component="logout"]').on('click', app.logout); + $('[component="logout"]').on('click', function () { + app.logout(); + }); configureSlidemenu(); setupNProgress(); diff --git a/public/src/app.js b/public/src/app.js index 7674c9dc2d..e06a5a5c85 100644 --- a/public/src/app.js +++ b/public/src/app.js @@ -57,7 +57,9 @@ app.cacheBuster = null; app.newTopic(); }); - $('#header-menu .container').on('click', '[component="user/logout"]', app.logout); + $('#header-menu .container').on('click', '[component="user/logout"]', function () { + app.logout(); + }); Visibility.change(function (event, state) { if (state === 'visible') { @@ -105,111 +107,27 @@ app.cacheBuster = null; }); }; - app.updateHeader = function (data, callback) { - /** - * data: - * header (obj) - * config (obj) - * next (string) - */ - require([ - 'benchpress', - 'translator', - 'forum/unread', - 'forum/header/notifications', - 'forum/header/chat', - ], function (Benchpress, translator, Unread, Notifications, Chat) { - app.user = data.header.user; - data.header.config = data.config; - config = data.config; - Benchpress.setGlobal('config', config); - - var htmlEl = $('html'); - htmlEl.attr('data-dir', data.header.languageDirection); - htmlEl.css('direction', data.header.languageDirection); - - // Manually reconnect socket.io - socket.close(); - socket.open(); - - // Re-render top bar menu - var toRender = { - 'slideout-menu': $('.slideout-menu'), - menu: $('#header-menu .container'), - 'chats-menu': $('#chats-menu'), - }; - Promise.all(Object.keys(toRender).map(function (tpl) { - return Benchpress.render('partials/' + tpl, data.header).then(function (render) { - return translator.Translator.create().translate(render); - }); - })).then(function (html) { - Object.keys(toRender) - .map(function (k) { return toRender[k]; }) - .forEach(function (element, idx) { - element.html(html[idx]); - }); - Unread.initUnreadTopics(); - Notifications.prepareDOM(); - Chat.prepareDOM(); - app.reskin(data.header.bootswatchSkin); - translator.switchTimeagoLanguage(callback); - bootbox.setLocale(config.userLang); - - if (config.searchEnabled) { - app.handleSearch(); - } - - handleStatusChange(); - - $(window).trigger('action:app.updateHeader'); - }); - }); - }; - - app.logout = function (e) { - if (e) { - e.preventDefault(); - } + app.logout = function (redirect) { + redirect = redirect === undefined ? true : redirect; $(window).trigger('action:app.logout'); - /* - Set session refresh flag (otherwise the session check will trip and throw invalid session modal) - We know the session is/will be invalid (uid mismatch) because the user is logging out - */ - app.flags = app.flags || {}; - app.flags._sessionRefresh = true; - $.ajax(config.relative_path + '/logout', { type: 'POST', headers: { 'x-csrf-token': config.csrf_token, }, success: function (data) { - // ACP logouts go to frontend via page load, not ajaxify - if (ajaxify.data.template.name.startsWith('admin/')) { - $(window).trigger('action:app.loggedOut', data); - window.location.href = config.relative_path + (data.next || '/'); - return; - } - - app.updateHeader(data, function () { - // Overwrite in hook (below) to redirect elsewhere - data.next = data.next || undefined; - - $(window).trigger('action:app.loggedOut', data); + $(window).trigger('action:app.loggedOut', data); + if (redirect) { if (data.next) { - if (data.next.startsWith('http')) { - window.location.href = data.next; - return; - } - - ajaxify.go(data.next); + window.location.href = data.next; } else { - ajaxify.refresh(); + window.location.reload(); } - }); + } }, }); + return false; }; app.alert = function (params) { @@ -249,26 +167,15 @@ app.cacheBuster = null; }; app.handleInvalidSession = function () { - if (app.flags && app.flags._sessionRefresh) { - return; - } - - app.flags = app.flags || {}; - app.flags._sessionRefresh = true; - socket.disconnect(); - - require(['translator'], function (translator) { - translator.translate('[[error:invalid-session-text]]', function (translated) { - bootbox.alert({ - title: '[[error:invalid-session]]', - message: translated, - closeButton: false, - callback: function () { - window.location.reload(); - }, - }); - }); + app.logout(false); + bootbox.alert({ + title: '[[error:invalid-session]]', + message: '[[error:invalid-session-text]]', + closeButton: false, + callback: function () { + window.location.reload(); + }, }); }; diff --git a/public/src/client/login.js b/public/src/client/login.js index 317e61ae3d..23d0d07d62 100644 --- a/public/src/client/login.js +++ b/public/src/client/login.js @@ -24,26 +24,18 @@ define('forum/login', [], function () { submitEl.addClass('disabled'); - /* - Set session refresh flag (otherwise the session check will trip and throw invalid session modal) - We know the session is/will be invalid (uid mismatch) because the user is attempting a login - */ - app.flags = app.flags || {}; - app.flags._sessionRefresh = true; formEl.ajaxSubmit({ headers: { 'x-csrf-token': config.csrf_token, }, success: function (data) { + var pathname = utils.urlToLocation(data.next).pathname; var params = utils.params({ url: data.next }); params.loggedin = true; + var qs = decodeURIComponent($.param(params)); - app.updateHeader(data, function () { - ajaxify.go(data.next); - app.flags._sessionRefresh = false; - $(window).trigger('action:app.loggedIn', data); - }); + window.location.href = pathname + '?' + qs; }, error: function (data) { if (data.status === 403 && data.responseText === 'Forbidden') { @@ -52,7 +44,6 @@ define('forum/login', [], function () { errorEl.find('p').translateText(data.responseText); errorEl.show(); submitEl.removeClass('disabled'); - app.flags._sessionRefresh = false; // Select the entire password if that field has focus if ($('#password:focus').length) { diff --git a/src/controllers/api.js b/src/controllers/api.js index faf4695533..fcf0e5e81e 100644 --- a/src/controllers/api.js +++ b/src/controllers/api.js @@ -93,7 +93,6 @@ apiController.loadConfig = async function (req) { config.topicSearchEnabled = settings.topicSearchEnabled || false; config.bootswatchSkin = (meta.config.disableCustomUserSkins !== 1 && settings.bootswatchSkin && settings.bootswatchSkin !== '') ? settings.bootswatchSkin : ''; config = await plugins.fireHook('filter:config.get', config); - req.res.locals.config = config; return config; }; diff --git a/src/controllers/authentication.js b/src/controllers/authentication.js index 2256b6b905..b3a4738e45 100644 --- a/src/controllers/authentication.js +++ b/src/controllers/authentication.js @@ -15,7 +15,6 @@ const plugins = require('../plugins'); const utils = require('../utils'); const translator = require('../translator'); const helpers = require('./helpers'); -const middleware = require('../middleware'); const privileges = require('../privileges'); const sockets = require('../socket.io'); @@ -159,9 +158,7 @@ authenticationController.registerComplete = function (req, res, next) { async.parallel(callbacks, async function (_blank, err) { if (err.length) { - err = err.filter(Boolean).map(function (err) { - return err.message; - }); + err = err.filter(Boolean).map(err => err.message); } if (err.length) { @@ -232,7 +229,7 @@ authenticationController.login = function (req, res, next) { }; function continueLogin(req, res, next) { - passport.authenticate('local', function (err, userData, info) { + passport.authenticate('local', async function (err, userData, info) { if (err) { return helpers.noScriptErrors(req, res, err.message, 403); } @@ -258,51 +255,28 @@ function continueLogin(req, res, next) { winston.verbose('[auth] Triggering password reset for uid ' + userData.uid + ' due to password policy'); req.session.passwordExpired = true; - async.series({ - code: async.apply(user.reset.generate, userData.uid), - buildHeader: async.apply(middleware.buildHeader, req, res), - header: async.apply(middleware.generateHeader, req, res, {}), - }, function (err, payload) { - if (err) { - return helpers.noScriptErrors(req, res, err.message, 403); - } - - res.status(200).send({ - next: nconf.get('relative_path') + '/reset/' + payload.code, - header: payload.header, - config: res.locals.config, - }); + const code = await user.reset.generate(userData.uid); + res.status(200).send({ + next: nconf.get('relative_path') + '/reset/' + code, }); } else { delete req.query.lang; + await authenticationController.doLogin(req, userData.uid); + var destination; + if (req.session.returnTo) { + destination = req.session.returnTo; + delete req.session.returnTo; + } else { + destination = nconf.get('relative_path') + '/'; + } - async.series({ - doLogin: async.apply(authenticationController.doLogin, req, userData.uid), - buildHeader: async.apply(middleware.buildHeader, req, res), - header: async.apply(middleware.generateHeader, req, res, {}), - }, function (err, payload) { - if (err) { - return helpers.noScriptErrors(req, res, err.message, 403); - } - - var destination; - if (!req.session.returnTo) { - destination = nconf.get('relative_path') + '/'; - } else { - destination = req.session.returnTo; - delete req.session.returnTo; - } - - if (req.body.noscript === 'true') { - res.redirect(destination + '?loggedin'); - } else { - res.status(200).send({ - next: destination, - header: payload.header, - config: res.locals.config, - }); - } - }); + if (req.body.noscript === 'true') { + res.redirect(destination + '?loggedin'); + } else { + res.status(200).send({ + next: destination, + }); + } } })(req, res, next); } @@ -416,6 +390,7 @@ const destroyAsync = util.promisify((req, callback) => req.session.destroy(callb authenticationController.logout = async function (req, res, next) { if (!req.loggedIn || !req.sessionID) { + res.clearCookie(nconf.get('sessionKey'), meta.configs.cookie.get()); return res.status(200).send('not-logged-in'); } const uid = req.uid; @@ -434,24 +409,16 @@ authenticationController.logout = async function (req, res, next) { await db.sortedSetAdd('users:online', Date.now() - (meta.config.onlineCutoff * 60000), uid); await plugins.fireHook('static:user.loggedOut', { req: req, res: res, uid: uid, sessionID: sessionID }); - const autoLocaleAsync = util.promisify(middleware.autoLocale); - await autoLocaleAsync(req, res); - // Force session check for all connected socket.io clients with the same session id sockets.in('sess_' + sessionID).emit('checkSession', 0); - if (req.body.noscript === 'true') { - return res.redirect(nconf.get('relative_path') + '/'); - } - const buildHeaderAsync = util.promisify(middleware.buildHeader); - const generateHeaderAsync = util.promisify(middleware.generateHeader); - - await buildHeaderAsync(req, res); - const header = await generateHeaderAsync(req, res, {}); const payload = { - header: header, - config: res.locals.config, + next: nconf.get('relative_path') + '/', }; plugins.fireHook('filter:user.logout', payload); + + if (req.body.noscript === 'true') { + return res.redirect(payload.next); + } res.status(200).send(payload); } catch (err) { next(err); diff --git a/src/middleware/header.js b/src/middleware/header.js index 8235417c2c..6963a06565 100644 --- a/src/middleware/header.js +++ b/src/middleware/header.js @@ -45,6 +45,7 @@ module.exports = function (middleware) { }, next); }, function (results, next) { + res.locals.config = results.config; // Return no arguments setImmediate(next); }, diff --git a/src/middleware/headers.js b/src/middleware/headers.js index 32d87721dd..2bce51b9dc 100644 --- a/src/middleware/headers.js +++ b/src/middleware/headers.js @@ -3,7 +3,6 @@ var os = require('os'); var winston = require('winston'); var _ = require('lodash'); -const nconf = require('nconf'); var meta = require('../meta'); var languages = require('../languages'); @@ -55,12 +54,6 @@ module.exports = function (middleware) { headers['X-Upstream-Hostname'] = os.hostname(); } - // Ensure that the session is valid. This block guards against edge-cases where the server-side session has - // been deleted (but client-side cookie still exists) - if (req.uid > 0 && !req.session.meta && !res.get('Set-Cookie')) { - res.clearCookie(nconf.get('sessionKey'), meta.configs.cookie.get()); - } - for (var key in headers) { if (headers.hasOwnProperty(key) && headers[key]) { res.setHeader(key, headers[key]); diff --git a/src/routes/authentication.js b/src/routes/authentication.js index 209c268a5c..555c75d778 100644 --- a/src/routes/authentication.js +++ b/src/routes/authentication.js @@ -87,8 +87,7 @@ Auth.reloadRoutes = async function (params) { // save returnTo for later usage in /register/complete // passport seems to remove `req.session.returnTo` after it redirects req.session.registration.returnTo = req.session.returnTo; - next(); - }, function (req, res, next) { + passport.authenticate(strategy.name, function (err, user) { if (err) { delete req.session.registration; diff --git a/src/webserver.js b/src/webserver.js index 45af13b745..3b2e0b8cd2 100644 --- a/src/webserver.js +++ b/src/webserver.js @@ -145,7 +145,7 @@ function setupExpressApp(app) { configureBodyParser(app); - app.use(cookieParser()); + app.use(cookieParser(nconf.get('secret'))); const userAgentMiddleware = useragent.express(); app.use(function userAgent(req, res, next) { userAgentMiddleware(req, res, next);