From 974c6071e828e049b50b9e5afff097fc945ca5f4 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Wed, 9 Nov 2016 11:47:35 +0100 Subject: [PATCH 1/7] =?UTF-8?q?=F0=9F=90=9B=20=20fix=20brute=20schema?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue - brute-knex uses timestamp type, see https://github.com/llambda/brute-knex/blob/master/index.js - so we also defined timestamp type - see http://stackoverflow.com/questions/9192027/invalid-default-value-for-create-date-timestamp-field - see http://stackoverflow.com/questions/35237278/mysql-invalid-default-value-for-timestamp-when-no-default-value-is-given - mysql does not allow a second timestamp without default value - we have to options: define a default value or allow null or use dateTime - let's use dateTime, as 1. we are using it for all our dates in Ghost and 2. it's recommended - read here http://www.sqlteam.com/article/timestamps-vs-datetime-data-types - there are some difference, i think the most important difference is that TIMESTAMP changes if the tz changes in your database - lifetime is timestamp not a bigInteger, this was a mistake i think (see https://github.com/llambda/brute-knex/blob/master/index.js#L115) --- core/server/data/schema/schema.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/server/data/schema/schema.js b/core/server/data/schema/schema.js index 791c02e9bc..3f5e495840 100644 --- a/core/server/data/schema/schema.js +++ b/core/server/data/schema/schema.js @@ -236,9 +236,9 @@ module.exports = { }, brute: { key: {type: 'string'}, - firstRequest: {type: 'timestamp'}, - lastRequest: {type: 'timestamp'}, - lifetime: {type: 'bigInteger'}, + firstRequest: {type: 'dateTime'}, + lastRequest: {type: 'dateTime'}, + lifetime: {type: 'dateTime'}, count: {type: 'integer'} } }; From 28fbaec49d1373302e7bfe6ecc36b6ba695a36ad Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Wed, 9 Nov 2016 12:13:51 +0100 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=9B=A0=20=20use=20tarball=20for=20bru?= =?UTF-8?q?te-knex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - current usage requires to have git installed - can cause trouble when deploying and git is not installed --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1dddb510a2..c3a04a2ebd 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "bluebird": "3.4.6", "body-parser": "1.15.2", "bookshelf": "0.10.2", - "brute-knex": "git://github.com/cobbspur/brute-knex.git#0985c50", + "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/0985c50b22f51d5745e6b54ed629edceb4e37c38", "bunyan": "1.8.1", "chalk": "1.1.3", "cheerio": "0.22.0", From c749fe2a45ec3b2cd56a944b4a65e119246b42ef Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Wed, 9 Nov 2016 20:55:45 +0100 Subject: [PATCH 3/7] =?UTF-8?q?=F0=9F=9B=A0=20=20brute-knex=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - do not access the database in the constructor - that causes Ghost to fail on mysql, because if the database does not exist, knex will fail --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c3a04a2ebd..36799fcaa0 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "bluebird": "3.4.6", "body-parser": "1.15.2", "bookshelf": "0.10.2", - "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/0985c50b22f51d5745e6b54ed629edceb4e37c38", + "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/0a7cf6265506e2be8151740d00518b53a53e6081", "bunyan": "1.8.1", "chalk": "1.1.3", "cheerio": "0.22.0", From cab46894f5d20551b260ba2865896cdd806f9608 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Wed, 9 Nov 2016 20:56:42 +0100 Subject: [PATCH 4/7] =?UTF-8?q?=F0=9F=90=9B=20=20do=20not=20return=20error?= =?UTF-8?q?=20on=20handleStoreError,=20throw=20it!?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ghost hangs if handleStoreError get's called - we have to throw the error! --- core/server/middleware/api/spam-prevention.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/server/middleware/api/spam-prevention.js b/core/server/middleware/api/spam-prevention.js index 8c48edda8d..3035a62c84 100644 --- a/core/server/middleware/api/spam-prevention.js +++ b/core/server/middleware/api/spam-prevention.js @@ -23,8 +23,12 @@ var ExpressBrute = require('express-brute'), logging = require('../../logging'), spamConfigKeys = ['freeRetries', 'minWait', 'maxWait', 'lifetime']; +// weird, but true handleStoreError = function handleStoreError(err) { - return new errors.NoPermissionError({message: 'DB error', err: err}); + err.next(new errors.NoPermissionError({ + message: 'Unknown error', + err: err.parent ? err.parent : err + })); }; // This is a global endpoint protection mechanism that will lock an endpoint if there are so many From 254b1a0632f918759d13ebcf8ba6c5c984a8c91a Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Thu, 10 Nov 2016 11:50:03 +0100 Subject: [PATCH 5/7] =?UTF-8?q?=F0=9F=90=9B=20=20fix=20bugs=20in=20brute-k?= =?UTF-8?q?nex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit see and read commit https://github.com/cobbspur/brute-knex/commit/0cb28fa8e3230dcbf6bca8b991dbb340b9fff6cc --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 36799fcaa0..0bee5eedd5 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "bluebird": "3.4.6", "body-parser": "1.15.2", "bookshelf": "0.10.2", - "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/0a7cf6265506e2be8151740d00518b53a53e6081", + "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/0cb28fa8e3230dcbf6bca8b991dbb340b9fff6cc", "bunyan": "1.8.1", "chalk": "1.1.3", "cheerio": "0.22.0", From 424f7fba0d66a3b4bedb7a807915463573108432 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Thu, 10 Nov 2016 11:50:23 +0100 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=8E=A8=20=20use=20bigInteger=20as=20t?= =?UTF-8?q?ype=20for=20brute=20schema?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - i thought of keeping our schema, because it might be less confusing - it's basically the same config brute-knex uses as default - see last commit why we are using this type definition --- core/server/data/schema/schema.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/server/data/schema/schema.js b/core/server/data/schema/schema.js index 3f5e495840..3b84da2452 100644 --- a/core/server/data/schema/schema.js +++ b/core/server/data/schema/schema.js @@ -236,9 +236,9 @@ module.exports = { }, brute: { key: {type: 'string'}, - firstRequest: {type: 'dateTime'}, - lastRequest: {type: 'dateTime'}, - lifetime: {type: 'dateTime'}, + firstRequest: {type: 'bigInteger'}, + lastRequest: {type: 'bigInteger'}, + lifetime: {type: 'bigInteger'}, count: {type: 'integer'} } }; From 049b26e67c31d2e1446368b24683eca0a586c013 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Thu, 10 Nov 2016 12:17:11 +0100 Subject: [PATCH 7/7] =?UTF-8?q?=F0=9F=90=9B=20=20err.next=20is=20not=20alw?= =?UTF-8?q?ays=20present?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - see https://github.com/AdamPflug/express-brute/issues/45 - we have to handle two cases ATM: with and without callback - in case we call the lib synchronous (which we should not actually), we will log the error so we get informed --- core/server/middleware/api/spam-prevention.js | 17 ++++++++++++++--- core/test/unit/auth/oauth_spec.js | 5 ++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/core/server/middleware/api/spam-prevention.js b/core/server/middleware/api/spam-prevention.js index 3035a62c84..b1165d7970 100644 --- a/core/server/middleware/api/spam-prevention.js +++ b/core/server/middleware/api/spam-prevention.js @@ -23,12 +23,23 @@ var ExpressBrute = require('express-brute'), logging = require('../../logging'), spamConfigKeys = ['freeRetries', 'minWait', 'maxWait', 'lifetime']; -// weird, but true handleStoreError = function handleStoreError(err) { - err.next(new errors.NoPermissionError({ + var customError = new errors.NoPermissionError({ message: 'Unknown error', err: err.parent ? err.parent : err - })); + }); + + // see https://github.com/AdamPflug/express-brute/issues/45 + // express-brute does not always forward a callback + // we are using reset as synchronous call, so we have to log the error if it occurs + // there is no way to try/catch, because the reset operation happens asynchronous + if (!err.next) { + err.level = 'critical'; + logging.error(err); + return; + } + + err.next(customError); }; // This is a global endpoint protection mechanism that will lock an endpoint if there are so many diff --git a/core/test/unit/auth/oauth_spec.js b/core/test/unit/auth/oauth_spec.js index 18465e8dcb..d52735570e 100644 --- a/core/test/unit/auth/oauth_spec.js +++ b/core/test/unit/auth/oauth_spec.js @@ -4,6 +4,7 @@ var sinon = require('sinon'), passport = require('passport'), testUtils = require('../../utils'), oAuth = require('../../../server/auth/oauth'), + spamPrevention = require('../../../server/middleware/api/spam-prevention'), api = require('../../../server/api'), errors = require('../../../server/errors'), models = require('../../../server/models'); @@ -20,6 +21,8 @@ describe('OAuth', function () { req = {}; res = {}; next = sandbox.spy(); + + sandbox.stub(spamPrevention.userLogin, 'reset'); }); afterEach(function () { @@ -32,7 +35,6 @@ describe('OAuth', function () { .returns(new Promise.resolve()); sandbox.stub(models.Refreshtoken, 'destroyAllExpired') .returns(new Promise.resolve()); - oAuth.init(); }); @@ -77,6 +79,7 @@ describe('OAuth', function () { json.should.have.property('expires_in'); json.should.have.property('token_type', 'Bearer'); next.called.should.eql(false); + spamPrevention.userLogin.reset.called.should.eql(true); done(); } catch (err) { done(err);