From c35b22e1acf2a539bc90913dd35a0d543972f421 Mon Sep 17 00:00:00 2001 From: egeoztass Date: Fri, 14 Aug 2026 14:11:16 +0300 Subject: [PATCH] fix: propagate SMTP transport errors from EmailService.sendEmail sendEmail caught transport failures internally and resolved to false or undefined, so every caller's .catch() was dead code and send failures were silently suppressed. Password recovery in particular reported "email sent successfully" while the user received nothing. sendEmail now throws an AppError carrying the underlying cause, and returns Promise instead of Promise. Call sites updated for the new contract: - UserService.registerUser: the existing .catch() now actually fires - UserService.requestRecovery: the failure reaches the client instead of a misleading success response - InviteService and the test-email endpoint: keep their user-facing messages and attach the SMTP cause - EmailProvider: catches and returns false, because NotificationsService fans providers out through Promise.all and a rejection there would abort the other channels' alerts Closes #3757 --- .../src/api/controllers/settingsController.ts | 42 +++++++++++-------- server/src/domain/invites/invite.service.ts | 6 ++- .../domain/notifications/providers/email.ts | 18 +++++--- server/src/domain/users/user.service.ts | 2 +- server/src/service/emailService.ts | 16 ++++++- .../notifications/emailProvider.test.ts | 10 +++-- .../test/unit/services/emailService.test.ts | 41 +++++++++++------- .../test/unit/services/inviteService.test.ts | 4 +- server/test/unit/services/userService.test.ts | 29 +++++++++++++ 9 files changed, 119 insertions(+), 49 deletions(-) diff --git a/server/src/api/controllers/settingsController.ts b/server/src/api/controllers/settingsController.ts index 3cb032e93..a8113ed2b 100644 --- a/server/src/api/controllers/settingsController.ts +++ b/server/src/api/controllers/settingsController.ts @@ -93,24 +93,30 @@ class SettingsController implements ISettingsController { if (!html) { throw new AppError({ message: "Failed to build email template.", status: 500 }); } - const messageId = await this.emailService.sendEmail(to, subject, html, { - systemEmailHost, - systemEmailPort, - systemEmailUser, - systemEmailAddress, - systemEmailDisplayName, - systemEmailPassword, - systemEmailConnectionHost, - systemEmailSecure, - systemEmailPool, - systemEmailIgnoreTLS, - systemEmailRequireTLS, - systemEmailRejectUnauthorized, - systemEmailTLSServername, - }); - - if (!messageId) { - throw new AppError({ message: "Failed to send test email.", status: 500 }); + let messageId: string; + try { + messageId = await this.emailService.sendEmail(to, subject, html, { + systemEmailHost, + systemEmailPort, + systemEmailUser, + systemEmailAddress, + systemEmailDisplayName, + systemEmailPassword, + systemEmailConnectionHost, + systemEmailSecure, + systemEmailPool, + systemEmailIgnoreTLS, + systemEmailRequireTLS, + systemEmailRejectUnauthorized, + systemEmailTLSServername, + }); + } catch (error: unknown) { + // Surface the underlying SMTP failure: diagnosing the settings is the whole + // point of the test email endpoint. + throw new AppError({ + message: error instanceof Error ? `Failed to send test email. ${error.message}` : "Failed to send test email.", + status: 500, + }); } return res.status(200).json({ diff --git a/server/src/domain/invites/invite.service.ts b/server/src/domain/invites/invite.service.ts index 3dcffd9e0..8182f4026 100644 --- a/server/src/domain/invites/invite.service.ts +++ b/server/src/domain/invites/invite.service.ts @@ -96,13 +96,15 @@ export class InviteService implements IInviteService { }); } - const result = await this.emailService.sendEmail(invite.email, "Welcome to Uptime Monitor", html); - if (!result) { + try { + await this.emailService.sendEmail(invite.email, "Welcome to Uptime Monitor", html); + } catch (error: unknown) { throw new AppError({ message: "Failed to send invite e-mail... Please verify your settings.", service: SERVICE_NAME, method: "sendInviteEmail", status: 500, + details: { cause: error instanceof Error ? error.message : "Unknown error" }, }); } }; diff --git a/server/src/domain/notifications/providers/email.ts b/server/src/domain/notifications/providers/email.ts index fa84be66d..2b5cde245 100644 --- a/server/src/domain/notifications/providers/email.ts +++ b/server/src/domain/notifications/providers/email.ts @@ -35,16 +35,18 @@ export class EmailProvider extends NotificationProvider { return false; } - const messageId = await this.emailService.sendEmail(notification.address, subject, html); - if (!messageId) { + try { + await this.emailService.sendEmail(notification.address, subject, html); + return true; + } catch (error: unknown) { this.logger.warn({ message: "Email test alert failed", service: SERVICE_NAME, method: "sendTestAlert", + stack: error instanceof Error ? error.stack : undefined, }); return false; } - return true; } async sendMessage(notification: Notification, message: NotificationMessage): Promise { @@ -64,16 +66,20 @@ export class EmailProvider extends NotificationProvider { return false; } - const messageId = await this.emailService.sendEmail(notification.address, subject, html); - if (!messageId) { + // Providers must resolve to a boolean: NotificationsService fans these out through + // Promise.all, so a rejection here would abort the other channels' alerts too. + try { + await this.emailService.sendEmail(notification.address, subject, html); + return true; + } catch (error: unknown) { this.logger.warn({ message: "Email notification failed", service: SERVICE_NAME, method: "sendMessage", + stack: error instanceof Error ? error.stack : undefined, }); return false; } - return true; } private buildSubject(message: NotificationMessage): string { diff --git a/server/src/domain/users/user.service.ts b/server/src/domain/users/user.service.ts index f042863aa..58a3ec1a3 100644 --- a/server/src/domain/users/user.service.ts +++ b/server/src/domain/users/user.service.ts @@ -30,7 +30,7 @@ export interface IUserService { currentUserEmail: string ): Promise; checkSuperadminExists(): Promise; - requestRecovery(email: string): Promise; + requestRecovery(email: string): Promise; validateRecovery(recoveryToken: string): Promise; resetPassword(password: string, recoveryToken: string): Promise<{ user: User; token: string }>; deleteUser(params: { userId: string; teamId: string; roles: UserRole[] }): Promise; diff --git a/server/src/service/emailService.ts b/server/src/service/emailService.ts index 26b89f84b..3aca5362c 100755 --- a/server/src/service/emailService.ts +++ b/server/src/service/emailService.ts @@ -2,6 +2,7 @@ import { fileURLToPath } from "url"; import { EmailTransportConfig } from "@/domain/app-settings/app-settings.type.js"; import { ISettingsService } from "@/domain/app-settings/app-settings.service.js"; import { ILogger } from "@/utils/logger.js"; +import { AppError } from "@/utils/AppError.js"; import fs from "node:fs"; import path from "node:path"; import nodemailer from "nodemailer"; @@ -20,7 +21,7 @@ type TemplateCompiler = (template: string) => (context: Record) export interface IEmailService { init(): void; buildEmail(template: string, context: Record): Promise; - sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise; + sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise; } export class EmailService implements IEmailService { @@ -151,7 +152,12 @@ export class EmailService implements IEmailService { method: "verifyTransporter", stack: error instanceof Error ? error.stack : undefined, }); - return false; + throw new AppError({ + message: "Email transporter verification failed", + service: SERVICE_NAME, + method: "sendEmail", + details: { cause: error instanceof Error ? error.message : "Unknown error" }, + }); } const trimmedDisplayName = systemEmailDisplayName?.trim(); @@ -172,6 +178,12 @@ export class EmailService implements IEmailService { method: "sendEmail", stack: error instanceof Error ? error.stack : undefined, }); + throw new AppError({ + message: "Failed to send email", + service: SERVICE_NAME, + method: "sendEmail", + details: { cause: error instanceof Error ? error.message : "Unknown error" }, + }); } }; } diff --git a/server/test/unit/providers/notifications/emailProvider.test.ts b/server/test/unit/providers/notifications/emailProvider.test.ts index fecce8ef6..7f3b34d4e 100644 --- a/server/test/unit/providers/notifications/emailProvider.test.ts +++ b/server/test/unit/providers/notifications/emailProvider.test.ts @@ -50,9 +50,9 @@ describe("EmailProvider", () => { expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Failed to build test email content" })); }); - it("returns false when sendEmail returns falsy", async () => { + it("returns false when sendEmail rejects", async () => { const { provider, emailService, logger } = createProvider(); - (emailService.sendEmail as jest.Mock).mockResolvedValue(false); + (emailService.sendEmail as jest.Mock).mockRejectedValue(new Error("SMTP auth failed")); expect(await provider.sendTestAlert(makeNotification())).toBe(false); expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Email test alert failed" })); }); @@ -82,9 +82,11 @@ describe("EmailProvider", () => { expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Failed to build email content" })); }); - it("returns false when sendEmail returns falsy", async () => { + // NotificationsService fans providers out through Promise.all, so a failed email must + // resolve to false rather than reject and take the other channels down with it. + it("returns false when sendEmail rejects", async () => { const { provider, emailService, logger } = createProvider(); - (emailService.sendEmail as jest.Mock).mockResolvedValue(undefined); + (emailService.sendEmail as jest.Mock).mockRejectedValue(new Error("SMTP auth failed")); expect(await provider.sendMessage(makeNotification() as any, makeMessage())).toBe(false); expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: "Email notification failed" })); }); diff --git a/server/test/unit/services/emailService.test.ts b/server/test/unit/services/emailService.test.ts index 971c712da..d18b83917 100644 --- a/server/test/unit/services/emailService.test.ts +++ b/server/test/unit/services/emailService.test.ts @@ -326,15 +326,17 @@ describe("EmailService", () => { expect(nodemailer.createTransport).toHaveBeenCalledWith(expect.objectContaining({ name: "localhost" })); }); - it("returns false when transporter verification fails", async () => { + // Callers wrap sendEmail in .catch()/try-catch to react to delivery failures, so a + // transport error has to reject rather than resolve to a falsy value. + it("throws when transporter verification fails", async () => { const transporter = createMockTransporter(); (transporter.verify as jest.Mock).mockRejectedValue(new Error("SMTP auth failed")); const nodemailer = createMockNodemailer(transporter); const { service, logger } = createService({ nodemailer }); - const result = await service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig()); - - expect(result).toBe(false); + await expect(service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig())).rejects.toThrow( + "Email transporter verification failed" + ); expect(logger.warn).toHaveBeenCalledWith( expect.objectContaining({ message: "Email transporter verification failed", @@ -344,27 +346,38 @@ describe("EmailService", () => { ); }); + it("reports the underlying cause when verification fails", async () => { + const transporter = createMockTransporter(); + (transporter.verify as jest.Mock).mockRejectedValue(new Error("SMTP auth failed")); + const nodemailer = createMockNodemailer(transporter); + const { service } = createService({ nodemailer }); + + await expect(service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig())).rejects.toMatchObject({ + status: 500, + details: { cause: "SMTP auth failed" }, + }); + }); + it("logs stack as undefined when verification fails with non-Error", async () => { const transporter = createMockTransporter(); (transporter.verify as jest.Mock).mockRejectedValue("connection refused"); const nodemailer = createMockNodemailer(transporter); const { service, logger } = createService({ nodemailer }); - const result = await service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig()); - - expect(result).toBe(false); + await expect(service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig())).rejects.toThrow(); expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ stack: undefined })); }); - it("returns undefined and logs error when sendMail throws", async () => { + it("throws and logs error when sendMail throws", async () => { const transporter = createMockTransporter(); (transporter.sendMail as jest.Mock).mockRejectedValue(new Error("Recipient rejected")); const nodemailer = createMockNodemailer(transporter); const { service, logger } = createService({ nodemailer }); - const result = await service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig()); - - expect(result).toBeUndefined(); + await expect(service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig())).rejects.toMatchObject({ + message: "Failed to send email", + details: { cause: "Recipient rejected" }, + }); expect(logger.error).toHaveBeenCalledWith( expect.objectContaining({ message: "Recipient rejected", @@ -380,9 +393,9 @@ describe("EmailService", () => { const nodemailer = createMockNodemailer(transporter); const { service, logger } = createService({ nodemailer }); - const result = await service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig()); - - expect(result).toBeUndefined(); + await expect(service.sendEmail("to@example.com", "Subject", "

body

", makeTransportConfig())).rejects.toMatchObject({ + details: { cause: "Unknown error" }, + }); expect(logger.error).toHaveBeenCalledWith( expect.objectContaining({ message: "Unknown error", diff --git a/server/test/unit/services/inviteService.test.ts b/server/test/unit/services/inviteService.test.ts index 59d472539..d60b0830e 100644 --- a/server/test/unit/services/inviteService.test.ts +++ b/server/test/unit/services/inviteService.test.ts @@ -127,11 +127,11 @@ describe("InviteService", () => { ).rejects.toThrow("Failed to build invite e-mail"); }); - it("throws 500 when sendEmail returns falsy", async () => { + it("throws 500 when sendEmail rejects", async () => { const { service } = createService({ emailService: { buildEmail: jest.fn().mockResolvedValue("ok"), - sendEmail: jest.fn().mockResolvedValue(false), + sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")), }, }); diff --git a/server/test/unit/services/userService.test.ts b/server/test/unit/services/userService.test.ts index 5187a3c60..ee47ffded 100644 --- a/server/test/unit/services/userService.test.ts +++ b/server/test/unit/services/userService.test.ts @@ -196,6 +196,22 @@ describe("UserService", () => { expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ method: "registerUser" })); }); + // The welcome email is fire-and-forget, but its .catch() only runs if sendEmail + // actually rejects; swallowing the transport error made that handler dead code. + it("logs a warning when the welcome email fails to send", async () => { + const { service, logger } = createService({ + emailService: { + buildEmail: jest.fn().mockResolvedValue("Welcome"), + sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")), + }, + }); + + const result = await service.registerUser({ password: "pass" }, "invite-token", null); + + expect(result.user).toBeDefined(); + expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ method: "registerUser", message: "SMTP auth failed" })); + }); + it("uses default role when invite has no role", async () => { const { service, usersRepository } = createService({ invitesRepository: { @@ -423,6 +439,19 @@ describe("UserService", () => { await expect(service.requestRecovery("test@example.com")).rejects.toThrow("Failed to build password reset email HTML"); }); + + // Previously the send failure was swallowed and the endpoint still reported + // "Password recovery email sent successfully" while the user got nothing. + it("propagates the failure when the recovery email cannot be sent", async () => { + const { service } = createService({ + emailService: { + buildEmail: jest.fn().mockResolvedValue("Reset"), + sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")), + }, + }); + + await expect(service.requestRecovery("test@example.com")).rejects.toThrow("SMTP auth failed"); + }); }); // ── validateRecovery ────────────────────────────────────────────────────