fix: let a deployment name itself in the SMTP greeting
CI / verify (pull_request) Successful in 42s
CI / infrastructure-smoke (pull_request) Successful in 20s
CI / migration-smoke (pull_request) Successful in 1m41s

A real deployment could not send mail through an IP-authorized relay.
Every attempt surfaced as MAIL_PROVIDER_UNAVAILABLE while the relay was
healthy and swaks sent from the same machine.

The cause was the greeting, and it is generic rather than tied to any
provider. An SMTP client identifies itself in EHLO, and with nothing
configured Nodemailer derives that from the machine hostname —
substituting the address literal [127.0.0.1] whenever the hostname is
not fully qualified, which is the normal state of a container. The relay
closed the connection on it with a 421 before any message was offered,
which is indistinguishable from the relay being down.

EKKLESIA_SMTP_HELO_NAME now lets the deployment say what to greet as. It
is validated as a fully qualified domain name — no scheme, port,
brackets, whitespace, or control characters, and at least one dot, since
a single unqualified label is exactly the input Nodemailer already
discards. It is not a credential: it travels in the clear on every
connection, so Doctor prints it where an operator can compare it against
what the relay expects, and Doctor's probe performs the same greeting the
runtime does.

Absent preserves Nodemailer's behaviour exactly. It is deliberately not
defaulted to the sender's domain: a machine whose hostname is already a
valid FQDN may be greeting correctly, and some relays check that against
reverse DNS, so replacing it silently would trade one deployment's bug
for another's.

The regression guard is a raw SMTP server that refuses any greeting but
the expected one, because smtp-server exposes no EHLO hook and its
earliest hook is never reached by a send-nothing probe.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
ekklesia
2026-08-25 20:28:30 -05:00
co-authored by Claude Opus 5
parent 450d65116f
commit 3563549e86
14 changed files with 504 additions and 8 deletions
@@ -363,6 +363,36 @@ describe("Doctor host-configuration parity — outbound email regression", () =>
expect(JSON.stringify(mailCheck)).not.toContain(smtpPassword);
});
it("probes with the SMTP client identity the runtime would use, and reports it", async () => {
const services = dependencies();
const { report } = await diagnose(
environment({ ...mailEnvironment, EKKLESIA_SMTP_HELO_NAME: "mail.example.org" }),
services,
);
const mailCheck = check(report, "outbound-email.configuration");
// Doctor builds its provider from the same validated configuration `startApi` does, so a relay
// that refuses the greeting fails here rather than only when a reset email silently never
// arrives. The name is announced in the clear on every connection, so showing it leaks nothing.
expect(services.probeMail).toHaveBeenCalledWith(
expect.objectContaining({ heloName: "mail.example.org" }),
);
expect(mailCheck.details[0]).toContain("HELO mail.example.org");
expect(JSON.stringify(mailCheck)).not.toContain(smtpPassword);
});
it("reports a malformed SMTP client identity as a typed failure, naming no value", async () => {
const { report } = await diagnose(
environment({ ...mailEnvironment, EKKLESIA_SMTP_HELO_NAME: "smtp://internal-host:2525" }),
);
const mailCheck = check(report, "outbound-email.configuration");
expect(mailCheck.status).toBe("failure");
expect(mailCheck.details[0]).toBe("The SMTP settings were rejected: MAIL_HELO_NAME_INVALID.");
expect(JSON.stringify(mailCheck)).not.toContain("internal-host");
});
it("leaves a malformed SMTP setting reported as a typed configuration failure", async () => {
const { report } = await diagnose(
environment({ ...mailEnvironment, EKKLESIA_SMTP_PORT: "70000" }),
@@ -71,6 +71,7 @@ const INTERPRETED_VARIABLES = Object.freeze([
"EKKLESIA_PUBLIC_SIGNUP_ENABLED",
"EKKLESIA_SMTP_FROM_ADDRESS",
"EKKLESIA_SMTP_FROM_NAME",
"EKKLESIA_SMTP_HELO_NAME",
"EKKLESIA_SMTP_HOST",
"EKKLESIA_SMTP_PASSWORD",
"EKKLESIA_SMTP_PORT",
+5
View File
@@ -20,6 +20,7 @@ const productionEnvironment = Object.freeze({
EKKLESIA_MODULE_SECRET__ANNOUNCEMENTS__STORAGE_CREDENTIAL: "module-secret-material",
EKKLESIA_SMTP_FROM_ADDRESS: "[email protected]",
EKKLESIA_SMTP_FROM_NAME: "Ekklesia",
EKKLESIA_SMTP_HELO_NAME: "mail.example.org",
EKKLESIA_SMTP_HOST: "smtp.example.org",
EKKLESIA_SMTP_PASSWORD: "sup3r-secret-relay-password",
EKKLESIA_SMTP_PORT: "587",
@@ -67,6 +68,7 @@ describe("host configuration loading", () => {
expect(host.mail).toEqual({
fromAddress: "[email protected]",
fromName: "Ekklesia",
heloName: "mail.example.org",
host: "smtp.example.org",
password: "sup3r-secret-relay-password",
port: "587",
@@ -80,6 +82,9 @@ describe("host configuration loading", () => {
});
expect(relayByAddress.mail.username).toBeUndefined();
expect(relayByAddress.mail.password).toBeUndefined();
// The SMTP client identity is optional too, and unset is the ordinary case: it matters only
// where the machine's own hostname is not a fully qualified name the relay accepts.
expect(relayByAddress.mail.heloName).toBeUndefined();
// A source-IP-authorized relay has no credentials at all, and that must still validate.
expect(createMailProviderConfig(relayByAddress.mail)).toBeDefined();
});
+34 -5
View File
@@ -161,15 +161,44 @@ describe("outbound email stays server-only", () => {
});
it("hardcodes no provider hostname in Core", async () => {
// Google Workspace is a deployment target, not the architecture.
// Google Workspace is a deployment target, not the architecture. It is the deployment that
// exposed the generic `EHLO` defect the HELO-name setting fixes, and it is named nowhere the
// code could come to depend on it.
for (const file of await readTextFiles("core/src/mail")) {
expect(file.text.toLowerCase(), file.path).not.toContain("gmail");
expect(file.text.toLowerCase(), file.path).not.toContain("smtp-relay");
expect(file.text.toLowerCase(), file.path).not.toContain("resend");
expect(file.text.toLowerCase(), file.path).not.toContain("brevo");
const text = file.text.toLowerCase();
for (const forbidden of [
"gmail",
"smtp-relay",
"googlemail",
"google.com",
"outlook",
"office365",
"sendgrid",
"mailgun",
"postmark",
"resend",
"brevo",
]) {
expect(text, `${file.path} names ${forbidden}`).not.toContain(forbidden);
}
}
});
it("defaults the SMTP client identity to nothing, so no deployment's name is baked in", async () => {
const config = await readFile(path.join(repositoryRoot, "core/src/mail/config.ts"), "utf8");
const provider = await readFile(
path.join(repositoryRoot, "core/src/mail/smtp-provider.ts"),
"utf8",
);
// The value that fixed a real deployment is configuration, never a constant. A fallback literal
// here would be one church's domain compiled into every installation.
expect(config).not.toContain("riverofglorymn");
expect(provider).not.toContain("riverofglorymn");
// The provider passes it through and supplies nothing of its own when it is absent.
expect(provider).toContain("config.heloName === undefined ? {} : { name: config.heloName }");
});
it("never disables TLS certificate verification", async () => {
for (const file of await readTextFiles("core/src/mail")) {
expect(file.text, file.path).not.toContain("rejectUnauthorized");
+110
View File
@@ -193,6 +193,116 @@ describe("host mail configuration", () => {
});
});
describe("SMTP client identity (HELO name)", () => {
/**
* A real deployment defect, and a generic one.
*
* An SMTP client must greet the server with its own identity. Nodemailer derives that from the
* machine hostname and falls back to the address literal `[127.0.0.1]` when the hostname is not
* fully qualified — the normal state of a container. Some relays close the connection on that
* greeting with a `421` before any message is offered, which reaches a caller as an
* indistinguishable `MAIL_PROVIDER_UNAVAILABLE`. The deployment can now say what to greet as.
*/
it("omits the setting entirely when the deployment configures none", () => {
const config = createMailProviderConfig(validConfig);
// Absent means Nodemailer's own derivation, unchanged. The key is not present at all rather
// than present-and-undefined, so nothing downstream can mistake it for a configured empty name.
expect(config).not.toHaveProperty("heloName");
expect(createMailProviderConfig({ ...validConfig, heloName: "" })).not.toHaveProperty(
"heloName",
);
expect(createMailProviderConfig({ ...validConfig, heloName: undefined })).not.toHaveProperty(
"heloName",
);
});
it.each([
["a two-label domain", "example.org"],
["a subdomain", "mail.example.org"],
["a deep subdomain", "smtp.relay.mail.example.org"],
["hyphens inside labels", "out-bound.example-church.org"],
["digits", "mx1.example2.org"],
["mixed case", "Mail.Example.ORG"],
])("accepts %s", (_label, heloName) => {
expect(createMailProviderConfig({ ...validConfig, heloName })).toMatchObject({ heloName });
});
it.each([
["a scheme", "smtp://example.org"],
["a port", "example.org:587"],
["a path", "example.org/relay"],
["a space", "example org"],
["a leading space", " example.org"],
["a trailing space", "example.org "],
["a carriage return", "example.org\r"],
["a line feed", "example.org\nEHLO evil.example.net"],
["a tab", "example.org\t"],
["a comma", "example.org,evil.example.net"],
["an unqualified single label", "localhost"],
["a machine name with no domain", "ekklesia-host"],
["a bracketed address literal", "[127.0.0.1]"],
["a bare IPv4 address", "192.0.2.1"],
["a trailing dot", "example.org."],
["a leading dot", ".example.org"],
["consecutive dots", "example..org"],
["a leading hyphen in a label", "-example.org"],
["a trailing hyphen in a label", "example-.org"],
["an email address", "[email protected]"],
["prose", "our church mail server"],
["nothing but dots", "..."],
["an over-long name", `${"a".repeat(250)}.example.org`],
])("refuses %s", (_label, heloName) => {
expect(() => createMailProviderConfig({ ...validConfig, heloName })).toThrow(
expect.objectContaining({ code: "MAIL_HELO_NAME_INVALID" }),
);
});
it("refuses an unqualified name, because that is the input the fallback already rejects", () => {
// Configuring a single label would reproduce the very failure this setting exists to fix:
// Nodemailer discards a non-FQDN hostname and substitutes the address literal.
expect(() => createMailProviderConfig({ ...validConfig, heloName: "ekklesia" })).toThrow(
MailConfigError,
);
});
it("never carries the offending value in the error", () => {
try {
createMailProviderConfig({ ...validConfig, heloName: "smtp://secret-internal-name:2525" });
throw new Error("expected a refusal");
} catch (error) {
const serialized = `${(error as Error).message} ${(error as Error).stack ?? ""}`;
expect(serialized).not.toContain("secret-internal-name");
expect(serialized).not.toContain("2525");
}
});
it("is described to an operator, because it is infrastructure rather than a credential", () => {
const described = describeMailProvider(
createMailProviderConfig({
...validConfig,
heloName: "mail.example.org",
password,
username: "[email protected]",
}) as Parameters<typeof describeMailProvider>[0],
);
// It is announced in the clear on every connection, so printing it leaks nothing — and it is
// exactly what an operator needs when a relay has refused the greeting.
expect(described).toContain("HELO mail.example.org");
expect(described).not.toContain(password);
expect(described).not.toContain("[email protected]");
});
it("says nothing about a HELO name when none is configured", () => {
const described = describeMailProvider(
createMailProviderConfig(validConfig) as Parameters<typeof describeMailProvider>[0],
);
expect(described).not.toContain("HELO");
});
});
describe("transactional message validation", () => {
const message = {
subject: "Verify your email address",
+175 -1
View File
@@ -1,4 +1,4 @@
import type { AddressInfo } from "node:net";
import type { AddressInfo, Socket } from "node:net";
import { createServer, type Server } from "node:net";
import { SMTPServer } from "smtp-server";
import { afterEach, describe, expect, it } from "vitest";
@@ -25,6 +25,8 @@ interface CapturedMessage {
readonly from: string;
readonly to: readonly string[];
readonly data: string;
/** The argument this client actually sent in `EHLO`, lowercased by the server. */
readonly greetedAs: string;
}
interface Harness {
@@ -52,6 +54,14 @@ interface HarnessOptions {
readonly rejectRecipients?: readonly string[];
/** Advertises STARTTLS. Off by default, so a `starttls` client must refuse to continue. */
readonly offerStartTls?: boolean;
/**
* Refuses any client whose `EHLO` argument is not exactly this name, as a relay that requires a
* fully qualified client identity does.
*
* The refusal is a `421`, the same class of response the real relay returned: a temporary failure
* that closes the connection before a message is offered.
*/
readonly requireGreeting?: string;
}
async function startSmtpServer(options: HarnessOptions = {}): Promise<Harness> {
@@ -81,11 +91,26 @@ async function startSmtpServer(options: HarnessOptions = {}): Promise<Harness> {
captured.push({
data,
from: session.envelope.mailFrom === false ? "" : session.envelope.mailFrom.address,
greetedAs: session.hostNameAppearsAs,
to: session.envelope.rcptTo.map((entry) => entry.address),
});
callback();
});
},
onMailFrom(_address, session, callback) {
if (
options.requireGreeting !== undefined &&
session.hostNameAppearsAs !== options.requireGreeting.toLowerCase()
) {
const error = new Error("Try again later, closing connection.") as Error & {
responseCode?: number;
};
error.responseCode = 421;
callback(error);
return;
}
callback();
},
onRcptTo(address, _session, callback) {
if (options.rejectRecipients?.includes(address.address)) {
// 550 is a permanent refusal of this mailbox, which is what a real relay returns.
@@ -107,6 +132,64 @@ async function startSmtpServer(options: HarnessOptions = {}): Promise<Harness> {
return { captured, close, port: address.port };
}
/**
* A relay that refuses the greeting itself, byte for byte as the real one did.
*
* `smtp-server` exposes no `EHLO` hook — its earliest usable one is `MAIL FROM`, which
* `transporter.verify()` never reaches — so modelling this failure at the point it actually happens
* needs a socket that speaks the handful of SMTP verbs involved. That matters: the whole reason the
* defect was hard to diagnose is that it aborts the session *before* a message is offered, which is
* also why Doctor's send-nothing probe can see it.
*/
async function startGreetingStrictRelay(expected: string): Promise<Harness> {
const sockets = new Set<Socket>();
const server = createServer((socket) => {
sockets.add(socket);
socket.on("close", () => sockets.delete(socket));
socket.on("error", () => undefined);
socket.write("220 relay.invalid ESMTP\r\n");
let buffer = "";
socket.on("data", (chunk: Buffer) => {
buffer += chunk.toString("utf8");
let index = buffer.indexOf("\r\n");
while (index !== -1) {
const line = buffer.slice(0, index);
buffer = buffer.slice(index + 2);
const [verb = "", argument = ""] = line.split(" ");
if (verb.toUpperCase() === "EHLO" || verb.toUpperCase() === "HELO") {
if (argument.toLowerCase() !== expected.toLowerCase()) {
// The exact shape the deployment saw: a 421 naming no reason, then the door shut.
socket.write("421-4.7.0 Try again later, closing connection. (EHLO)\r\n");
socket.write("421 4.7.0 Goodbye\r\n");
socket.end();
return;
}
socket.write("250-relay.invalid\r\n250 OK\r\n");
} else if (verb.toUpperCase() === "QUIT") {
socket.write("221 Bye\r\n");
socket.end();
return;
} else {
socket.write("250 OK\r\n");
}
index = buffer.indexOf("\r\n");
}
});
});
await new Promise<void>((resolve) => server.listen(0, "127.0.0.1", resolve));
const address = server.address() as AddressInfo;
const close = async (): Promise<void> => {
// `close()` waits for open sockets, and a refused client may still be holding one.
for (const socket of sockets) {
socket.destroy();
}
await new Promise<void>((resolve) => server.close(() => resolve()));
};
openHarnesses.push(close);
return { captured: [], close, port: address.port };
}
function config(port: number, overrides: Record<string, unknown> = {}): MailProviderConfig {
const built = createMailProviderConfig(
{
@@ -355,3 +438,94 @@ describe.sequential("live SMTP transport", () => {
expect(harness.captured).toHaveLength(1);
});
});
describe("SMTP client identity on the wire", () => {
/**
* The regression guard for a real production defect.
*
* A deployment relaying through an IP-authorized relay could not send at all: every attempt
* surfaced as `MAIL_PROVIDER_UNAVAILABLE`. The relay was healthy — `swaks` sent from the same
* machine — and the cause was the greeting. An SMTP client identifies itself in `EHLO`, and with
* nothing configured Nodemailer derives that from the machine hostname, substituting the address
* literal `[127.0.0.1]` whenever the hostname is not fully qualified. The relay closed the
* connection on it with a `421`.
*
* These tests are written against the wire rather than against the transport options, because the
* defect was invisible in the options: everything about the configuration looked correct.
*/
it("greets with the configured name, exactly as configured", async () => {
const harness = await startSmtpServer();
const mail = provider(config(harness.port, { heloName: "mail.example.org" }));
await mail.send(
validateMailMessage({ subject: "Greeting", text: "Body.", to: "[email protected]" }),
);
expect(harness.captured[0]?.greetedAs).toBe("mail.example.org");
});
it("never greets as the address literal when a name is configured", async () => {
const harness = await startSmtpServer();
const mail = provider(config(harness.port, { heloName: "mail.example.org" }));
await mail.send(
validateMailMessage({ subject: "Greeting", text: "Body.", to: "[email protected]" }),
);
// The exact greeting the relay refused.
expect(harness.captured[0]?.greetedAs).not.toBe("[127.0.0.1]");
});
it("delivers to a relay that demands a fully qualified greeting", async () => {
const harness = await startSmtpServer({ requireGreeting: "mail.example.org" });
const mail = provider(config(harness.port, { heloName: "mail.example.org" }));
await mail.send(
validateMailMessage({ subject: "Accepted", text: "Body.", to: "[email protected]" }),
);
expect(harness.captured).toHaveLength(1);
expect(harness.captured[0]?.greetedAs).toBe("mail.example.org");
});
it("is refused by that same relay when no name is configured", async () => {
const harness = await startGreetingStrictRelay("mail.example.org");
const mail = provider(config(harness.port));
// Deterministic regardless of what this machine is called: the server accepts exactly one name,
// and Nodemailer's derivation cannot produce `mail.example.org` on any host.
await expect(
mail.send(validateMailMessage({ subject: "Refused", text: "Body.", to: "[email protected]" })),
).rejects.toMatchObject({ code: "MAIL_PROVIDER_UNAVAILABLE" });
expect(harness.captured).toHaveLength(0);
});
it("carries the refused greeting into no error a caller can see", async () => {
const harness = await startGreetingStrictRelay("mail.example.org");
const mail = provider(config(harness.port));
const failure = await mail
.send(validateMailMessage({ subject: "Refused", text: "Body.", to: "[email protected]" }))
.catch((error: unknown) => error);
// A `421` is SMTP's "try later", so it stays `MAIL_PROVIDER_UNAVAILABLE` rather than becoming a
// permanent rejection — and the server's response text never reaches the caller.
const serialized = `${(failure as Error).message} ${(failure as Error).stack ?? ""}`;
expect(serialized).not.toContain("Try again later");
expect(serialized).not.toContain("closing connection");
});
it("still verifies through the same greeting Doctor's probe will use", async () => {
const harness = await startGreetingStrictRelay("mail.example.org");
const configured = provider(config(harness.port, { heloName: "mail.example.org" }));
const unconfigured = provider(config(harness.port));
// Doctor builds its provider from the identical validated configuration, so an operator whose
// relay refuses the greeting sees it in `pnpm doctor` rather than only when a reset email
// silently fails to arrive.
await expect(configured.verify()).resolves.toBeUndefined();
await expect(unconfigured.verify()).rejects.toMatchObject({
code: "MAIL_PROVIDER_UNAVAILABLE",
});
});
});