🎨 optimisations for brute (#7867)

closes #7766, refs #7579

- ensure we are using the correct brute keys
- ensure we are using req.ip as Ghost is configured  with trust proxy option
- tidy up a little

core/server/api/app.js

@@ -166,9 +166,7 @@ function apiRoutes() {
 
     // ## Authentication
     apiRouter.post('/authentication/passwordreset',
-        // Prevent more than 5 password resets from an ip in an hour for any email address
         brute.globalReset,
-        // Prevent more than 5 password resets in an hour for an email+IP pair
         brute.userReset,
         api.http(api.authentication.generateResetToken)
     );

core/server/api/authentication.js

@@ -266,7 +266,7 @@ authentication = {
      * @param {Object} object
      * @returns {Promise<Object>} message
      */
-    resetPassword: function resetPassword(object) {
+    resetPassword: function resetPassword(object, opts) {
         var tasks, tokenIsCorrect, dbHash, options = {context: {internal: true}}, tokenParts;
 
         function validateRequest() {
@@ -341,7 +341,7 @@ authentication = {
                         }));
                     }
 
-                    spamPrevention.userLogin.reset(null, options.data.connection + tokenParts.email + 'login');
+                    spamPrevention.userLogin.reset(opts.ip, tokenParts.email + 'login');
 
                     return models.User.changePassword({
                         oldPassword: oldPassword,

core/server/api/index.js

@@ -220,7 +220,7 @@ http = function http(apiMethod) {
     return function apiHandler(req, res, next) {
         // We define 2 properties for using as arguments in API calls:
         var object = req.body,
-            options = _.extend({}, req.file, req.query, req.params, {
+            options = _.extend({}, req.file, {ip: req.ip}, req.query, req.params, {
                 context: {
                     // @TODO: forward the client and user obj in 1.0 (options.context.user.id)
                     user: ((req.user && req.user.id) || (req.user && models.User.isExternalUser(req.user.id))) ? req.user.id : null,

core/server/auth/oauth.js

@@ -21,7 +21,8 @@ function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done)
                     refreshExpires = Date.now() + utils.ONE_WEEK_MS;
 
                 if (token.expires > Date.now()) {
-                    spamPrevention.userLogin.reset(null, authInfo.ip + body.refresh_token + 'login');
+                    spamPrevention.userLogin.reset(authInfo.ip, body.refresh_token + 'login');
+
                     models.Accesstoken.add({
                         token: accessToken,
                         user_id: token.user_id,
@@ -42,11 +43,12 @@ function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done)
 }
 // We are required to pass in authInfo in order to reset spam counter for user login
 function exchangePassword(client, username, password, scope, body, authInfo, done) {
-    // Validate the client
     models.Client.findOne({slug: client.slug})
         .then(function then(client) {
             if (!client) {
-                return done(new errors.NoPermissionError({message: i18n.t('errors.middleware.oauth.invalidClient')}), false);
+                return done(new errors.NoPermissionError({
+                    message: i18n.t('errors.middleware.oauth.invalidClient')
+                }), false);
             }
 
             // Validate the user
@@ -55,8 +57,7 @@ function exchangePassword(client, username, password, scope, body, authInfo, don
                     return authenticationAPI.createTokens({}, {context: {client_id: client.id, user: user.id}});
                 })
                 .then(function then(response) {
-                    // Reset spam count for username and IP pair
-                    spamPrevention.userLogin.reset(null, authInfo.ip + username + 'login');
+                    spamPrevention.userLogin.reset(authInfo.ip, username + 'login');
                     return done(null, response.access_token, response.refresh_token, {expires_in: response.expires_in});
                 });
         })
@@ -86,7 +87,7 @@ function exchangeAuthorizationCode(req, res, next) {
             }));
         }
 
-        spamPrevention.userLogin.reset(null, req.authInfo.ip + req.body.authorizationCode + 'login');
+        spamPrevention.userLogin.reset(req.authInfo.ip, req.body.authorizationCode + 'login');
 
         authenticationAPI.createTokens({}, {context: {client_id: req.client.id, user: user.id}})
             .then(function then(response) {
@@ -147,6 +148,17 @@ oauth = {
     // ### Generate access token Middleware
     // register the oauth2orize middleware for password and refresh token grants
     generateAccessToken: function generateAccessToken(req, res, next) {
+        /**
+         * TODO:
+         * https://github.com/jaredhanson/oauth2orize/issues/182
+         * oauth2orize only offers the option to forward request information via authInfo object
+         *
+         * Important: only used for resetting the brute count (access to req.ip)
+         */
+        req.authInfo = {
+            ip: req.ip
+        };
+
         return oauthServer.token()(req, res, next);
     }
 };

core/server/middleware/api/spam-prevention.js

@@ -74,7 +74,7 @@ globalReset = new ExpressBrute(store,
             }));
         },
         handleStoreError: handleStoreError
-    }, _.pick(spamGlobalBlock, spamConfigKeys))
+    }, _.pick(spamGlobalReset, spamConfigKeys))
 );
 
 // Stops login attempts for a user+IP pair with an increasing time period starting from 10 minutes

core/server/middleware/brute.js

@@ -1,63 +1,64 @@
-var spamPrevention = require('./api/spam-prevention');
+var url = require('url'),
+    spamPrevention = require('./api/spam-prevention');
 
+/**
+ * We set ignoreIP to false, because we tell brute-knex to use `req.ip`.
+ * We can use `req.ip`, because express trust proxy option is enabled.
+ */
 module.exports = {
+    /**
+     * block per route per ip
+     */
     globalBlock: spamPrevention.globalBlock.getMiddleware({
-        // We want to ignore req.ip and instead use req.connection.remoteAddress
-        ignoreIP: true,
+        ignoreIP: false,
         key: function (req, res, next) {
-            req.authInfo = req.authInfo || {};
-            req.authInfo.ip = req.connection.remoteAddress;
-            req.body.connection = req.connection.remoteAddress;
-
-            next(req.authInfo.ip);
+            next(url.parse(req.url).pathname);
         }
     }),
+    /**
+     * block per route per ip
+     */
     globalReset: spamPrevention.globalReset.getMiddleware({
-        ignoreIP: true,
+        ignoreIP: false,
         key: function (req, res, next) {
-            req.authInfo = req.authInfo || {};
-            req.authInfo.ip = req.connection.remoteAddress;
-            // prevent too many attempts for the same email address but keep separate to login brute force prevention
-            next(req.authInfo.ip);
+            next(url.parse(req.url).pathname);
         }
     }),
+    /**
+     * block per user
+     * username === email!
+     */
     userLogin: spamPrevention.userLogin.getMiddleware({
-        ignoreIP: true,
+        ignoreIP: false,
         key: function (req, res, next) {
-            req.authInfo = req.authInfo || {};
-            req.authInfo.ip = req.connection.remoteAddress || req.ip;
-            // prevent too many attempts for the same username
             if (req.body.username) {
-                return next(req.authInfo.ip + req.body.username + 'login');
+                return next(req.body.username + 'login');
             }
 
             if (req.body.authorizationCode) {
-                return next(req.authInfo.ip + req.body.authorizationCode + 'login');
+                return next(req.body.authorizationCode + 'login');
             }
 
             if (req.body.refresh_token) {
-                return next(req.authInfo.ip + req.body.refresh_token + 'login');
+                return next(req.body.refresh_token + 'login');
             }
 
             return next();
         }
     }),
+    /**
+     * block per user
+     */
     userReset: spamPrevention.userReset.getMiddleware({
-        ignoreIP: true,
+        ignoreIP: false,
         key: function (req, res, next) {
-            req.authInfo = req.authInfo || {};
-            req.authInfo.ip = req.connection.remoteAddress;
-            // prevent too many attempts for the same email address but keep separate to login brute force prevention
-            next(req.authInfo.ip + req.body.username + 'reset');
+            next(req.body.username + 'reset');
         }
     }),
     privateBlog: spamPrevention.privateBlog.getMiddleware({
-        ignoreIP: true,
+        ignoreIP: false,
         key: function (req, res, next) {
-            req.authInfo = req.authInfo || {};
-            req.authInfo.ip = req.connection.remoteAddress;
-            // prevent too many attempts for the same email address but keep separate to login brute force prevention
-            next(req.authInfo.ip + 'private');
+            next('privateblog');
         }
     })
 };

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

@@ -2,7 +2,10 @@ var supertest     = require('supertest'),
     should        = require('should'),
     testUtils     = require('../../../utils'),
     user          = testUtils.DataGenerator.forModel.users[0],
+    userForKnex   = testUtils.DataGenerator.forKnex.users[0],
+    models        = require('../../../../../core/server/models'),
     config        = require('../../../../../core/server/config'),
+    utils         = require('../../../../../core/server/utils'),
     ghost         = testUtils.startGhost,
     request;
 
@@ -127,7 +130,9 @@ describe('Authentication API', function () {
                 if (err) {
                     return done(err);
                 }
+
                 var refreshToken = res.body.refresh_token;
+
                 request.post(testUtils.API.getApiQuery('authentication/token'))
                     .set('Origin', config.get('url'))
                     .send({
@@ -173,4 +178,39 @@ describe('Authentication API', function () {
                 done();
             });
     });
+
+    it('reset password', function (done) {
+        models.Settings
+            .findOne({key: 'dbHash'})
+            .then(function (response) {
+                var token = utils.tokens.resetToken.generateHash({
+                    expires: Date.now() + (1000 * 60),
+                    email: user.email,
+                    dbHash: response.attributes.value,
+                    password: userForKnex.password
+                });
+
+                request.put(testUtils.API.getApiQuery('authentication/passwordreset'))
+                    .set('Origin', config.get('url'))
+                    .set('Accept', 'application/json')
+                    .send({
+                        passwordreset: [{
+                            token: token,
+                            newPassword: 'abcdefgh',
+                            ne2Password: 'abcdefgh'
+                        }]
+                    })
+                    .expect('Content-Type', /json/)
+                    .expect('Cache-Control', testUtils.cacheRules.private)
+                    .expect(200)
+                    .end(function (err) {
+                        if (err) {
+                            return done(err);
+                        }
+
+                        done();
+                    });
+            })
+            .catch(done);
+    });
 });

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

@@ -1,11 +1,14 @@
 var supertest     = require('supertest'),
     should        = require('should'),
     testUtils     = require('../../../utils'),
+    db            = require('../../../../../core/server/data/db'),
     config        = require('../../../../../core/server/config'),
     ghost         = testUtils.startGhost,
     failedLoginAttempt,
     count,
+    checkBruteTable,
     tooManyFailedLoginAttempts,
+    successLoginAttempt,
     request;
 
 describe('Spam Prevention API', function () {
@@ -57,7 +60,8 @@ describe('Spam Prevention API', function () {
                     password: 'wrong-password',
                     client_id: 'ghost-admin',
                     client_secret: 'not_available'
-                }).expect('Content-Type', /json/)
+                })
+                .expect('Content-Type', /json/)
                 .expect(429)
                 .end(function (err, res) {
                     if (err) {
@@ -84,12 +88,14 @@ describe('Spam Prevention API', function () {
                     password: 'wrong-password',
                     client_id: 'ghost-admin',
                     client_secret: 'not_available'
-                }).expect('Content-Type', /json/)
+                })
+                .expect('Content-Type', /json/)
                 .expect(401)
                 .end(function (err) {
                     if (err) {
                         return done(err);
                     }
+
                     if (count <  config.get('spam:user_login:freeRetries') + 1) {
                         return failedLoginAttempt(email);
                     }
@@ -160,4 +166,66 @@ describe('Spam Prevention API', function () {
 
         failedLoginAttempt(owner.email);
     });
+
+    it('Ensure reset works: password grant type', function (done) {
+        count = 0;
+
+        checkBruteTable = function checkBruteTable() {
+            return db.knex('brute').select();
+        };
+
+        successLoginAttempt = function successLoginAttempt(email) {
+            request.post(testUtils.API.getApiQuery('authentication/token'))
+                .set('Origin', config.get('url'))
+                .send({
+                    grant_type: 'password',
+                    username: email,
+                    password: 'Sl1m3rson',
+                    client_id: 'ghost-admin',
+                    client_secret: 'not_available'
+                }).expect('Content-Type', /json/)
+                .expect(200)
+                .end(function (err) {
+                    if (err) {
+                        return done(err);
+                    }
+
+                    checkBruteTable()
+                        .then(function (rows) {
+                            // if reset works, the key is deleted and only one key remains in the database
+                            // the one key is the key for global block
+                            rows.length.should.eql(1);
+                            done();
+                        });
+                });
+        };
+
+        failedLoginAttempt = function failedLoginAttempt(email) {
+            count += 1;
+
+            request.post(testUtils.API.getApiQuery('authentication/token'))
+                .set('Origin', config.get('url'))
+                .send({
+                    grant_type: 'password',
+                    username: email,
+                    password: 'wrong-password',
+                    client_id: 'ghost-admin',
+                    client_secret: 'not_available'
+                }).expect('Content-Type', /json/)
+                .expect(401)
+                .end(function (err) {
+                    if (err) {
+                        return done(err);
+                    }
+
+                    if (count < config.get('spam:user_login:freeRetries') - 1) {
+                        return failedLoginAttempt(email);
+                    }
+
+                    successLoginAttempt(email);
+                });
+        };
+
+        failedLoginAttempt(owner.email);
+    });
 });

core/test/integration/api/api_authentication_spec.js

@@ -348,7 +348,7 @@ describe('Authentication API', function () {
                 }).catch(done);
             });
 
-            it('should allow a password reset', function (done) {
+            it('should not allow a password reset', function (done) {
                 AuthAPI.resetPassword(testReset).then(function () {
                     done(new Error('password reset did not fail on token validation'));
                 }).catch(function (err) {