From 842b8abb84c7ab1581a765cb650899c53dc1e99e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bar=C4=B1=C5=9F=20Soner=20U=C5=9Fakl=C4=B1?= Date: Thu, 4 Jun 2020 01:14:46 -0400 Subject: [PATCH] feat: add buildHeaderAsync (#8367) * feat: add buildHeaderAsync make helphers.notAllowed async * fix: remove csrf from buildHeader * fix: remove unused method, use middleware * fix: /post/pid redirect doesn't need buildHeader use buildHeaderAsync --- src/controllers/404.js | 9 ++--- src/controllers/errors.js | 17 ++++---- src/controllers/helpers.js | 75 ++++++++++++++++------------------- src/middleware/header.js | 6 +-- src/middleware/index.js | 10 ++++- src/middleware/maintenance.js | 3 +- src/routes/api.js | 8 +--- src/routes/helpers.js | 2 +- src/routes/index.js | 2 +- 9 files changed, 60 insertions(+), 72 deletions(-) diff --git a/src/controllers/404.js b/src/controllers/404.js index c1d6578d68..3e93c4ac32 100644 --- a/src/controllers/404.js +++ b/src/controllers/404.js @@ -6,6 +6,7 @@ const validator = require('validator'); const meta = require('../meta'); const plugins = require('../plugins'); +const middleware = require('../middleware'); exports.handle404 = function handle404(req, res) { const relativePath = nconf.get('relative_path'); @@ -36,14 +37,12 @@ exports.handle404 = function handle404(req, res) { } }; -exports.send404 = function (req, res) { +exports.send404 = async function (req, res) { res.status(404); const path = String(req.path || ''); if (res.locals.isAPI) { return res.json({ path: validator.escape(path.replace(/^\/api/, '')), title: '[[global:404.title]]' }); } - const middleware = require('../middleware'); - middleware.buildHeader(req, res, function () { - res.render('404', { path: validator.escape(path), title: '[[global:404.title]]' }); - }); + await middleware.buildHeaderAsync(req, res); + res.render('404', { path: validator.escape(path), title: '[[global:404.title]]' }); }; diff --git a/src/controllers/errors.js b/src/controllers/errors.js index 0000b36b9b..2052952067 100644 --- a/src/controllers/errors.js +++ b/src/controllers/errors.js @@ -4,8 +4,9 @@ var nconf = require('nconf'); var winston = require('winston'); var validator = require('validator'); var plugins = require('../plugins'); +var middleware = require('../middleware'); -exports.handleURIErrors = function handleURIErrors(err, req, res, next) { +exports.handleURIErrors = async function handleURIErrors(err, req, res, next) { // Handle cases where malformed URIs are passed in if (err instanceof URIError) { const cleanPath = req.path.replace(new RegExp('^' + nconf.get('relative_path')), ''); @@ -23,10 +24,8 @@ exports.handleURIErrors = function handleURIErrors(err, req, res, next) { error: '[[global:400.title]]', }); } else { - var middleware = require('../middleware'); - middleware.buildHeader(req, res, function () { - res.status(400).render('400', { error: validator.escape(String(err.message)) }); - }); + await middleware.buildHeaderAsync(req, res); + res.status(400).render('400', { error: validator.escape(String(err.message)) }); } } } else { @@ -46,7 +45,7 @@ exports.handleErrors = function handleErrors(err, req, res, next) { // eslint-di res.status(403).type('text/plain').send(err.message); }, }; - var defaultHandler = function () { + var defaultHandler = async function () { // Display NodeBB error page var status = parseInt(err.status, 10); if ((status === 302 || status === 308) && err.path) { @@ -61,10 +60,8 @@ exports.handleErrors = function handleErrors(err, req, res, next) { // eslint-di if (res.locals.isAPI) { res.json({ path: validator.escape(path), error: err.message }); } else { - var middleware = require('../middleware'); - middleware.buildHeader(req, res, function () { - res.render('500', { path: validator.escape(path), error: validator.escape(String(err.message)) }); - }); + await middleware.buildHeaderAsync(req, res); + res.render('500', { path: validator.escape(path), error: validator.escape(String(err.message)) }); } }; diff --git a/src/controllers/helpers.js b/src/controllers/helpers.js index 8ce5811cf0..1d3a5ff424 100644 --- a/src/controllers/helpers.js +++ b/src/controllers/helpers.js @@ -2,7 +2,6 @@ const nconf = require('nconf'); const validator = require('validator'); -const winston = require('winston'); const querystring = require('querystring'); const _ = require('lodash'); @@ -15,21 +14,19 @@ const middleware = require('../middleware'); const helpers = module.exports; -helpers.noScriptErrors = function (req, res, error, httpStatus) { +helpers.noScriptErrors = async function (req, res, error, httpStatus) { if (req.body.noscript !== 'true') { return res.status(httpStatus).send(error); } - const middleware = require('../middleware'); const httpStatusString = httpStatus.toString(); - middleware.buildHeader(req, res, function () { - res.status(httpStatus).render(httpStatusString, { - path: req.path, - loggedIn: req.loggedIn, - error: error, - returnLink: true, - title: '[[global:' + httpStatusString + '.title]]', - }); + await middleware.buildHeaderAsync(req, res); + res.status(httpStatus).render(httpStatusString, { + path: req.path, + loggedIn: req.loggedIn, + error: error, + returnLink: true, + title: '[[global:' + httpStatusString + '.title]]', }); }; @@ -104,41 +101,37 @@ helpers.buildTerms = function (url, term, query) { }]; }; -helpers.notAllowed = function (req, res, error) { - plugins.fireHook('filter:helpers.notAllowed', { +helpers.notAllowed = async function (req, res, error) { + const data = await plugins.fireHook('filter:helpers.notAllowed', { req: req, res: res, error: error, - }, function (err) { - if (err) { - return winston.error(err); - } - if (req.loggedIn || req.uid === -1) { - if (res.locals.isAPI) { - res.status(403).json({ - path: req.path.replace(/^\/api/, ''), - loggedIn: req.loggedIn, - error: error, - title: '[[global:403.title]]', - }); - } else { - middleware.buildHeader(req, res, function () { - res.status(403).render('403', { - path: req.path, - loggedIn: req.loggedIn, - error: error, - title: '[[global:403.title]]', - }); - }); - } - } else if (res.locals.isAPI) { - req.session.returnTo = req.url.replace(/^\/api/, ''); - res.status(401).json('not-authorized'); + }); + + if (req.loggedIn || req.uid === -1) { + if (res.locals.isAPI) { + res.status(403).json({ + path: req.path.replace(/^\/api/, ''), + loggedIn: req.loggedIn, + error: data.error, + title: '[[global:403.title]]', + }); } else { - req.session.returnTo = req.url; - res.redirect(nconf.get('relative_path') + '/login'); + await middleware.buildHeaderAsync(req, res); + res.status(403).render('403', { + path: req.path, + loggedIn: req.loggedIn, + error: data.error, + title: '[[global:403.title]]', + }); } - }); + } else if (res.locals.isAPI) { + req.session.returnTo = req.url.replace(/^\/api/, ''); + res.status(401).json('not-authorized'); + } else { + req.session.returnTo = req.url; + res.redirect(nconf.get('relative_path') + '/login'); + } }; helpers.redirect = function (res, url) { diff --git a/src/middleware/header.js b/src/middleware/header.js index 101f7523fd..ea208895ab 100644 --- a/src/middleware/header.js +++ b/src/middleware/header.js @@ -3,6 +3,7 @@ var nconf = require('nconf'); var jsesc = require('jsesc'); var _ = require('lodash'); +var util = require('util'); var db = require('../database'); var user = require('../user'); @@ -26,9 +27,6 @@ module.exports = function (middleware) { middleware.buildHeader = helpers.try(async function buildHeader(req, res, next) { res.locals.renderHeader = true; res.locals.isAPI = false; - if (req.uid >= 0) { - await middleware.applyCSRFAsync(req, res); - } const [config] = await Promise.all([ controllers.api.loadConfig(req), plugins.fireHook('filter:middleware.buildHeader', { req: req, locals: res.locals }), @@ -37,6 +35,8 @@ module.exports = function (middleware) { next(); }); + middleware.buildHeaderAsync = util.promisify(middleware.buildHeader); + async function generateHeader(req, res, data) { var registrationType = meta.config.registrationType || 'normal'; res.locals.config = res.locals.config || {}; diff --git a/src/middleware/index.js b/src/middleware/index.js index fe4897a342..b84cfd35af 100644 --- a/src/middleware/index.js +++ b/src/middleware/index.js @@ -33,7 +33,7 @@ middleware.regexes = { timestampedUpload: /^\d+-.+$/, }; -middleware.applyCSRF = csrf({ +const csurfMiddleware = csrf({ cookie: nconf.get('url_parsed').protocol === 'https:' ? { secure: true, sameSite: 'Strict', @@ -41,7 +41,13 @@ middleware.applyCSRF = csrf({ } : true, }); -middleware.applyCSRFAsync = util.promisify(middleware.applyCSRF); +middleware.applyCSRF = function (req, res, next) { + if (req.uid >= 0) { + csurfMiddleware(req, res, next); + } else { + next(); + } +}; middleware.ensureLoggedIn = ensureLoggedIn.ensureLoggedIn(nconf.get('relative_path') + '/login'); diff --git a/src/middleware/maintenance.js b/src/middleware/maintenance.js index 089aa7549a..2c7f4b9f0c 100644 --- a/src/middleware/maintenance.js +++ b/src/middleware/maintenance.js @@ -35,8 +35,7 @@ module.exports = function (middleware) { if (res.locals.isAPI) { return res.json(data); } - const buildHeaderAsync = util.promisify(middleware.buildHeader); - await buildHeaderAsync(req, res); + await middleware.buildHeaderAsync(req, res); res.render('503', data); }); }; diff --git a/src/routes/api.js b/src/routes/api.js index 7d71051e92..1e94de1d96 100644 --- a/src/routes/api.js +++ b/src/routes/api.js @@ -8,13 +8,7 @@ module.exports = function (app, middleware, controllers) { var router = express.Router(); app.use('/api', router); - router.get('/config', function (req, res, next) { - if (req.uid >= 0) { - middleware.applyCSRF(req, res, next); - } else { - setImmediate(next); - } - }, controllers.api.getConfig); + router.get('/config', middleware.applyCSRF, controllers.api.getConfig); router.get('/me', controllers.user.getCurrentUser); router.get('/user/uid/:uid', middleware.canViewUsers, controllers.user.getUserByUID); diff --git a/src/routes/helpers.js b/src/routes/helpers.js index 85c3dd1196..8c2085d42d 100644 --- a/src/routes/helpers.js +++ b/src/routes/helpers.js @@ -5,7 +5,7 @@ var helpers = module.exports; helpers.setupPageRoute = function (router, name, middleware, middlewares, controller) { middlewares = [middleware.maintenanceMode, middleware.registrationComplete, middleware.pageView, middleware.pluginHooks].concat(middlewares); - router.get(name, middleware.busyCheck, middleware.buildHeader, middlewares, helpers.tryRoute(controller)); + router.get(name, middleware.busyCheck, middleware.applyCSRF, middleware.buildHeader, middlewares, helpers.tryRoute(controller)); router.get('/api' + name, middlewares, helpers.tryRoute(controller)); }; diff --git a/src/routes/index.js b/src/routes/index.js index fcab003624..2274a7f418 100644 --- a/src/routes/index.js +++ b/src/routes/index.js @@ -56,7 +56,7 @@ function topicRoutes(app, middleware, controllers) { function postRoutes(app, middleware, controllers) { const middlewares = [middleware.maintenanceMode, middleware.registrationComplete, middleware.pluginHooks]; - app.get('/post/:pid', middleware.busyCheck, middleware.buildHeader, middlewares, controllers.posts.redirectToPost); + app.get('/post/:pid', middleware.busyCheck, middlewares, controllers.posts.redirectToPost); app.get('/api/post/:pid', middlewares, controllers.posts.redirectToPost); }