Nemcio/NodeBB
1
0
Fork 0
mirror of https://github.com/NodeBB/NodeBB.git synced 2025-11-02 20:16:04 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix for #11119, restore password reset rate limiting (#11120)

Browse Source 
* chore: incrementing version number - v2.8.1

* chore: update changelog for v2.8.1

* fix: accidental clearing of reset rate limiting on reset send

* test: move user reset tests to its own file, add failing test for user reset locks

* fix: #11119, counter attempted flooding of user reset route

* test: fix password reset socket test to check for error now

* test: same user sending multiple reset emails

should work after waiting the correct amount of time

* lint: fixes

* chore: rename outdated `cleanTokensAndUids` method

* test: no need to create user for new test

Co-authored-by: Misty Release Bot <deploy@nodebb.org>
Co-authored-by: Barış Soner Uşaklı <barisusakli@gmail.com>
This commit is contained in:
Julian Lam
2023-01-04 11:24:46 -05:00
committed by GitHub
parent 19c2b3509a
commit a344e6ec0c
7 changed files with 241 additions and 130 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
CHANGELOG.md
View File

@@ -1,3 +1,37 @@
#### v2.8.1 (2022-12-30)
##### Chores
* fallbacks for new language string (8a69e740)
* remove extraneous lines from changelog (bbaf26ce)
* incrementing version number - v2.8.0 (8e77673d)
* update changelog for v2.8.0 (a5c2edb9)
* incrementing version number - v2.7.0 (96cc0617)
* incrementing version number - v2.6.1 (7e52a7a5)
* incrementing version number - v2.6.0 (e7fcf482)
* incrementing version number - v2.5.8 (dec0e7de)
* incrementing version number - v2.5.7 (5836bf4a)
* incrementing version number - v2.5.6 (c7bd7dbf)
* incrementing version number - v2.5.5 (3509ed94)
* incrementing version number - v2.5.4 (e83260ca)
* incrementing version number - v2.5.3 (7e922936)
* incrementing version number - v2.5.2 (babcd17e)
* incrementing version number - v2.5.1 (ce3aa950)
* incrementing version number - v2.5.0 (01d276cb)
* incrementing version number - v2.4.5 (dd3e1a28)
* incrementing version number - v2.4.4 (d5525c87)
* incrementing version number - v2.4.3 (9c647c6c)
* incrementing version number - v2.4.2 (3aa7b855)
* incrementing version number - v2.4.1 (60cbd148)
* incrementing version number - v2.4.0 (4834cde3)
* incrementing version number - v2.3.1 (d2425942)
* incrementing version number - v2.3.0 (046ea120)
##### Bug Fixes
* vulnerability in socket.io nested namespaces (#11117) (586eed14)
* lock post/reply similar to user.create (1ea9481a)
#### v2.8.0 (2022-12-21) #### v2.8.0 (2022-12-21)
##### Chores ##### Chores

2
install/package.json
View File

@@ -2,7 +2,7 @@
"name": "nodebb", "name": "nodebb",
"license": "GPL-3.0", "license": "GPL-3.0",
"description": "NodeBB Forum", "description": "NodeBB Forum",
"version": "2.8.0", "version": "2.8.1",
"homepage": "http://www.nodebb.org", "homepage": "http://www.nodebb.org",
"repository": { "repository": {
"type": "git", "type": "git",

2
src/socket.io/user.js
View File

@@ -62,7 +62,7 @@ SocketUser.reset.send = async function (socket, email) {
} catch (err) { } catch (err) {
await logEvent(err.message); await logEvent(err.message);
await sleep(2500 + ((Math.random() * 500) - 250)); await sleep(2500 + ((Math.random() * 500) - 250));
const internalErrors = ['[[error:invalid-email]]', '[[error:reset-rate-limited]]']; const internalErrors = ['[[error:invalid-email]]'];
if (!internalErrors.includes(err.message)) { if (!internalErrors.includes(err.message)) {
throw err; throw err;
} }

67
src/user/reset.js
View File

@@ -17,6 +17,8 @@ const UserReset = module.exports;
const twoHours = 7200000; const twoHours = 7200000;
UserReset.minSecondsBetweenEmails = 60;
UserReset.validate = async function (code) { UserReset.validate = async function (code) {
const uid = await db.getObjectField('reset:uid', code); const uid = await db.getObjectField('reset:uid', code);
if (!uid) { if (!uid) {
@@ -39,31 +41,45 @@ UserReset.generate = async function (uid) {
return code; return code;
}; };
async function canGenerate(uid) {
const score = await db.sortedSetScore('reset:issueDate:uid', uid);
if (score > Date.now() - (1000 * 60)) {
throw new Error('[[error:reset-rate-limited]]');
}
}
UserReset.send = async function (email) { UserReset.send = async function (email) {
const uid = await user.getUidByEmail(email); const uid = await user.getUidByEmail(email);
if (!uid) { if (!uid) {
throw new Error('[[error:invalid-email]]'); throw new Error('[[error:invalid-email]]');
} }
await canGenerate(uid); await lockReset(uid, '[[error:reset-rate-limited]]');
await db.sortedSetAdd('reset:issueDate:uid', Date.now(), uid); try {
const code = await UserReset.generate(uid); await canGenerate(uid);
await emailer.send('reset', uid, { await db.sortedSetAdd('reset:issueDate:uid', Date.now(), uid);
reset_link: `${nconf.get('url')}/reset/${code}`, const code = await UserReset.generate(uid);
subject: '[[email:password-reset-requested]]', await emailer.send('reset', uid, {
template: 'reset', reset_link: `${nconf.get('url')}/reset/${code}`,
uid: uid, subject: '[[email:password-reset-requested]]',
}).catch(err => winston.error(`[emailer.send] ${err.stack}`)); template: 'reset',
uid: uid,
}).catch(err => winston.error(`[emailer.send] ${err.stack}`));
return code; return code;
} finally {
db.deleteObjectField('locks', `reset${uid}`);
}
}; };
async function lockReset(uid, error) {
const value = `reset${uid}`;
const count = await db.incrObjectField('locks', value);
if (count > 1) {
throw new Error(error);
}
return value;
}
async function canGenerate(uid) {
const score = await db.sortedSetScore('reset:issueDate:uid', uid);
if (score > Date.now() - (UserReset.minSecondsBetweenEmails * 1000)) {
throw new Error('[[error:reset-rate-limited]]');
}
}
UserReset.commit = async function (code, password) { UserReset.commit = async function (code, password) {
user.isPasswordValid(password); user.isPasswordValid(password);
const validated = await UserReset.validate(code); const validated = await UserReset.validate(code);
@@ -122,16 +138,13 @@ UserReset.updateExpiry = async function (uid) {
}; };
UserReset.clean = async function () { UserReset.clean = async function () {
const [tokens, uids] = await Promise.all([ const tokens = await db.getSortedSetRangeByScore('reset:issueDate', 0, -1, '-inf', Date.now() - twoHours);
db.getSortedSetRangeByScore('reset:issueDate', 0, -1, '-inf', Date.now() - twoHours), if (!tokens.length) {
db.getSortedSetRangeByScore('reset:issueDate:uid', 0, -1, '-inf', Date.now() - twoHours),
]);
if (!tokens.length && !uids.length) {
return; return;
} }
winston.verbose(`[UserReset.clean] Removing ${tokens.length} reset tokens from database`); winston.verbose(`[UserReset.clean] Removing ${tokens.length} reset tokens from database`);
await cleanTokensAndUids(tokens, uids); await cleanTokens(tokens);
}; };
UserReset.cleanByUid = async function (uid) { UserReset.cleanByUid = async function (uid) {
@@ -153,13 +166,15 @@ UserReset.cleanByUid = async function (uid) {
} }
winston.verbose(`[UserReset.cleanByUid] Found ${tokensToClean.length} token(s), removing...`); winston.verbose(`[UserReset.cleanByUid] Found ${tokensToClean.length} token(s), removing...`);
await cleanTokensAndUids(tokensToClean, uid); await Promise.all([
cleanTokens(tokensToClean),
db.deleteObjectField('locks', `reset${uid}`),
]);
}; };
async function cleanTokensAndUids(tokens, uids) { async function cleanTokens(tokens) {
await Promise.all([ await Promise.all([
db.deleteObjectFields('reset:uid', tokens), db.deleteObjectFields('reset:uid', tokens),
db.sortedSetRemove('reset:issueDate', tokens), db.sortedSetRemove('reset:issueDate', tokens),
db.sortedSetRemove('reset:issueDate:uid', uids),
]); ]);
} }

2
test/socket.io.js
View File

@@ -733,7 +733,7 @@ describe('socket.io', () => {
it('should not generate code if rate limited', (done) => { it('should not generate code if rate limited', (done) => {
socketUser.reset.send({ uid: 0 }, 'regular@test.com', (err) => { socketUser.reset.send({ uid: 0 }, 'regular@test.com', (err) => {
assert.ifError(err); assert(err);
async.parallel({ async.parallel({
count: async.apply(db.sortedSetCount.bind(db), 'reset:issueDate', 0, Date.now()), count: async.apply(db.sortedSetCount.bind(db), 'reset:issueDate', 0, Date.now()),

101
test/user.js
View File

@@ -15,7 +15,6 @@ const User = require('../src/user');
const Topics = require('../src/topics'); const Topics = require('../src/topics');
const Categories = require('../src/categories'); const Categories = require('../src/categories');
const Posts = require('../src/posts'); const Posts = require('../src/posts');
const Password = require('../src/password');
const groups = require('../src/groups'); const groups = require('../src/groups');
const messaging = require('../src/messaging'); const messaging = require('../src/messaging');
const helpers = require('./helpers'); const helpers = require('./helpers');
@@ -587,106 +586,6 @@ describe('User', () => {
}); });
}); });
describe('passwordReset', () => {
let uid;
let code;
before(async () => {
uid = await User.create({ username: 'resetuser', password: '123456' });
await User.setUserField(uid, 'email', 'reset@me.com');
await User.email.confirmByUid(uid);
});
it('.generate() should generate a new reset code', (done) => {
User.reset.generate(uid, (err, _code) => {
assert.ifError(err);
assert(_code);
code = _code;
done();
});
});
it('.generate() should invalidate a previous generated reset code', async () => {
const _code = await User.reset.generate(uid);
const valid = await User.reset.validate(code);
assert.strictEqual(valid, false);
code = _code;
});
it('.validate() should ensure that this new code is valid', (done) => {
User.reset.validate(code, (err, valid) => {
assert.ifError(err);
assert.strictEqual(valid, true);
done();
});
});
it('.validate() should correctly identify an invalid code', (done) => {
User.reset.validate(`${code}abcdef`, (err, valid) => {
assert.ifError(err);
assert.strictEqual(valid, false);
done();
});
});
it('.send() should create a new reset code and reset password', async () => {
code = await User.reset.send('reset@me.com');
});
it('.commit() should update the user\'s password and confirm their email', (done) => {
User.reset.commit(code, 'newpassword', (err) => {
assert.ifError(err);
async.parallel({
userData: function (next) {
User.getUserData(uid, next);
},
password: function (next) {
db.getObjectField(`user:${uid}`, 'password', next);
},
}, (err, results) => {
assert.ifError(err);
Password.compare('newpassword', results.password, true, (err, match) => {
assert.ifError(err);
assert(match);
assert.strictEqual(results.userData['email:confirmed'], 1);
done();
});
});
});
});
it('.should error if same password is used for reset', async () => {
const uid = await User.create({ username: 'badmemory', email: 'bad@memory.com', password: '123456' });
const code = await User.reset.generate(uid);
let err;
try {
await User.reset.commit(code, '123456');
} catch (_err) {
err = _err;
}
assert.strictEqual(err.message, '[[error:reset-same-password]]');
});
it('should not validate email if password reset is due to expiry', async () => {
const uid = await User.create({ username: 'resetexpiry', email: 'reset@expiry.com', password: '123456' });
let confirmed = await User.getUserField(uid, 'email:confirmed');
let [verified, unverified] = await groups.isMemberOfGroups(uid, ['verified-users', 'unverified-users']);
assert.strictEqual(confirmed, 0);
assert.strictEqual(verified, false);
assert.strictEqual(unverified, true);
await User.setUserField(uid, 'passwordExpiry', Date.now());
const code = await User.reset.generate(uid);
await User.reset.commit(code, '654321');
confirmed = await User.getUserField(uid, 'email:confirmed');
[verified, unverified] = await groups.isMemberOfGroups(uid, ['verified-users', 'unverified-users']);
assert.strictEqual(confirmed, 0);
assert.strictEqual(verified, false);
assert.strictEqual(unverified, true);
});
});
describe('hash methods', () => { describe('hash methods', () => {
it('should return uid from email', (done) => { it('should return uid from email', (done) => {
User.getUidByEmail('john@example.com', (err, uid) => { User.getUidByEmail('john@example.com', (err, uid) => {

163
test/user/reset.js Normal file
View File

@@ -0,0 +1,163 @@
'use strict';
const assert = require('assert');
const async = require('async');
const db = require('../mocks/databasemock');
const user = require('../../src/user');
const groups = require('../../src/groups');
const password = require('../../src/password');
const utils = require('../../src/utils');
const socketUser = require('../../src/socket.io/user');
describe('Password reset (library methods)', () => {
let uid;
let code;
before(async () => {
uid = await user.create({ username: 'resetuser', password: '123456' });
await user.setUserField(uid, 'email', 'reset@me.com');
await user.email.confirmByUid(uid);
});
it('.generate() should generate a new reset code', (done) => {
user.reset.generate(uid, (err, _code) => {
assert.ifError(err);
assert(_code);
code = _code;
done();
});
});
it('.generate() should invalidate a previous generated reset code', async () => {
const _code = await user.reset.generate(uid);
const valid = await user.reset.validate(code);
assert.strictEqual(valid, false);
code = _code;
});
it('.validate() should ensure that this new code is valid', (done) => {
user.reset.validate(code, (err, valid) => {
assert.ifError(err);
assert.strictEqual(valid, true);
done();
});
});
it('.validate() should correctly identify an invalid code', (done) => {
user.reset.validate(`${code}abcdef`, (err, valid) => {
assert.ifError(err);
assert.strictEqual(valid, false);
done();
});
});
it('.send() should create a new reset code and reset password', async () => {
code = await user.reset.send('reset@me.com');
});
it('.commit() should update the user\'s password and confirm their email', (done) => {
user.reset.commit(code, 'newpassword', (err) => {
assert.ifError(err);
async.parallel({
userData: function (next) {
user.getUserData(uid, next);
},
password: function (next) {
db.getObjectField(`user:${uid}`, 'password', next);
},
}, (err, results) => {
assert.ifError(err);
password.compare('newpassword', results.password, true, (err, match) => {
assert.ifError(err);
assert(match);
assert.strictEqual(results.userData['email:confirmed'], 1);
done();
});
});
});
});
it('.should error if same password is used for reset', async () => {
const uid = await user.create({ username: 'badmemory', email: 'bad@memory.com', password: '123456' });
const code = await user.reset.generate(uid);
let err;
try {
await user.reset.commit(code, '123456');
} catch (_err) {
err = _err;
}
assert.strictEqual(err.message, '[[error:reset-same-password]]');
});
it('should not validate email if password reset is due to expiry', async () => {
const uid = await user.create({ username: 'resetexpiry', email: 'reset@expiry.com', password: '123456' });
let confirmed = await user.getUserField(uid, 'email:confirmed');
let [verified, unverified] = await groups.isMemberOfGroups(uid, ['verified-users', 'unverified-users']);
assert.strictEqual(confirmed, 0);
assert.strictEqual(verified, false);
assert.strictEqual(unverified, true);
await user.setUserField(uid, 'passwordExpiry', Date.now());
const code = await user.reset.generate(uid);
await user.reset.commit(code, '654321');
confirmed = await user.getUserField(uid, 'email:confirmed');
[verified, unverified] = await groups.isMemberOfGroups(uid, ['verified-users', 'unverified-users']);
assert.strictEqual(confirmed, 0);
assert.strictEqual(verified, false);
assert.strictEqual(unverified, true);
});
});
describe.only('locks', () => {
let uid;
let email;
beforeEach(async () => {
const [username, password] = [utils.generateUUID().slice(0, 10), utils.generateUUID()];
uid = await user.create({ username, password });
email = `${username}@nodebb.org`;
await user.setUserField(uid, 'email', email);
await user.email.confirmByUid(uid);
});
it('should disallow reset request if one was made within the minute', async () => {
await user.reset.send(email);
await assert.rejects(user.reset.send(email), {
message: '[[error:reset-rate-limited]]',
});
});
it('should not allow multiple calls to the reset method at the same time', async () => {
await assert.rejects(Promise.all([
user.reset.send(email),
user.reset.send(email),
]), {
message: '[[error:reset-rate-limited]]',
});
});
it('should not allow multiple socket calls to the reset method either', async () => {
await assert.rejects(Promise.all([
socketUser.reset.send({ uid: 0 }, email),
socketUser.reset.send({ uid: 0 }, email),
]), {
message: '[[error:reset-rate-limited]]',
});
});
it('should properly unlock user reset', async () => {
await user.reset.send(email);
await assert.rejects(user.reset.send(email), {
message: '[[error:reset-rate-limited]]',
});
user.reset.minSecondsBetweenEmails = 3;
const util = require('util');
const sleep = util.promisify(setTimeout);
await sleep(4 * 1000); // wait 4 seconds
await user.reset.send(email);
user.reset.minSecondsBetweenEmails = 60;
});
});