fix(plugin-email): refuse a present-but-unreadable smtp_port instead of silently sending on 587

.changeset/smtp-port-unreadable-refused.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/plugin-email": patch
+---
+
+fix(plugin-email): refuse a present-but-unreadable `smtp_port` instead of silently sending on 587 (#13190)
+
+`smtpOptionsFromMailSettings` coerced `smtp_port` with `Number(...)` and then
+OMITTED the key whenever the result was not finite. `SmtpTransport` then applied
+its built-in 587, so a stored `smtp_port: 'abc'` became a working-looking
+connection to a port nobody chose — and `describe()`, the diagnostic surface,
+reported 587 as though it had been selected. Nothing threw, nothing warned.
+
+The refusal already existed one layer down: `SmtpTransport`'s constructor rejects
+a port outside `SMTP_PORT_MIN`–`SMTP_PORT_MAX` with the generated sentence from
+`smtp-port-contract.ts`. The omission is what converted that loud refusal into a
+silent default, one layer above the guard that exists to be loud. This fix stops
+hiding the value from the guard; ⛔ no second, parallel refusal was added.
+
+`absent` and `present but unreadable` are now distinct, and all of it is pinned:
+
+| stored `smtp_port` | before | after |
+|:---|:---|:---|
+| absent | 587 | 587 — unchanged; a legitimate default |
+| `''` | 587 | 587 — unchanged, deliberately: an empty field spells "not set" |
+| `'abc'` / `'Infinity'` / `{}` | **587, in silence** | refused: `SmtpTransport: invalid port 'NaN' (expected 1-65535)` |
+| `'99999'` | refused | refused — unchanged |
+| `'465'` | 465 | 465 — unchanged |
+
+Behaviour change for a deployment that currently carries an unreadable port: it
+was delivering mail on 587, and will now refuse to build the SMTP transport —
+reporting the reason at boot and under **Send test email** — instead of sending.
+That is the point: the setting was being ignored without a word, which is the
+declared-but-not-delivered shape this package has closed twice before.
+
+Reachability is measured rather than argued. `OS_MAIL_SMTP_PORT=abc` resolves as
+the string `'abc'` with `source: 'env'` and `locked: true` — a non-numeric value
+is not *outside* a numeric window, so the mail manifest's declared
+`min: 1, max: 65535` does not reject it — and the ordinary settings save path
+stores the same value for the same reason.

packages/plugins/plugin-email/src/transports/smtp.test.ts

@@ -6,6 +6,7 @@
 
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { SmtpTransport, smtpOptionsFromMailSettings } from './smtp.js';
+import { formatInvalidSmtpPortNotice } from './smtp-port-contract.js';
 import { makeTransport } from './index.js';
 
 const nm = vi.hoisted(() => ({
@@ -216,4 +217,55 @@ describe('smtpOptionsFromMailSettings', () => {
  expect(smtpOptionsFromMailSettings({ smtp_host: ' ' }).host).toBe('');
  expect(smtpOptionsFromMailSettings({ smtp_host: 'smtp.x' })).toEqual({ host: 'smtp.x' });
  });
+
+ // #13190 — `absent` vs `present but unreadable`, which this function used to
+ // collapse into one bucket. A port that could not be read was DELETED here,
+ // and `SmtpTransport` then applied its built-in 587: a configured `abc`
+ // became a working-looking connection nobody chose, and `describe()`
+ // reported 587 as though it had been selected. The refusal already exists
+ // one layer down; this function's only job is to stop hiding the value from
+ // it, so these three buckets are pinned TOGETHER — pinning the refusal
+ // alone would leave the two fall-back buckets free to drift into it, and
+ // turning `smtp_port: ''` into a refusal breaks working deployments.
+ describe('absent vs present-but-unreadable (#13190)', () => {
+ it('omits `port` when the setting is ABSENT — 587 remains a legitimate default', () => {
+ const opts = smtpOptionsFromMailSettings({ smtp_host: 'smtp.x' });
+ expect(opts).not.toHaveProperty('port');
+ expect(new SmtpTransport(opts).describe().port).toBe(587);
+ });
+
+ it("keeps `smtp_port: ''` in the absent bucket — an empty field means 'not set'", () => {
+ const opts = smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: '' });
+ expect(opts).not.toHaveProperty('port');
+ expect(new SmtpTransport(opts).describe().port).toBe(587);
+ });
+
+ it('passes a present-but-unreadable port through so the guard refuses it by name', () => {
+ const opts = smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: 'abc' });
+ // The key must SURVIVE the mapping — dropping it is the defect, and a
+ // transport built from these options is the thing that must not exist.
+ expect(opts).toHaveProperty('port');
+ expect(Number.isNaN(opts.port as number)).toBe(true);
+ // Asserted through the contract module's own generator (#12993), never a
+ // re-spelled `(expected 1-65535)`: a bare `.toThrow()` here would also
+ // pass on the unrelated `host is required` refusal two lines above it.
+ expect(() => new SmtpTransport(opts)).toThrow(formatInvalidSmtpPortNotice(NaN));
+ });
+
+ it('refuses every unreadable form, and still maps every readable one', () => {
+ for (const unreadable of ['abc', 'Infinity', '12x', {}, []] as unknown[]) {
+ const opts = smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: unreadable });
+ expect(() => new SmtpTransport(opts), `smtp_port=${JSON.stringify(unreadable)}`).toThrow(
+ /SmtpTransport: invalid port/,
+ );
+ }
+ // Unchanged in both directions: a readable in-range port still arrives,
+ // and a readable out-of-range one was already refused before this fix.
+ expect(smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: '465' }).port).toBe(465);
+ expect(new SmtpTransport(smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: 465 }))
+ .describe().port).toBe(465);
+ expect(() => new SmtpTransport(smtpOptionsFromMailSettings({ smtp_host: 'smtp.x', smtp_port: '99999' })))
+ .toThrow(formatInvalidSmtpPortNotice(99999));
+ });
+ });
 });

packages/plugins/plugin-email/src/transports/smtp.ts

@@ -262,6 +262,31 @@ export class SmtpTransport implements IEmailTransport {
  * Values arrive typed from the resolver but may be strings when supplied via
  * `OS_MAIL_SMTP_*` env — coerced here, at that boundary, and nowhere else.
  * `host` comes back `''` when unset; the caller decides how loud that is.
+ *
+ * ## `absent` and `present but unreadable` are not the same thing
+ *
+ * Only an ABSENT `smtp_port` — and the empty string, deliberately — omits
+ * `port` and lets {@link SmtpTransport} apply its built-in 587. A port that
+ * is PRESENT and does not coerce to a finite number is passed through as the
+ * number it coerced to, so the transport's own guard refuses it by name
+ * (`isValidSmtpPort` / `formatInvalidSmtpPortNotice`).
+ *
+ * This function used to drop the key in BOTH cases, which converted a loud
+ * refusal into a silent default one layer above the guard that exists to be
+ * loud: `smtp_port: 'abc'` became a working-looking connection to 587, and
+ * `describe()` reported 587 as though it had been chosen. Both doors that
+ * feed this function really do admit such a value — MEASURED, not argued:
+ * the `OS_MAIL_SMTP_PORT` env override resolves as the string `'abc'`
+ * (`source: 'env'`, `locked: true`), because a non-numeric value is not
+ * *outside* a numeric window and the settings service's declared
+ * `min: 1, max: 65535` therefore does not reject it; and the ordinary save
+ * path stores the same value for the same reason. So the refusal has to
+ * happen at or below this line — and below is where it already lives.
+ * ⛔ Do not add a second, parallel refusal here.
+ *
+ * `smtp_port: ''` stays in the absent bucket on purpose: an empty field is
+ * how a settings form spells "not set", and refusing it would convert a
+ * working configuration into a boot-time refusal.
  */
 export function smtpOptionsFromMailSettings(values: Record<string, unknown>): SmtpTransportOptions {
  const str = (v: unknown): string | undefined => {
@@ -282,7 +307,11 @@ export function smtpOptionsFromMailSettings(values: Record<string, unknown>): Sm
  const password = str(values.smtp_password);
  return {
  host: str(values.smtp_host) ?? '',
- ...(port != null && Number.isFinite(port) ? { port } : {}),
+ // `undefined` is the ONLY bucket that may fall back to the transport's
+ // 587: it means the key was absent (or empty). A present-but-unreadable
+ // port keeps its coerced value — `NaN`, `Infinity` — so `SmtpTransport`
+ // refuses it, instead of this line silently deleting the setting.
+ ...(port !== undefined ? { port } : {}),
  ...(secure != null ? { secure } : {}),
  ...(user ? { user } : {}),
  ...(password ? { password } : {}),