mirror of
				https://github.com/NodeBB/NodeBB.git
				synced 2025-10-31 02:55:58 +01:00 
			
		
		
		
	feat: rate limit file uploads
This commit is contained in:
		
				
					committed by
					
						 Andrew Rodrigues
						Andrew Rodrigues
					
				
			
			
				
	
			
			
			
						parent
						
							0a3a22db9d
						
					
				
				
					commit
					a9978fcfd2
				
			| @@ -36,6 +36,8 @@ | |||||||
|     "allowAccountDelete": 1, |     "allowAccountDelete": 1, | ||||||
|     "privateUploads": 0, |     "privateUploads": 0, | ||||||
|     "allowedFileExtensions": "png,jpg,bmp,txt", |     "allowedFileExtensions": "png,jpg,bmp,txt", | ||||||
|  |     "uploadRateLimitThreshold": 10, | ||||||
|  |     "uploadRateLimitCooldown": 60, | ||||||
|     "allowUserHomePage": 1, |     "allowUserHomePage": 1, | ||||||
|     "allowMultipleBadges": 0, |     "allowMultipleBadges": 0, | ||||||
|     "maximumFileSize": 2048, |     "maximumFileSize": 2048, | ||||||
|   | |||||||
| @@ -21,6 +21,10 @@ | |||||||
| 	"topic-thumb-size": "Topic Thumb Size", | 	"topic-thumb-size": "Topic Thumb Size", | ||||||
| 	"allowed-file-extensions": "Allowed File Extensions", | 	"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.", | 	"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", | 	"profile-avatars": "Profile Avatars", | ||||||
| 	"allow-profile-image-uploads": "Allow users to upload profile images", | 	"allow-profile-image-uploads": "Allow users to upload profile images", | ||||||
| 	"convert-profile-image-png": "Convert profile image uploads to PNG", | 	"convert-profile-image-png": "Convert profile image uploads to PNG", | ||||||
|   | |||||||
| @@ -103,6 +103,7 @@ | |||||||
| 	"file-too-big": "Maximum allowed file size is %1 kB - please upload a smaller file", | 	"file-too-big": "Maximum allowed file size is %1 kB - please upload a smaller file", | ||||||
| 	"guest-upload-disabled": "Guest uploading has been disabled", | 	"guest-upload-disabled": "Guest uploading has been disabled", | ||||||
| 	"cors-error": "Unable to upload image due to misconfigured CORS", | 	"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.", |   	"scheduling-to-past": "Please select a date in the future.", | ||||||
|   	"invalid-schedule-date": "Please enter a valid date and time.", |   	"invalid-schedule-date": "Please enter a valid date and time.", | ||||||
|   | |||||||
| @@ -66,6 +66,7 @@ Object.assign(middleware, { | |||||||
| require('./render')(middleware); | require('./render')(middleware); | ||||||
| require('./maintenance')(middleware); | require('./maintenance')(middleware); | ||||||
| require('./user')(middleware); | require('./user')(middleware); | ||||||
|  | require('./uploads')(middleware); | ||||||
| require('./headers')(middleware); | require('./headers')(middleware); | ||||||
| require('./expose')(middleware); | require('./expose')(middleware); | ||||||
| middleware.assert = require('./assert'); | middleware.assert = require('./assert'); | ||||||
|   | |||||||
							
								
								
									
										32
									
								
								src/middleware/uploads.js
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										32
									
								
								src/middleware/uploads.js
									
									
									
									
									
										Normal file
									
								
							| @@ -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(); | ||||||
|  | 	}); | ||||||
|  | }; | ||||||
| @@ -27,8 +27,14 @@ module.exports = function (app, middleware, controllers) { | |||||||
|  |  | ||||||
| 	const multipart = require('connect-multiparty'); | 	const multipart = require('connect-multiparty'); | ||||||
| 	const multipartMiddleware = multipart(); | 	const multipartMiddleware = multipart(); | ||||||
| 	const middlewares = [middleware.maintenanceMode, multipartMiddleware, middleware.validateFiles, middleware.applyCSRF]; | 	const middlewares = [ | ||||||
| 	router.post('/post/upload', middlewares, uploadsController.uploadPost); | 		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); | 	router.post('/user/:userslug/uploadpicture', middlewares.concat([middleware.exposeUid, middleware.authenticateRequest, middleware.ensureLoggedIn, middleware.canViewUsers, middleware.checkAccountPermissions]), controllers.accounts.edit.uploadPicture); | ||||||
| }; | }; | ||||||
|   | |||||||
| @@ -36,7 +36,7 @@ module.exports = function () { | |||||||
| 	setupApiRoute(router, 'delete', '/:tid/tags', [...middlewares, middleware.assert.topic], controllers.write.topics.deleteTags); | 	setupApiRoute(router, 'delete', '/:tid/tags', [...middlewares, middleware.assert.topic], controllers.write.topics.deleteTags); | ||||||
|  |  | ||||||
| 	setupApiRoute(router, 'get', '/:tid/thumbs', [], controllers.write.topics.getThumbs); | 	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, '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, '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); | 	setupApiRoute(router, 'put', '/:tid/thumbs/order', [...middlewares, middleware.checkRequired.bind(null, ['path', 'order'])], controllers.write.topics.reorderThumbs); | ||||||
|   | |||||||
| @@ -101,6 +101,26 @@ | |||||||
| 					[[admin/settings/uploads:allowed-file-extensions-help]] | 					[[admin/settings/uploads:allowed-file-extensions-help]] | ||||||
| 				</p> | 				</p> | ||||||
| 			</div> | 			</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> | 		</form> | ||||||
| 	</div> | 	</div> | ||||||
|  |  | ||||||
|   | |||||||
| @@ -24,6 +24,7 @@ describe('Upload Controllers', () => { | |||||||
| 	let pid; | 	let pid; | ||||||
| 	let adminUid; | 	let adminUid; | ||||||
| 	let regularUid; | 	let regularUid; | ||||||
|  | 	let maliciousUid; | ||||||
|  |  | ||||||
| 	before((done) => { | 	before((done) => { | ||||||
| 		async.series({ | 		async.series({ | ||||||
| @@ -39,12 +40,16 @@ describe('Upload Controllers', () => { | |||||||
| 			regularUid: function (next) { | 			regularUid: function (next) { | ||||||
| 				user.create({ username: 'regular', password: 'zugzug' }, next); | 				user.create({ username: 'regular', password: 'zugzug' }, next); | ||||||
| 			}, | 			}, | ||||||
|  | 			maliciousUid: function (next) { | ||||||
|  | 				user.create({ username: 'malicioususer', password: 'herpderp' }, next); | ||||||
|  | 			}, | ||||||
| 		}, (err, results) => { | 		}, (err, results) => { | ||||||
| 			if (err) { | 			if (err) { | ||||||
| 				return done(err); | 				return done(err); | ||||||
| 			} | 			} | ||||||
| 			adminUid = results.adminUid; | 			adminUid = results.adminUid; | ||||||
| 			regularUid = results.regularUid; | 			regularUid = results.regularUid; | ||||||
|  | 			maliciousUid = results.maliciousUid; | ||||||
| 			cid = results.category.cid; | 			cid = results.category.cid; | ||||||
|  |  | ||||||
| 			topics.post({ uid: adminUid, title: 'test topic title', content: 'test topic content', cid: results.category.cid }, (err, result) => { | 			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) => { | 		it('should upload a file to a post', (done) => { | ||||||
| 			const oldValue = meta.config.allowedFileExtensions; | 			const oldValue = meta.config.allowedFileExtensions; | ||||||
| 			meta.config.allowedFileExtensions = 'png,jpg,bmp,html'; | 			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) => { | 		it('should fail if file is not an image', (done) => { | ||||||
| 			image.isFileTypeAllowed(path.join(__dirname, '../test/files/notanimage.png'), (err) => { | 			image.isFileTypeAllowed(path.join(__dirname, '../test/files/notanimage.png'), (err) => { | ||||||
| 				assert.strictEqual(err.message, 'Input file contains unsupported image format'); | 				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', () => { | 	describe('admin uploads', () => { | ||||||
| 		let jar; | 		let jar; | ||||||
| 		let csrf_token; | 		let csrf_token; | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user