Skip to content

Commit

Permalink
Standardise responses for invalid new passwords (#1411)
Browse files Browse the repository at this point in the history 
The fix is pushed deeper into the `util/crypto.js` code than where validation previously occurred.  This means that validation only needs to be performed in a single place, rather than every point of use.  This in turn means it should be harder to forget validation & edge cases in future use of `hashPassword()`.

Closes #1407
  • Loading branch information
@alxndrsn
alxndrsn authored Feb 28, 2025
1 parent bccfe17 commit bf49b89
Show file tree
Hide file tree
Showing 9 changed files with 53 additions and 51 deletions.
8 changes: 1 addition & 7 deletions lib/model/query/users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ const { map } = require('ramda');
const { Actor, User } = require('../frames');
const { hashPassword } = require('../../util/crypto');
const { unjoiner, page, sqlEquals, QueryOptions } = require('../../util/db');
const { reject } = require('../../util/promise');
const Problem = require('../../util/problem');

const create = (user) => ({ Actors }) => Actors.createSubtype(user);
create.audit = (user) => (log) => log('user.create', user.actor, { data: user });
Expand All @@ -34,11 +32,7 @@ const update = (user, data) => ({ run, one }) => {
update.audit = (user, data) => (log) => log('user.update', user.actor, { data: data.with(data.actor) });

const updatePassword = (user, cleartext) => ({ run }) =>
(cleartext.length < 10
? reject(Problem.user.passwordTooShort())
: Buffer.from(cleartext).length > 72
? reject(Problem.user.passwordTooLong())
: hashPassword(cleartext))
hashPassword(cleartext)
.then((hash) => run(sql`update users set password=${hash} where "actorId"=${user.actor.id}`));
updatePassword.audit = (user) => (log) => log('user.update', user.actor, { data: { password: true } });

Expand Down
10 changes: 7 additions & 3 deletions lib/util/crypto.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const { PartialPipe } = require('./stream');
const { unpadPkcs1OaepMgf1Sha256 } = require('./quarantine/oaep');
const { unpadPkcs7 } = require('./quarantine/pkcs7');
const { promisify } = require('util');
const { resolve } = require('./promise');
const { resolve, reject } = require('./promise');
const Problem = require('./problem');
const { isBlank } = require('./util');

Expand All @@ -33,8 +33,12 @@ const { isBlank } = require('./util');
const BCRYPT_COST_FACTOR = process.env.BCRYPT === 'insecure' ? 1 : 12;

// These functions call into bcrypt to hash or verify passwords.
const hashPassword = (plain) =>
(isBlank(plain) ? resolve(null) : bcrypt.hash(plain, BCRYPT_COST_FACTOR));
const hashPassword = (plain) => {
if (typeof plain !== 'string') return reject(Problem.user.invalidDataTypeOfParameter({ value: plain, expected: 'string' }));
if (plain.length < 10) return reject(Problem.user.passwordTooShort());
if (plain.length > 72) return reject(Problem.user.passwordTooLong());
return isBlank(plain) ? resolve(null) : bcrypt.hash(plain, BCRYPT_COST_FACTOR);
};
const verifyPassword = (plain, hash) => ((typeof plain !== 'string' || isBlank(plain) || isBlank(hash))
? resolve(false)
: bcrypt.compare(plain, hash));
Expand Down
16 changes: 8 additions & 8 deletions test/integration/api/sessions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ describe('api: /sessions', () => {

it('should return a new session if the information is valid', testService((service) =>
service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'chelsea' })
.send({ email: '[email protected]', password: 'password4chelsea' })
.expect(200)
.then(({ body }) => {
body.should.be.a.Session();
Expand All @@ -20,9 +20,9 @@ describe('api: /sessions', () => {
// and reject them before passing the values to bcrypt.
describe('weird bcrypt implementation details', () => {
[
[ 'repeated once', 'chelsea\0chelsea' ], // eslint-disable-line no-multi-spaces
[ 'repeated twice', 'chelsea\0chelsea\0chelsea' ], // eslint-disable-line no-multi-spaces
[ 'repeated until truncation', 'chelsea\0chelsea\0chelsea\0chelsea\0chelsea\0chelsea\0chelsea\0chelsea\0chelsea\0' ],
[ 'repeated once', 'password4chelsea\0password4chelsea' ], // eslint-disable-line no-multi-spaces
[ 'repeated twice', 'password4chelsea\0password4chelsea\0password4chelsea' ], // eslint-disable-line no-multi-spaces
[ 'repeated until truncation', 'password4chelsea\0password4chelsea\0password4chelsea\0password4chelsea\0password4' ],
].forEach(([ description, password ]) => {
it(`should treat a password ${description} as the singular version of the same`, testService((service) =>
service.post('/v1/sessions')
Expand All @@ -36,23 +36,23 @@ describe('api: /sessions', () => {

it('should treat email addresses case insensitively', testService((service) =>
service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'chelsea' })
.send({ email: '[email protected]', password: 'password4chelsea' })
.expect(200)
.then(({ body }) => {
body.should.be.a.Session();
})));

it('should provide a csrf token when the session returns', testService((service) =>
service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'chelsea' })
.send({ email: '[email protected]', password: 'password4chelsea' })
.expect(200)
.then(({ body }) => {
body.csrf.should.be.a.token();
})));

it('should set cookie information when the session returns', testService((service) =>
service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'chelsea' })
.send({ email: '[email protected]', password: 'password4chelsea' })
.expect(200)
.then(({ body, headers }) => {
// i don't know how this becomes an array but i think superagent does it.
Expand All @@ -71,7 +71,7 @@ describe('api: /sessions', () => {

it('should log the action in the audit log', testService((service) =>
service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'alice' })
.send({ email: '[email protected]', password: 'password4alice' })
.set('User-Agent', 'central/tests')
.expect(200)
.then(({ body }) => body.token)
Expand Down
46 changes: 27 additions & 19 deletions test/integration/api/users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,19 @@ describe('api: /users', () => {
.then(({ password }) => { should.not.exist(password); })
])))));

it('should not accept a password that is too short', testService((service) =>
service.login('alice', (asAlice) =>
asAlice.post('/v1/users')
.send({ email: '[email protected]', password: 'short' })
.expect(400))));
[
[ 'too short', 'short' ],
[ 'too long', 'loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong' ], // eslint-disable-line no-multi-spaces
[ 'object', {} ], // eslint-disable-line no-multi-spaces
[ 'array', [] ], // eslint-disable-line no-multi-spaces
[ 'number', 123 ], // eslint-disable-line no-multi-spaces
].forEach(([ description, password ]) => {
it(`should not accept ${description} password`, testService((service) =>
service.login('alice', (asAlice) =>
asAlice.post('/v1/users')
.send({ email: '[email protected]', password })
.expect(400))));
});

it('should send an email to provisioned users', testService((service) =>
service.login('alice', (asAlice) =>
Expand Down Expand Up @@ -350,7 +358,7 @@ describe('api: /users', () => {
email.subject.should.equal('ODK Central account password reset');

return service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'bob' })
.send({ email: '[email protected]', password: 'password4bob' })
.expect(401);
}))));

Expand Down Expand Up @@ -536,7 +544,7 @@ describe('api: /users', () => {
} else {
after.body.email.should.equal('[email protected]');
return service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'bob' })
.send({ email: '[email protected]', password: 'password4bob' })
.expect(200);
}
})))));
Expand Down Expand Up @@ -623,7 +631,7 @@ describe('api: /users', () => {
asAlice.get('/v1/users/current')
.expect(200)
.then(({ body }) => asAlice.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(404)))));
});
} else {
Expand All @@ -634,13 +642,13 @@ describe('api: /users', () => {
.expect(200)
.then(({ body }) => service.login('chelsea', (asChelsea) =>
asChelsea.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: 'chelsea' })
.send({ old: 'password4alice', new: 'chelsea' })
.expect(403))))));

it('should reject if the user does not exist', testService((service) =>
service.login('alice', (asAlice) =>
asAlice.put('/v1/users/9999/password')
.send({ old: 'alice', new: 'chelsea' })
.send({ old: 'password4alice', new: 'password4chelsea' })
.expect(404))));

it('should reject if the old password is not correct', testService((service) =>
Expand All @@ -656,7 +664,7 @@ describe('api: /users', () => {
asAlice.get('/v1/users/current')
.expect(200)
.then(({ body }) => asAlice.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(200))
.then(({ body }) => {
body.success.should.equal(true);
Expand All @@ -670,14 +678,14 @@ describe('api: /users', () => {
asAlice.get('/v1/users/current')
.expect(200)
.then(({ body }) => asAlice.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: '123456789' })
.send({ old: 'password4alice', new: '123456789' })
.expect(400))))); // 400.21

it('should allow nonadministrator users to set their own password', testService((service) =>
service.login('chelsea', (asChelsea) =>
asChelsea.get('/v1/users/current').expect(200).then(({ body }) => body.id)
.then((chelseaId) => asChelsea.put(`/v1/users/${chelseaId}/password`)
.send({ old: 'chelsea', new: 'newchelsea' })
.send({ old: 'password4chelsea', new: 'newchelsea' })
.expect(200)
.then(() => service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'newchelsea' })
Expand All @@ -690,7 +698,7 @@ describe('api: /users', () => {
.expect(200);
await anotherAlice.get('/v1/users/current').expect(200);
await asAlice.put(`/v1/users/${id}/password`)
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(200);
// The other session has been deleted.
await anotherAlice.get('/v1/users/current').expect(401);
Expand All @@ -702,11 +710,11 @@ describe('api: /users', () => {
const asAlice = await service.login('alice');
const { body: { id } } = await asAlice.get('/v1/users/current')
.expect(200);
const basic = Buffer.from('[email protected]:alice').toString('base64');
const basic = Buffer.from('[email protected]:password4alice').toString('base64');
await service.put(`/v1/users/${id}/password`)
.set('Authorization', `Basic ${basic}`)
.set('X-Forwarded-Proto', 'https')
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(200);
await asAlice.get('/v1/users/current').expect(401);
}));
Expand All @@ -716,7 +724,7 @@ describe('api: /users', () => {
asAlice.get('/v1/users/current')
.expect(200)
.then(({ body }) => asAlice.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(200)
.then(() => {
const email = global.inbox.pop();
Expand All @@ -730,7 +738,7 @@ describe('api: /users', () => {
asAlice.get('/v1/users/current')
.expect(200)
.then(({ body }) => asAlice.put(`/v1/users/${body.id}/password`)
.send({ old: 'alice', new: 'newpassword' })
.send({ old: 'password4alice', new: 'newpassword' })
.expect(200)
.then(() => Promise.all([
Users.getByEmail('[email protected]').then((o) => o.get()),
Expand Down Expand Up @@ -820,7 +828,7 @@ describe('api: /users', () => {
}
} else {
return service.post('/v1/sessions')
.send({ email: '[email protected]', password: 'chelsea' })
.send({ email: '[email protected]', password: 'password4chelsea' })
.expect(401);
}
}))))));
Expand Down
6 changes: 3 additions & 3 deletions test/integration/fixtures/01-users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ module.exports = ({ Assignments, Users, Projects }) => {
const proj = new Project({ name: 'Default Project' });

const users = [
new User({ email: '[email protected]', password: 'alice' }, { actor: new Actor({ type: 'user', displayName: 'Alice' }) }),
new User({ email: '[email protected]', password: 'bob' }, { actor: new Actor({ type: 'user', displayName: 'Bob' }) }),
new User({ email: '[email protected]', password: 'chelsea' }, { actor: new Actor({ type: 'user', displayName: 'Chelsea' }) })
new User({ email: '[email protected]', password: 'password4alice' }, { actor: new Actor({ type: 'user', displayName: 'Alice' }) }),
new User({ email: '[email protected]', password: 'password4bob' }, { actor: new Actor({ type: 'user', displayName: 'Bob' }) }),
new User({ email: '[email protected]', password: 'password4chelsea' }, { actor: new Actor({ type: 'user', displayName: 'Chelsea' }) })
];

// hash the passwords, create our three test users, then add grant Alice and Bob their rights.
Expand Down
4 changes: 2 additions & 2 deletions test/integration/other/basic-auth.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ describe('basic authentication', () => {
it('should not accept email and password', testService((service) =>
service.get('/v1/users/current')
.set('x-forwarded-proto', 'https')
.auth('[email protected]', 'alice')
.auth('[email protected]', 'password4alice')
.expect(401)));
} else {
it('should accept email and password', testService((service) =>
service.get('/v1/users/current')
.set('x-forwarded-proto', 'https')
.auth('[email protected]', 'alice')
.auth('[email protected]', 'password4alice')
.expect(200)));
}
});
Expand Down
4 changes: 2 additions & 2 deletions test/unit/http/preprocessors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,12 @@ describe('preprocessors', () => {
)).should.be.rejectedWith(Problem, { problemCode: 401.2 }));

it('should set the appropriate session if valid Basic auth credentials are given @slow', () =>
hashPassword('alice').then((hashed) =>
hashPassword('password4alice').then((hashed) =>
Promise.resolve(authHandler(
{ Auth, Users: mockUsers('[email protected]', hashed) },
new Context(
createRequest({ headers: {
Authorization: `Basic ${Buffer.from('[email protected]:alice', 'utf8').toString('base64')}`,
Authorization: `Basic ${Buffer.from('[email protected]:password4alice', 'utf8').toString('base64')}`,
'X-Forwarded-Proto': 'https'
} }),
{ fieldKey: Option.none() }
Expand Down
8 changes: 2 additions & 6 deletions test/unit/util/crypto.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,8 @@ describe('util/crypto', () => {
hashPassword('password', 'hashhash').should.be.a.Promise();
});

it('should return a Promise of null given a blank plaintext', (done) => {
hashPassword('').then((result) => {
should(result).equal(null);
done();
});
});
it('should reject given a blank plaintext', () =>
hashPassword('').should.be.rejectedWith('The password or passphrase provided does not meet the required length.'));

it('should not attempt to verify empty plaintext', (done) => {
verifyPassword('', '$2a$12$hCRUXz/7Hx2iKPLCduvrWugC5Q/j5e3bX9KvaYvaIvg/uvFYEpzSy').then((result) => {
Expand Down
2 changes: 1 addition & 1 deletion test/util/authenticate-user.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = async (service, user, includeCsrf) => {
return body.token;
} else {
const credentials = (typeof user === 'string')
? { email: `${user}@getodk.org`, password: user }
? { email: `${user}@getodk.org`, password: `password4${user}` }
: user;
const { body } = await service.post('/v1/sessions')
.send(credentials)
Expand Down

0 comments on commit bf49b89

Please sign in to comment.