From c215626d2b63e65d8988f63c9b783649b6dde848 Mon Sep 17 00:00:00 2001
From: jamesbloomer <github2@thebloomers.co.uk>
Date: Thu, 19 Sep 2013 07:26:26 +0100
Subject: [PATCH] Use file mime type rather than extension to check server side
 if image upload is a valid file

closes #705
- uses the file type passed by express/connect
- relies on the type being set correctly by the browser upload
- doesn't reread the file to check
---
 core/server/controllers/admin.js | 10 ++++-----
 core/test/unit/admin_spec.js     | 36 ++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/core/server/controllers/admin.js b/core/server/controllers/admin.js
index 32f2db72c1..37b44920fb 100644
--- a/core/server/controllers/admin.js
+++ b/core/server/controllers/admin.js
@@ -73,11 +73,12 @@ adminControllers = {
     'uploader': function (req, res) {
 
         var currentDate = moment(),
-            month = currentDate.format("MMM"),
-            year =  currentDate.format("YYYY"),
+            month = currentDate.format('MMM'),
+            year =  currentDate.format('YYYY'),
             tmp_path = req.files.uploadimage.path,
             dir = path.join('content/images', year, month),
             ext = path.extname(req.files.uploadimage.name).toLowerCase(),
+            type = req.files.uploadimage.type,
             basename = path.basename(req.files.uploadimage.name, ext).replace(/[\W]/gi, '_');
 
         function renameFile(target_path) {
@@ -105,13 +106,12 @@ adminControllers = {
             });
         }
 
-        // TODO: is it better to use file type eg. image/png?
-        if (ext === ".jpg" || ext === ".jpeg"  || ext === ".png" || ext === ".gif") {
+        if (type === 'image/jpeg' || type === 'image/png' || type === 'image/gif') {
             getUniqueFileName(dir, basename, ext, null, function (filename) {
                 renameFile(filename);
             });
         } else {
-            res.send(404, "Invalid filetype");
+            res.send(404, 'Invalid filetype');
         }
     },
     'login': function (req, res) {
diff --git a/core/test/unit/admin_spec.js b/core/test/unit/admin_spec.js
index 40c9614409..0366fd7210 100644
--- a/core/test/unit/admin_spec.js
+++ b/core/test/unit/admin_spec.js
@@ -7,6 +7,7 @@ var testUtils = require('./testUtils'),
 
     // Stuff we are testing
     admin = require('../../server/controllers/admin');
+
 describe('Admin Controller', function() {
     describe('uploader', function() {
 
@@ -30,7 +31,8 @@ describe('Admin Controller', function() {
         describe('can not upload invalid file', function() {
             it('should return 404 for invalid file type', function() {
                 res.send = sinon.stub();
-                req.files.uploadimage.name = "INVALID.FILE";
+                req.files.uploadimage.name = 'INVALID.FILE';
+                req.files.uploadimage.type = 'application/octet-stream'
                 admin.uploader(req, res);
                 res.send.calledOnce.should.be.true;
                 res.send.args[0][0].should.equal(404);
@@ -38,13 +40,25 @@ describe('Admin Controller', function() {
             });
         });
 
+        describe('can not upload file with valid extension but invalid type', function() {
+            it('should return 404 for invalid file type', function() {
+                res.send = sinon.stub();
+                req.files.uploadimage.name = 'INVALID.jpg';
+                req.files.uploadimage.type = 'application/octet-stream'
+                admin.uploader(req, res);
+                res.send.calledOnce.should.be.true;
+                res.send.args[0][0].should.equal(404);
+                res.send.args[0][1].should.equal('Invalid filetype');
+            });
+        });
 
         describe('valid file', function() {
 
             var clock;
 
             beforeEach(function() {
-                req.files.uploadimage.name = "IMAGE.jpg";
+                req.files.uploadimage.name = 'IMAGE.jpg';
+                req.files.uploadimage.type = 'image/jpeg';
                 sinon.stub(fs, 'mkdirs').yields();
                 sinon.stub(fs, 'copy').yields();
                 sinon.stub(fs, 'unlink').yields();
@@ -69,8 +83,20 @@ describe('Admin Controller', function() {
                 admin.uploader(req, res);
             });
 
+            it('can upload jpg with incorrect extension', function(done) {
+                req.files.uploadimage.name = 'IMAGE.xjpg';
+                clock = sinon.useFakeTimers(42);
+                sinon.stub(res, 'send', function(data) {
+                    data.should.not.equal(404);
+                    return done();
+                });
+
+                admin.uploader(req, res);
+            });
+
             it('can upload png', function(done) {
-                req.files.uploadimage.name = "IMAGE.png";
+                req.files.uploadimage.name = 'IMAGE.png';
+                req.files.uploadimage.type = 'image/png';
                 clock = sinon.useFakeTimers(42);
                 sinon.stub(res, 'send', function(data) {
                     data.should.not.equal(404);
@@ -81,7 +107,8 @@ describe('Admin Controller', function() {
             });
 
             it('can upload gif', function(done) {
-                req.files.uploadimage.name = "IMAGE.gif";
+                req.files.uploadimage.name = 'IMAGE.gif';
+                req.files.uploadimage.type = 'image/gif';
                 clock = sinon.useFakeTimers(42);
                 sinon.stub(res, 'send', function(data) {
                     data.should.not.equal(404);
@@ -145,6 +172,7 @@ describe('Admin Controller', function() {
             });
 
             it('should not leave temporary file when uploading', function(done) {
+                // Sun Sep 08 2013 10:57
                 clock = sinon.useFakeTimers(new Date(2013, 8, 8, 10, 57).getTime());
                 sinon.stub(res, 'send', function(data) {
                     fs.unlink.calledOnce.should.be.true;