Skip to content

Commit

Permalink
feat: rate limit file uploads
Browse files Browse the repository at this point in the history
  • Loading branch information
psychobunny committed Apr 19, 2021
1 parent 0a3a22d commit a9978fc
Show file tree
Hide file tree
Showing 9 changed files with 115 additions and 14 deletions.
2 changes: 2 additions & 0 deletions install/data/defaults.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
"allowAccountDelete": 1,
"privateUploads": 0,
"allowedFileExtensions": "png,jpg,bmp,txt",
"uploadRateLimitThreshold": 10,
"uploadRateLimitCooldown": 60,
"allowUserHomePage": 1,
"allowMultipleBadges": 0,
"maximumFileSize": 2048,
Expand Down
4 changes: 4 additions & 0 deletions public/language/en-GB/admin/settings/uploads.json
Original file line number Diff line number Diff line change
Expand Up @@ -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. <code>pdf,xls,doc</code>). 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",
Expand Down
1 change: 1 addition & 0 deletions public/language/en-GB/error.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
1 change: 1 addition & 0 deletions src/middleware/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
32 changes: 32 additions & 0 deletions src/middleware/uploads.js
Original file line number Diff line number Diff line change
@@ -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();
});
};
10 changes: 8 additions & 2 deletions src/routes/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
2 changes: 1 addition & 1 deletion src/routes/write/topics.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 20 additions & 0 deletions src/views/admin/settings/uploads.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,26 @@
[[admin/settings/uploads:allowed-file-extensions-help]]
</p>
</div>

<div class="form-group">
<label for="uploadRateLimitThreshold">[[admin/settings/uploads:upload-limit-threshold]]</label>
<div class="row">
<div class="col-xs-2">
<input type="text" class="form-control" data-field="uploadRateLimitThreshold" />
</div>
<div class="col-xs-1">
<label style="vertical-align: -5px;">[[admin/settings/uploads:upload-limit-threshold-per]]</label>
</div>
<div class="col-xs-2">
<select class="form-control" data-field="uploadRateLimitCooldown">
<option value="60">1 [[admin/settings/uploads:upload-limit-threshold-per-minute]]</option>
<option value="300">5 [[admin/settings/uploads:upload-limit-threshold-per-minutes]]</option>
<option value="900">15 [[admin/settings/uploads:upload-limit-threshold-per-minutes]]</option>
<option value="3600">60 [[admin/settings/uploads:upload-limit-threshold-per-minutes]]</option>
</select>
</div>
</div>
</div>
</form>
</div>

Expand Down
57 changes: 46 additions & 11 deletions test/uploads.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('Upload Controllers', () => {
let pid;
let adminUid;
let regularUid;
let maliciousUid;

before((done) => {
async.series({
Expand All @@ -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) => {
Expand Down Expand Up @@ -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';
Expand All @@ -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');
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit a9978fc

Please sign in to comment.