feat(mailer): say so when SMTP certificate verification is off

SMTP_SKIP_TLS_VERIFY (and the matching admin setting) turns off certificate
checking for outgoing mail. That escape hatch stays — an install behind an
internal relay with a self-signed certificate depends on it — but until now it
downgraded the connection without a word, and a transport is built per send, so
whoever flipped it had nothing to go on afterwards.

It now warns once per process, naming the host it applies to. Once, not per
mail: a line on every notification is a line nobody reads.
This commit is contained in:
Maurice
2026-08-25 10:45:14 +02:00
committed by Maurice
parent 23196aa038
commit fb76074e7c
2 changed files with 169 additions and 1 deletions
@@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common';
import nodemailer from 'nodemailer';
import { PASSWORD_RESET_I18N } from '@trek/shared/i18n/externalNotifications';
import { readEnv } from '../../../app-config';
import { logError, logInfo, logDebug } from '../../audit/audit-log.logger';
import { logError, logInfo, logDebug, logWarn } from '../../audit/audit-log.logger';
import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto';
import { DatabaseService } from '../../database/database.service';
import { buildEmailHtml, buildPasswordResetHtml } from './email-html';
@@ -28,6 +28,8 @@ interface SmtpConfig {
*/
@Injectable()
export class MailerService {
private skippedTlsWarned = false;
constructor(private readonly db: DatabaseService) {}
private getAppSetting(key: string): string | null {
@@ -61,15 +63,35 @@ export class MailerService {
*/
private createTransport(config: SmtpConfig) {
const skipTls = readEnv().smtp.skipTlsVerify || this.getAppSetting('smtp_skip_tls_verify') === 'true';
if (skipTls) this.warnOnceAboutSkippedTls(config);
return nodemailer.createTransport({
host: config.host,
port: config.port,
secure: config.secure,
auth: config.user ? { user: config.user, pass: config.pass } : undefined,
// The operator's opt-out for a relay with a self-signed certificate, off by
// default and reachable only through SMTP_SKIP_TLS_VERIFY or the matching
// admin setting. It stays because self-hosted installs behind an internal
// relay depend on it; warnOnceAboutSkippedTls is what keeps it from being a
// silent downgrade.
...(skipTls ? { tls: { rejectUnauthorized: false } } : {}),
});
}
/**
* Once per process, not once per mail — a transport is built on every send, and
* a line on every notification would train the operator to scroll past it.
*/
private warnOnceAboutSkippedTls(config: SmtpConfig): void {
if (this.skippedTlsWarned) return;
this.skippedTlsWarned = true;
logWarn(
`SECURITY: SMTP certificate verification is disabled for ${config.host}:${config.port}. ` +
'Mail — including password-reset links and the SMTP credentials — travels over a channel an ' +
'active attacker can impersonate. Keep this on only for a relay you control on a trusted network.',
);
}
/** Is SMTP configured at the instance level? (Independent of any one user's address.) */
isSmtpConfigured(): boolean {
return !!(readEnv().smtp.host || this.getAppSetting('smtp_host'));
@@ -0,0 +1,146 @@
/**
* mailer.service.test.ts
*
* Covers the SMTP transport options MailerService hands to nodemailer, and in
* particular the skip-TLS opt-out: it must stay reachable for operators behind an
* internal relay, and it must announce itself instead of downgrading quietly.
* Constructed directly (no TestingModule, repo convention).
*/
const { testDb, dbMock } = vi.hoisted(() => {
const Database = require('better-sqlite3');
const db = new Database(':memory:');
db.exec('PRAGMA journal_mode = WAL');
db.exec('PRAGMA foreign_keys = ON');
db.exec('PRAGMA busy_timeout = 5000');
const mock = {
db,
closeDb: () => {},
reinitialize: () => {},
canAccessTrip: () => undefined,
isOwner: () => false,
};
return { testDb: db, dbMock: mock };
});
const { sendMail, createTransport } = vi.hoisted(() => {
const send = vi.fn().mockResolvedValue({ messageId: 'test' });
return {
sendMail: send,
createTransport: vi.fn((_options: Record<string, unknown>) => ({ sendMail: send })),
};
});
vi.mock('../../../src/db/database', () => dbMock);
vi.mock('nodemailer', () => ({ default: { createTransport } }));
vi.mock('../../../src/nest/audit/audit-log.logger', () => ({
logInfo: vi.fn(),
logDebug: vi.fn(),
logError: vi.fn(),
logWarn: vi.fn(),
}));
import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vitest';
import { createTables } from '../../../src/db/schema';
import { runMigrations } from '../../../src/db/migrations';
import { resetTestDb } from '../../helpers/test-db';
import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service';
import { DatabaseService } from '../../../src/nest/database/database.service';
import { logWarn } from '../../../src/nest/audit/audit-log.logger';
function setAppSetting(key: string, value: string): void {
testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run(key, value);
}
/** The minimum that makes getSmtpConfig() return a config instead of null. */
function configureSmtp(): void {
setAppSetting('smtp_host', 'mail.internal.example');
setAppSetting('smtp_port', '587');
setAppSetting('smtp_from', 'trek@example.com');
}
function newMailer(): MailerService {
return new MailerService(new DatabaseService(testDb));
}
/** The options object of the most recent nodemailer.createTransport() call. */
function lastTransportOptions(): Record<string, unknown> {
const calls = createTransport.mock.calls;
return calls[calls.length - 1][0] as Record<string, unknown>;
}
beforeAll(() => {
createTables(testDb);
runMigrations(testDb);
});
beforeEach(() => {
resetTestDb(testDb);
vi.clearAllMocks();
});
afterAll(() => {
testDb.close();
});
describe('MailerService TLS options', () => {
it('MAILER-001: leaves certificate verification on by default', async () => {
configureSmtp();
expect(await newMailer().sendEmail('someone@example.com', 'Subject', 'Body')).toBe(true);
expect(lastTransportOptions()).not.toHaveProperty('tls');
expect(logWarn).not.toHaveBeenCalled();
});
it('MAILER-002: the smtp_skip_tls_verify setting turns verification off', async () => {
configureSmtp();
setAppSetting('smtp_skip_tls_verify', 'true');
await newMailer().sendEmail('someone@example.com', 'Subject', 'Body');
expect(lastTransportOptions().tls).toEqual({ rejectUnauthorized: false });
});
it('MAILER-003: anything other than "true" leaves verification on', async () => {
configureSmtp();
setAppSetting('smtp_skip_tls_verify', 'false');
await newMailer().sendEmail('someone@example.com', 'Subject', 'Body');
expect(lastTransportOptions()).not.toHaveProperty('tls');
expect(logWarn).not.toHaveBeenCalled();
});
it('MAILER-004: skipping verification is announced, and names the host it applies to', async () => {
configureSmtp();
setAppSetting('smtp_skip_tls_verify', 'true');
await newMailer().sendEmail('someone@example.com', 'Subject', 'Body');
expect(logWarn).toHaveBeenCalledTimes(1);
const warning = vi.mocked(logWarn).mock.calls[0][0];
expect(warning).toContain('mail.internal.example:587');
expect(warning).toContain('SECURITY');
});
it('MAILER-005: the warning is logged once per process, not once per mail', async () => {
configureSmtp();
setAppSetting('smtp_skip_tls_verify', 'true');
const mailer = newMailer();
await mailer.sendEmail('first@example.com', 'One', 'Body');
await mailer.sendEmail('second@example.com', 'Two', 'Body');
await mailer.sendPasswordResetEmail('third@example.com', 'https://trek.example/reset', null);
expect(createTransport).toHaveBeenCalledTimes(3);
expect(logWarn).toHaveBeenCalledTimes(1);
});
it('MAILER-006: a send with no SMTP configured builds no transport at all', async () => {
expect(await newMailer().sendEmail('someone@example.com', 'Subject', 'Body')).toBe(false);
expect(createTransport).not.toHaveBeenCalled();
expect(sendMail).not.toHaveBeenCalled();
});
});