Skip to content

Commit adc2400

Browse files
titanismtitanism
committed
fix: drop scmp in favor of native node.js timing comparison, optimize onAuth (reduced speed by ~100ms), updated snapshots
1 parent b9d0b74 commit adc2400

File tree

9 files changed

+688
-553
lines changed

9 files changed

+688
-553
lines changed

‎helpers/is-valid-password.js‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,32 @@
33
* SPDX-License-Identifier: BUSL-1.1
44
*/
55

6+
const crypto = require('node:crypto');
67
const { Buffer } = require('node:buffer');
78

89
//
910
// NOTE: scmp and tsse appear to be identical
1011
// <https://github.com/freewil/scmp/issues/18>
1112
// <https://github.com/simonepri/tsse/issues/5>
1213
//
13-
const scmp = require('scmp');
14+
// const scmp = require('scmp');
1415
// const tsse = require('tsse');
1516

1617
const pbkdf2 = require('./pbkdf2');
1718

1819
const config = require('#config');
1920

21+
function timingSafeCompare(a, b) {
22+
if (a.length !== b.length) {
23+
// To make this timing-safe, we can compare the hash of the user-provided
24+
// value with itself, which will take a constant amount of time.
25+
crypto.timingSafeEqual(a, a);
26+
return false;
27+
}
28+
29+
return crypto.timingSafeEqual(a, b);
30+
}
31+
2032
async function isValidPassword(tokens = [], password) {
2133
if (
2234
typeof tokens !== 'object' ||
@@ -46,12 +58,12 @@ async function isValidPassword(tokens = [], password) {
4658
digestAlgorithm: config.passportLocalMongoose.digestAlgorithm
4759
});
4860

49-
if (
50-
scmp(
51-
rawHash,
52-
Buffer.from(token.hash, config.passportLocalMongoose.encoding)
53-
)
54-
) {
61+
// const scmpResult = scmp(
62+
const scmpResult = timingSafeCompare(
63+
rawHash,
64+
Buffer.from(token.hash, config.passportLocalMongoose.encoding)
65+
);
66+
if (scmpResult) {
5567
match = true;
5668
break;
5769
}

‎helpers/on-auth.js‎

Lines changed: 173 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,24 @@ async function onAuth(auth, session, fn) {
4444
// TODO: replace usage of config.recordPrefix with config.paidPrefix and config.freePrefix
4545

4646
try {
47+
// Cache server type checks for performance (conditional logic simplification)
48+
const isIMAPServer = this.server instanceof IMAPServer;
49+
const isPOP3Server = this.server instanceof POP3Server;
50+
const isIMAPorPOP3 = isIMAPServer || isPOP3Server;
51+
const isCalDAV = this?.constructor?.name === 'CalDAV';
52+
const isCardDAV = this?.constructor?.name === 'CardDAV';
53+
const isAPI = this?.constructor?.name === 'API';
54+
const isIMAP = this?.constructor?.name === 'IMAP';
55+
const isPOP3 = this?.constructor?.name === 'POP3';
56+
4757
//
4858
// NOTE: until onConnect is available for IMAP and POP3 servers
4959
// we leverage the existing SMTP helper in the interim
5060
// <https://github.com/nodemailer/wildduck/issues/540>
5161
// <https://github.com/nodemailer/wildduck/issues/721>
5262
// (see this same comment in `helpers/on-connect.js`)
5363
//
54-
if (this.server instanceof IMAPServer || this.server instanceof POP3Server)
55-
await onConnectPromise.call(this, session);
64+
if (isIMAPorPOP3) await onConnectPromise.call(this, session);
5665

5766
// check if server is in the process of shutting down
5867
if (this.isClosing) throw new ServerShutdownError();
@@ -62,7 +71,7 @@ async function onAuth(auth, session, fn) {
6271

6372
// NOTE: WildDuck POP3 doesn't expose socket on session yet (also see similar comment in onAuth helper)
6473
// check if socket is still connected (only applicable for IMAP and POP3 servers)
65-
if (this.server instanceof IMAPServer) {
74+
if (isIMAPServer) {
6675
const socket =
6776
(session.socket && session.socket._parent) || session.socket;
6877
if (!socket || socket?.destroyed || socket?.readyState !== 'open')
@@ -71,8 +80,7 @@ async function onAuth(auth, session, fn) {
7180

7281
// NOTE: this is only required for WildDuck servers (IMAP/POP3)
7382
// override session.getQueryResponse (safeguard)
74-
if (this.server instanceof IMAPServer || this.server instanceof POP3Server)
75-
session.getQueryResponse = getQueryResponse;
83+
if (isIMAPorPOP3) session.getQueryResponse = getQueryResponse;
7684

7785
// username must be a valid email address
7886
if (
@@ -163,55 +171,163 @@ async function onAuth(auth, session, fn) {
163171
}
164172
);
165173

166-
const domain = await Domains.findOne({
167-
name: domainName,
168-
verification_record: verifications[0],
169-
plan: { $in: ['enhanced_protection', 'team'] }
170-
})
171-
.populate(
172-
'members.user',
173-
`id group plan ${config.userFields.isBanned} ${config.userFields.hasVerifiedEmail} ${config.userFields.planExpiresAt} ${config.userFields.stripeSubscriptionID} ${config.userFields.paypalSubscriptionID} timezone`
174-
)
175-
.select('+tokens.hash +tokens.salt')
176-
.lean()
177-
.exec();
174+
//
175+
// OPTIMIZATION: Combined domain and alias query using aggregation pipeline
176+
// This reduces two separate database queries into one
177+
//
178+
179+
const results = await Domains.aggregate([
180+
{
181+
$match: {
182+
name: domainName,
183+
verification_record: verifications[0],
184+
plan: { $in: ['enhanced_protection', 'team'] }
185+
}
186+
},
187+
{
188+
$lookup: {
189+
from: 'aliases',
190+
let: { domainId: '$_id' },
191+
pipeline: [
192+
{
193+
$match: {
194+
$expr: {
195+
$and: [
196+
{ $eq: ['$domain', '$$domainId'] },
197+
{ $eq: ['$name', name] }
198+
]
199+
}
200+
}
201+
},
202+
{
203+
$lookup: {
204+
from: 'users',
205+
localField: 'user',
206+
foreignField: '_id',
207+
as: 'user'
208+
}
209+
},
210+
{
211+
$unwind: {
212+
path: '$user',
213+
preserveNullAndEmptyArrays: true
214+
}
215+
},
216+
{
217+
$addFields: {
218+
user: {
219+
_id: '$user._id',
220+
id: '$user.id',
221+
[config.userFields
222+
.isBanned]: `$user.${config.userFields.isBanned}`,
223+
[config.userFields
224+
.smtpLimit]: `$user.${config.userFields.smtpLimit}`,
225+
email: '$user.email',
226+
[config.lastLocaleField]: `$user.${config.lastLocaleField}`,
227+
timezone: '$user.timezone'
228+
}
229+
}
230+
}
231+
],
232+
as: 'alias'
233+
}
234+
},
235+
{
236+
$lookup: {
237+
from: 'users',
238+
localField: 'members.user',
239+
foreignField: '_id',
240+
as: 'memberUsers'
241+
}
242+
},
243+
{
244+
$addFields: {
245+
members: {
246+
$map: {
247+
input: '$members',
248+
as: 'member',
249+
in: {
250+
$mergeObjects: [
251+
'$$member',
252+
{
253+
user: {
254+
$arrayElemAt: [
255+
{
256+
$filter: {
257+
input: '$memberUsers',
258+
as: 'u',
259+
cond: { $eq: ['$$u._id', '$$member.user'] }
260+
}
261+
},
262+
0
263+
]
264+
}
265+
}
266+
]
267+
}
268+
}
269+
}
270+
}
271+
},
272+
{
273+
$project: {
274+
memberUsers: 0,
275+
'members.user.password': 0,
276+
'members.user.api_token': 0,
277+
'members.user.otp_recovery_keys': 0,
278+
'members.user.otp_enabled': 0,
279+
'members.user.reset_token_expires_at': 0,
280+
'members.user.reset_token': 0
281+
}
282+
},
283+
{
284+
$addFields: {
285+
alias: {
286+
$cond: {
287+
if: { $gt: [{ $size: '$alias' }, 0] },
288+
// eslint-disable-next-line unicorn/no-thenable
289+
then: { $arrayElemAt: ['$alias', 0] },
290+
else: null
291+
}
292+
}
293+
}
294+
}
295+
]);
296+
297+
const result = results[0];
298+
if (!result) {
299+
throw new SMTPError(
300+
`Domain not found or does not have a valid plan, go to ${
301+
config.urls.web
302+
}/my-account/domains/${punycode.toASCII(
303+
domainName
304+
)} and click "Verify"`,
305+
{
306+
responseCode: 535,
307+
ignoreHook: true
308+
}
309+
);
310+
}
311+
312+
const domain = result;
313+
const alias = name === '*' ? null : result.alias;
178314

179315
// validate domain
180316
validateDomain(domain, domainName);
181317

182-
let alias;
183-
if (name !== '*')
184-
alias = await Aliases.findOne({
185-
name,
186-
domain: domain._id
187-
})
188-
.populate(
189-
'user',
190-
`id ${config.userFields.isBanned} ${config.userFields.smtpLimit} email ${config.lastLocaleField} timezone`
191-
)
192-
.select('+tokens.hash +tokens.salt +is_rekey')
193-
.lean()
194-
.exec();
195-
196318
// validate alias (will throw an error if !alias)
197-
if (
198-
alias ||
199-
this.server instanceof IMAPServer ||
200-
this.server instanceof POP3Server
201-
)
202-
validateAlias(alias, domain.name, name);
319+
if (alias || isIMAPorPOP3) validateAlias(alias, domain.name, name);
203320

204321
//
205322
// validate the `auth.password` provided
206323
//
207324

208325
// IMAP/POP3/CalDAV/CardDAV/API servers can only validate against aliases
209326
if (
210-
this.server instanceof IMAPServer ||
211-
this.server instanceof POP3Server ||
212-
(alias && this?.constructor?.name === 'CalDAV') ||
213-
(alias && this?.constructor?.name === 'CardDAV') ||
214-
(alias && this?.constructor?.name === 'API')
327+
isIMAPorPOP3 ||
328+
(alias && isCalDAV) ||
329+
(alias && isCardDAV) ||
330+
(alias && isAPI)
215331
) {
216332
if (
217333
alias &&
@@ -295,8 +411,7 @@ async function onAuth(auth, session, fn) {
295411
// we allow users to use a generated token for the domain
296412
//
297413
if (
298-
!(this.server instanceof IMAPServer) &&
299-
!(this.server instanceof POP3Server) &&
414+
!isIMAPorPOP3 &&
300415
!isValid &&
301416
Array.isArray(domain.tokens) &&
302417
domain.tokens.length > 0
@@ -349,11 +464,16 @@ async function onAuth(auth, session, fn) {
349464
);
350465
}
351466

352-
// Clear authentication limit for this IP address
353-
await this.client.del(`auth_limit_${config.env}:${session.remoteAddress}`);
354-
355-
// this also ensures that at least one admin has a verified email address
356-
const obj = await Domains.getToAndMajorityLocaleByDomain(domain);
467+
//
468+
// OPTIMIZATION: Parallelize independent async operations
469+
// Clear auth limit and get locale info can run in parallel
470+
//
471+
const [, obj] = await Promise.all([
472+
// Clear authentication limit for this IP address
473+
this.client.del(`auth_limit_${config.env}:${session.remoteAddress}`),
474+
// this also ensures that at least one admin has a verified email address
475+
Domains.getToAndMajorityLocaleByDomain(domain)
476+
]);
357477

358478
const to = [];
359479

@@ -376,7 +496,7 @@ async function onAuth(auth, session, fn) {
376496
// alert them by email to inform them they need to enable SMTP
377497
// (otherwise calendar invites will not be sent out automatically)
378498
//
379-
if (alias && this?.constructor?.name === 'CalDAV' && !domain.has_smtp) {
499+
if (alias && isCalDAV && !domain.has_smtp) {
380500
this.client
381501
.get(`caldav_smtp_check:${domain.id}`)
382502
.then(async (cache) => {
@@ -439,13 +559,7 @@ async function onAuth(auth, session, fn) {
439559
alias &&
440560
!alias.has_imap &&
441561
!_.isDate(alias.imap_not_enabled_sent_at) &&
442-
(this.server instanceof IMAPServer ||
443-
this.server instanceof POP3Server ||
444-
this?.constructor?.name === 'CalDAV' ||
445-
this?.constructor?.name === 'CardDAV' ||
446-
this?.constructor?.name === 'API' ||
447-
this?.constructor?.name === 'IMAP' ||
448-
this?.constructor?.name === 'POP3')
562+
(isIMAPorPOP3 || isCalDAV || isCardDAV || isAPI || isIMAP || isPOP3)
449563
) {
450564
this.client
451565
.get(`imap_check:${alias.id}`)
@@ -538,7 +652,7 @@ async function onAuth(auth, session, fn) {
538652
// NOTE: this is only for IMAP servers
539653
// <https://github.com/zone-eu/wildduck/issues/629#issuecomment-1956910918>
540654
//
541-
if (this?.constructor?.name === 'IMAP' && this.server) {
655+
if (isIMAP && this.server) {
542656
const key = `concurrent_${this.constructor.name.toLowerCase()}_${
543657
config.env
544658
}:${alias ? alias.id : domain.id}`;
@@ -672,14 +786,7 @@ async function onAuth(auth, session, fn) {
672786
//
673787
// if we're on IMAP, POP3, or CalDAV server then sync messages with user
674788
//
675-
if (
676-
alias &&
677-
alias.has_imap &&
678-
(this.server instanceof IMAPServer ||
679-
this.server instanceof POP3Server ||
680-
this?.constructor?.name === 'CalDAV') &&
681-
this.wsp
682-
) {
789+
if (alias && alias.has_imap && (isIMAPorPOP3 || isCalDAV) && this.wsp) {
683790
// sync with tmp db
684791
this.wsp
685792
.request(

‎helpers/pbkdf2.js‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@
44
*/
55

66
const crypto = require('node:crypto');
7+
const { promisify } = require('node:util');
78

8-
const pify = require('pify');
9-
10-
const cryptoPbkdf2 = pify(crypto.pbkdf2);
9+
const cryptoPbkdf2 = promisify(crypto.pbkdf2);
1110

1211
function pbkdf2(options) {
1312
return cryptoPbkdf2(

0 commit comments

Comments
 (0)