Skip to content

Commit

Permalink
Fix brute for token exchanges (#7725)
Browse files Browse the repository at this point in the history 
closes #7722

- fixes issue where token exhanges are logged with an undefined email address causing lockouts
- use more relevant translations for errors
  • Loading branch information
@cobbspur @kirrg001
cobbspur authored and kirrg001 committed Nov 17, 2016
1 parent 7eb316b commit e2bbf7d
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 25 deletions.
6 changes: 4 additions & 2 deletions core/server/auth/oauth.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var oauth2orize = require('oauth2orize'),
oauthServer,
oauth;

function exchangeRefreshToken(client, refreshToken, scope, done) {
function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done) {
models.Refreshtoken.findOne({token: refreshToken})
.then(function then(model) {
if (!model) {
Expand All @@ -21,6 +21,7 @@ function exchangeRefreshToken(client, refreshToken, scope, done) {
refreshExpires = Date.now() + utils.ONE_WEEK_MS;

if (token.expires > Date.now()) {
spamPrevention.userLogin.reset(null, authInfo.ip + body.refresh_token + 'login');
models.Accesstoken.add({
token: accessToken,
user_id: token.user_id,
Expand Down Expand Up @@ -70,7 +71,6 @@ function exchangeAuthorizationCode(req, res, next) {
message: i18n.t('errors.middleware.auth.accessDenied')
}));
}

req.query.code = req.body.authorizationCode;

passport.authenticate('ghost', {session: false, failWithError: false}, function authenticate(err, user) {
Expand All @@ -86,6 +86,8 @@ function exchangeAuthorizationCode(req, res, next) {
}));
}

spamPrevention.userLogin.reset(null, req.authInfo.ip + req.body.authorizationCode + 'login');

authenticationAPI.createTokens({}, {context: {client_id: req.client.id, user: user.id}})
.then(function then(response) {
res.json({
Expand Down
22 changes: 11 additions & 11 deletions core/server/middleware/api/spam-prevention.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ globalBlock = new ExpressBrute(store,
message: 'Too many attempts try again in ' + moment(nextValidRequestDate).fromNow(true),
context: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.error',
{rfa: spamGlobalBlock.freeRetries + 1 || 5, rfp: spamGlobalBlock.lifetime || 60 * 60}),
help: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.context')
help: i18n.t('errors.middleware.spamprevention.tooManyAttempts')
}));
},
handleStoreError: handleStoreError
Expand Down Expand Up @@ -86,10 +86,10 @@ userLogin = new ExpressBrute(store,
attachResetToRequest: true,
failCallback: function (req, res, next, nextValidRequestDate) {
return next(new errors.TooManyRequestsError({
message: 'Too many attempts try again in ' + moment(nextValidRequestDate).fromNow(true),
context: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.error',
{rfa: spamUserLogin.freeRetries + 1 || 5, rfp: spamUserLogin.lifetime || 60 * 60}),
help: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.context')
message: 'Too many sign-in attempts try again in ' + moment(nextValidRequestDate).fromNow(true),
// TODO add more options to i18n
context: i18n.t('errors.middleware.spamprevention.tooManySigninAttempts.context'),
help: i18n.t('errors.middleware.spamprevention.tooManySigninAttempts.context')
}));
},
handleStoreError: handleStoreError
Expand All @@ -104,10 +104,10 @@ userReset = new ExpressBrute(store,
attachResetToRequest: true,
failCallback: function (req, res, next, nextValidRequestDate) {
return next(new errors.TooManyRequestsError({
message: 'Too many attempts try again in ' + moment(nextValidRequestDate).fromNow(true),
context: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.error',
message: 'Too many password reset attempts try again in ' + moment(nextValidRequestDate).fromNow(true),
context: i18n.t('errors.middleware.spamprevention.forgottenPasswordEmail.error',
{rfa: spamUserReset.freeRetries + 1 || 5, rfp: spamUserReset.lifetime || 60 * 60}),
help: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.context')
help: i18n.t('errors.middleware.spamprevention.forgottenPasswordEmail.context')
}));
},
handleStoreError: handleStoreError
Expand All @@ -121,13 +121,13 @@ privateBlog = new ExpressBrute(store,
attachResetToRequest: false,
failCallback: function (req, res, next, nextValidRequestDate) {
logging.error(new errors.GhostError({
message: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.error',
message: i18n.t('errors.middleware.spamprevention.tooManySigninAttempts.error',
{rfa: spamPrivateBlog.freeRetries + 1 || 5, rfp: spamPrivateBlog.lifetime || 60 * 60}),
context: i18n.t('errors.middleware.spamprevention.forgottenPasswordIp.context')
context: i18n.t('errors.middleware.spamprevention.tooManySigninAttempts.context')
}));

return next(new errors.GhostError({
message: 'Too many attempts try again in ' + moment(nextValidRequestDate).fromNow(true)
message: 'Too many private sign-in attempts try again in ' + moment(nextValidRequestDate).fromNow(true)
}));
},
handleStoreError: handleStoreError
Expand Down
17 changes: 15 additions & 2 deletions core/server/middleware/brute.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module.exports = {
req.authInfo = req.authInfo || {};
req.authInfo.ip = req.connection.remoteAddress;
req.body.connection = req.connection.remoteAddress;

next(req.authInfo.ip);
}
}),
Expand All @@ -24,9 +25,21 @@ module.exports = {
ignoreIP: true,
key: function (req, res, next) {
req.authInfo = req.authInfo || {};
req.authInfo.ip = req.connection.remoteAddress;
req.authInfo.ip = req.connection.remoteAddress || req.ip;
// prevent too many attempts for the same username
next(req.authInfo.ip + req.body.username + 'login');
if (req.body.username) {
return next(req.authInfo.ip + req.body.username + 'login');
}

if (req.body.authorizationCode) {
return next(req.authInfo.ip + req.body.authorizationCode + 'login');
}

if (req.body.refresh_token) {
return next(req.authInfo.ip + req.body.refresh_token + 'login');
}

return next();
}
}),
userReset: spamPrevention.userReset.getMiddleware({
Expand Down
4 changes: 2 additions & 2 deletions core/test/functional/routes/api/spam_prevention_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('Spam Prevention API', function () {
}).then(function (user) {
author = user;
done();
}).then(testUtils.clearBruteData)
})
.catch(done);
});

Expand Down Expand Up @@ -67,7 +67,7 @@ describe('Spam Prevention API', function () {
var error = res.body.errors[0];
should.exist(error.errorType);
error.errorType.should.eql('TooManyRequestsError');
error.message.should.eql('Too many attempts try again in 10 minutes');
error.message.should.eql('Too many sign-in attempts try again in 10 minutes');

done();
});
Expand Down
25 changes: 17 additions & 8 deletions core/test/unit/auth/oauth_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ describe('OAuth', function () {
req.client = {
slug: 'test'
};
req.authInfo = {};
req.authInfo.ip = '127.0.0.1';
req.connection = {remoteAddress: '127.0.0.1'};
req.authInfo = {ip: '127.0.0.1'};

req.body.grant_type = 'password';
req.body.username = 'username';
Expand Down Expand Up @@ -95,6 +95,7 @@ describe('OAuth', function () {
slug: 'test'
};

req.authInfo = {ip: '127.0.0.1'};
req.body.grant_type = 'password';
req.body.username = 'username';
req.body.password = 'password';
Expand All @@ -116,6 +117,7 @@ describe('OAuth', function () {
slug: 'test'
};

req.authInfo = {ip: '127.0.0.1'};
req.body.grant_type = 'password';
req.body.username = 'username';
req.body.password = 'password';
Expand Down Expand Up @@ -159,7 +161,8 @@ describe('OAuth', function () {
req.client = {
slug: 'test'
};

req.authInfo = {ip: '127.0.0.1'};
req.connection = {remoteAddress: '127.0.0.1'};
req.body.grant_type = 'refresh_token';
req.body.refresh_token = 'token';
res.setHeader = {};
Expand Down Expand Up @@ -204,7 +207,8 @@ describe('OAuth', function () {
req.client = {
slug: 'test'
};

req.connection = {remoteAddress: '127.0.0.1'};
req.authInfo = {ip: '127.0.0.1'};
req.body.grant_type = 'refresh_token';
req.body.refresh_token = 'token';
res.setHeader = {};
Expand All @@ -224,7 +228,8 @@ describe('OAuth', function () {
req.client = {
slug: 'test'
};

req.connection = {remoteAddress: '127.0.0.1'};
req.authInfo = {ip: '127.0.0.1'};
req.body.grant_type = 'refresh_token';
req.body.refresh_token = 'token';
res.setHeader = {};
Expand All @@ -250,7 +255,8 @@ describe('OAuth', function () {
req.client = {
slug: 'test'
};

req.connection = {remoteAddress: '127.0.0.1'};
req.authInfo = {ip: '127.0.0.1'};
req.body.grant_type = 'refresh_token';
req.body.refresh_token = 'token';
res.setHeader = {};
Expand Down Expand Up @@ -296,7 +302,8 @@ describe('OAuth', function () {
req.client = {
id: 1
};

req.authInfo = {ip: '127.0.0.1'};
req.connection = {remoteAddress: '127.0.0.1'};
req.body.grant_type = 'authorization_code';
req.body.authorizationCode = '1234';

Expand Down Expand Up @@ -329,6 +336,8 @@ describe('OAuth', function () {
id: 1
};

req.authInfo = {ip: '127.0.0.1'};
req.connection = {remoteAddress: '127.0.0.1'};
req.body.grant_type = 'authorization_code';
req.body.authorizationCode = '1234';

Expand All @@ -351,7 +360,7 @@ describe('OAuth', function () {
req.client = {
id: 1
};

req.connection = {remoteAddress: '127.0.0.1'};
req.body.grant_type = 'authorization_code';

oAuth.generateAccessToken(req, res, function (err) {
Expand Down

0 comments on commit e2bbf7d

Please sign in to comment.