From 5c1a7a7349ea89a3aced4967167d314552121dda Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Thu, 17 Jul 2014 12:11:23 +0100 Subject: [PATCH] Revert "Restore spam prevention" --- core/server/errors/index.js | 10 +- core/server/middleware/middleware.js | 35 +--- core/server/middleware/oauth.js | 4 +- core/server/models/user.js | 9 +- core/server/routes/api.js | 1 - core/test/functional/client/signin_test.js | 227 ++++++++++----------- 6 files changed, 112 insertions(+), 174 deletions(-) diff --git a/core/server/errors/index.js b/core/server/errors/index.js index aaed368efb..75205dc3d6 100644 --- a/core/server/errors/index.js +++ b/core/server/errors/index.js @@ -245,15 +245,7 @@ errors = { } errors.renderErrorPage(err.status || 500, err, req, res, next); } else { - // generate a valid JSON response - var statusCode = 500, - errorContent = {}; - - statusCode = err.code || 500; - - errorContent.message = _.isString(err) ? err : (_.isObject(err) ? err.message : 'Unknown Error'); - errorContent.type = err.type || 'InternalServerError'; - res.json(statusCode, errorContent); + res.send(err.status || 500, err); } } }; diff --git a/core/server/middleware/middleware.js b/core/server/middleware/middleware.js index 0d6a68513d..d6fb889b9c 100644 --- a/core/server/middleware/middleware.js +++ b/core/server/middleware/middleware.js @@ -9,13 +9,11 @@ var _ = require('lodash'), path = require('path'), api = require('../api'), passport = require('passport'), - errors = require('../errors'), expressServer, oauthServer, ONE_HOUR_MS = 60 * 60 * 1000, - ONE_YEAR_MS = 365 * 24 * ONE_HOUR_MS, - loginSecurity = []; + ONE_YEAR_MS = 365 * 24 * ONE_HOUR_MS; function isBlackListedFileType(file) { var blackListedFileTypes = ['.hbs', '.md', '.json'], @@ -151,31 +149,6 @@ var middleware = { }); }, - // ### Spam prevention Middleware - // limit signin requests to one every two seconds - spamPrevention: function (req, res, next) { - var currentTime = process.hrtime()[0], - remoteAddress = req.connection.remoteAddress, - denied = ''; - - // filter for IPs that tried to login in the last 2 sec - loginSecurity = _.filter(loginSecurity, function (logTime) { - return (logTime.time + 2 > currentTime); - }); - - // check if IP tried to login in the last 2 sec - denied = _.find(loginSecurity, function (logTime) { - return (logTime.ip === remoteAddress); - }); - - if (!denied) { - loginSecurity.push({ip: remoteAddress, time: currentTime}); - next(); - } else { - return next(new errors.UnauthorizedError('Slow down, there are way too many login attempts!')); - } - }, - // work around to handle missing client_secret // oauth2orize needs it, but untrusted clients don't have it addClientSecret: function (req, res, next) { @@ -184,15 +157,9 @@ var middleware = { } next(); }, - - // ### Authenticate Client Middleware - // authenticate client that is asking for an access token authenticateClient: function (req, res, next) { return passport.authenticate(['oauth2-client-password'], { session: false })(req, res, next); }, - - // ### Generate access token Middleware - // register the oauth2orize middleware for password and refresh token grants generateAccessToken: function (req, res, next) { return oauthServer.token()(req, res, next); }, diff --git a/core/server/middleware/oauth.js b/core/server/middleware/oauth.js index c97dd48d5e..5695c2257b 100644 --- a/core/server/middleware/oauth.js +++ b/core/server/middleware/oauth.js @@ -43,8 +43,8 @@ oauth = { }).catch(function () { return done(null, false); }); - }).catch(function (error) { - return done(error); + }).catch(function () { + return done(null, false); }); }); })); diff --git a/core/server/models/user.js b/core/server/models/user.js index 250dd0c19c..39585ab25a 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -270,16 +270,15 @@ User = ghostBookshelf.Model.extend({ var self = this, s; return this.getByEmail(object.email).then(function (user) { - if (!user || user.get('status') === 'invited' || user.get('status') === 'invited-pending' - || user.get('status') === 'inactive') { - return when.reject(new errors.NotFoundError('NotFound')); + if (!user || user.get('status') === 'invited' || user.get('status') === 'inactive') { + return when.reject(new Error('NotFound')); } if (user.get('status') !== 'locked') { return nodefn.call(bcrypt.compare, object.password, user.get('password')).then(function (matched) { if (!matched) { return when(self.setWarning(user)).then(function (remaining) { s = (remaining > 1) ? 's' : ''; - return when.reject(new errors.UnauthorizedError('Your password is incorrect.
' + + return when.reject(new Error('Your password is incorrect.
' + remaining + ' attempt' + s + ' remaining!')); }); } @@ -289,7 +288,7 @@ User = ghostBookshelf.Model.extend({ }); }, errors.logAndThrowError); } - return when.reject(new errors.NoPermissionError('Your account is locked due to too many ' + + return when.reject(new Error('Your account is locked due to too many ' + 'login attempts. Please reset your password to log in again by clicking ' + 'the "Forgotten password?" link!')); diff --git a/core/server/routes/api.js b/core/server/routes/api.js index 4d549c5566..07f2f88e53 100644 --- a/core/server/routes/api.js +++ b/core/server/routes/api.js @@ -73,7 +73,6 @@ apiRoutes = function (middleware) { router.post('/authentication/setup', api.http(api.authentication.setup)); router.get('/authentication/setup', api.http(api.authentication.isSetup)); router.post('/authentication/token', - middleware.spamPrevention, middleware.addClientSecret, middleware.authenticateClient, middleware.generateAccessToken diff --git a/core/test/functional/client/signin_test.js b/core/test/functional/client/signin_test.js index ef6ed6b791..687263ec4f 100644 --- a/core/test/functional/client/signin_test.js +++ b/core/test/functional/client/signin_test.js @@ -4,35 +4,35 @@ /*globals CasperTest, casper, url, newUser, user, falseUser */ // TODO fix signup vs setup testing -CasperTest.begin('Ensure a User is Registered', 3, function suite(test) { - casper.thenOpenAndWaitForPageLoad('setup', function checkUrl() { - test.assertUrlMatch(/ghost\/setup\/$/, 'Landed on the correct URL'); - }); - - casper.waitForOpaque('.setup-box', - function then() { - this.fillAndAdd('#setup', newSetup); - }, - function onTimeout() { - test.fail('Set up form didn\'t fade in.'); - }); - - casper.captureScreenshot('login_register_test.png'); - - casper.waitForSelectorTextChange('.notification-error', function onSuccess() { - test.assertSelectorHasText('.notification-error', 'already registered'); - // If the previous assert succeeds, then we should skip the next check and just pass. - casper.echoConcise('Already registered!'); - casper.captureScreenshot('already_registered.png'); - }, function onTimeout() { - test.assertUrlMatch(/ghost\/\d+\/$/, 'If we\'re not already registered, we should be logged in.'); - casper.echoConcise('Successfully registered.'); - }, 2000); - - casper.thenOpenAndWaitForPageLoad('signout', function then() { - test.assertUrlMatch(/ghost\/signin/, 'We got redirected to signin page.'); - }); -}, true); +//CasperTest.begin('Ensure a User is Registered', 3, function suite(test) { +// casper.thenOpenAndWaitForPageLoad('signup', function checkUrl() { +// test.assertUrlMatch(/ghost\/signup\/$/, 'Landed on the correct URL'); +// }); +// +// casper.waitForOpaque('.signup-box', +// function then() { +// this.fillAndSave('#signup', newUser); +// }, +// function onTimeout() { +// test.fail('Sign up form didn\'t fade in.'); +// }); +// +// casper.captureScreenshot('login_register_test.png'); +// +// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { +// test.assertSelectorHasText('.notification-error', 'already registered'); +// // If the previous assert succeeds, then we should skip the next check and just pass. +// casper.echoConcise('Already registered!'); +// casper.captureScreenshot('already_registered.png'); +// }, function onTimeout() { +// test.assertUrlMatch(/ghost\/\d+\/$/, 'If we\'re not already registered, we should be logged in.'); +// casper.echoConcise('Successfully registered.'); +// }, 2000); +// +// casper.thenOpenAndWaitForPageLoad('signout', function then() { +// test.assertUrlMatch(/ghost\/signin/, 'We got redirected to signin page.'); +// }); +//}, true); CasperTest.begin('Ghost admin will load login page', 3, function suite(test) { casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { @@ -59,84 +59,73 @@ CasperTest.begin('Redirects login to signin', 2, function suite(test) { }); }, true); +// TODO: please uncomment when the spam prevention bug is fixed (https://github.com/TryGhost/Ghost/issues/3128) +// CasperTest.begin('Can\'t spam it', 4, function suite(test) { +// casper.thenOpenAndWaitForPageLoad('signin', function testTitle() { +// test.assertTitle('Ghost Admin', 'Ghost admin has no title'); +// test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); +// }); -CasperTest.begin('Can\'t spam it', 4, function suite(test) { - // init user to prevent redirect to setup - CasperTest.Routines.setup.run(test); - CasperTest.Routines.signout.run(test); - - casper.thenOpenAndWaitForPageLoad('signin', function testTitle() { - test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); - }); - - casper.waitForOpaque('.login-box', - function then() { - this.fillAndSave('#login', falseUser); - }, - function onTimeout() { - test.fail('Sign in form didn\'t fade in.'); - }); +// casper.waitForOpaque('.login-box', +// function then() { +// this.fillAndSave('#login', falseUser); +// }, +// function onTimeout() { +// test.fail('Sign in form didn\'t fade in.'); +// }); - casper.captureScreenshot('login_spam_test.png'); +// casper.captureScreenshot('login_spam_test.png'); - casper.waitForText('attempts remaining!', function then() { - this.fillAndSave('#login', falseUser); - }); +// casper.waitForText('attempts remaining!', function then() { +// this.fillAndSave('#login', falseUser); +// }); - casper.captureScreenshot('login_spam_test2.png'); +// casper.captureScreenshot('login_spam_test2.png'); - casper.waitForText('Slow down, there are way too many login attempts!', function onSuccess() { - test.assert(true, 'Spamming the login did result in an error notification'); - test.assertSelectorDoesntHaveText('.notification-error', '[object Object]'); - }, function onTimeout() { - test.assert(false, 'Spamming the login did not result in an error notification'); - }); +// casper.waitForText('Slow down, there are way too many login attempts!', function onSuccess() { +// test.assert(true, 'Spamming the login did result in an error notification'); +// test.assertSelectorDoesntHaveText('.notification-error', '[object Object]'); +// }, function onTimeout() { +// test.assert(false, 'Spamming the login did not result in an error notification'); +// }); - // This test causes the spam notification - // add a wait to ensure future tests don't get tripped up by this. - casper.wait(2000); -}, true); +// // This test causes the spam notification +// // add a wait to ensure future tests don't get tripped up by this. +// casper.wait(2000); +// }, true); -CasperTest.begin('Login limit is in place', 4, function suite(test) { - // init user to prevent redirect to setup - CasperTest.Routines.setup.run(test); - CasperTest.Routines.signout.run(test); +// TODO: please uncomment when the spam prevention bug is fixed (https://github.com/TryGhost/Ghost/issues/3128) +// CasperTest.begin('Login limit is in place', 4, function suite(test) { +// casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { +// test.assertTitle('Ghost Admin', 'Ghost admin has no title'); +// test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); +// }); - casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { - test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); - }); +// casper.waitForOpaque('.login-box', +// function then() { +// this.fillAndSave('#login', falseUser); +// }, +// function onTimeout() { +// test.fail('Sign in form didn\'t fade in.'); +// }); - casper.waitForOpaque('.login-box', - function then() { - this.fillAndSave('#login', falseUser); - }, - function onTimeout() { - test.fail('Sign in form didn\'t fade in.'); - }); +// casper.wait(2100, function doneWait() { +// this.fillAndSave('#login', falseUser); +// }); - casper.wait(2100, function doneWait() { - this.fillAndSave('#login', falseUser); - }); - - casper.waitForText('remaining', function onSuccess() { - test.assert(true, 'The login limit is in place.'); - test.assertSelectorDoesntHaveText('.notification-error', '[object Object]'); - }, function onTimeout() { - test.assert(false, 'We did not trip the login limit.'); - }); - // This test used login, add a wait to - // ensure future tests don't get tripped up by this. - casper.wait(2000); -}, true); +// casper.waitForText('remaining', function onSuccess() { +// test.assert(true, 'The login limit is in place.'); +// test.assertSelectorDoesntHaveText('.notification-error', '[object Object]'); +// }, function onTimeout() { +// test.assert(false, 'We did not trip the login limit.'); +// }); +// // This test used login, add a wait to +// // ensure future tests don't get tripped up by this. +// casper.wait(2000); +// }, true); CasperTest.begin('Can login to Ghost', 5, function suite(test) { - // init user - CasperTest.Routines.setup.run(test); - CasperTest.Routines.signout.run(test); - casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); @@ -158,10 +147,6 @@ CasperTest.begin('Can login to Ghost', 5, function suite(test) { }, true); CasperTest.begin('Authenticated user is redirected', 8, function suite(test) { - // init user - CasperTest.Routines.setup.run(test); - CasperTest.Routines.signout.run(test); - casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); @@ -190,31 +175,27 @@ CasperTest.begin('Authenticated user is redirected', 8, function suite(test) { }); }, true); +// TODO: please uncomment when the validation problem is fixed (https://github.com/TryGhost/Ghost/issues/3120) +// CasperTest.begin('Ensure email field form validation', 3, function suite(test) { +// casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { +// test.assertTitle('Ghost Admin', 'Ghost admin has no title'); +// test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); +// }); -CasperTest.begin('Ensure email field form validation', 3, function suite(test) { - // init user to prevent redirect to setup - CasperTest.Routines.setup.run(test); - CasperTest.Routines.signout.run(test); +// casper.waitForOpaque('.js-login-box', +// function then() { +// this.fillAndSave('form.login-form', { +// 'email': 'notanemail' +// }); +// }, +// function onTimeout() { +// test.fail('Login form didn\'t fade in.'); +// }); - casper.thenOpenAndWaitForPageLoad('signin', function testTitleAndUrl() { - test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/signin\/$/, 'Landed on the correct URL'); - }); +// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { +// test.assertSelectorHasText('.notification-error', 'Invalid Email'); +// }, function onTimeout() { +// test.fail('Email validation error did not appear'); +// }, 2000); - casper.waitForOpaque('.js-login-box', - function then() { - this.fillAndSave('form.login-form', { - 'identification': 'notanemail' - }); - }, - function onTimeout() { - test.fail('Login form didn\'t fade in.'); - }); - - casper.waitForSelectorTextChange('.notification-error', function onSuccess() { - test.assertSelectorHasText('.notification-error', 'Invalid Email'); - }, function onTimeout() { - test.fail('Email validation error did not appear'); - }, 2000); - -}, true); +// }, true);