Improves error handling and rate limiting triggered by errors

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(() => {

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();
 };

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,17 +219,21 @@ 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;
           return res.status(err.statusCode).type('text/plain').send(err.message);
         } 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 });
           }
         }
 

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;
         }
 

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 => {

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,16 +94,18 @@ 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 {
       next();
     }
   }
 
-  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);

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;

lib/views/error.html

@@ -1,6 +1,6 @@
 <%- include('header.html'); %>
 
-<h2>Something went wrong</h2>
+<h2><%= title %></h2>
 <p class="status"><%= status %></p>
 <p>Please contact the administrator, and include the URL and this message:</p>
 <p class="message"><%= message %></p>

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/);
   });

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';

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('<title>Not Found — Armadietto</title>');
-    expect(res.text).to.match(/<h\d>Something went wrong<\/h\d>/);
+    expect(res.text).to.match(/<title>(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&#39;t exist<');