🐛 subscriber: sanitize email vol. 2 (#8280)

no issue


🐛  subscriber: sanitize email vol. 2
- ensure email get's sanitized for every error case

🐛  validator.isEmptyOrURL doesn't accept non strings
- otherwise it shows a weird error message in the client

✨  new tests for subscriber app
- routing tests

* change tests for Ghost 1.0
* it took me 15min to find this 😡

core/server/apps/subscribers/lib/router.js

@@ -22,9 +22,16 @@ function controller(req, res) {
     return res.render(templates.pickTemplate(templateName, defaultTemplate), data);
 }
 
+/**
+ * Takes care of sanitizing the email input.
+ * XSS prevention.
+ * For success cases, we don't have to worry, because then the input contained a valid email address.
+ */
 function errorHandler(error, req, res, next) {
     /*jshint unused:false */
 
+    req.body.email = '';
+
     if (error.statusCode !== 404) {
         res.locals.error = error;
         return controller(req, res);
@@ -44,7 +51,7 @@ function honeyPot(req, res, next) {
 }
 
 function santizeUrl(url) {
-    return validator.isEmptyOrURL(url) ? url : '';
+    return validator.isEmptyOrURL(url || '') ? url : '';
 }
 
 function handleSource(req, res, next) {
@@ -76,8 +83,6 @@ function storeSubscriber(req, res, next) {
     if (_.isEmpty(req.body.email)) {
         return next(new errors.ValidationError({message: 'Email cannot be blank.'}));
     } else if (!validator.isEmail(req.body.email)) {
-        // sanitize email
-        req.body.email = '';
         return next(new errors.ValidationError({message: 'Invalid email.'}));
     }
 

core/server/apps/subscribers/tests/router_spec.js

@@ -1,18 +0,0 @@
-var should = require('should'),
-    router = require('../lib/router');
-
-describe('UNIT: Apps Subscriber Router', function () {
-    it('[failure] email is invalid, ensure it`s sanitized', function (done) {
-        var req = {
-            body: {
-                email: 'something-evil'
-            }
-        }, res = {};
-
-        router.storeSubscriber(req, res, function next(err) {
-            req.body.email.should.eql('');
-            should.exist(err);
-            done();
-        });
-    });
-});

core/server/apps/subscribers/tests/routing_spec.js

@@ -0,0 +1,122 @@
+var supertest = require('supertest'),
+    should = require('should'),
+    sinon = require('sinon'),
+    testUtils = require('../../../../test/utils'),
+    labs = require('../../../utils/labs'),
+    config = require('../../../config'),
+    ghost = testUtils.startGhost,
+    sandbox = sinon.sandbox.create();
+
+describe('Subscriber: Routing', function () {
+    var ghostServer, request;
+
+    before(function (done) {
+        ghost().then(function (_ghostServer) {
+            ghostServer = _ghostServer;
+            return ghostServer.start();
+        }).then(function () {
+            request = supertest.agent(config.get('url'));
+            done();
+        }).catch(function (e) {
+            console.log('Ghost Error: ', e);
+            console.log(e.stack);
+            done(e);
+        });
+    });
+
+    after(function () {
+        return ghostServer.stop();
+    });
+
+    before(function () {
+        sandbox.stub(labs, 'isSet', function (key) {
+            if (key === 'subscribers') {
+                return true;
+            }
+        });
+    });
+
+    after(function () {
+        sandbox.restore();
+    });
+
+    describe('GET', function () {
+        it('[success]', function (done) {
+            request.get('/subscribe/')
+                .expect(200)
+                .end(function (err) {
+                    should.not.exist(err);
+                    done();
+                });
+        });
+    });
+
+    describe('POST', function () {
+        it('[success]', function (done) {
+            request.post('/subscribe/')
+                .set('Content-type', 'application/x-www-form-urlencoded')
+                .send({
+                    email: 'test@ghost.org',
+                    location: 'http://localhost:2368',
+                    confirm: ''
+                })
+                .expect(200)
+                .end(function (err, res) {
+                    should.not.exist(err);
+                    res.text.should.containEql('Subscribed!');
+                    res.text.should.containEql('test@ghost.org');
+                    done();
+                });
+        });
+
+        it('[error] email is invalid', function (done) {
+            request.post('/subscribe/')
+                .set('Content-type', 'application/x-www-form-urlencoded')
+                .send({
+                    email: 'alphabetazeta',
+                    location: 'http://localhost:2368',
+                    confirm: ''
+                })
+                .expect(200)
+                .end(function (err, res) {
+                    should.not.exist(err);
+                    res.text.should.containEql('http://localhost:2368');
+                    res.text.should.not.containEql('Subscribed!');
+                    res.text.should.not.containEql('alphabetazeta');
+                    done();
+                });
+        });
+
+        it('[error] location is not defined', function (done) {
+            request.post('/subscribe/')
+                .set('Content-type', 'application/x-www-form-urlencoded')
+                .send({
+                    email: 'test@ghost.org',
+                    confirm: ''
+                })
+                .expect(200)
+                .end(function (err, res) {
+                    should.not.exist(err);
+                    res.text.should.not.containEql('Subscribed!');
+                    res.text.should.not.containEql('test@ghost.org');
+                    done();
+                });
+        });
+
+        it('[error] confirm is not defined', function (done) {
+            request.post('/subscribe/')
+                .set('Content-type', 'application/x-www-form-urlencoded')
+                .send({
+                    email: 'test@ghost.org',
+                    location: 'http://localhost:2368'
+                })
+                .expect(200)
+                .end(function (err, res) {
+                    should.not.exist(err);
+                    res.text.should.not.containEql('Subscribed!');
+                    res.text.should.not.containEql('test@ghost.org');
+                    done();
+                });
+        });
+    });
+});

core/test/unit/auth/validation_spec.js

@@ -11,7 +11,7 @@ describe('UNIT: auth validation', function () {
         models.init();
     });
 
-    beforeEach(function () {
+    afterEach(function () {
         sandbox.restore();
     });
 

core/test/unit/scheduling/post-scheduling/index_spec.js

@@ -45,9 +45,9 @@ describe('Scheduling: Post Scheduling', function () {
 
         sandbox.stub(schedulingUtils, 'createAdapter').returns(Promise.resolve(scope.adapter));
 
-        models.Client.findOne = function () {
+        sandbox.stub(models.Client, 'findOne', function () {
             return Promise.resolve(scope.client);
-        };
+        });
 
         sandbox.spy(scope.adapter, 'schedule');
         sandbox.spy(scope.adapter, 'reschedule');