From 388e0658c7791f26c9a7650977cd91d731c01ad4 Mon Sep 17 00:00:00 2001 From: "P. Douglas Reeder" Date: Tue, 23 Jul 2024 18:52:32 -0400 Subject: [PATCH] Improves error handling and rate limiting triggered by errors --- lib/appFactory.js | 33 +++++++++++++------- lib/middleware/sanityCheckUsername.js | 4 +-- lib/routes/S3_store_router.js | 44 +++++++++++++++++++-------- lib/routes/admin.js | 4 +-- lib/routes/oauth.js | 2 +- lib/routes/storage_common.js | 13 +++++--- lib/util/errorPage.js | 5 ++- lib/views/error.html | 2 +- spec/modular/m_not_found.spec.js | 12 +++++--- spec/modular/m_storage_common.spec.js | 8 +++++ spec/not_found.spec.js | 4 +-- spec/store_handler.spec.js | 30 ++++++++++++++++++ 12 files changed, 118 insertions(+), 43 deletions(-) diff --git a/lib/appFactory.js b/lib/appFactory.js index 3aaff0f0..4ca2ef6f 100644 --- a/lib/appFactory.js +++ b/lib/appFactory.js @@ -16,6 +16,7 @@ const adminFactory = require('./routes/admin'); const session = require('express-session'); const MemoryStore = require('memorystore')(session); const { rateLimiterPenalty, rateLimiterBlock, rateLimiterMiddleware } = require('./middleware/rateLimiterMiddleware'); +const errToMessages = require('./util/errToMessages'); module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRouter, basePath = '' }) { if (basePath && !basePath.startsWith('/')) { basePath = '/' + basePath; } @@ -106,9 +107,9 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou fallthrough: true, index: false, maxAge: '25m' })); app.use(`${basePath}/assets`, async (req, res, _next) => { + await rateLimiterPenalty(req.ip); res.set('Cache-Control', 'public, max-age=1500'); res.status(404).end(); - await rateLimiterPenalty(req.ip); }); app.use(`${basePath}/signup`, requestInviteRouter(storeRouter)); @@ -160,34 +161,44 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou } const subpath = req.path.slice(basePath.length).split('/')?.[1]; const name = req.path.slice(1); - if (['.well-known', 'storage', 'oauth', 'account', 'admin', 'crossdomain.xml', 'sitemap.xml'].includes(subpath)) { + if (['.well-known', 'account', 'admin', 'crossdomain.xml', 'sitemap.xml'].includes(subpath) && ['GET', 'HEAD'].includes(req.method)) { errorPage(req, res, 404, { title: 'Not Found', message: `“${name}” doesn't exist` }); } else { // probably hostile - res.logNotes.add(`“${name}” shouldn't and doesn't exist`); - res.status(404).end(); if (Object.keys(req.session?.privileges || {}).length > 0) { + res.logNotes.add('suspicious request; applying rate penalty'); await rateLimiterPenalty(req.ip, 2); } else { + res.logNotes.add('suspicious request; blocking'); await rateLimiterBlock(req.ip, 61); } + res.status(404).end(); } }); // redirect for paths outside the app app.use(async function (req, res) { - res.status(308).set('Location', basePath).end(); await rateLimiterPenalty(req.ip); + res.status(308).set('Location', basePath).end(); }); // error handler app.use(async function (err, req, res, _next) { - const message = err?.message || err?.errors?.find(e => e.message).message || err?.cause?.message || 'indescribable error'; - errorPage(req, res, err.status || 500, { - title: shorten(message, 30), - message, - error: req.app.get('env') === 'development' ? err : {} - }); + const messages = new Set(); + errToMessages(err, messages); + const messageStr = process.env.NODE_ENV !== 'production' ? Array.from(messages).join(' ') : 'An error occurred'; + + const msgId = Math.floor(Math.random() * 1_000_000_000); + res.logNotes.add(`msgId=${msgId}`); + for (const msg of messages.values()) { + res.logNotes.add(msg); + } + await rateLimiterPenalty(req.ip); + errorPage(req, res, err.statusCode || 500, { + title: shorten(messageStr, 30), + message: messageStr + ` msgId=${msgId}`, + error: null + }); }); setTimeout(() => { diff --git a/lib/middleware/sanityCheckUsername.js b/lib/middleware/sanityCheckUsername.js index 217df031..9de9ce2a 100644 --- a/lib/middleware/sanityCheckUsername.js +++ b/lib/middleware/sanityCheckUsername.js @@ -6,7 +6,7 @@ module.exports = async function sanityCheckUsername (req, res, next) { return next(); } - res.logNotes.add('invalid user'); - res.status(400).end(); + res.logNotes.add('invalid username; blocking'); await rateLimiterBlock(req.ip, 61); + res.status(400).end(); }; diff --git a/lib/routes/S3_store_router.js b/lib/routes/S3_store_router.js index d5640a05..ff6a9880 100644 --- a/lib/routes/S3_store_router.js +++ b/lib/routes/S3_store_router.js @@ -28,6 +28,7 @@ const { calcContactURL } = require('../../lib/util/protocols'); const NoSuchUserError = require('../util/NoSuchUserError'); const NoSuchBlobError = require('../util/NoSuchBlobError'); const shorten = require('../util/shorten'); +const { rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); const ID_NUM_BITS = 64; const MAX_KEY_LENGTH = 910; // MinIO in /var/minio; OpenIO: 1023; AWS & play.min.io: 1024 @@ -127,8 +128,8 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP return pipeline(Body, res); } } catch (err) { - if (['NotFound', 'NoSuchKey', 'Forbidden', 'AccessDenied', '403'].includes(err.name) || - err.$metadata?.httpStatusCode === 403) { + if (['NoSuchBucket', 'NotFound', 'NoSuchKey', 'Forbidden', 'AccessDenied', '403'].includes(err.name) || + [404, 403].includes(err.$metadata?.httpStatusCode)) { if (isFolderRequest) { const folderJson = JSON.stringify(EMPTY_FOLDER); const digest = createHash('md5').update(folderJson).digest('hex'); @@ -146,8 +147,12 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP res.logNotes.add(err.message); res.logLevel = err.logLevel; return res.status(err.statusCode).type('text/plain').send(err.message); + } else if (err.code === 'ECONNREFUSED') { + errToMessages(err, res.logNotes); + res.set({ 'Retry-After': '60' }); + return res.status(503).end(); } else { - return next(Object.assign(err, { status: 502 })); + return next(Object.assign(err, { statusCode: err.$metadata?.httpStatusCode || 502 })); } } } @@ -157,9 +162,10 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP async function (req, res, next) { try { if (req.url.length === 0 || /\/\/|(^|\/)\.($|\/)|(^|\/)\.\.($|\/)|\0/.test(req.url)) { - next(Object.assign(new ParameterError('A parameter value is bad'), { status: 400 })); return; + next(Object.assign(new ParameterError('A parameter value is bad'), { statusCode: 400 })); return; } if (req.url.endsWith('/')) { + await rateLimiterPenalty(req.ip, 2); res.status(409).end(); return; } const bucketName = calcBucketName(req.params.username); @@ -175,10 +181,14 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP currentETag = normalizeETag(headResponse.ETag); } catch (err) { if (err.$metadata?.httpStatusCode === 400 || err.name === '400') { - return next(Object.assign(new ParameterError('A parameter value is bad', { cause: err }), { status: 400 })); + return next(Object.assign(new ParameterError('A parameter value is bad', { cause: err }), { statusCode: 400 })); + } else if (err.code === 'ECONNREFUSED') { + errToMessages(err, res.logNotes); + res.set({ 'Retry-After': '60' }); + return res.status(503).end(); } else if (!(['NotFound', 'NoSuchKey', 'Forbidden', 'AccessDenied', '403'].includes(err.name) || err.$metadata?.httpStatusCode === 403)) { - return next(Object.assign(err, { status: 502 })); + return next(Object.assign(err, { statusCode: err.$metadata?.httpStatusCode || 502 })); } } @@ -209,7 +219,8 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP } catch (err) { if (err.Code === 'SlowDown' || err.$metadata?.httpStatusCode === 503) { setPauseUntil(res.get('Retry-After')); - return next(Object.assign(err, { status: err?.$metadata?.httpStatusCode || 502 })); + res.set({ 'Retry-After': '10' }); + return next(Object.assign(err, { statusCode: err?.$metadata?.httpStatusCode || 503 })); } else if (err.name === 'EndResponseError') { res.logNotes.add(err.message); res.logLevel = err.logLevel; @@ -217,9 +228,12 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP } else if (err.name === 'TimeoutError' || err.Code === 'RequestTimeout') { return res.status(504).end(); } else if (err.$metadata?.httpStatusCode === 400 || ['400', 'NoSuchBucket'].includes(err.name)) { - return next(Object.assign(new ParameterError('A parameter value is bad', { cause: err }), { status: 400 })); + return next(Object.assign(new ParameterError('A parameter value is bad', { cause: err }), { statusCode: 400 })); + } else if (err.code === 'ECONNREFUSED') { + errToMessages(err, res.logNotes); + return res.status(503).end(); } else { - return next(Object.assign(err, { status: err?.$metadata?.httpStatusCode || 502 })); + return next(Object.assign(err, { statusCode: err?.$metadata?.httpStatusCode || 502 })); } } }); @@ -231,6 +245,7 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP const s3Path = calcS3Path(BLOB_PREFIX, req.params[0]); const headResponse = await s3client.send(new HeadObjectCommand({ Bucket: bucketName, Key: s3Path })); if (headResponse.ContentType === FOLDER_FLAG) { + rateLimiterPenalty(req.ip, 2); res.logNotes.add('blocked attempt to delete folder directly'); res.status(409).type('text/plain').send('can\'t delete folder directly: ' + s3Path); return; } @@ -258,14 +273,18 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP err.$metadata?.httpStatusCode === 403) { res.status(404).end(); } else if (err.$metadata?.httpStatusCode === 400 || err.name === '400' || /\bBucket\b/.test(err.message)) { + rateLimiterPenalty(req.ip); errToMessages(err, res.logNotes); res.status(400).type('text/plain').send('A parameter value is bad'); } else if (err.name === 'EndResponseError') { res.logNotes.add(err.message); res.logLevel = err.logLevel; return res.status(err.statusCode).type('text/plain').send(err.message); + } else if (err.code === 'ECONNREFUSED') { + errToMessages(err, res.logNotes); + return res.status(503).end(); } else { - return next(Object.assign(err, { status: 502 })); + return next(Object.assign(err, { statusCode: err.$metadata?.httpStatusCode || 502 })); } } } @@ -626,7 +645,8 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP } catch (err) { if (!(['NotFound', 'NoSuchKey', 'Forbidden', 'AccessDenied', '403'].includes(err.name) || err.$metadata?.httpStatusCode === 403)) { - throw Object.assign(err, { status: 502 }); + err.statusCode ||= err.$metadata?.httpStatusCode || 502; + throw err; } } ancestorPath = dirname(ancestorPath); @@ -655,7 +675,7 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP err.$metadata?.httpStatusCode === 403) { parentETag = null; } else { - throw Object.assign(err, { status: 502 }); + throw Object.assign(err, { statusCode: err.$metadata?.httpStatusCode || 502 }); } } diff --git a/lib/routes/admin.js b/lib/routes/admin.js index 378b6cd1..3aa223f1 100644 --- a/lib/routes/admin.js +++ b/lib/routes/admin.js @@ -11,7 +11,7 @@ const { initProtocols, assembleContactURL, calcContactURL, protocolOptions } = r const nameFromUseragent = require('../util/nameFromUseragent'); const removeUserDataFromSession = require('../util/removeUserDataFromSession'); const ParameterError = require('../util/ParameterError'); -const { rateLimiterBlock, rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); +const { rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); /* eslint no-unused-vars: ["warn", { "varsIgnorePattern": "^_", "argsIgnorePattern": "^_" }] */ /* eslint-disable no-case-declarations */ @@ -396,9 +396,9 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute async (req, res, _next) => { try { if (!(req.session.privileges?.ADMIN || (req.session.user && req.body.contacturl === req.session.user?.contactURL))) { + await rateLimiterPenalty(req.ip); res.logNotes.add('session does not have ADMIN privilege'); res.status(401).type('text/plain').send('Ask an admin to send the invite'); - await rateLimiterBlock(req.ip, 61); return; } diff --git a/lib/routes/oauth.js b/lib/routes/oauth.js index 24ef605c..f842264b 100644 --- a/lib/routes/oauth.js +++ b/lib/routes/oauth.js @@ -93,7 +93,7 @@ module.exports = function (hostIdentity, jwtSecret, account) { redirectOrigin = new URL(req.session.oauthParams.redirect_uri).origin; } catch (err) { await rateLimiterBlock(req.ip, 61); - throw new Error('Application origin is bad', { cause: err }); + throw new Error('Application origin is bad; blocking', { cause: err }); } const scopes = req.session.oauthParams.scope.split(/\s+/).map(scope => { diff --git a/lib/routes/storage_common.js b/lib/routes/storage_common.js index 9f272c9e..d1307b4c 100644 --- a/lib/routes/storage_common.js +++ b/lib/routes/storage_common.js @@ -6,6 +6,7 @@ const sanityCheckUsername = require('../middleware/sanityCheckUsername'); const isSecureRequest = require('../util/isSecureRequest'); const { expressjwt: jwt } = require('express-jwt'); const { getHost } = require('../util/getHost'); +const { rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware'); module.exports = function (hostIdentity, jwtSecret) { const router = express.Router(); @@ -77,12 +78,13 @@ module.exports = function (hostIdentity, jwtSecret) { } ); - function validPathParam (req, res, next) { + async function validPathParam (req, res, next) { req.blobPath = req.params[0]; const path = req.blobPath.endsWith('/') ? req.blobPath.slice(0, -1) : req.blobPath; if (path.length > 0) { for (const segment of path.split('/')) { if (segment.length === 0 || /^\.+$|\0/.test(segment)) { + await rateLimiterPenalty(req.ip, 2); res.logNotes.add('invalid path'); res.status(400).end(); return; @@ -92,8 +94,9 @@ module.exports = function (hostIdentity, jwtSecret) { next(); } - function validWritePath (req, res, next) { + async function validWritePath (req, res, next) { if (req.blobPath.endsWith('/')) { + await rateLimiterPenalty(req.ip, 2); res.logNotes.add('can\'t write to folder'); res.status(400).type('text/plain').send('can\'t write to folder'); } else { @@ -101,7 +104,8 @@ module.exports = function (hostIdentity, jwtSecret) { } } - function jwtErrors (err, req, res, next) { + async function jwtErrors (err, req, res, next) { + await rateLimiterPenalty(req.ip, 2); if (Number.isInteger(err.status)) { const blobPathComponents = req.blobPath?.split('/'); const requiredScopeName = (blobPathComponents[1] === 'public' ? blobPathComponents[2] : blobPathComponents[1]) || '*'; @@ -172,7 +176,8 @@ module.exports = function (hostIdentity, jwtSecret) { * @param {string} requiredScope - required scope that was not granted * @param {string|Error} [logMsg] - should concisely give details & be distinct from other calls */ - function unauthorized (req, res, status, errMsg, requiredScope, logMsg) { + async function unauthorized (req, res, status, errMsg, requiredScope, logMsg) { + await rateLimiterPenalty(req.ip, 2); const value = `Bearer realm="${getHost(req)}" scope="${requiredScope}"` + (errMsg ? ` error="${errMsg}"` : ''); res.set('WWW-Authenticate', value); res.logNotes.add(logMsg || errMsg); diff --git a/lib/util/errorPage.js b/lib/util/errorPage.js index cb36801c..8bc18824 100644 --- a/lib/util/errorPage.js +++ b/lib/util/errorPage.js @@ -4,15 +4,14 @@ function errorPage (req, res, status = 500, messageOrLocals, logNote = '', logLe res.status(status); const locals = { - title: 'Error', + title: 'Something went wrong', status, params: {}, - error: null, host: getHost(req), ...(typeof messageOrLocals === 'string' ? { message: messageOrLocals } : messageOrLocals) }; - if (logNote || locals.error || locals.message) { + if (res.logNotes.size === 0 && (logNote || locals.error || locals.message)) { res.logNotes.add(logNote || locals.error || locals.message); } res.logLevel = logLevel; diff --git a/lib/views/error.html b/lib/views/error.html index a05cd84b..c823fb63 100644 --- a/lib/views/error.html +++ b/lib/views/error.html @@ -1,6 +1,6 @@ <%- include('header.html'); %> -

Something went wrong

+

<%= title %>

<%= status %>

Please contact the administrator, and include the URL and this message:

<%= message %>

diff --git a/spec/modular/m_not_found.spec.js b/spec/modular/m_not_found.spec.js index 26f96eb7..6a341ab1 100644 --- a/spec/modular/m_not_found.spec.js +++ b/spec/modular/m_not_found.spec.js @@ -18,7 +18,7 @@ describe('Nonexistant resource (modular)', function () { hostIdentity: this.hostIdentity, jwtSecret: 'swordfish', accountMgr: mockAccountFactory(this.hostIdentity), - storeRouter: (_req, _res, next) => next() + storeRouter: (_req, res, _next) => { res.status(404).end(); } }); app.locals.title = 'Test Armadietto'; app.locals.host = 'localhost:xxxx'; @@ -47,6 +47,12 @@ describe('Nonexistant resource (modular)', function () { expect(res.text).to.equal(''); }); + it('should curtly refuse POSTs without a handler', async function () { + const res = await chai.request(this.app).post('/admin/login'); + expect(res).to.have.status(404); + expect(res.text).to.equal(''); + }); + /** This tests that 404 for nonexistent assets is cache-able */ it('should return cache headers for asset', async function () { const res = await chai.request(this.app).get('/assets/not-there').set('Origin', this.hostIdentity); @@ -87,10 +93,6 @@ describe('Nonexistant resource (modular)', function () { expect(res).not.to.have.header('X-Powered-By'); expect(res).to.have.header('X-XSS-Protection', '0'); // disabled because counterproductive - expect(res).to.have.header('Content-Type', /^text\/html/); - expect(parseInt(res.get('Content-Length'))).to.be.greaterThan(0); - - expect(res).to.have.header('ETag'); expect(res).to.have.header('Cache-Control', /\bno-cache\b/); expect(res).to.have.header('Cache-Control', /\bpublic\b/); }); diff --git a/spec/modular/m_storage_common.spec.js b/spec/modular/m_storage_common.spec.js index 5209ddfc..aeb748cf 100644 --- a/spec/modular/m_storage_common.spec.js +++ b/spec/modular/m_storage_common.spec.js @@ -147,6 +147,14 @@ describe('Storage (modular)', function () { }); describe('GET (only implemented by modular)', function () { + describe('when the client uses double-dot for a username', function () { + it('blocks responses for a while', async function () { + const res = await get(this.app, '/storage/ /cracking', this.good_token); + expect(res).to.have.status(400); + expect(res).to.have.header('Access-Control-Allow-Origin', 'https://rs-app.com:2112'); + }); + }); + describe('when a valid access token is used', function () { it('returns Cache-Control: public for a public document', async function () { this.store.content = 'a value'; diff --git a/spec/not_found.spec.js b/spec/not_found.spec.js index 31c9753b..8e4b1bc4 100644 --- a/spec/not_found.spec.js +++ b/spec/not_found.spec.js @@ -18,8 +18,8 @@ exports.shouldHandleNonexistingResource = function () { expect(parseInt(res.get('Content-Length'))).to.be.greaterThan(1500); // expect(res).to.have.header('Cache-Control', /\bmax-age=\d{4}/); // expect(res).to.have.header('ETag'); - // expect(res.text).to.contain('Not Found — Armadietto'); - expect(res.text).to.match(/Something went wrong<\/h\d>/); + expect(res.text).to.match(/(Not Found|Something went wrong) — Armadietto<\/title>/i); + expect(res.text).to.match(/<h\d>(Not Found|Something went wrong)<\/h\d>/i); expect(res.text).to.contain('>404<'); expect(res.text).to.contain('>“account/wildebeest/” doesn't exist<'); diff --git a/spec/store_handler.spec.js b/spec/store_handler.spec.js index 5f1ed7c4..2addacd1 100644 --- a/spec/store_handler.spec.js +++ b/spec/store_handler.spec.js @@ -123,6 +123,21 @@ module.exports.shouldStoreStreams = function () { describe('for files', function () { describe('unversioned', function () { + it('returns Not Found for a non-existing user', async function () { + this.timeout(360_000); + const [_req, res, next] = await callMiddleware(this.handler, { + method: 'GET', + url: '/not-the-user/public/who-knows' + }); + + expect(res.statusCode).to.equal(404); + expect(res._getBuffer().toString()).to.equal(''); + expect(Boolean(res.get('Content-Length'))).to.be.false; + expect(Boolean(res.get('Content-Type'))).to.be.false; + expect(Boolean(res.get('ETag'))).to.be.false; + expect(next).not.to.have.been.called; + }); + it('returns Not Found for a non-existing path', async function () { this.timeout(360_000); const [_req, res, next] = await callMiddleware(this.handler, { @@ -286,6 +301,21 @@ module.exports.shouldStoreStreams = function () { expect(next).not.to.have.been.called; }); + it('returns listing with no items for a non-existing public folder', async function () { + const [_req, res, next] = await callMiddleware(this.handler, { + method: 'GET', + url: `/${this.userIdStore}/public/some-category/` + }); + + expect(res.statusCode).to.equal(200); + expect(res.get('Content-Type')).to.equal('application/ld+json'); + const folder = res._getJSONData(); + expect(folder['@context']).to.equal('http://remotestorage.io/spec/folder-description'); + expect(folder.items).to.deep.equal({}); + expect(res.get('ETag')).to.match(/^"[#-~!]{6,128}"$/); + expect(next).not.to.have.been.called; + }); + it('returns listing with no items for a non-existing folder in non-existing category', async function () { const [_req, res, next] = await callMiddleware(this.handler, { method: 'GET',