feature: upload validation middleware (#7208)

no issue

- Source out validation logic into a upload validation middleware for all upload types (csv, image, subscribers). This unit can be later used for Ghost 1.0 as a pre validation core unit. 
- More usage of route tests than controller tests. These are use case tests, a use case only changes if the product changes

core/server/api/db.js

@@ -1,16 +1,13 @@
 // # DB API
 // API for DB operations
-var _                = require('lodash'),
-    Promise          = require('bluebird'),
+var Promise          = require('bluebird'),
     exporter         = require('../data/export'),
     importer         = require('../data/importer'),
     backupDatabase   = require('../data/migration').backupDatabase,
     models           = require('../models'),
     errors           = require('../errors'),
     utils            = require('./utils'),
     pipeline         = require('../utils/pipeline'),
-    i18n             = require('../i18n'),
-
     api              = {},
     docName      = 'db',
     db;
@@ -62,31 +59,8 @@ db = {
      */
     importContent: function (options) {
         var tasks = [];
-
         options = options || {};
 
-        function validate(options) {
-            options.name = options.originalname;
-            options.type = options.mimetype;
-
-            // Check if a file was provided
-            if (!utils.checkFileExists(options)) {
-                return Promise.reject(new errors.ValidationError(i18n.t('errors.api.db.selectFileToImport')));
-            }
-
-            // Check if the file is valid
-            if (!utils.checkFileIsValid(options, importer.getTypes(), importer.getExtensions())) {
-                return Promise.reject(new errors.UnsupportedMediaTypeError(
-                    i18n.t('errors.api.db.unsupportedFile') +
-                        _.reduce(importer.getExtensions(), function (memo, ext) {
-                            return memo ? memo + ', ' + ext : ext;
-                        })
-                ));
-            }
-
-            return options;
-        }
-
         function importContent(options) {
             return importer.importFromFile(options)
                 .then(function () {
@@ -96,7 +70,6 @@ db = {
         }
 
         tasks = [
-            validate,
             utils.handlePermissions(docName, 'importContent'),
             importContent
         ];

core/server/api/subscribers.js

@@ -266,23 +266,6 @@ subscribers = {
         var tasks = [];
         options = options || {};
 
-        function validate(options) {
-            options.name = options.originalname;
-            options.type = options.mimetype;
-
-            // Check if a file was provided
-            if (!utils.checkFileExists(options)) {
-                return Promise.reject(new errors.ValidationError(i18n.t('errors.api.db.selectFileToImport')));
-            }
-
-            // Check for valid file type
-            if (!utils.checkFileIsValid(options, ['text/csv','application/csv'], ['.csv'])) {
-                return Promise.reject(new errors.ValidationError(i18n.t('errors.api.subscribers.selectValidFile')));
-            }
-
-            return options;
-        }
-
         function importCSV(options) {
             var filePath = options.path,
                 fulfilled = 0,
@@ -326,7 +309,6 @@ subscribers = {
         }
 
         tasks = [
-            validate,
             utils.handlePermissions(docName, 'add'),
             importCSV
         ];

core/server/api/upload.js

@@ -1,12 +1,7 @@
-var config  = require('../config'),
-    Promise = require('bluebird'),
+var Promise = require('bluebird'),
     fs      = require('fs-extra'),
     pUnlink = Promise.promisify(fs.unlink),
     storage = require('../storage'),
-    errors  = require('../errors'),
-    utils   = require('./utils'),
-    i18n    = require('../i18n'),
-
     upload;
 
 /**
@@ -26,21 +21,6 @@ upload = {
     add: Promise.method(function (options) {
         var store = storage.getStorage();
 
-        // Public interface of the storage module's `save` method requires
-        // the file's name to be on the .name property.
-        options.name = options.originalname;
-        options.type = options.mimetype;
-
-        // Check if a file was provided
-        if (!utils.checkFileExists(options)) {
-            throw new errors.NoPermissionError(i18n.t('errors.api.upload.pleaseSelectImage'));
-        }
-
-        // Check if the file is valid
-        if (!utils.checkFileIsValid(options, config.uploads.contentTypes, config.uploads.extensions)) {
-            throw new errors.UnsupportedMediaTypeError(i18n.t('errors.api.upload.pleaseSelectValidImage'));
-        }
-
         return store.save(options).finally(function () {
             // Remove uploaded file from tmp location
             return pUnlink(options.path);

core/server/config/index.js

@@ -225,10 +225,21 @@ ConfigManager.prototype.set = function (config) {
             'signin', 'signout', 'signup', 'user', 'users', 'wp-admin', 'wp-login'],
             protected: ['ghost', 'rss']
         },
+        // used in middleware/validation/upload.js
+        // if we finish the data/importer logic, each type selects an importer
         uploads: {
-            // Used by the upload API to limit uploads to images
-            extensions: ['.jpg', '.jpeg', '.gif', '.png', '.svg', '.svgz'],
-            contentTypes: ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml']
+            subscribers: {
+                extensions: ['.csv'],
+                contentTypes: ['text/csv','application/csv']
+            },
+            images: {
+                extensions: ['.jpg', '.jpeg', '.gif', '.png', '.svg', '.svgz'],
+                contentTypes: ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml']
+            },
+            db: {
+                extensions: ['.json'],
+                contentTypes: ['application/octet-stream', 'application/json']
+            }
         },
         deprecatedItems: ['updateCheck', 'mail.fromaddress'],
         // create a hash for cache busting assets

core/server/data/importer/handlers/image.js

@@ -8,8 +8,8 @@ var _       = require('lodash'),
 
 ImageHandler = {
     type: 'images',
-    extensions: config.uploads.extensions,
-    types: config.uploads.contentTypes,
+    extensions: config.uploads.images.extensions,
+    contentTypes: config.uploads.images.contentTypes,
     directories: ['images', 'content'],
 
     loadFile: function (files, baseDir) {

core/server/data/importer/handlers/json.js

@@ -8,7 +8,7 @@ var _            = require('lodash'),
 JSONHandler = {
     type: 'data',
     extensions: ['.json'],
-    types: ['application/octet-stream', 'application/json'],
+    contentTypes: ['application/octet-stream', 'application/json'],
     directories: [],
 
     loadFile: function (files, startDir) {

core/server/data/importer/handlers/markdown.js

@@ -83,7 +83,7 @@ processMarkdownFile = function (filename, content) {
 MarkdownHandler = {
     type: 'data',
     extensions: ['.md', '.markdown'],
-    types: ['application/octet-stream', 'text/plain'],
+    contentTypes: ['application/octet-stream', 'text/plain'],
     directories: [],
 
     loadFile: function (files, startDir) {

core/server/data/importer/index.js

@@ -25,7 +25,7 @@ var _            = require('lodash'),
 
 defaults = {
     extensions: ['.zip'],
-    types: ['application/zip', 'application/x-zip-compressed'],
+    contentTypes: ['application/zip', 'application/x-zip-compressed'],
     directories: []
 };
 
@@ -55,8 +55,8 @@ _.extend(ImportManager.prototype, {
      * Get an array of all the mime types for which we have handlers
      * @returns {string[]}
      */
-    getTypes: function () {
-        return _.flatten(_.union(_.map(this.handlers, 'types'), defaults.types));
+    getContentTypes: function () {
+        return _.flatten(_.union(_.map(this.handlers, 'contentTypes'), defaults.contentTypes));
     },
     /**
      * Get an array of directories for which we have handlers

core/server/middleware/index.js

@@ -30,6 +30,7 @@ var bodyParser      = require('body-parser'),
     maintenance     = require('./maintenance'),
     versionMatch     = require('./api/version-match'),
     cors             = require('./cors'),
+    validation       = require('./validation'),
     netjet           = require('netjet'),
     labs             = require('./labs'),
     helpers          = require('../helpers'),
@@ -42,6 +43,7 @@ var bodyParser      = require('body-parser'),
 
 middleware = {
     upload: multer({dest: tmpdir()}),
+    validation: validation,
     cacheControl: cacheControl,
     spamPrevention: spamPrevention,
     oauth: oauth,

core/server/middleware/validation/index.js

@@ -0,0 +1 @@
+exports.upload = require('./upload');

core/server/middleware/validation/upload.js

@@ -0,0 +1,30 @@
+var apiUtils = require('../../api/utils'),
+    errors = require('../../errors'),
+    config = require('../../config'),
+    i18n = require('../../i18n');
+
+module.exports = function upload(options) {
+    var type = options.type;
+
+    // if we finish the data/importer logic, we forward the request to the specified importer
+    return function (req, res, next) {
+        var extensions = (config.uploads[type] && config.uploads[type].extensions) || [],
+            contentTypes = (config.uploads[type] && config.uploads[type].contentTypes) || [];
+
+        req.file = req.file || {};
+        req.file.name = req.file.originalname;
+        req.file.type = req.file.mimetype;
+
+        // Check if a file was provided
+        if (!apiUtils.checkFileExists(req.file)) {
+            return next(new errors.NoPermissionError(i18n.t('errors.api.' + type + '.missingFile')));
+        }
+
+        // Check if the file is valid
+        if (!apiUtils.checkFileIsValid(req.file, contentTypes, extensions)) {
+            return next(new errors.UnsupportedMediaTypeError(i18n.t('errors.api.' + type + '.invalidFile', {extensions: extensions})));
+        }
+
+        next();
+    };
+};

core/server/routes/api.js

@@ -82,6 +82,7 @@ apiRoutes = function apiRoutes(middleware) {
         middleware.api.labs.subscribers,
         authenticatePrivate,
         middleware.upload.single('subscribersfile'),
+        middleware.validation.upload({type: 'subscribers'}),
         api.http(api.subscribers.importCSV)
     );
     router.get('/subscribers/:id', middleware.api.labs.subscribers, authenticatePrivate, api.http(api.subscribers.read));
@@ -109,7 +110,12 @@ apiRoutes = function apiRoutes(middleware) {
 
     // ## DB
     router.get('/db', authenticatePrivate, api.http(api.db.exportContent));
-    router.post('/db', authenticatePrivate, middleware.upload.single('importfile'), api.http(api.db.importContent));
+    router.post('/db',
+        authenticatePrivate,
+        middleware.upload.single('importfile'),
+        middleware.validation.upload({type: 'db'}),
+        api.http(api.db.importContent)
+    );
     router.del('/db', authenticatePrivate, api.http(api.db.deleteAllContent));
 
     // ## Mail
@@ -138,7 +144,13 @@ apiRoutes = function apiRoutes(middleware) {
     router.post('/authentication/revoke', authenticatePrivate, api.http(api.authentication.revoke));
 
     // ## Uploads
-    router.post('/uploads', authenticatePrivate, middleware.upload.single('uploadimage'), api.http(api.uploads.add));
+    // @TODO: rename endpoint to /images/upload (or similar)
+    router.post('/uploads',
+        authenticatePrivate,
+        middleware.upload.single('uploadimage'),
+        middleware.validation.upload({type: 'images'}),
+        api.http(api.uploads.add)
+    );
 
     // API Router middleware
     router.use(middleware.api.errorHandler);

core/server/translations/en.json

@@ -309,10 +309,10 @@
                 "invalidKey": "Invalid key"
             },
             "db": {
+                "missingFile": "Please select a database file to import.",
+                "invalidFile": "Unsupported file. Please try any of the following formats: {extensions}",
                 "noPermissionToExportData": "You do not have permission to export data (no rights).",
-                "noPermissionToImportData": "You do not have permission to import data (no rights).",
-                "selectFileToImport": "Please select a file to import.",
-                "unsupportedFile": "Unsupported file. Please try any of the following formats: "
+                "noPermissionToImportData": "You do not have permission to import data (no rights)."
             },
             "mail": {
                 "noPermissionToSendEmail": "You do not have permission to send mail.",
@@ -346,9 +346,10 @@
                 "unknownSlugType": "Unknown slug type '{type}'."
             },
             "subscribers": {
+                "missingFile": "Please select a csv.",
+                "invalidFile": "Please select a valid CSV file to import.",
                 "subscriberNotFound": "Subscriber not found.",
-                "subscriberAlreadyExists": "Email address is already subscribed.",
-                "selectValidFile": "Please select a valid CSV file to import."
+                "subscriberAlreadyExists": "Email address is already subscribed."
             },
             "tags": {
                 "tagNotFound": "Tag not found."
@@ -359,9 +360,9 @@
                 "themeDoesNotExist": "Theme does not exist.",
                 "invalidRequest": "Invalid request."
             },
-            "upload": {
-                "pleaseSelectImage": "Please select an image.",
-                "pleaseSelectValidImage": "Please select a valid image."
+            "images": {
+                "missingFile": "Please select an image.",
+                "invalidFile": "Please select a valid image."
             },
             "users": {
                 "userNotFound": "User not found.",

core/test/functional/routes/api/db_spec.js

@@ -1,5 +1,6 @@
 var supertest     = require('supertest'),
     should        = require('should'),
+    path          = require('path'),
     testUtils     = require('../../../utils'),
     ghost         = require('../../../../../core'),
     request;
@@ -61,4 +62,33 @@ describe('DB API', function () {
                 done();
             });
     });
+
+    it('import should fail without file', function (done) {
+        request.post(testUtils.API.getApiQuery('db/'))
+            .set('Authorization', 'Bearer ' + accesstoken)
+            .expect('Content-Type', /json/)
+            .expect(403)
+            .end(function (err) {
+                if (err) {
+                    return done(err);
+                }
+
+                done();
+            });
+    });
+
+    it('import should fail with unsupported file', function (done) {
+        request.post(testUtils.API.getApiQuery('db/'))
+            .set('Authorization', 'Bearer ' + accesstoken)
+            .expect('Content-Type', /json/)
+            .attach('importfile',  path.join(__dirname, '/../../../utils/fixtures/csv/single-column-with-header.csv'))
+            .expect(415)
+            .end(function (err) {
+                if (err) {
+                    return done(err);
+                }
+
+                done();
+            });
+    });
 });