Skip to content

Commit 20fd6e4

Browse files
Copilotpaustint
andauthored
fix: address review feedback - fire-and-forget email sends and fix comment wording
Agent-Logs-Url: https://github.com/jetstreamapp/jetstream/sessions/8600f048-7db8-4f29-aa6a-60ac22861223 Co-authored-by: paustint <5461649+paustint@users.noreply.github.com>
1 parent 40f9dcf commit 20fd6e4

2 files changed

Lines changed: 15 additions & 6 deletions

File tree

apps/api/src/app/controllers/__tests__/auth.controller.spec.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,8 @@ describe('auth.controller - placeholder session email suppression', () => {
215215
authServerMocks.ensureAuthError.mockImplementation((error: unknown) => error);
216216
authServerMocks.getApiAddressFromReq.mockReturnValue('127.0.0.1');
217217
authServerMocks.validateRedirectUrl.mockImplementation((url: string) => url || 'https://client.test');
218+
emailMocks.sendEmailVerification.mockResolvedValue(undefined);
219+
emailMocks.sendVerificationCode.mockResolvedValue(undefined);
218220
});
219221

220222
describe('register callback', () => {

apps/api/src/app/controllers/auth.controller.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -608,15 +608,22 @@ const callback = createRoute(
608608

609609
// Suppress verification emails for placeholder sessions (e.g. registration with an already-registered email).
610610
// The verify flow for a placeholder user is a dead-end that destroys the session, so the email would be
611-
// pointless and would spam the real owner of the inbox. The /auth/verify redirect still happens below, so
612-
// the enumeration defense (attacker sees the same response either way) is preserved.
611+
// pointless and would spam the real owner of the inbox. To avoid introducing a timing side-channel between
612+
// placeholder and real-user flows, do not await email delivery here; dispatch it in the background for
613+
// non-placeholder users so both branches return with comparable latency.
613614
const isPlaceholderUser = !!req.session.sessionDetails?.isTemporary || req.session.user.id === PLACEHOLDER_USER_ID;
614615

615616
if (!isPlaceholderUser) {
616617
if (initialVerification.type === 'email') {
617-
await sendEmailVerification(req.session.user.email, initialVerification.token, EMAIL_VERIFICATION_TOKEN_DURATION_HOURS);
618+
void sendEmailVerification(req.session.user.email, initialVerification.token, EMAIL_VERIFICATION_TOKEN_DURATION_HOURS).catch(
619+
(error) => {
620+
console.error('Failed to send email verification', getExceptionLog(error));
621+
}
622+
);
618623
} else if (initialVerification.type === '2fa-email') {
619-
await sendVerificationCode(req.session.user.email, initialVerification.token, TOKEN_DURATION_MINUTES);
624+
void sendVerificationCode(req.session.user.email, initialVerification.token, TOKEN_DURATION_MINUTES).catch((error) => {
625+
console.error('Failed to send verification code', getExceptionLog(error));
626+
});
620627
}
621628
}
622629

@@ -862,8 +869,8 @@ const resendVerification = createRoute(routeDefinition.resendVerification.valida
862869
});
863870

864871
// Suppress verification emails for placeholder sessions (e.g. registration with an already-registered email).
865-
// See the matching guard in the login handler above for details — the verify flow is a dead-end for these
866-
// sessions and the email would only spam the real owner of the inbox.
872+
// See the matching guard in the callback handler for credentials login/register for details — the verify
873+
// flow is a dead-end for these sessions and the email would only spam the real owner of the inbox.
867874
const isPlaceholderUser = !!req.session.sessionDetails?.isTemporary || req.session.user.id === PLACEHOLDER_USER_ID;
868875

869876
if (!isPlaceholderUser) {

0 commit comments

Comments
 (0)