Check file type and file extension when importing csv (#7185)

issue #7144 - added a check for file type and file extension - added an error message to the localization file - added integration test
TryGhost · Aug 11, 2016 · a028830 · a028830
1 parent cd4fb88
commit a028830
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 4 deletions.
diff --git a/core/server/api/subscribers.js b/core/server/api/subscribers.js
@@ -275,7 +275,10 @@ subscribers = {
                 return Promise.reject(new errors.ValidationError(i18n.t('errors.api.db.selectFileToImport')));
             }
 
-            // TODO: check for valid entries
+            // 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;
         }

diff --git a/core/server/translations/en.json b/core/server/translations/en.json
@@ -347,7 +347,8 @@
             },
             "subscribers": {
                 "subscriberNotFound": "Subscriber not found.",
-                "subscriberAlreadyExists": "Email address is already subscribed."
+                "subscriberAlreadyExists": "Email address is already subscribed.",
+                "selectValidFile": "Please select a valid CSV file to import."
             },
             "tags": {
                 "tagNotFound": "Tag not found."

diff --git a/core/test/integration/api/api_subscription_spec.js b/core/test/integration/api/api_subscription_spec.js
@@ -238,13 +238,15 @@ describe('Subscribers API', function () {
     });
 
     describe('Read CSV', function () {
-        var scope = {};
+        var scope = {},
+            stub;
 
         beforeEach(function () {
             sinon.stub(fs, 'unlink', function (path, cb) {
                 cb();
             });
             sinon.stub(apiUtils, 'checkFileExists').returns(true);
+            stub = sinon.stub(apiUtils, 'checkFileIsValid').returns(true);
             sinon.stub(serverUtils, 'readCSV', function () {
                 if (scope.csvError) {
                     return Promise.reject(new Error('csv'));
@@ -257,7 +259,9 @@ describe('Subscribers API', function () {
         afterEach(function () {
             fs.unlink.restore();
             apiUtils.checkFileExists.restore();
+            apiUtils.checkFileIsValid.restore();
             serverUtils.readCSV.restore();
+            scope.csvError = false;
         });
 
         it('check that fn works in general', function (done) {
@@ -286,7 +290,7 @@ describe('Subscribers API', function () {
                 .catch(done);
         });
 
-        it('read csv throws an error', function (done) {
+        it('read csv throws a not found error', function (done) {
             scope.csvError = true;
 
             SubscribersAPI.importCSV(_.merge(testUtils.context.internal, {path: '/somewhere'}))
@@ -298,5 +302,18 @@ describe('Subscribers API', function () {
                     done();
                 });
         });
+
+        it('throws an invalid file error', function (done) {
+            stub.returns(false);
+
+            SubscribersAPI.importCSV(_.merge(testUtils.context.internal, {path: '/somewhere'}))
+                .then(function () {
+                    done(new Error('we expected an error here!'));
+                })
+                .catch(function (err) {
+                    err.message.should.eql('Please select a valid CSV file to import.');
+                    done();
+                });
+        });
     });
 });