diff --git a/packages/frontend-2/lib/auth/composables/passwordReset.ts b/packages/frontend-2/lib/auth/composables/passwordReset.ts index 33837063e..d2ad5241c 100644 --- a/packages/frontend-2/lib/auth/composables/passwordReset.ts +++ b/packages/frontend-2/lib/auth/composables/passwordReset.ts @@ -20,7 +20,7 @@ export function usePasswordReset() { triggerNotification({ type: ToastNotificationType.Info, title: 'Password reset email sent', - description: `We've sent the password reset instructions to ${email}` + description: `If the email address '${email}' is associated with a registered user, we have sent password reset instructions to that address.` }) } catch (e) { triggerNotification({ diff --git a/packages/server/modules/auth/tests/helpers/registration.ts b/packages/server/modules/auth/tests/helpers/registration.ts index 77760e368..d1854b12c 100644 --- a/packages/server/modules/auth/tests/helpers/registration.ts +++ b/packages/server/modules/auth/tests/helpers/registration.ts @@ -213,7 +213,7 @@ export const localAuthRestApi = (params: { express: Express }) => { const user = await authCheck({ token }) expect(user).to.be.ok - expect(user.email).to.equal(params.email) + expect(user.email.toLowerCase()).to.equal(params.email.toLowerCase()) return user } diff --git a/packages/server/modules/core/utils/ratelimiter.ts b/packages/server/modules/core/utils/ratelimiter.ts index 8a6575b23..f64ab3e54 100644 --- a/packages/server/modules/core/utils/ratelimiter.ts +++ b/packages/server/modules/core/utils/ratelimiter.ts @@ -167,6 +167,16 @@ export const LIMITS = { duration: 1 * TIME.minute } }, + 'POST /auth/pwdreset/request': { + regularOptions: { + limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), + duration: 10 * TIME.minute + }, + burstOptions: { + limitCount: getIntFromEnv('RATELIMIT_BURST_GET_AUTH', '10'), + duration: 30 * TIME.minute + } + }, '/auth/local/login': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), diff --git a/packages/server/modules/pwdreset/rest/index.ts b/packages/server/modules/pwdreset/rest/index.ts index 29c59cc9e..d3c4d3ceb 100644 --- a/packages/server/modules/pwdreset/rest/index.ts +++ b/packages/server/modules/pwdreset/rest/index.ts @@ -22,12 +22,14 @@ import { BadRequestError } from '@/modules/shared/errors' import { withOperationLogging } from '@/observability/domain/businessLogging' import { ensureError } from '@speckle/shared' import type { Express } from 'express' +import { UserNotFoundError } from '@/modules/core/errors/user' export default function (app: Express) { const getUserByEmail = getUserByEmailFactory({ db }) // sends a password recovery email. app.post('/auth/pwdreset/request', async (req, res) => { + const responseMessage = 'Password reset email sent.' try { const email = req.body.email const logger = req.log.child({ email }) @@ -46,10 +48,16 @@ export default function (app: Express) { operationDescription: `Requesting password recovery` }) - return res.status(200).send('Password reset email sent.') + return res.status(200).send(responseMessage) } catch (e: unknown) { - req.log.info({ err: e }, 'Error while requesting password recovery.') - res.status(400).send(ensureError(e).message) + const err = ensureError(e, 'Unknown error while requesting password recovery') + req.log.info({ err }, 'Error while requesting password recovery.') + if (err instanceof UserNotFoundError) { + // always 200 and use same response message to avoid user enumeration + res.status(200).send(responseMessage) + } else { + res.status(400).send(err.message) + } } }) diff --git a/packages/server/modules/pwdreset/services/request.ts b/packages/server/modules/pwdreset/services/request.ts index 06f4b6a17..ddb0571dd 100644 --- a/packages/server/modules/pwdreset/services/request.ts +++ b/packages/server/modules/pwdreset/services/request.ts @@ -1,5 +1,6 @@ import type { GetServerInfo } from '@/modules/core/domain/server/operations' import type { GetUserByEmail } from '@/modules/core/domain/users/operations' +import { UserNotFoundError } from '@/modules/core/errors/user' import { getPasswordResetFinalizationRoute } from '@/modules/core/helpers/routeHelper' import type { EmailTemplateParams } from '@/modules/emails/domain/operations' import type { renderEmail } from '@/modules/emails/services/emailRendering' @@ -31,9 +32,7 @@ const initializeNewTokenFactory = ]) if (!user) { - throw new InvalidPasswordRecoveryRequestError( - 'No user with that e-mail address found' - ) + throw new UserNotFoundError('No user with that e-mail address found') } if (tokenAlreadyExists) { diff --git a/packages/server/modules/pwdreset/tests/pwdrest.spec.ts b/packages/server/modules/pwdreset/tests/pwdrest.spec.ts index 9d98faf07..a5d2f1d33 100644 --- a/packages/server/modules/pwdreset/tests/pwdrest.spec.ts +++ b/packages/server/modules/pwdreset/tests/pwdrest.spec.ts @@ -7,6 +7,8 @@ import { localAuthRestApi } from '@/modules/auth/tests/helpers/registration' import { expectToThrow } from '@/test/assertionHelper' import { expect } from 'chai' import { createTestUser } from '@/test/authHelper' +import { createRandomEmail } from '@/modules/core/helpers/testHelpers' +import cryptoRandomString from 'crypto-random-string' const ResetTokens = () => knex('pwdreset_tokens') @@ -19,9 +21,9 @@ describe('Password reset requests @passwordresets', () => { it('Should carefully send a password request email', async () => { const userA = await createTestUser({ - name: 'd1', - email: 'd@speckle.systems', - password: 'wowwow8charsplease', + name: cryptoRandomString({ length: 10 }), + email: createRandomEmail(), + password: cryptoRandomString({ length: 8 }), id: '' }) @@ -31,8 +33,8 @@ describe('Password reset requests @passwordresets', () => { // non-existent user await request(app) .post('/auth/pwdreset/request') - .send({ email: 'doesnot@exist.here' }) - .expect(400) + .send({ email: createRandomEmail() }) // does not exist + .expect(200) // always 200 to prevent user enumeration // good request await request(app) @@ -43,20 +45,20 @@ describe('Password reset requests @passwordresets', () => { // already has expiration token, fall back await request(app) .post('/auth/pwdreset/request') - .send({ email: 'd@speckle.systems' }) + .send({ email: userA.email }) .expect(400) }) it('Should reset passwords', async () => { const userB = await createTestUser({ - name: 'd2', - email: 'd2@speckle.systems', - password: 'w0ww0w8charsplease', + name: cryptoRandomString({ length: 10 }), + email: createRandomEmail(), + password: cryptoRandomString({ length: 8 }), id: '' }) const authRestApi = localAuthRestApi({ express: app }) - const newPassword = '12345678' + const newPassword = cryptoRandomString({ length: 8 }) // trigger request await request(app) @@ -88,7 +90,7 @@ describe('Password reset requests @passwordresets', () => { // token used up, should fail await request(app) .post('/auth/pwdreset/finalize') - .send({ tokenId: token.id, password: 'abc12345678' }) + .send({ tokenId: token.id, password: cryptoRandomString({ length: 8 }) }) .expect(400) // should be able to log in with new pw