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<string> instead of Promise<string | false | undefined>. 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
This commit is contained in:
1 parent
f97ca8565a
commit
c35b22e1ac
9 files changed
+119
-49
No files matched your search
@@ -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({
|
||||
|
||||
@@ -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" },
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
@@ -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<boolean> {
|
||||
@@ -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 {
|
||||
|
||||
@@ -30,7 +30,7 @@ export interface IUserService {
|
||||
currentUserEmail: string
|
||||
): Promise<User>;
|
||||
checkSuperadminExists(): Promise<boolean>;
|
||||
requestRecovery(email: string): Promise<string | false | undefined>;
|
||||
requestRecovery(email: string): Promise<string>;
|
||||
validateRecovery(recoveryToken: string): Promise<void>;
|
||||
resetPassword(password: string, recoveryToken: string): Promise<{ user: User; token: string }>;
|
||||
deleteUser(params: { userId: string; teamId: string; roles: UserRole[] }): Promise<void>;
|
||||
|
||||
@@ -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<string, unknown>)
|
||||
export interface IEmailService {
|
||||
init(): void;
|
||||
buildEmail(template: string, context: Record<string, unknown>): Promise<string | undefined>;
|
||||
sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise<string | false | undefined>;
|
||||
sendEmail(to: string, subject: string, html: string, transportConfig?: EmailTransportConfig): Promise<string>;
|
||||
}
|
||||
|
||||
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" },
|
||||
});
|
||||
}
|
||||
};
|
||||
}
|
||||
@@ -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" }));
|
||||
});
|
||||
|
||||
@@ -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", "<p>body</p>", makeTransportConfig());
|
||||
|
||||
expect(result).toBe(false);
|
||||
await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", 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", "<p>body</p>", 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", "<p>body</p>", makeTransportConfig());
|
||||
|
||||
expect(result).toBe(false);
|
||||
await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", 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", "<p>body</p>", makeTransportConfig());
|
||||
|
||||
expect(result).toBeUndefined();
|
||||
await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", 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", "<p>body</p>", makeTransportConfig());
|
||||
|
||||
expect(result).toBeUndefined();
|
||||
await expect(service.sendEmail("to@example.com", "Subject", "<p>body</p>", makeTransportConfig())).rejects.toMatchObject({
|
||||
details: { cause: "Unknown error" },
|
||||
});
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
message: "Unknown error",
|
||||
|
||||
@@ -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("<html>ok</html>"),
|
||||
sendEmail: jest.fn().mockResolvedValue(false),
|
||||
sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")),
|
||||
},
|
||||
});
|
||||
|
||||
|
||||
@@ -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("<html>Welcome</html>"),
|
||||
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("<html>Reset</html>"),
|
||||
sendEmail: jest.fn().mockRejectedValue(new Error("SMTP auth failed")),
|
||||
},
|
||||
});
|
||||
|
||||
await expect(service.requestRecovery("test@example.com")).rejects.toThrow("SMTP auth failed");
|
||||
});
|
||||
});
|
||||
|
||||
// ── validateRecovery ────────────────────────────────────────────────────
|
||||
|
||||
Reference in new issue
Block a user