Fix upload size bug in upload.js and add more tests

app/routes/upload.js

@@ -1,17 +1,18 @@
-var express = require('express');
+const express = require('express');
-var router = express.Router();
+const router = express.Router();
+const config = require('config');
 
-var User = require('../models/User.js');
+const User = require('../models/User.js');
-var Upload = require('../models/Upload.js');
+const Upload = require('../models/Upload.js');
-var Key = require('../models/Key.js');
+const Key = require('../models/Key.js');
 
-var multer = require('multer');
+const multer = require('multer');
-var dest = multer({dest: 'uploads/'});
+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
     };
 

config/default.json

@@ -1,4 +1,5 @@
 {
   "dbHost": "mongodb://localhost:27017/shimapan",
+  "uploadPath": "uploads",
   "httpLogLevel": "dev"
 }

config/dev.json

@@ -1,4 +1,5 @@
 {
   "dbHost": "mongodb://localhost:27017/shimapan",
+  "uploadPath": "uploads",
   "httpLogLevel": "dev"
 }

config/test.json

@@ -1,4 +1,5 @@
 {
   "dbHost": "mongodb://localhost:27017/shimapan-test",
+  "uploadPath": "uploads-test",
   "httpLogLevel": "dev"
 }

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')
                 ]);
             });
         });

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) =>
+exports.createTestFile = (size, name) =>
-    fs.writeFile(name, Buffer.allocUnsafe(size));
+    fsPromises.writeFile(name, Buffer.allocUnsafe(size));
 
-exports.deleteTestFile = async name =>
+//---------------- FILESYSTEM ----------------//
-    fs.unlink(name);
+
+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 ----------------//