From 5ef1ce04c5b30dc7451567c400c5df786f242982 Mon Sep 17 00:00:00 2001 From: Xiao Gui <xgui3783@gmail.com> Date: Thu, 15 Jul 2021 12:33:06 +0200 Subject: [PATCH] bugfix: proper fallback when redis fails chore: reworked csp --- deploy/app.js | 134 +++++++++++++++++---------------- deploy/app.spec.js | 4 + deploy/csp/index.js | 27 ++----- deploy/csp/index.spec.js | 4 +- deploy/datasets/query.js | 7 +- deploy/lruStore/index.js | 125 ++++++++++++++++++------------ deploy/package.json | 2 +- deploy/plugins/index.js | 6 +- deploy/saneUrl/index.js | 32 +------- deploy/saneUrl/index.spec.js | 142 ++++++++++++++++------------------- 10 files changed, 238 insertions(+), 245 deletions(-) diff --git a/deploy/app.js b/deploy/app.js index 0d237b64d..ce991b8cf 100644 --- a/deploy/app.js +++ b/deploy/app.js @@ -41,74 +41,72 @@ app.use((req, _, next) => { }) /** - * load env first, then load other modules + * configure Auth + * async function, but can start server without */ -const { configureAuth, ready: authReady } = require('./auth') +const _ = (async () => { +/** + * load env first, then load other modules + */ -const store = (() => { + const { configureAuth, ready: authReady } = require('./auth') + + const store = await (async () => { + + const { USE_DEFAULT_MEMORY_STORE } = process.env + if (!!USE_DEFAULT_MEMORY_STORE) { + console.warn(`USE_DEFAULT_MEMORY_STORE is set to true, memleak expected. Do NOT use in prod.`) + return null + } + + const { _initPr, redisURL, StoreType } = require('./lruStore') + await _initPr + console.log('StoreType', redisURL, StoreType) + if (!!redisURL) { + const redis = require('redis') + const RedisStore = require('connect-redis')(session) + const client = redis.createClient({ + url: redisURL + }) + return new RedisStore({ + client + }) + } + + /** + * memorystore (or perhaps lru-cache itself) does not properly close when server shuts + * this causes problems during tests + * So when testing app.js, set USE_DEFAULT_MEMORY_STORE to true + * see app.spec.js + */ + const MemoryStore = require('memorystore')(session) + return new MemoryStore({ + checkPeriod: 86400000 + }) + + })() - const { USE_DEFAULT_MEMORY_STORE } = process.env - if (!!USE_DEFAULT_MEMORY_STORE) { - console.warn(`USE_DEFAULT_MEMORY_STORE is set to true, memleak expected. Do NOT use in prod.`) - return null - } + const SESSIONSECRET = process.env.SESSIONSECRET || 'this is not really a random session secret' - const { redisURL } = require('./lruStore') - if (!!redisURL) { - const redis = require('redis') - const RedisStore = require('connect-redis')(session) - const client = redis.createClient({ - url: redisURL - }) - return new RedisStore({ - client - }) - } - /** - * memorystore (or perhaps lru-cache itself) does not properly close when server shuts - * this causes problems during tests - * So when testing app.js, set USE_DEFAULT_MEMORY_STORE to true - * see app.spec.js + * passport application of oidc requires session */ - const MemoryStore = require('memorystore')(session) - return new MemoryStore({ - checkPeriod: 86400000 - }) - -})() + app.use(session({ + secret: SESSIONSECRET, + resave: true, + saveUninitialized: false, + store + })) -const SESSIONSECRET = process.env.SESSIONSECRET || 'this is not really a random session secret' - -/** - * passport application of oidc requires session - */ -app.use(session({ - secret: SESSIONSECRET, - resave: true, - saveUninitialized: false, - store -})) - -/** - * configure CSP - */ -if (process.env.DISABLE_CSP && process.env.DISABLE_CSP === 'true') { - console.warn(`DISABLE_CSP is set to true, csp will not be enabled`) -} else { - require('./csp')(app) -} - -/** - * configure Auth - * async function, but can start server without - */ - -(async () => { await configureAuth(app) app.use('/user', require('./user')) + + /** + * saneUrl end points + */ + app.use('/saneUrl', require('./saneUrl')) })() const PUBLIC_PATH = process.env.NODE_ENV === 'production' @@ -164,7 +162,21 @@ app.use(require('./devBanner')) * populate nonce token */ const { indexTemplate } = require('./constants') -app.get('/', bkwdMdl, cookieParser(), (req, res) => { +app.get('/', (req, res, next) => { + + /** + * configure CSP + */ + if (process.env.DISABLE_CSP && process.env.DISABLE_CSP === 'true') { + console.warn(`DISABLE_CSP is set to true, csp will not be enabled`) + next() + } else { + const { bootstrapReportViolation, middelware } = require('./csp') + bootstrapReportViolation(app) + middelware(req, res, next) + } + +}, bkwdMdl, cookieParser(), (req, res) => { const iavError = req.cookies && req.cookies['iav-error'] res.setHeader('Content-Type', 'text/html') @@ -212,12 +224,6 @@ if (LOCAL_CDN_FLAG) setAlwaysOff(true) app.use(compressionMiddleware, express.static(PUBLIC_PATH)) -/** - * saneUrl end points - */ -const saneUrlRouter = require('./saneUrl') -app.use('/saneUrl', saneUrlRouter) - const jsonMiddleware = (req, res, next) => { if (!res.get('Content-Type')) res.set('Content-Type', 'application/json') next() diff --git a/deploy/app.spec.js b/deploy/app.spec.js index 9683de23b..dee5f9aea 100644 --- a/deploy/app.spec.js +++ b/deploy/app.spec.js @@ -58,6 +58,10 @@ describe('authentication', () => { }) after(() => { + delete require.cache[require.resolve('./datasets')] + delete require.cache[require.resolve('./saneUrl')] + delete require.cache[require.resolve('./user')] + delete require.cache[require.resolve('./constants')] server.close() }) it('> auth middleware is called', async () => { diff --git a/deploy/csp/index.js b/deploy/csp/index.js index 303ae384c..87a6fe19e 100644 --- a/deploy/csp/index.js +++ b/deploy/csp/index.js @@ -1,6 +1,5 @@ const csp = require('helmet-csp') const bodyParser = require('body-parser') -const crypto = require('crypto') let WHITE_LIST_SRC, CSP_CONNECT_SRC, SCRIPT_SRC @@ -49,13 +48,9 @@ const connectSrc = [ ...CSP_CONNECT_SRC ] -module.exports = (app) => { - app.use((req, res, next) => { - if (req.path === '/') res.locals.nonce = crypto.randomBytes(16).toString('hex') - next() - }) +module.exports = { + middelware: (req, res, next) => { - app.use((req, res, next) => { const permittedCsp = (req.session && req.session.permittedCsp) || {} const userConnectSrc = [] const userScriptSrc = [] @@ -69,18 +64,6 @@ module.exports = (app) => { ...(permittedCsp[key]['scriptSrc'] || []) ) } - res.locals.userCsp = { - userConnectSrc, - userScriptSrc, - } - next() - }) - - app.use((req, res, next) => { - const { - userConnectSrc = [], - userScriptSrc = [], - } = res.locals.userCsp || {} csp({ directives: { defaultSrc: [ @@ -130,9 +113,9 @@ module.exports = (app) => { }, reportOnly })(req, res, next) - }) - if (!CSP_REPORT_URI) { + }, + bootstrapReportViolation: app => { app.post('/report-violation', bodyParser.json({ type: ['json', 'application/csp-report'] }), (req, res) => { @@ -144,4 +127,4 @@ module.exports = (app) => { res.status(204).end() }) } -} \ No newline at end of file +} diff --git a/deploy/csp/index.spec.js b/deploy/csp/index.spec.js index 70b4e1442..93a478e25 100644 --- a/deploy/csp/index.spec.js +++ b/deploy/csp/index.spec.js @@ -1,6 +1,6 @@ const express = require('express') const app = express() -const csp = require('./index') +const { middelware: cspMiddleware } = require('./index') const got = require('got') const { expect, assert } = require('chai') @@ -45,7 +45,7 @@ describe('> csp/index.js', () => { } before(done => { app.use(middleware) - csp(app) + app.use(cspMiddleware) app.get('/', (req, res) => { res.status(200).send('OK') }) diff --git a/deploy/datasets/query.js b/deploy/datasets/query.js index 500bc6867..4911e4f80 100644 --- a/deploy/datasets/query.js +++ b/deploy/datasets/query.js @@ -7,7 +7,7 @@ const { getPreviewFile, hasPreview } = require('./supplements/previewFile') const { constants, init: kgQueryUtilInit, getUserKGRequestParam, filterDatasets, filterDatasetsByRegion } = require('./util') const ibc = require('./importIBS') const { returnAdditionalDatasets } = require('../regionalFeatures') -const { store } = require('../lruStore') +const lruStore = require('../lruStore') const IAV_DS_CACHE_KEY = 'IAV_DS_CACHE_KEY' const IAV_DS_TIMESTAMP_KEY = 'IAV_DS_TIMESTAMP_KEY' @@ -55,6 +55,8 @@ const fetchDatasetFromKg = async ({ user } = {}) => { } const refreshCache = async () => { + await lruStore._initPr + const { store } = lruStore store.set(IAV_DS_REFRESH_TIMESTAMP_KEY, new Date().toString()) const text = await fetchDatasetFromKg() await store.set(IAV_DS_CACHE_KEY, text) @@ -64,6 +66,9 @@ const refreshCache = async () => { const getPublicDs = async () => { console.log(`fetching public ds ...`) + + await lruStore._initPr + const { store } = lruStore let cachedData = await store.get(IAV_DS_CACHE_KEY) if (!cachedData) { diff --git a/deploy/lruStore/index.js b/deploy/lruStore/index.js index 6a3c8a908..474f23d5e 100644 --- a/deploy/lruStore/index.js +++ b/deploy/lruStore/index.js @@ -33,7 +33,7 @@ const userPass = (() => { : `${returnString}@` })() -const redisURL = redisAddr && `${redisProto || ''}://${userPass}${redisAddr}:${redisPort}` +const _redisURL = redisAddr && `${redisProto || ''}://${userPass}${redisAddr}:${redisPort}` const crypto = require('crypto') @@ -56,76 +56,109 @@ const ensureString = val => { if (typeof val !== 'string') throw new Error(`both key and val must be string`) } -if (redisURL) { - const redis = require('redis') - const { promisify } = require('util') - const client = redis.createClient({ - url: redisURL - }) - - const asyncGet = promisify(client.get).bind(client) - const asyncSet = promisify(client.set).bind(client) - const asyncDel = promisify(client.del).bind(client) - - const keys = [] - - /** - * maxage in milli seconds - */ - exports.store = { - set: async (key, val, { maxAge } = {}) => { - ensureString(key) - ensureString(val) - asyncSet(key, val, ...( maxAge ? [ 'PX', maxAge ] : [] )) - keys.push(key) - }, - get: async (key) => { - ensureString(key) - return asyncGet(key) - }, - clear: async auth => { - if (auth !== authKey) { - getAuthKey() - throw new Error(`unauthorized`) - } - await asyncDel(keys.splice(0)) - } +class ExportObj { + constructor(){ + this.StoreType = null + this.redisURL = null + this.store = null + this._rs = null + this._rj = null + + this._initPr = new Promise((rs, rj) => { + this._rs = rs + this._rj = rj + }) } +} - exports.StoreType = `redis` - exports.redisURL = redisURL - console.log(`redis`) +const exportObj = new ExportObj() + +const setupLru = () => { -} else { const LRU = require('lru-cache') - const store = new LRU({ + const lruStore = new LRU({ max: 1024 * 1024 * 1024, // 1gb length: (n, key) => n.length, maxAge: Infinity, // never expires }) - exports.store = { + exportObj.store = { /** * maxage in milli seconds */ set: async (key, val, { maxAge } = {}) => { ensureString(key) ensureString(val) - store.set(key, val, ...( maxAge ? [ maxAge ] : [] )) + lruStore.set(key, val, ...( maxAge ? [ maxAge ] : [] )) }, get: async (key) => { ensureString(key) - return store.get(key) + return lruStore.get(key) }, clear: async auth => { if (auth !== authKey) { getAuthKey() throw new Error(`unauthorized`) } - store.reset() + lruStore.reset() } } - exports.StoreType = `lru-cache` - console.log(`lru-cache`) + exportObj.StoreType = `lru-cache` + console.log(`using lru-cache`) + exportObj._rs() } + +if (_redisURL) { + const redis = require('redis') + const { promisify } = require('util') + const client = redis.createClient({ + url: _redisURL + }) + + client.on('ready', () => { + + const asyncGet = promisify(client.get).bind(client) + const asyncSet = promisify(client.set).bind(client) + const asyncDel = promisify(client.del).bind(client) + + const keys = [] + + /** + * maxage in milli seconds + */ + exportObj.store = { + set: async (key, val, { maxAge } = {}) => { + ensureString(key) + ensureString(val) + asyncSet(key, val, ...( maxAge ? [ 'PX', maxAge ] : [] )) + keys.push(key) + }, + get: async (key) => { + ensureString(key) + return asyncGet(key) + }, + clear: async auth => { + if (auth !== authKey) { + getAuthKey() + throw new Error(`unauthorized`) + } + await asyncDel(keys.splice(0)) + } + } + + exportObj.StoreType = `redis` + exportObj.redisURL = _redisURL + console.log(`using redis`) + exportObj._rs() + }).on('error', () => { + console.warn(`setting up Redis error, fallback to setupLru`) + setupLru() + client.quit() + }) + +} else { + setupLru() +} + +module.exports = exportObj \ No newline at end of file diff --git a/deploy/package.json b/deploy/package.json index 5aae0cdb9..3b422f04a 100644 --- a/deploy/package.json +++ b/deploy/package.json @@ -5,7 +5,7 @@ "main": "index.js", "scripts": { "start": "node server.js", - "test": "DISABLE_LIMITER=1 node -r dotenv/config ./node_modules/.bin/mocha './**/*.spec.js' --exclude 'node_modules/*' --timeout 60000", + "test": "DISABLE_LIMITER=1 node ./node_modules/.bin/mocha './**/*.spec.js' --exclude 'node_modules/*' --timeout 60000", "mocha": "mocha" }, "keywords": [], diff --git a/deploy/plugins/index.js b/deploy/plugins/index.js index cd829c98d..929f19499 100644 --- a/deploy/plugins/index.js +++ b/deploy/plugins/index.js @@ -4,7 +4,7 @@ */ const express = require('express') -const { store } = require('../lruStore') +const lruStore = require('../lruStore') const got = require('got') const router = express.Router() const PLUGIN_URLS = (process.env.PLUGIN_URLS && process.env.PLUGIN_URLS.split(';')) || [] @@ -26,6 +26,10 @@ router.get('/manifests', async (_req, res) => { ...STAGING_PLUGIN_URLS ].map(async url => { const key = getKey(url) + + await lruStore._initPr + const { store } = lruStore + try { const storedManifest = await store.get(key) if (storedManifest) return JSON.parse(storedManifest) diff --git a/deploy/saneUrl/index.js b/deploy/saneUrl/index.js index b3adb8907..2be8a15a3 100644 --- a/deploy/saneUrl/index.js +++ b/deploy/saneUrl/index.js @@ -2,40 +2,12 @@ const router = require('express').Router() const RateLimit = require('express-rate-limit') const RedisStore = require('rate-limit-redis') const { Store, NotFoundError } = require('./store') +const { redisURL } = require('../lruStore') const bodyParser = require('body-parser') const { readUserData, saveUserData } = require('../user/store') const store = new Store() - -const { - REDIS_PROTO, - REDIS_ADDR, - REDIS_PORT, - - REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_PROTO, - REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_ADDR, - REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_PORT, - - REDIS_USERNAME, - REDIS_PASSWORD, - - HOSTNAME, - HOST_PATHNAME, - DISABLE_LIMITER, -} = process.env - -const redisProto = REDIS_PROTO || REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_PROTO || 'redis' -const redisAddr = REDIS_ADDR || REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_ADDR || null -const redisPort = REDIS_PORT || REDIS_RATE_LIMITING_DB_EPHEMERAL_PORT_6379_TCP_PORT || 6379 - -/** - * nb this way to set username and pswd can be risky, but given that site adnimistrator sets the username and pswd via env var - * it should not be a security concern - */ -const userPass = (REDIS_USERNAME || REDIS_PASSWORD) && `${REDIS_USERNAME || ''}:${REDIS_PASSWORD || ''}@` - -const redisURL = redisAddr && `${redisProto}://${userPass || ''}${redisAddr}:${redisPort}` - +const { DISABLE_LIMITER, HOSTNAME, HOST_PATHNAME } = process.env const limiter = new RateLimit({ windowMs: 1e3 * 5, max: 5, diff --git a/deploy/saneUrl/index.spec.js b/deploy/saneUrl/index.spec.js index cb0038819..a0be5ad9f 100644 --- a/deploy/saneUrl/index.spec.js +++ b/deploy/saneUrl/index.spec.js @@ -1,9 +1,19 @@ const sinon = require('sinon') const cookie = require('cookie') -const { Store, NotFoundError } = require('./store') const userStore = require('../user/store') +class StubStore { + async set(key, val) { + + } + async get(key) { + + } +} + +class StubNotFoundError extends Error{} + const savedUserDataPayload = { otherData: 'not relevant data', savedCustomLinks: [ @@ -21,69 +31,75 @@ const saveUserDataStub = sinon .returns(Promise.resolve()) const express = require('express') -const router = require('./index') const got = require('got') const { expect } = require('chai') -const app = express() -let user -app.use('', (req, res, next) => { - req.user = user - next() -}, router) +describe('> saneUrl/index.js', () => { -const name = `nameme` + let getTokenStub, app, user, server, setStub, getStub + const payload = { + ver: '0.0.1', + queryString: 'test_test' + } -const payload = { - ver: '0.0.1', - queryString: 'test_test' -} + const name = `nameme` -describe('> saneUrl/index.js', () => { + after(() => { - let getTokenStub - before(() => { - getTokenStub = sinon - .stub(Store.prototype, 'getToken') - .returns(Promise.resolve(`--fake-token--`)) - }) + server.close() + setStub && setStub.restore() - after(() => { - getTokenStub.restore() + delete require.cache[require.resolve('../lruStore')] + delete require.cache[require.resolve('./store')] }) - describe('> router', () => { + before(() => { + getStub = sinon.stub(StubStore.prototype, 'get') + setStub = sinon.stub(StubStore.prototype, 'set') + require.cache[require.resolve('../lruStore')] = { + exports: { + redisURL: null + } + } - let server, setStub - before(() => { + require.cache[require.resolve('./store')] = { + exports: { + Store: StubStore, + NotFoundError: StubNotFoundError + } + } - setStub = sinon - .stub(Store.prototype, 'set') - .returns(Promise.resolve()) - server = app.listen(50000) - }) + app = express() - afterEach(() => { - setStub.resetHistory() - }) + const router = require('./index') + app.use('', (req, res, next) => { + req.user = user + next() + }, router) - after(() => { - server.close() - setStub.restore() - }) + server = app.listen(50000) + }) + + after(() => { + getTokenStub && getTokenStub.restore() + }) + + afterEach(() => { + setStub.resetHistory() + getStub.resetHistory() + }) + + describe('> router', () => { it('> works', async () => { const body = { ...payload } - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.resolve(JSON.stringify(body))) + getStub.returns(Promise.resolve(JSON.stringify(body))) const { body: respBody } = await got(`http://localhost:50000/${name}`) expect(getStub.calledWith(name)).to.be.true expect(respBody).to.equal(JSON.stringify(body)) - getStub.restore() }) it('> get on expired returns 404', async () => { @@ -91,16 +107,13 @@ describe('> saneUrl/index.js', () => { ...payload, expiry: Date.now() - 1e3 * 60 } - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.resolve(JSON.stringify(body))) + getStub.returns(Promise.resolve(JSON.stringify(body))) const { statusCode } = await got(`http://localhost:50000/${name}`, { throwHttpErrors: false }) expect(statusCode).to.equal(404) expect(getStub.calledWith(name)).to.be.true - getStub.restore() }) it('> get on expired with txt html header sets cookie and redirect', async () => { @@ -109,9 +122,7 @@ describe('> saneUrl/index.js', () => { ...payload, expiry: Date.now() - 1e3 * 60 } - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.resolve(JSON.stringify(body))) + getStub.returns(Promise.resolve(JSON.stringify(body))) const { statusCode, headers } = await got(`http://localhost:50000/${name}`, { headers: { @@ -123,7 +134,6 @@ describe('> saneUrl/index.js', () => { expect(statusCode).to.be.lessThan(303) expect(getStub.calledWith(name)).to.be.true - getStub.restore() const c = cookie.parse(...headers['set-cookie']) expect(!!c['iav-error']).to.be.true @@ -133,9 +143,7 @@ describe('> saneUrl/index.js', () => { it('> checks if the name is available', async () => { - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.reject(new NotFoundError())) + getStub.returns(Promise.reject(new StubNotFoundError())) await got(`http://localhost:50000/${name}`, { method: 'POST', @@ -150,16 +158,12 @@ describe('> saneUrl/index.js', () => { expect(storedName).to.equal(name) expect(getStub.called).to.be.true expect(setStub.called).to.be.true - - getStub.restore() }) it('> if file exist, will return 409 conflict', async () => { - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.resolve('{}')) + getStub.returns(Promise.resolve('{}')) const { statusCode } = await got(`http://localhost:50000/${name}`, { method: 'POST', @@ -174,14 +178,11 @@ describe('> saneUrl/index.js', () => { expect(getStub.called).to.be.true expect(setStub.called).to.be.false - getStub.restore() }) it('> if other error, will return 500', async () => { - const getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.reject(new Error(`other errors`))) + getStub.returns(Promise.reject(new Error(`other errors`))) const { statusCode } = await got(`http://localhost:50000/${name}`, { method: 'POST', @@ -195,21 +196,12 @@ describe('> saneUrl/index.js', () => { expect(statusCode).to.equal(500) expect(getStub.called).to.be.true expect(setStub.called).to.be.false - - getStub.restore() }) describe('> set with unauthenticated user', () => { - let getStub before(() => { - getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.reject(new NotFoundError())) - }) - - after(() => { - getStub.restore() + getStub.returns(Promise.reject(new StubNotFoundError())) }) it('> set with anonymous user has user undefined and expiry as defined', async () => { @@ -236,13 +228,7 @@ describe('> saneUrl/index.js', () => { describe('> set with authenticated user', () => { before(() => { - getStub = sinon - .stub(Store.prototype, 'get') - .returns(Promise.reject(new NotFoundError())) - }) - - after(() => { - getStub.restore() + getStub.returns(Promise.reject(new StubNotFoundError())) }) before(() => { -- GitLab