From a9978fcfd2880464a00df210406cd2819f7cd7f9 Mon Sep 17 00:00:00 2001 From: psychobunny Date: Sun, 18 Apr 2021 19:03:35 -0400 Subject: [PATCH] feat: rate limit file uploads --- install/data/defaults.json | 2 + .../en-GB/admin/settings/uploads.json | 4 ++ public/language/en-GB/error.json | 1 + src/middleware/index.js | 1 + src/middleware/uploads.js | 32 +++++++++++ src/routes/api.js | 10 +++- src/routes/write/topics.js | 2 +- src/views/admin/settings/uploads.tpl | 20 +++++++ test/uploads.js | 57 +++++++++++++++---- 9 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 src/middleware/uploads.js diff --git a/install/data/defaults.json b/install/data/defaults.json index b50e707446..47765eb796 100644 --- a/install/data/defaults.json +++ b/install/data/defaults.json @@ -36,6 +36,8 @@ "allowAccountDelete": 1, "privateUploads": 0, "allowedFileExtensions": "png,jpg,bmp,txt", + "uploadRateLimitThreshold": 10, + "uploadRateLimitCooldown": 60, "allowUserHomePage": 1, "allowMultipleBadges": 0, "maximumFileSize": 2048, diff --git a/public/language/en-GB/admin/settings/uploads.json b/public/language/en-GB/admin/settings/uploads.json index 34da585485..9c815b7dc5 100644 --- a/public/language/en-GB/admin/settings/uploads.json +++ b/public/language/en-GB/admin/settings/uploads.json @@ -21,6 +21,10 @@ "topic-thumb-size": "Topic Thumb Size", "allowed-file-extensions": "Allowed File Extensions", "allowed-file-extensions-help": "Enter comma-separated list of file extensions here (e.g. pdf,xls,doc). An empty list means all extensions are allowed.", + "upload-limit-threshold": "Rate limit user uploads to:", + "upload-limit-threshold-per": "per", + "upload-limit-threshold-per-minute": "Minute", + "upload-limit-threshold-per-minutes": "Minutes", "profile-avatars": "Profile Avatars", "allow-profile-image-uploads": "Allow users to upload profile images", "convert-profile-image-png": "Convert profile image uploads to PNG", diff --git a/public/language/en-GB/error.json b/public/language/en-GB/error.json index 93f34f180f..2a849f7c5c 100644 --- a/public/language/en-GB/error.json +++ b/public/language/en-GB/error.json @@ -103,6 +103,7 @@ "file-too-big": "Maximum allowed file size is %1 kB - please upload a smaller file", "guest-upload-disabled": "Guest uploading has been disabled", "cors-error": "Unable to upload image due to misconfigured CORS", + "upload-ratelimit-reached": "You have uploaded too many files at one time. Please try again later.", "scheduling-to-past": "Please select a date in the future.", "invalid-schedule-date": "Please enter a valid date and time.", diff --git a/src/middleware/index.js b/src/middleware/index.js index f2487ca939..9de7a70a98 100644 --- a/src/middleware/index.js +++ b/src/middleware/index.js @@ -66,6 +66,7 @@ Object.assign(middleware, { require('./render')(middleware); require('./maintenance')(middleware); require('./user')(middleware); +require('./uploads')(middleware); require('./headers')(middleware); require('./expose')(middleware); middleware.assert = require('./assert'); diff --git a/src/middleware/uploads.js b/src/middleware/uploads.js new file mode 100644 index 0000000000..64635bf20c --- /dev/null +++ b/src/middleware/uploads.js @@ -0,0 +1,32 @@ +'use strict'; + +const LRU = require('lru-cache'); +const meta = require('../meta'); +const helpers = require('./helpers'); +const user = require('../user'); +const controllerHelpers = require('../controllers/helpers'); + +const cache = new LRU({ + maxAge: meta.config.uploadRateLimitThreshold * 1000, +}); + +module.exports = function (middleware) { + middleware.ratelimitUploads = helpers.try(async (req, res, next) => { + const { uid } = req; + if (!uid) { + return controllerHelpers.notAllowed(req, res); + } + + if (!meta.config.uploadRateLimitThreshold || await user.isAdminOrGlobalMod(req.uid)) { + return next(); + } + + const count = (cache.peek(`${uid}:uploaded_file_count`) || 0) + req.files.files.length; + if (count > meta.config.uploadRateLimitThreshold) { + return next(new Error(['[[error:upload-ratelimit-reached]]'])); + } + + cache.set(`${uid}:uploaded_file_count`, count); + next(); + }); +}; diff --git a/src/routes/api.js b/src/routes/api.js index 3b180e7981..9e78f03705 100644 --- a/src/routes/api.js +++ b/src/routes/api.js @@ -27,8 +27,14 @@ module.exports = function (app, middleware, controllers) { const multipart = require('connect-multiparty'); const multipartMiddleware = multipart(); - const middlewares = [middleware.maintenanceMode, multipartMiddleware, middleware.validateFiles, middleware.applyCSRF]; - router.post('/post/upload', middlewares, uploadsController.uploadPost); + const middlewares = [ + middleware.maintenanceMode, + multipartMiddleware, + middleware.validateFiles, + middleware.ratelimitUploads, + middleware.applyCSRF, + ]; + router.post('/post/upload', middlewares, uploadsController.uploadPost); router.post('/user/:userslug/uploadpicture', middlewares.concat([middleware.exposeUid, middleware.authenticateRequest, middleware.ensureLoggedIn, middleware.canViewUsers, middleware.checkAccountPermissions]), controllers.accounts.edit.uploadPicture); }; diff --git a/src/routes/write/topics.js b/src/routes/write/topics.js index 4b8d41d293..2e6e3bcbbe 100644 --- a/src/routes/write/topics.js +++ b/src/routes/write/topics.js @@ -36,7 +36,7 @@ module.exports = function () { setupApiRoute(router, 'delete', '/:tid/tags', [...middlewares, middleware.assert.topic], controllers.write.topics.deleteTags); setupApiRoute(router, 'get', '/:tid/thumbs', [], controllers.write.topics.getThumbs); - setupApiRoute(router, 'post', '/:tid/thumbs', [multipartMiddleware, middleware.validateFiles, ...middlewares], controllers.write.topics.addThumb); + setupApiRoute(router, 'post', '/:tid/thumbs', [multipartMiddleware, middleware.validateFiles, middleware.ratelimitUploads, ...middlewares], controllers.write.topics.addThumb); setupApiRoute(router, 'put', '/:tid/thumbs', [...middlewares, middleware.checkRequired.bind(null, ['tid'])], controllers.write.topics.migrateThumbs); setupApiRoute(router, 'delete', '/:tid/thumbs', [...middlewares, middleware.checkRequired.bind(null, ['path'])], controllers.write.topics.deleteThumb); setupApiRoute(router, 'put', '/:tid/thumbs/order', [...middlewares, middleware.checkRequired.bind(null, ['path', 'order'])], controllers.write.topics.reorderThumbs); diff --git a/src/views/admin/settings/uploads.tpl b/src/views/admin/settings/uploads.tpl index dca5f135f4..382dc21cc6 100644 --- a/src/views/admin/settings/uploads.tpl +++ b/src/views/admin/settings/uploads.tpl @@ -101,6 +101,26 @@ [[admin/settings/uploads:allowed-file-extensions-help]]

+ +
+ +
+
+ +
+
+ +
+
+ +
+
+
diff --git a/test/uploads.js b/test/uploads.js index f8fe857d23..04c509d01e 100644 --- a/test/uploads.js +++ b/test/uploads.js @@ -24,6 +24,7 @@ describe('Upload Controllers', () => { let pid; let adminUid; let regularUid; + let maliciousUid; before((done) => { async.series({ @@ -39,12 +40,16 @@ describe('Upload Controllers', () => { regularUid: function (next) { user.create({ username: 'regular', password: 'zugzug' }, next); }, + maliciousUid: function (next) { + user.create({ username: 'malicioususer', password: 'herpderp' }, next); + }, }, (err, results) => { if (err) { return done(err); } adminUid = results.adminUid; regularUid = results.regularUid; + maliciousUid = results.maliciousUid; cid = results.category.cid; topics.post({ uid: adminUid, title: 'test topic title', content: 'test topic content', cid: results.category.cid }, (err, result) => { @@ -132,7 +137,6 @@ describe('Upload Controllers', () => { }); }); - it('should upload a file to a post', (done) => { const oldValue = meta.config.allowedFileExtensions; meta.config.allowedFileExtensions = 'png,jpg,bmp,html'; @@ -157,16 +161,6 @@ describe('Upload Controllers', () => { }); }); - it('should fail to upload image to post if image is broken', (done) => { - helpers.uploadFile(`${nconf.get('url')}/api/post/upload`, path.join(__dirname, '../test/files/brokenimage.png'), {}, jar, csrf_token, (err, res, body) => { - assert.ifError(err); - assert.strictEqual(res.statusCode, 500); - assert(body && body.status && body.status.message); - assert(body.status.message.startsWith('Input file has corrupt header: pngload: end of stream')); - done(); - }); - }); - it('should fail if file is not an image', (done) => { image.isFileTypeAllowed(path.join(__dirname, '../test/files/notanimage.png'), (err) => { assert.strictEqual(err.message, 'Input file contains unsupported image format'); @@ -322,6 +316,47 @@ describe('Upload Controllers', () => { }); }); + describe('regular user uploads rate limits', () => { + let jar; + let csrf_token; + + before((done) => { + helpers.loginUser('malicioususer', 'herpderp', (err, _jar, _csrf_token) => { + assert.ifError(err); + jar = _jar; + csrf_token = _csrf_token; + privileges.global.give(['groups:upload:post:file'], 'registered-users', done); + }); + }); + + it('should fail if the user exceeds the upload rate limit threshold', (done) => { + const oldValue = meta.config.allowedFileExtensions; + meta.config.allowedFileExtensions = 'png,jpg,bmp,html'; + + // why / 2? see: helpers.uploadFile for a weird quirk where we actually upload 2 files per upload in our tests. + async.times(meta.config.uploadRateLimitThreshold / 2, (i, next) => { + helpers.uploadFile(`${nconf.get('url')}/api/post/upload`, path.join(__dirname, '../test/files/503.html'), {}, jar, csrf_token, (err, res, body) => { + if (i + 1 > meta.config.uploadRateLimitThreshold / 2) { + assert.strictEqual(res.statusCode, 500); + assert.strictEqual(body.error, '[[error:upload-ratelimit-reached]]'); + } else { + assert.ifError(err); + assert.strictEqual(res.statusCode, 200); + assert(body && body.status && body.response && body.response.images); + assert(Array.isArray(body.response.images)); + assert(body.response.images[0].url); + } + + next(err); + }); + }, (err) => { + meta.config.allowedFileExtensions = oldValue; + assert.ifError(err); + done(); + }); + }); + }); + describe('admin uploads', () => { let jar; let csrf_token;