server: improve security of password reset flow #213

Closed
Johann150 wants to merge 1 commit from refactor/password-reset into main
2 changed files with 41 additions and 32 deletions

View file

@ -4,7 +4,7 @@ import config from '@/config/index.js';
import { Users, UserProfiles, PasswordResetRequests } from '@/models/index.js'; import { Users, UserProfiles, PasswordResetRequests } from '@/models/index.js';
import { sendEmail } from '@/services/send-email.js'; import { sendEmail } from '@/services/send-email.js';
import { genId } from '@/misc/gen-id.js'; import { genId } from '@/misc/gen-id.js';
import { HOUR } from '@/const.js'; import { DAY } from '@/const.js';
import define from '../define.js'; import define from '../define.js';
export const meta = { export const meta = {
@ -15,12 +15,8 @@ export const meta = {
description: 'Request a users password to be reset.', description: 'Request a users password to be reset.',
limit: { limit: {
duration: HOUR, duration: DAY,
max: 3, max: 1,
},
errors: {
}, },
} as const; } as const;
@ -35,25 +31,26 @@ export const paramDef = {
// eslint-disable-next-line import/no-default-export // eslint-disable-next-line import/no-default-export
export default define(meta, paramDef, async (ps) => { export default define(meta, paramDef, async (ps) => {
const user = await Users.findOneBy({ const [user, profile] = await Promise.all([
usernameLower: ps.username.toLowerCase(), Users.findOneBy({
host: IsNull(), usernameLower: ps.username.toLowerCase(),
host: IsNull(),
}),
UserProfiles.findOneByOrFail({ userId: user.id }),
]);
const resetRequestExists = await PasswordResetRequests.countBy({
userId: user.id,
}); });
// 合致するユーザーが登録されていなかったら無視 // This is an "arithmetical or" to try to prevent timing attacks which might reveal
if (user == null) { // a users email address.
return; if (
} user == null
| profile.email !== ps.email
const profile = await UserProfiles.findOneByOrFail({ userId: user.id }); | !profile.emailVerified
| resetRequestExists > 0
// 合致するメアドが登録されていなかったら無視 ) {
if (profile.email !== ps.email) {
return;
}
// メアドが認証されていなかったら無視
if (!profile.emailVerified) {
return; return;
} }
@ -69,6 +66,6 @@ export default define(meta, paramDef, async (ps) => {
const link = `${config.url}/reset-password/${token}`; const link = `${config.url}/reset-password/${token}`;
sendEmail(ps.email, 'Password reset requested', sendEmail(ps.email, 'Password reset requested',
`To reset password, please click this link:<br><a href="${link}">${link}</a>`, `Hello ${user.username}!<p>You requested to reset your password. To do so, please follow this link:</p><a href="${link}">${link}</a><p>The link is valid for 30 minutes.</p>`,
`To reset password, please click this link: ${link}`); `Hello ${user.username}!\nYou requested to reset your password. To do so, please follow this link: ${link}\nThe link is valid for 30 minutes.`);
}); });

View file

@ -1,5 +1,6 @@
import bcrypt from 'bcryptjs'; import bcrypt from 'bcryptjs';
import { UserProfiles, PasswordResetRequests } from '@/models/index.js'; import { UserProfiles, PasswordResetRequests } from '@/models/index.js';
import { MINUTE } from '@/const.js';
import define from '../define.js'; import define from '../define.js';
export const meta = { export const meta = {
@ -10,7 +11,11 @@ export const meta = {
description: 'Complete the password reset that was previously requested.', description: 'Complete the password reset that was previously requested.',
errors: { errors: {
noSuchResetRequest: {
message: 'No such password reset request.',
code: 'NO_SUCH_RESET_REQUEST',
id: '2c455231-675b-41be-b313-1181646a3ad3',
},
}, },
} as const; } as const;
@ -25,13 +30,20 @@ export const paramDef = {
// eslint-disable-next-line import/no-default-export // eslint-disable-next-line import/no-default-export
export default define(meta, paramDef, async (ps, user) => { export default define(meta, paramDef, async (ps, user) => {
const req = await PasswordResetRequests.findOneByOrFail({ const req = await PasswordResetRequests.findOneBy({
token: ps.token, token: ps.token,
}); });
// 発行してから30分以上経過していたら無効 if (req == null) throw new ApiError(meta.errors.noSuchResetRequest);
if (Date.now() - req.createdAt.getTime() > 1000 * 60 * 30) {
throw new Error(); // TODO // Check that the has not expired yet after 30 minutes.
Johann150 marked this conversation as resolved
Review

Guessing that the comment meant to say

// Check that the request has not expired yet after 30 minutes.
Guessing that the comment meant to say ``` // Check that the request has not expired yet after 30 minutes. ```
//
// This is a secondary check just in case the expiry task is broken,
// the expiry task schedule is not aligned to the timing, or something
// else strange is going on. After all this is security critical.
if (Date.now() - req.createdAt.getTime() > 30 * MINUTE) {
await PasswordResetRequests.delete(req.id);
throw new ApiError(meta.errors.noSuchResetRequest);
} }
// Generate hash of password // Generate hash of password
@ -42,5 +54,5 @@ export default define(meta, paramDef, async (ps, user) => {
password: hash, password: hash,
}); });
PasswordResetRequests.delete(req.id); await PasswordResetRequests.delete(req.id);
}); });