From 4b579be93e8ca7f8141ec5d3e8e70a4b9211159c Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 09:41:28 +0200 Subject: [PATCH 1/7] Adding the first few lines of user model test Signed-off-by: Claudius --- package.json | 3 ++- test/user.js | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 test/user.js diff --git a/package.json b/package.json index 980241f..fdbe613 100644 --- a/package.json +++ b/package.json @@ -5,9 +5,10 @@ "main": "app.js", "license": "AGPL-3.0", "scripts": { - "test": "npm run-script eslint && npm run-script jsonlint && mocha", + "test": "npm run-script eslint && npm run-script jsonlint && npm run-script mocha-suite", "eslint": "node_modules/.bin/eslint lib public test app.js", "jsonlint": "find . -not -path './node_modules/*' -type f -name '*.json' -o -type f -name '*.json.example' | while read json; do echo $json ; jq . $json; done", + "mocha-suite": "NODE_ENV=test CMD_DB_URL=\"sqlite::memory:\" mocha --exit", "standard": "echo 'standard is no longer being used, use `npm run eslint` instead!' && exit 1", "dev": "webpack --config webpack.dev.js --progress --colors --watch", "heroku-prebuild": "bin/heroku", diff --git a/test/user.js b/test/user.js new file mode 100644 index 0000000..6159ebf --- /dev/null +++ b/test/user.js @@ -0,0 +1,37 @@ +/* eslint-env node, mocha */ + +'use strict' + +const assert = require('assert') + +const models = require('../lib/models') +const User = models.User + +describe('User Sequelize model', function () { + beforeEach(() => { + return models.sequelize.sync({ force: true }) + }) + + it('stores a password hash on creation and verifies that password', function () { + const userData = { + password: 'test123' + } + const intentionallyInvalidPassword = 'stuff' + + return User.create(userData).then(u => { + assert(u.verifyPassword(userData.password)) + assert(!u.verifyPassword(intentionallyInvalidPassword)) + }) + }) + + it('can cope with password stored in standard scrypt header format', function () { + const testKey = '736372797074000e00000008000000018c7b8c1ac273fd339badde759b3efc418bc61b776debd02dfe95989383cf9980ad21d2403dce33f4b551f5e98ce84edb792aee62600b1303ab8d4e6f0a53b0746e73193dbf557b888efc83a2d6a055a9' + const validPassword = 'test' + const intentionallyInvalidPassword = 'stuff' + + const u = User.build() + u.setDataValue('password', testKey) // this circumvents the setter - which we don't need in this case! + assert(u.verifyPassword(validPassword)) + assert(!u.verifyPassword(intentionallyInvalidPassword)) + }) +}) From df666dd2140c8955765972230260d6e4bd5de42a Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 10:55:37 +0200 Subject: [PATCH 2/7] getting password hashing into a hook where it could be async Signed-off-by: Claudius --- lib/models/user.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/models/user.js b/lib/models/user.js index 648db73..bcf3c09 100644 --- a/lib/models/user.js +++ b/lib/models/user.js @@ -5,7 +5,7 @@ var scrypt = require('@mlink/scrypt') // core var logger = require('../logger') -var {generateAvatarURL} = require('../letter-avatars') +var { generateAvatarURL } = require('../letter-avatars') module.exports = function (sequelize, DataTypes) { var User = sequelize.define('User', { @@ -41,11 +41,7 @@ module.exports = function (sequelize, DataTypes) { } }, password: { - type: Sequelize.TEXT, - set: function (value) { - var hash = scrypt.kdfSync(value, scrypt.paramsSync(0.1)).toString('hex') - this.setDataValue('password', hash) - } + type: Sequelize.TEXT } }, { instanceMethods: { @@ -153,5 +149,17 @@ module.exports = function (sequelize, DataTypes) { } }) + function updatePasswordHashHook (user, options, done) { + // suggested way to hash passwords to be able to do this asynchronously: + // @see https://github.com/sequelize/sequelize/issues/1821#issuecomment-44265819 + if (!user.changed('password')) { return done() } + const hash = scrypt.kdfSync(user.get('password'), scrypt.paramsSync(0.1)).toString('hex') + user.setDataValue('password', hash) + done() + } + + User.beforeCreate(updatePasswordHashHook) + User.beforeUpdate(updatePasswordHashHook) + return User } From 1d403e183d50001bf0f20113d15994007b14696b Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 11:51:05 +0200 Subject: [PATCH 3/7] asyncified setting and verifying the password Signed-off-by: Claudius --- docs/setup/manual-setup.md | 1 - lib/models/user.js | 16 +++++++--------- lib/web/auth/email/index.js | 10 ++++++++-- package.json | 4 ++-- test/user.js | 35 +++++++++++++++++++++++++++++++---- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/docs/setup/manual-setup.md b/docs/setup/manual-setup.md index 82ed085..b612853 100644 --- a/docs/setup/manual-setup.md +++ b/docs/setup/manual-setup.md @@ -7,7 +7,6 @@ Manual Installation - Database (PostgreSQL, MySQL, MariaDB, SQLite, MSSQL) use charset `utf8` - npm (and its dependencies, [node-gyp](https://github.com/nodejs/node-gyp#installation)) - yarn -- `libssl-dev` for building scrypt (see [here](https://github.com/ml1nk/node-scrypt/blob/master/README.md#installation-instructions) for further information) - Bash (for the setup script) - For **building** CodiMD we recommend to use a machine with at least **2GB** RAM diff --git a/lib/models/user.js b/lib/models/user.js index bcf3c09..8da12b8 100644 --- a/lib/models/user.js +++ b/lib/models/user.js @@ -1,7 +1,7 @@ 'use strict' // external modules var Sequelize = require('sequelize') -var scrypt = require('@mlink/scrypt') +var scrypt = require('scrypt-kdf') // core var logger = require('../logger') @@ -46,11 +46,7 @@ module.exports = function (sequelize, DataTypes) { }, { instanceMethods: { verifyPassword: function (attempt) { - if (scrypt.verifyKdfSync(Buffer.from(this.password, 'hex'), attempt)) { - return this - } else { - return false - } + return scrypt.verify(Buffer.from(this.password, 'hex'), attempt) } }, classMethods: { @@ -153,9 +149,11 @@ module.exports = function (sequelize, DataTypes) { // suggested way to hash passwords to be able to do this asynchronously: // @see https://github.com/sequelize/sequelize/issues/1821#issuecomment-44265819 if (!user.changed('password')) { return done() } - const hash = scrypt.kdfSync(user.get('password'), scrypt.paramsSync(0.1)).toString('hex') - user.setDataValue('password', hash) - done() + + scrypt.kdf(user.getDataValue('password'), { logN: 15 }).then(keyBuf => { + user.setDataValue('password', keyBuf.toString('hex')) + done() + }) } User.beforeCreate(updatePasswordHashHook) diff --git a/lib/web/auth/email/index.js b/lib/web/auth/email/index.js index f7e58d4..daa4a8c 100644 --- a/lib/web/auth/email/index.js +++ b/lib/web/auth/email/index.js @@ -23,8 +23,14 @@ passport.use(new LocalStrategy({ } }).then(function (user) { if (!user) return done(null, false) - if (!user.verifyPassword(password)) return done(null, false) - return done(null, user) + user.verifyPassword(password).then(verified => { + if (verified) { + return done(null, user) + } else { + logger.warn('invalid password given for %s', user.email) + return done(null, false) + } + }) }).catch(function (err) { logger.error(err) return done(err) diff --git a/package.json b/package.json index fdbe613..8cfcfe6 100644 --- a/package.json +++ b/package.json @@ -58,7 +58,6 @@ "jquery-ui": "^1.12.1", "js-cookie": "^2.1.3", "js-sequence-diagrams": "git+https://github.com/codimd/js-sequence-diagrams.git", - "wurl": "^2.5.3", "js-yaml": "^3.13.1", "jsdom-nogyp": "^0.8.3", "keymaster": "^1.6.2", @@ -111,7 +110,7 @@ "readline-sync": "^1.4.7", "request": "^2.88.0", "reveal.js": "~3.7.0", - "@mlink/scrypt": "^6.1.2", + "scrypt-kdf": "^2.0.1", "select2": "^3.5.2-browserify", "sequelize": "^3.28.0", "sequelize-cli": "^2.5.1", @@ -132,6 +131,7 @@ "viz.js": "^1.7.0", "winston": "^3.1.0", "ws": "^6.0.0", + "wurl": "^2.5.3", "xss": "^1.0.3" }, "resolutions": { diff --git a/test/user.js b/test/user.js index 6159ebf..f2c8a22 100644 --- a/test/user.js +++ b/test/user.js @@ -19,8 +19,10 @@ describe('User Sequelize model', function () { const intentionallyInvalidPassword = 'stuff' return User.create(userData).then(u => { - assert(u.verifyPassword(userData.password)) - assert(!u.verifyPassword(intentionallyInvalidPassword)) + return Promise.all([ + u.verifyPassword(userData.password).then(result => assert.strictEqual(result, true)), + u.verifyPassword(intentionallyInvalidPassword).then(result => assert.strictEqual(result, false)) + ]).catch(e => assert.fail(e)) }) }) @@ -31,7 +33,32 @@ describe('User Sequelize model', function () { const u = User.build() u.setDataValue('password', testKey) // this circumvents the setter - which we don't need in this case! - assert(u.verifyPassword(validPassword)) - assert(!u.verifyPassword(intentionallyInvalidPassword)) + return Promise.all([ + u.verifyPassword(validPassword).then(result => assert.strictEqual(result, true)), + u.verifyPassword(intentionallyInvalidPassword).then(result => assert.strictEqual(result, false)) + ]).catch(e => assert.fail(e)) + }) + + it('deals with various characters correctly', function () { + const combinations = [ + // ['correct password', 'scrypt syle hash'] + ['test', '736372797074000e00000008000000018c7b8c1ac273fd339badde759b3efc418bc61b776debd02dfe95989383cf9980ad21d2403dce33f4b551f5e98ce84edb792aee62600b1303ab8d4e6f0a53b0746e73193dbf557b888efc83a2d6a055a9'], + ['my secret pw', /* different hash! */ '736372797074000f0000000800000001f6083e9593365acd07550f7c72f19973fb7d52c3ef0a78026ff66c48ab14493843c642167b5e6b7f31927e8eeb912bc2639e41955fae15da5099998948cfeacd022f705624931c3b30104e6bb296b805'], + ['ohai', '736372797074000e00000008000000010efec4e5ce6a5294491f1b1cccc38d3562f84844b9271aef635f8bc338cf4e0e0bac62ebb11379e85894c1f694e038fc39b087b4fdacd1280b50a7382d7ffbfc82f2190bef70d47708d2a94b75126294'], + ['my secret pw', '736372797074000f0000000800000001ffb4cd10a1dfe9e64c1e5416fd6d55b390b6822e78b46fd1f963fe9f317a1e05f9c5fee15e1f618286f4e38b55364ae1e7dc295c9dc33ee0f5712e86afe37e5784ff9c7cf84cf0e631dd11f84f3621e7'], + ['i am so extremely long, it\'s not even funny. Wait, you\'re still reading?', '736372797074000f00000008000000012d205f7bb529bb3a8b8bb25f5ab46197c7e9baf1aad64cf5e7b2584c84748cacf5e60631d58d21cb51fa34ea93b517e2fe2eb722931db5a70ff5a1330d821288ee7380c4136369f064b71b191a785a5b'] + ] + const intentionallyInvalidPassword = 'stuff' + + return Promise.all(combinations.map((combination, index) => { + const u = User.build() + u.setDataValue('password', combination[1]) + return Promise.all([ + u.verifyPassword(combination[0]) + .then(result => assert.strictEqual(result, true, `password #${index} "${combination[0]}" should have been verified`)), + u.verifyPassword(intentionallyInvalidPassword) + .then(result => assert.strictEqual(result, false, `password #${index} "${combination[0]}" should NOT have been verified`)) + ]) + })).catch(e => assert.fail(e)) }) }) From 4833f300c527b96cc5d6d16c605df8cf04d06c1d Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 12:55:03 +0200 Subject: [PATCH 4/7] polyfilling scrypt for node 8.5+ Signed-off-by: Claudius --- lib/models/user.js | 17 +++++++++++++---- package.json | 1 + test/user.js | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/models/user.js b/lib/models/user.js index 8da12b8..76e20a3 100644 --- a/lib/models/user.js +++ b/lib/models/user.js @@ -1,11 +1,20 @@ 'use strict' // external modules -var Sequelize = require('sequelize') -var scrypt = require('scrypt-kdf') +const Sequelize = require('sequelize') +const crypto = require('crypto') +if (!crypto.scrypt) { + // polyfill for node.js 8.0, see https://github.com/chrisveness/scrypt-kdf#openssl-implementation + const scryptAsync = require('scrypt-async') + crypto.scrypt = function (password, salt, keylen, options, callback) { + const opt = Object.assign({}, options, { dkLen: keylen }) + scryptAsync(password, salt, opt, (derivedKey) => callback(null, Buffer.from(derivedKey))) + } +} +const scrypt = require('scrypt-kdf') // core -var logger = require('../logger') -var { generateAvatarURL } = require('../letter-avatars') +const logger = require('../logger') +const { generateAvatarURL } = require('../letter-avatars') module.exports = function (sequelize, DataTypes) { var User = sequelize.define('User', { diff --git a/package.json b/package.json index 8cfcfe6..374a047 100644 --- a/package.json +++ b/package.json @@ -110,6 +110,7 @@ "readline-sync": "^1.4.7", "request": "^2.88.0", "reveal.js": "~3.7.0", + "scrypt-async": "^2.0.1", "scrypt-kdf": "^2.0.1", "select2": "^3.5.2-browserify", "sequelize": "^3.28.0", diff --git a/test/user.js b/test/user.js index f2c8a22..38776a8 100644 --- a/test/user.js +++ b/test/user.js @@ -43,9 +43,9 @@ describe('User Sequelize model', function () { const combinations = [ // ['correct password', 'scrypt syle hash'] ['test', '736372797074000e00000008000000018c7b8c1ac273fd339badde759b3efc418bc61b776debd02dfe95989383cf9980ad21d2403dce33f4b551f5e98ce84edb792aee62600b1303ab8d4e6f0a53b0746e73193dbf557b888efc83a2d6a055a9'], - ['my secret pw', /* different hash! */ '736372797074000f0000000800000001f6083e9593365acd07550f7c72f19973fb7d52c3ef0a78026ff66c48ab14493843c642167b5e6b7f31927e8eeb912bc2639e41955fae15da5099998948cfeacd022f705624931c3b30104e6bb296b805'], ['ohai', '736372797074000e00000008000000010efec4e5ce6a5294491f1b1cccc38d3562f84844b9271aef635f8bc338cf4e0e0bac62ebb11379e85894c1f694e038fc39b087b4fdacd1280b50a7382d7ffbfc82f2190bef70d47708d2a94b75126294'], ['my secret pw', '736372797074000f0000000800000001ffb4cd10a1dfe9e64c1e5416fd6d55b390b6822e78b46fd1f963fe9f317a1e05f9c5fee15e1f618286f4e38b55364ae1e7dc295c9dc33ee0f5712e86afe37e5784ff9c7cf84cf0e631dd11f84f3621e7'], + ['my secret pw', /* different hash! */ '736372797074000f0000000800000001f6083e9593365acd07550f7c72f19973fb7d52c3ef0a78026ff66c48ab14493843c642167b5e6b7f31927e8eeb912bc2639e41955fae15da5099998948cfeacd022f705624931c3b30104e6bb296b805'], ['i am so extremely long, it\'s not even funny. Wait, you\'re still reading?', '736372797074000f00000008000000012d205f7bb529bb3a8b8bb25f5ab46197c7e9baf1aad64cf5e7b2584c84748cacf5e60631d58d21cb51fa34ea93b517e2fe2eb722931db5a70ff5a1330d821288ee7380c4136369f064b71b191a785a5b'] ] const intentionallyInvalidPassword = 'stuff' From 806ebe6e1abea47583ec57b6811b67958a2b3369 Mon Sep 17 00:00:00 2001 From: Claudius Date: Sun, 12 May 2019 10:01:38 +0200 Subject: [PATCH 5/7] drop node 6 support We will no longer test on node6 and instead focus on 8+. This won't break node6 immediately, but we will no longer go out of our way supporting a version that does not receive security updates. Signed-off-by: Claudius --- .babelrc | 2 +- .travis.yml | 14 +++++++------- docs/setup/manual-setup.md | 2 +- package.json | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.babelrc b/.babelrc index 26e5c92..ad37aa7 100644 --- a/.babelrc +++ b/.babelrc @@ -2,7 +2,7 @@ "presets": [ ["env", { "targets": { - "node": "6", + "node": "8", "uglify": true } }] diff --git a/.travis.yml b/.travis.yml index a2fce83..a5c50de 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,16 +4,10 @@ cache: yarn env: global: - CXX=g++-4.8 - - YARN_VERSION=1.15.2 + - YARN_VERSION=1.16.0 jobs: include: - - env: task=npm-test - node_js: - - 6 - before_install: - - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - - export PATH="$HOME/.yarn/bin:$PATH" - env: task=npm-test node_js: - 8 @@ -26,6 +20,12 @@ jobs: before_install: - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - export PATH="$HOME/.yarn/bin:$PATH" + - env: task=npm-test + node_js: + - 12 + before_install: + - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" + - export PATH="$HOME/.yarn/bin:$PATH" - env: task=ShellCheck script: - shellcheck bin/heroku bin/setup diff --git a/docs/setup/manual-setup.md b/docs/setup/manual-setup.md index b612853..e15e624 100644 --- a/docs/setup/manual-setup.md +++ b/docs/setup/manual-setup.md @@ -3,7 +3,7 @@ Manual Installation ## Requirements on your server -- Node.js 6.x or up (test up to 7.5.0) and <10.x +- Node.js 8.5 or up - Database (PostgreSQL, MySQL, MariaDB, SQLite, MSSQL) use charset `utf8` - npm (and its dependencies, [node-gyp](https://github.com/nodejs/node-gyp#installation)) - yarn diff --git a/package.json b/package.json index 374a047..490fe5a 100644 --- a/package.json +++ b/package.json @@ -119,7 +119,7 @@ "socket.io": "~2.1.1", "socket.io-client": "~2.1.1", "spin.js": "^2.3.2", - "sqlite3": "^4.0.1", + "sqlite3": "^4.0.7", "store": "^2.0.12", "string": "^3.3.3", "tedious": "^1.14.0", @@ -141,7 +141,7 @@ "**/request": "^2.88.0" }, "engines": { - "node": ">=6.x" + "node": ">=8.x" }, "bugs": "https://github.com/codimd/server/issues", "keywords": [ From aa57b76a4fd291e67d75400e568266c35576ef94 Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 13:27:17 +0200 Subject: [PATCH 6/7] updating travis config: readable job names, more recent distro Signed-off-by: Claudius --- .travis.yml | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index a5c50de..393fba2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,5 @@ language: node_js -dist: trusty +dist: xenial cache: yarn env: global: @@ -8,33 +8,43 @@ env: jobs: include: - - env: task=npm-test + - name: Node.js 8 node_js: - 8 before_install: - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - export PATH="$HOME/.yarn/bin:$PATH" - - env: task=npm-test + script: + - yarn run mocha-suite + - name: Node.js 10 node_js: - 10 before_install: - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - export PATH="$HOME/.yarn/bin:$PATH" - - env: task=npm-test + script: + - yarn run mocha-suite + - name: Node.js 12 node_js: - 12 before_install: - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - export PATH="$HOME/.yarn/bin:$PATH" - - env: task=ShellCheck + script: + - yarn run mocha-suite + - name: eslint + script: + - yarn run eslint + language: generic + - name: ShellCheck script: - shellcheck bin/heroku bin/setup language: generic - - env: task=json-lint + - name: json-lint addons: apt: packages: - jq script: - - npm run jsonlint + - yarn run jsonlint language: generic From 1da5a5bcccf7ce1e2f88ef501619f40ccb689220 Mon Sep 17 00:00:00 2001 From: Claudius Date: Mon, 13 May 2019 14:05:02 +0200 Subject: [PATCH 7/7] travis config is now in stages Signed-off-by: Claudius --- .travis.yml | 46 ++++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/.travis.yml b/.travis.yml index 393fba2..e73ad33 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,41 +1,15 @@ language: node_js dist: xenial cache: yarn -env: - global: - - CXX=g++-4.8 - - YARN_VERSION=1.16.0 jobs: include: - - name: Node.js 8 - node_js: - - 8 - before_install: - - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - - export PATH="$HOME/.yarn/bin:$PATH" - script: - - yarn run mocha-suite - - name: Node.js 10 + - stage: Static Tests + name: eslint node_js: - 10 - before_install: - - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - - export PATH="$HOME/.yarn/bin:$PATH" - script: - - yarn run mocha-suite - - name: Node.js 12 - node_js: - - 12 - before_install: - - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version "$YARN_VERSION" - - export PATH="$HOME/.yarn/bin:$PATH" - script: - - yarn run mocha-suite - - name: eslint script: - yarn run eslint - language: generic - name: ShellCheck script: - shellcheck bin/heroku bin/setup @@ -48,3 +22,19 @@ jobs: script: - yarn run jsonlint language: generic + - stage: Dynamic Tests + name: Node.js 8 + node_js: + - 8 + script: + - yarn run mocha-suite + - name: Node.js 10 + node_js: + - 10 + script: + - yarn run mocha-suite + - name: Node.js 12 + node_js: + - 12 + script: + - yarn run mocha-suite