From eda74de1de2f61481ee70ce2af75c2209a72eb57 Mon Sep 17 00:00:00 2001 From: Jack Foltz Date: Thu, 26 Jul 2018 16:54:08 -0400 Subject: [PATCH] Fix upload size bug in upload.js and add more tests --- app/routes/upload.js | 25 ++++++++++++----------- config/default.json | 1 + config/dev.json | 1 + config/test.json | 1 + test/api.js | 57 +++++++++++++++++++++++++++++++++++++++++++--------- test/testUtil.js | 47 +++++++++++++++++++++++++++++++------------ 6 files changed, 98 insertions(+), 34 deletions(-) diff --git a/app/routes/upload.js b/app/routes/upload.js index b4d85bd..b9773e4 100644 --- a/app/routes/upload.js +++ b/app/routes/upload.js @@ -1,17 +1,18 @@ -var express = require('express'); -var router = express.Router(); +const express = require('express'); +const router = express.Router(); +const config = require('config'); -var User = require('../models/User.js'); -var Upload = require('../models/Upload.js'); -var Key = require('../models/Key.js'); +const User = require('../models/User.js'); +const Upload = require('../models/Upload.js'); +const Key = require('../models/Key.js'); -var multer = require('multer'); -var dest = multer({dest: 'uploads/'}); +const multer = require('multer'); +const fileUpload = multer({dest: config.uploadPath}).single('file'); +const fsPromises = require('fs').promises; const requireAuth = require('../util/requireAuth').requireAuth; const wrap = require('../util/wrap.js').wrap; - const generatedIdExists = async id => await Upload.countDocuments({id: id}) === 1; @@ -37,21 +38,21 @@ const updateStats = async req => ]); -router.post('/', requireAuth('file.upload'), dest.single('file'), wrap(async (req, res, next) => { +router.post('/', requireAuth('file.upload'), fileUpload, wrap(async (req, res, next) => { if (!req.file) return res.status(400).json({message: 'No file specified.'}); // Max file size is 128 MiB - if (req.file.size > 1024 * 1024 * 128) + if (req.file.size > 1024 * 1024 * 128) { + await fsPromises.unlink(req.file.path); return res.status(413).json({message: 'File too large.'}); + } const upload = { - name: req.file.originalname, id: await generateId(), uploader: req.authUser, uploaderKey: req.authKey, date: Date.now(), - mime: req.file.mimetype, file: req.file }; diff --git a/config/default.json b/config/default.json index 84be811..54f9ee7 100644 --- a/config/default.json +++ b/config/default.json @@ -1,4 +1,5 @@ { "dbHost": "mongodb://localhost:27017/shimapan", + "uploadPath": "uploads", "httpLogLevel": "dev" } diff --git a/config/dev.json b/config/dev.json index 84be811..54f9ee7 100644 --- a/config/dev.json +++ b/config/dev.json @@ -1,4 +1,5 @@ { "dbHost": "mongodb://localhost:27017/shimapan", + "uploadPath": "uploads", "httpLogLevel": "dev" } diff --git a/config/test.json b/config/test.json index 376b1b6..607063b 100644 --- a/config/test.json +++ b/config/test.json @@ -1,4 +1,5 @@ { "dbHost": "mongodb://localhost:27017/shimapan-test", + "uploadPath": "uploads-test", "httpLogLevel": "dev" } \ No newline at end of file diff --git a/test/api.js b/test/api.js index 886add0..bfef6a7 100644 --- a/test/api.js +++ b/test/api.js @@ -7,6 +7,7 @@ const should = chai.should(); const User = require('../app/models/User.js'); const Invite = require('../app/models/Invite.js'); const Upload = require('../app/models/Upload.js'); +const Key = require('../app/models/Key.js'); const util = require('./testUtil.js'); const canonicalize = require('../app/util/canonicalize').canonicalize; @@ -26,7 +27,7 @@ after(() => { server.close(); }); -describe('Users', function() { +describe('Accounts', function() { beforeEach(async () => util.clearDatabase()); describe('/POST register', () => { @@ -41,10 +42,10 @@ describe('Users', function() { res.body.should.have.property('message').eql('Registration successful.'); const userCount = await User.countDocuments({username: user.username}); - userCount.should.eql(1); + userCount.should.equal(1); const inviteCount = await Invite.countDocuments({code: user.invite, recipient: canonicalize(user.username)}); - inviteCount.should.eql(1); + inviteCount.should.equal(1); } it('MUST register a valid user with a valid invite', async () => @@ -69,6 +70,9 @@ describe('Users', function() { res.should.have.status(422); res.body.should.be.a('object'); res.body.should.have.property('message').eql(message); + + const inviteCount = await Invite.countDocuments({code: user.invite, recipient: canonicalize(user.username)}); + inviteCount.should.equal(0); } it('MUST NOT register a nonexistant invite', async () => @@ -90,7 +94,10 @@ describe('Users', function() { const res = await util.registerUser(user, agent); res.should.have.status(422); res.body.should.be.a('object'); - res.body.should.have.property('message').eql(message); + res.body.should.have.property('message').equal(message); + + const inviteCount = await Invite.countDocuments({code: user.invite, recipient: canonicalize(user.username)}); + inviteCount.should.equal(0); } it('MUST NOT register a duplicate username', async () => { @@ -186,19 +193,51 @@ describe('Uploads', () => { beforeEach(async () => util.clearDatabase()); describe('/POST upload', () => { - async function verifySuccessfulUpload(file) { + async function verifySuccessfulUpload(file, user) { + // Get file stats beforehand + const [fileHash, fileSize] = await Promise.all([util.fileHash(file), util.fileSize(file)]); + + // Get the user stats beforehand + const userBefore = await User.findOne({canonicalname: user}, {_id: 0, uploadCount: 1, uploadSize: 1}); + + // Submit the upload and verify the result const res = await util.upload(file, agent); res.should.have.status(200); res.body.should.be.a('object'); res.body.should.have.property('url'); res.body.should.have.property('id').match(/^[a-z]{6}$/); + + // Find the uploaded file in the database + const upload = await Upload.findOne({id: res.body.id}, {_id: 0, id: 1, file: 1}); + const uploadFile = upload.file.path; + upload.should.be.a('object'); + upload.id.should.equal(res.body.id); + + // Verify the uploaded file is the same as the file now on disk + const [uploadHash, uploadSize] = await Promise.all([util.fileHash(uploadFile), util.fileSize(uploadFile)]); + uploadHash.should.equal(fileHash); + uploadSize.should.equal(fileSize); + + // Verify the user's stats have been updated correctly + const userAfter = await User.findOne({canonicalname: user}, {_id: 0, uploadCount: 1, uploadSize: 1}); + userAfter.uploadCount.should.equal(userBefore.uploadCount + 1); + userAfter.uploadSize.should.equal(userBefore.uploadSize + fileSize); } async function verifyFailedUpload(file, status, message) { + const fileCountBefore = await util.directoryFileCount('uploads'); + const uploadCountBefore = await Upload.countDocuments({}); + const res = await util.upload(file, agent); res.should.have.status(status); res.body.should.be.a('object'); res.body.should.have.property('message').equal(message); + + const fileCountAfter = await util.directoryFileCount('uploads'); + fileCountAfter.should.equal(fileCountBefore, 'File should not have been written to disk'); + + const uploadCountAfter = await Upload.countDocuments({}); + uploadCountAfter.should.equal(uploadCountBefore, 'No uploads should have been written to the database'); } describe('0 Valid Request', () => { @@ -208,11 +247,11 @@ describe('Uploads', () => { util.createTestFile(2048, 'test.bin') ]); - await verifySuccessfulUpload('test.bin'); + await verifySuccessfulUpload('test.bin', 'user'); return Promise.all([ util.logout(agent), - util.deleteTestFile('test.bin') + util.deleteFile('test.bin') ]); }); }); @@ -233,7 +272,7 @@ describe('Uploads', () => { return Promise.all([ util.logout(agent), - util.deleteTestFile('test.bin') + util.deleteFile('test.bin') ]); }); }); @@ -249,7 +288,7 @@ describe('Uploads', () => { return Promise.all([ util.logout(agent), - util.deleteTestFile('large.bin') + util.deleteFile('large.bin') ]); }); }); diff --git a/test/testUtil.js b/test/testUtil.js index 726c9cc..ca92dcd 100644 --- a/test/testUtil.js +++ b/test/testUtil.js @@ -11,11 +11,12 @@ const Key = require('../app/models/Key.js'); const Buffer = require('buffer').Buffer; const crypto = require('crypto'); -const fs = require('fs').promises; +const fs = require('fs'); +const fsPromises = fs.promises; //---------------- DATABASE UTIL ----------------// -exports.clearDatabase = async () => +exports.clearDatabase = () => Promise.all([ User.remove({}), Invite.remove({}), @@ -25,7 +26,7 @@ exports.clearDatabase = async () => //---------------- API ROUTES ----------------// -exports.login = async (credentials, agent) => +exports.login = (credentials, agent) => agent .post('/api/auth/login') .send(credentials); @@ -34,25 +35,25 @@ exports.logout = agent => agent .post('/api/auth/logout'); -exports.createInvite = async (invite) => +exports.createInvite = (invite) => Invite.create(invite); -exports.registerUser = async (user, agent) => +exports.registerUser = (user, agent) => agent .post('/api/auth/register') .send(user); -exports.whoami = async (agent) => +exports.whoami = (agent) => agent .get('/api/auth/whoami') .send(); //---------------- TEST ENTRY CREATION ----------------// -exports.createTestInvite = async () => +exports.createTestInvite = () => exports.createInvite({code: 'code', scope: ['file.upload']}); -exports.createTestInvites = async (n) => +exports.createTestInvites = (n) => Promise.all( Array.from(new Array(n), (val, index) => 'code' + index) .map(code => exports.createInvite({code: code})) @@ -65,14 +66,34 @@ exports.createTestUser = async agent => { exports.createTestSession = async agent => { await exports.createTestUser(agent); - await exports.login({username: 'user', password: 'pass'}, agent); + return exports.login({username: 'user', password: 'pass'}, agent); }; -exports.createTestFile = async (size, name) => - fs.writeFile(name, Buffer.allocUnsafe(size)); +exports.createTestFile = (size, name) => + fsPromises.writeFile(name, Buffer.allocUnsafe(size)); -exports.deleteTestFile = async name => - fs.unlink(name); +//---------------- FILESYSTEM ----------------// + +exports.deleteFile = file => + fsPromises.unlink(file); + +exports.fileExists = file => + fsPromises.access(file, fs.constants.R_OK); + +exports.fileSize = async file => + (await fsPromises.stat(file)).size; + +exports.fileHash = file => + new Promise((resolve, reject) => { + const hash = crypto.createHash('MD5'); + fs.createReadStream(file) + .on('error', reject) + .on('data', chunk => hash.update(chunk)) + .on('end', () => resolve(hash.digest('hex'))); + }); + +exports.directoryFileCount = async dir => + (await fsPromises.readdir(dir)).length; //---------------- UPLOADS ----------------//