From bf492933cc4a0c2716ccdc8b81c8e77dad0a05c5 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 16 Apr 2020 17:12:53 +0200 Subject: [PATCH 1/5] fix(cache): explicitly disable caching for most api routes --- package.json | 1 + website/server/controllers/api-v4/faq.js | 8 ++------ website/server/libs/routes.js | 19 +++++++++++++------ website/server/middlewares/cache.js | 14 ++++++++++++++ 4 files changed, 30 insertions(+), 12 deletions(-) create mode 100644 website/server/middlewares/cache.js diff --git a/package.json b/package.json index 20a597921f..b5b32ef81b 100644 --- a/package.json +++ b/package.json @@ -52,6 +52,7 @@ "morgan": "^1.10.0", "nconf": "^0.10.0", "node-gcm": "^1.0.2", + "on-headers": "^1.0.2", "passport": "^0.4.1", "passport-facebook": "^3.0.0", "passport-google-oauth2": "^0.2.0", diff --git a/website/server/controllers/api-v4/faq.js b/website/server/controllers/api-v4/faq.js index 171e72edad..4dfce05f24 100644 --- a/website/server/controllers/api-v4/faq.js +++ b/website/server/controllers/api-v4/faq.js @@ -1,5 +1,4 @@ import _ from 'lodash'; -import { query } from 'express-validator/check'; import { langCodes } from '../../libs/i18n'; import apiError from '../../libs/apiError'; import common from '../../../common'; @@ -55,12 +54,9 @@ function _deleteOtherPlatformsAnswers (faqObject, platform) { api.faq = { method: 'GET', url: '/faq', - middlewares: [ - query('platform') - .optional() - .isIn(['web', 'android', 'ios']).withMessage(apiError('invalidPlatform')), - ], async handler (req, res) { + req.checkQuery('platform').optional().isIn(['web', 'android', 'ios'], apiError('guildsPaginateBooleanString')); + const validationErrors = req.validationErrors(); if (validationErrors) throw validationErrors; diff --git a/website/server/libs/routes.js b/website/server/libs/routes.js index a596200ad5..25541ce65e 100644 --- a/website/server/libs/routes.js +++ b/website/server/libs/routes.js @@ -3,6 +3,9 @@ import _ from 'lodash'; import { getUserLanguage, } from '../middlewares/language'; +import { + disableCache, +} from '../middlewares/cache'; // Wrapper function to handler `async` route handlers that return promises // It takes the async function, execute it and pass any error to next (args[2]) @@ -28,22 +31,26 @@ export function readController (router, controller, overrides = []) { return false; }); - const middlewaresToAdd = [getUserLanguage]; + method = method.toLowerCase(); - if (action.noLanguage !== true) { + // all get routes with mandatory or optional authentication + if (method === 'get' && authMiddlewareIndex !== -1) { + middlewares.unshift(disableCache); + } + + if (action.noLanguage !== true) { // unless getting the language is explictly disabled // the user will be authenticated, getUserLanguage after authentication if (authMiddlewareIndex !== -1) { if (authMiddlewareIndex === middlewares.length - 1) { - middlewares.push(...middlewaresToAdd); + middlewares.push(getUserLanguage); } else { - middlewares.splice(authMiddlewareIndex + 1, 0, ...middlewaresToAdd); + middlewares.splice(authMiddlewareIndex + 1, 0, getUserLanguage); } } else { // no auth, getUserLanguage as the first middleware - middlewares.unshift(...middlewaresToAdd); + middlewares.unshift(getUserLanguage); } } - method = method.toLowerCase(); const fn = handler ? _wrapAsyncFn(handler) : noop; router[method](url, ...middlewares, fn); diff --git a/website/server/middlewares/cache.js b/website/server/middlewares/cache.js new file mode 100644 index 0000000000..3ed02dc2d5 --- /dev/null +++ b/website/server/middlewares/cache.js @@ -0,0 +1,14 @@ +import onHeaders from 'on-headers'; + +export function disableCache (req, res, next) { + res.header('Cache-Control', 'no-store'); + + // Remove the etag header when caching is disabled + // Unfortunately it's not possible to prevent the creation right now + // See this issue https://github.com/expressjs/express/issues/2472 + onHeaders(res, function removeEtag () { + this.removeHeader('ETag'); + }); + + return next(); +} From f757e645b7000e60383b8bdea55e505a42863e54 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 16 Apr 2020 17:17:17 +0200 Subject: [PATCH 2/5] disable caching for the /status api route --- website/server/controllers/api-v3/status.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/website/server/controllers/api-v3/status.js b/website/server/controllers/api-v3/status.js index c957388d59..fe42c417eb 100644 --- a/website/server/controllers/api-v3/status.js +++ b/website/server/controllers/api-v3/status.js @@ -1,3 +1,7 @@ +import { + disableCache, +} from '../../middlewares/cache'; + const api = {}; /** @@ -15,6 +19,8 @@ const api = {}; api.getStatus = { method: 'GET', url: '/status', + // explicitly disable caching so that the server is always checked + middlewares: [disableCache], async handler (req, res) { res.respond(200, { status: 'up', From 24e1bfdfba182a4db4b68c3463b559828f0e4ef0 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Fri, 17 Apr 2020 14:50:09 +0200 Subject: [PATCH 3/5] add basic test, disable etag on post routes as well, paypal ipn: prevent set headers after response error --- test/api/unit/middlewares/cache.test.js | 31 +++++++++++++++++++ .../controllers/top-level/payments/paypal.js | 5 ++- website/server/libs/routes.js | 4 +-- website/server/middlewares/cache.js | 2 +- 4 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 test/api/unit/middlewares/cache.test.js diff --git a/test/api/unit/middlewares/cache.test.js b/test/api/unit/middlewares/cache.test.js new file mode 100644 index 0000000000..6501054273 --- /dev/null +++ b/test/api/unit/middlewares/cache.test.js @@ -0,0 +1,31 @@ +import { + generateRes, + generateReq, + generateNext, +} from '../../../helpers/api-unit.helper'; +import { + disableCache, +} from '../../../../website/server/middlewares/cache'; + +describe('cache middlewares', () => { + let res; let req; let + next; + + beforeEach(() => { + req = generateReq(); + res = generateRes(); + next = generateNext(); + }); + + describe('disableCache', () => { + it('sets the correct headers', () => { + disableCache(req, res, next); + expect(res.set).to.have.been.calledWith('Cache-Control', 'no-store'); + expect(next).to.have.been.calledOnce; + }); + + xit('removes the etag header', () => { + // @TODO how to stub onHeaders + }); + }); +}); diff --git a/website/server/controllers/top-level/payments/paypal.js b/website/server/controllers/top-level/payments/paypal.js index 5885076a0f..84ff60d7ab 100644 --- a/website/server/controllers/top-level/payments/paypal.js +++ b/website/server/controllers/top-level/payments/paypal.js @@ -1,5 +1,6 @@ /* eslint-disable camelcase */ import paypalPayments from '../../../libs/payments/paypal'; +import logger from '../../../libs/logger'; import shared from '../../../../common'; import { authWithSession, @@ -171,7 +172,9 @@ api.ipn = { async handler (req, res) { res.sendStatus(200); - await paypalPayments.ipn(req.body); + paypalPayments + .ipn(req.body) + .catch(err => logger.error(err)); }, }; diff --git a/website/server/libs/routes.js b/website/server/libs/routes.js index 25541ce65e..f46ba997ed 100644 --- a/website/server/libs/routes.js +++ b/website/server/libs/routes.js @@ -33,8 +33,8 @@ export function readController (router, controller, overrides = []) { method = method.toLowerCase(); - // all get routes with mandatory or optional authentication - if (method === 'get' && authMiddlewareIndex !== -1) { + // disable caching for all routes with mandatory or optional authentication + if (authMiddlewareIndex !== -1) { middlewares.unshift(disableCache); } diff --git a/website/server/middlewares/cache.js b/website/server/middlewares/cache.js index 3ed02dc2d5..4a80985950 100644 --- a/website/server/middlewares/cache.js +++ b/website/server/middlewares/cache.js @@ -4,7 +4,7 @@ export function disableCache (req, res, next) { res.header('Cache-Control', 'no-store'); // Remove the etag header when caching is disabled - // Unfortunately it's not possible to prevent the creation right now + // @TODO Unfortunately it's not possible to prevent the creation right now // See this issue https://github.com/expressjs/express/issues/2472 onHeaders(res, function removeEtag () { this.removeHeader('ETag'); From cda5c6fbb0a4fc3c4d8d0053d311948836bc8156 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Fri, 17 Apr 2020 14:57:53 +0200 Subject: [PATCH 4/5] res.header -> res.set --- website/server/middlewares/cache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/server/middlewares/cache.js b/website/server/middlewares/cache.js index 4a80985950..228e7d14eb 100644 --- a/website/server/middlewares/cache.js +++ b/website/server/middlewares/cache.js @@ -1,7 +1,7 @@ import onHeaders from 'on-headers'; export function disableCache (req, res, next) { - res.header('Cache-Control', 'no-store'); + res.set('Cache-Control', 'no-store'); // Remove the etag header when caching is disabled // @TODO Unfortunately it's not possible to prevent the creation right now From 8da36bf27c62ba0397a6af260c20d35a17f3d911 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Fri, 17 Apr 2020 15:09:12 +0200 Subject: [PATCH 5/5] 4.140.5 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0f02698412..2237fd2178 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "habitica", - "version": "4.140.4", + "version": "4.140.5", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index b5b32ef81b..f4141aa111 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "habitica", "description": "A habit tracker app which treats your goals like a Role Playing Game.", - "version": "4.140.4", + "version": "4.140.5", "main": "./website/server/index.js", "dependencies": { "@babel/core": "^7.9.0",