Skip to content

Commit 33fbd35

Browse files
Elon Muskclaude
andauthored
fix(plugin-email): refuse a fractional SMTP port at construction, in the sentence that promised to (#13375)
`isValidSmtpPort` was `Number.isFinite && >= SMTP_PORT_MIN && <= SMTP_PORT_MAX` with no integrality test, so `port: 587.5` was accepted at construction and `describe()` read it straight back. It can never connect: `net.connect` refuses a fractional port with `ERR_SOCKET_BAD_PORT`, and once nodemailer has re-coded it the operator sees, at send time, a bare `RangeError` (`code: 'ECONNECTION'`) reading `Port should be >= 0 and < 65536. Received type number (587.5).` — a TCP rule naming no part of the Settings field they typed it into. The refusal the guard did emit stated the integer rule without enforcing it: `587.5` is inside `1-65535`, so `(expected 1-65535)` described a door that had just let it through. Both move together — the guard tests `Number.isInteger` (which subsumes the old finiteness check) and the generated sentence now reads `(expected an integer 1-65535)`. The range is still rendered from the constants; neither bound was re-spelled. The deliberate accept-set pin in `smtp-port-contract.test.ts` is UPDATED, not deleted: the legacy oracle stays, an exhaustive sweep over every integer from below the floor to above the ceiling shows the integer accept set did not move at all, and the one axis that did move is named value by value. Claude-Session: https://claude.ai/code/session_012WkdHQwHr2KQmaX7P1BHzi Co-authored-by: Claude <noreply@anthropic.com>
1 parent aa16721 commit 33fbd35

5 files changed

Lines changed: 309 additions & 28 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/plugin-email": patch
3+
---
4+
5+
fix(plugin-email): refuse a fractional SMTP port at construction, in the sentence that promised to (#13189)
6+
7+
`SmtpTransport`'s port guard was `Number.isFinite && >= 1 && <= 65535` — no
8+
integrality test — so `port: 587.5` was ACCEPTED at construction and
9+
`describe()` read it straight back. It could never connect. `net.connect`
10+
refuses a fractional port with `ERR_SOCKET_BAD_PORT`, and by the time
11+
nodemailer has re-coded it the operator sees, at SEND time, a bare
12+
`RangeError` (`code: 'ECONNECTION'`) reading `Port should be >= 0 and < 65536.
13+
Received type number (587.5).` — a TCP rule naming no part of the
14+
Settings → Mail → Port field they typed it into.
15+
16+
⭐ And the refusal this door *did* emit stated the integer rule without
17+
enforcing it: `587.5` **is** inside `1-65535`, so `(expected 1-65535)`
18+
described a door that had just let it through. The check and its own sentence
19+
disagreed, and only one of them could be satisfied.
20+
21+
Both move together, or the lie moves instead of leaving:
22+
23+
| `port` | before | after |
24+
|:---|:---|:---|
25+
| `587.5` | accepted; `RangeError` at send time | refused at construction: `SmtpTransport: invalid port '587.5' (expected an integer 1-65535)` |
26+
| `1` / `465` / `587` / `65535` | accepted | accepted — unchanged |
27+
| `0` / `-1` / `99999` | refused | refused — unchanged |
28+
| `NaN` / `±Infinity` | refused | refused — unchanged |
29+
| absent, or `smtp_port: ''` | 587 | 587 — unchanged; an empty field spells "not set" |
30+
31+
The sentence stays GENERATED from `SMTP_PORT_MIN` / `SMTP_PORT_MAX` (#12993) —
32+
"an integer" is prose about the predicate, and neither bound was re-spelled.
33+
⚠️ It therefore reads `(expected an integer 1-65535)` from this release on,
34+
including in the refusal quoted by the #13190 entry above, which was written
35+
before this change landed.
36+
37+
This narrows the accept set, deliberately and in exactly one dimension. The
38+
values affected are the fractions strictly inside the range: they were
39+
unbindable addresses, and a deployment carrying one has never delivered mail on
40+
it — it was failing at send time, under a name that pointed nowhere near the
41+
setting. It now fails at boot, saying which value and which rule.
42+
43+
⚠️ This is not a divergence from the CLI's port contract but a convergence with
44+
it. `packages/cli/src/utils/port-contract.ts` is emphatic that a door may not
45+
narrow what boots, and it is read as a precedent the other way — but its width
46+
is about how a port may be *spelled* (`3e3`, `0x0BB8`, `3000.0`, `+3000`,
47+
`08080`), every one of which `parseInt` reduces to an integer before any range
48+
check. Both of its readers then test `Number.isInteger` outright. On
49+
integrality that contract has been strict all along.

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

Lines changed: 141 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,17 @@
3131
* mechanism `mail-manifest-providers.contract.test.ts` already uses for the
3232
* provider dropdown over that same devDependency.
3333
*
34+
* ## #13189 — the accept set narrowed, and this file is where that is visible
35+
*
36+
* `isValidSmtpPort` now tests INTEGRALITY. That is a deliberate narrowing of
37+
* the set #12993 pinned, and the pin below was written to make exactly this
38+
* kind of change loud rather than to forbid it — so it was UPDATED, never
39+
* deleted: the legacy oracle stays, an exhaustive integer sweep says the
40+
* integer accept set did not move at all, and the one axis that did move is
41+
* named value by value. The sentence moved with the guard (`expected an
42+
* integer 1-65535`), because `587.5` is inside `1-65535` and a door that
43+
* refuses it while saying only that is stating a rule it does not enforce.
44+
*
3445
* ## ⛔ The floor is 1 and must never become 0
3546
*
3647
* The CLI's listen range floors at 0 ("let the OS choose"). This one floors at
@@ -181,7 +192,7 @@ describe('#12993 — one SMTP port range, every door states it from there', () =
181192
// independent spelling, so there is nothing left to drift.
182193
expect(SMTP_PORT_RANGE_TEXT).toBe(`${SMTP_PORT_MIN}-${SMTP_PORT_MAX}`);
183194
expect(formatInvalidSmtpPortNotice('abc'))
184-
.toBe(`SmtpTransport: invalid port 'abc' (expected ${SMTP_PORT_MIN}-${SMTP_PORT_MAX})`);
195+
.toBe(`SmtpTransport: invalid port 'abc' (expected an integer ${SMTP_PORT_MIN}-${SMTP_PORT_MAX})`);
185196

186197
// The ceiling is typed exactly once even inside its own module — if the
187198
// notice re-spelled the range, this would be 2.
@@ -211,27 +222,146 @@ describe('#12993 — one SMTP port range, every door states it from there', () =
211222
.toBe(true);
212223
});
213224

214-
it('refactors the enforcement without narrowing what it accepts', () => {
215-
// The predicate `smtp.ts` had before this card, kept verbatim as the
216-
// oracle. Reading the bound from the module here would assert `x === x`;
217-
// the point is that the OLD expression and the NEW function agree.
225+
it('narrows the accept set in exactly ONE dimension — integrality — and nowhere else (#13189)', () => {
226+
// ⚠️ This case was `refactors the enforcement without narrowing what it
227+
// accepts` when #12993 moved the predicate here, and `587.5` sat in its
228+
// table as MEASURED, not endorsed. #13189 is the card that SPENDS that
229+
// pin: the accept set really does narrow now, and the pin's job was always
230+
// to make such a change visible rather than to prevent one. So the oracle
231+
// and the values stay exactly where they were; what changed is that the
232+
// two are now expected to disagree on ONE axis, asserted term by term so
233+
// that a second narrowing — or any widening — still fails right here.
218234
const legacyAccepts = (port: number): boolean =>
219235
!(!Number.isFinite(port) || port < 1 || port > 65535);
220236

221-
const table = [
237+
const integers = [
222238
1, 25, 465, 587, 2525, 65535, // inside
223239
0, -1, 65536, 99999, // outside
240+
];
241+
const nonIntegers = [
242+
587.5, 1.5, 2525.25, 65534.5, // INSIDE the range — accepted until this card
243+
0.5, 65535.5, -0.5, 65536.5, // outside it — the old bounds refused these too
224244
Number.NaN, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY, // not finite
225-
587.5, 0.5, 65535.5, // non-integers: accepted iff they were accepted before
226245
];
227-
for (const port of table) {
246+
247+
// ── UNCHANGED: on every integer the predicate is still, bound for bound,
248+
// the expression `smtp.ts` carried before #12993.
249+
for (const port of integers) {
228250
expect(isValidSmtpPort(port), `accept set changed for ${String(port)}`)
229251
.toBe(legacyAccepts(port));
230252
}
231253

232-
// The table is not vacuous in either direction.
233-
expect(table.filter(legacyAccepts).length).toBeGreaterThan(0);
234-
expect(table.filter((p) => !legacyAccepts(p)).length).toBeGreaterThan(0);
254+
// ⭐ …and the strongest available form of "and nowhere else": EVERY
255+
// integer from below the floor to above the ceiling, not ten sampled
256+
// ones. 65k comparisons of two cheap predicates costs milliseconds and
257+
// closes the gap a table cannot.
258+
let divergences = 0;
259+
for (let port = -2; port <= SMTP_PORT_MAX + 2; port += 1) {
260+
if (isValidSmtpPort(port) !== legacyAccepts(port)) divergences += 1;
261+
}
262+
expect(divergences, 'the integer accept set moved somewhere the table does not sample')
263+
.toBe(0);
264+
265+
// Control for that zero — the same sweep against a floor deliberately one
266+
// too high, which MUST find the one integer it disagrees about. Without
267+
// this the loop above could be counting nothing at all.
268+
let seen = 0;
269+
for (let port = -2; port <= SMTP_PORT_MAX + 2; port += 1) {
270+
if ((port >= 2 && port <= SMTP_PORT_MAX) !== legacyAccepts(port)) seen += 1;
271+
}
272+
expect(seen, 'the integer sweep is a dead loop').toBe(1);
273+
274+
// ── THE ONE CHANGE: nothing non-integral is accepted any more.
275+
for (const port of nonIntegers) {
276+
expect(isValidSmtpPort(port), `${String(port)} is still accepted`).toBe(false);
277+
}
278+
279+
// …and exactly WHICH values this card moved, spelled out rather than
280+
// summarised: accepted yesterday, refused today, and not one of them could
281+
// ever have completed a connection.
282+
//
283+
// ⚠️ MEASURED, and it corrected a first draft of this very list: a
284+
// fraction only moved if it was INSIDE the range, so `0.5` and `65535.5`
285+
// belong in the half below, not here. `0.5 < SMTP_PORT_MIN` and
286+
// `65535.5 > SMTP_PORT_MAX`, so the old bounds already refused both — and
287+
// listing them as "narrowed by this card" would have overstated the
288+
// change while still passing a weaker assertion.
289+
const moved = [587.5, 1.5, 2525.25, 65534.5];
290+
expect(moved.filter(legacyAccepts), 'these were not accepted before, so nothing moved')
291+
.toEqual(moved);
292+
expect(moved.filter((port) => isValidSmtpPort(port)), 'a fractional port is accepted again')
293+
.toEqual([]);
294+
295+
// The other half, asserted rather than left implied: every remaining
296+
// non-integer was ALREADY refused, so this card narrowed the in-range
297+
// fractions and nothing else.
298+
const unmoved = nonIntegers.filter((port) => !moved.includes(port));
299+
expect(unmoved.filter(legacyAccepts), 'this card narrowed more than the in-range fractions')
300+
.toEqual([]);
301+
expect(unmoved.length, 'the unmoved half is empty — it asserts nothing').toBeGreaterThan(0);
302+
303+
// The tables are not vacuous in either direction.
304+
expect(integers.filter(legacyAccepts).length).toBeGreaterThan(0);
305+
expect(integers.filter((p) => !legacyAccepts(p)).length).toBeGreaterThan(0);
306+
expect(nonIntegers.filter(legacyAccepts).length).toBeGreaterThan(0);
307+
});
308+
309+
it('⭐ states the rule it enforces — the sentence and the guard cannot disagree (#13189)', () => {
310+
// The defect this card repairs, in one line: `587.5` **is** inside
311+
// `1-65535`, so a refusal reading `(expected 1-65535)` described a door
312+
// that had just let it through. The sentence is the operator's only view
313+
// of the rule, so a guard that tests integrality without saying so would
314+
// have moved the lie rather than removed it.
315+
const notice = formatInvalidSmtpPortNotice(587.5);
316+
expect(notice, 'the refusal states a range the guard no longer enforces alone')
317+
.toContain('integer');
318+
319+
// ⭐ The mechanical form, and the reason this is not a `toContain` on a
320+
// word: read the range back OUT of the rendered sentence and confirm that
321+
// a value satisfying it is refused anyway — which is precisely why the
322+
// range can no longer be the whole sentence.
323+
const rendered = notice.match(/\((?:[^()]*?)(\d+)-(\d+)\)/);
324+
expect(rendered, 'the refusal no longer renders the range at all').not.toBeNull();
325+
const low = Number(rendered![1]);
326+
const high = Number(rendered![2]);
327+
expect(low, 'the rendered floor drifted from the constant').toBe(SMTP_PORT_MIN);
328+
expect(high, 'the rendered ceiling drifted from the constant').toBe(SMTP_PORT_MAX);
329+
expect(587.5 >= low && 587.5 <= high, 'the example stopped being inside the stated range')
330+
.toBe(true);
331+
expect(isValidSmtpPort(587.5), '587.5 is accepted again').toBe(false);
332+
333+
// ⛔ The range is still GENERATED, never re-typed — the word added above
334+
// is prose about the predicate and must not have dragged a literal in
335+
// with it. (`SMTP_PORT_RANGE_TEXT` is asserted against the constants two
336+
// cases up; this holds the notice to that same construct.)
337+
expect(notice).toContain(SMTP_PORT_RANGE_TEXT);
338+
});
339+
340+
it('refuses a fractional port AT CONSTRUCTION, under its own name (#13189)', () => {
341+
// BEFORE, MEASURED on `origin/main@56c5b1dbe` through the built
342+
// `dist/index.js`: construction ACCEPTED `587.5`, `describe().port` read
343+
// it straight back, and the operator's first sight of the problem came at
344+
// SEND time as a bare `RangeError` — `code: 'ECONNECTION'` once nodemailer
345+
// has re-coded `ERR_SOCKET_BAD_PORT` — reading `Port should be >= 0 and <
346+
// 65536. Received type number (587.5).`, which names a TCP rule and no
347+
// part of the Settings field the operator typed in.
348+
expect(() => new SmtpTransport({ host: 'smtp.example.test', port: 587.5 }))
349+
.toThrow(formatInvalidSmtpPortNotice(587.5));
350+
351+
// The refusal carries the operator's OWN value. Asserted through the
352+
// contract's generator above and then, separately, on the spelling — a
353+
// bare `.toThrow()` here would also pass on the `host is required`
354+
// refusal that guards the line before it.
355+
expect(() => new SmtpTransport({ host: 'smtp.example.test', port: 587.5 }))
356+
.toThrow(/invalid port '587\.5'/);
357+
358+
// ⛔ The fence on the repair: integer ports still construct, at both
359+
// bounds. A guard that refused `587.5` by refusing everything would
360+
// satisfy every line above this one.
361+
for (const port of [SMTP_PORT_MIN, 25, 465, 587, SMTP_PORT_MAX]) {
362+
expect(new SmtpTransport({ host: 'smtp.example.test', port }).describe().port)
363+
.toBe(port);
364+
}
235365
});
236366

237367
it('⛔ floors at 1, not at 0 — this range is not the CLI listen range', () => {

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

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -79,28 +79,67 @@ export const SMTP_PORT_MAX = 65535;
7979
export const SMTP_PORT_RANGE_TEXT = `${SMTP_PORT_MIN}-${SMTP_PORT_MAX}`;
8080

8181
/**
82-
* Is `port` inside the range this transport will connect on?
83-
*
84-
* ⚠️ This is the enforcement `smtp.ts` already had, moved and NOT narrowed.
85-
* The accept set is unchanged in both directions, deliberately: a refactor
86-
* that quietly rejected a value which worked yesterday would be a behaviour
87-
* change wearing a cleanup's clothes. In particular a **non-integer** inside
88-
* the range (`587.5`) is accepted here exactly as it was before, and is
89-
* refused later by `net.connect` under an internal name — filed separately
90-
* rather than repaired here, because that is an accept-set defect and this
91-
* card is about the duplication.
82+
* Is `port` a port this transport can actually connect on?
83+
*
84+
* Three conditions, and `Number.isInteger` carries the first two of them:
85+
* finite (it refuses `NaN` and both infinities), whole, and inside the range.
86+
*
87+
* ## ⭐ Why integrality is part of the contract (#13189)
88+
*
89+
* This predicate arrived from `smtp.ts` as `Number.isFinite` and no more, and
90+
* #12993 kept it that way on purpose — narrowing an accept set inside a
91+
* refactor whose whole claim is behaviour preservation would have been a
92+
* behaviour change wearing a cleanup's clothes. It is narrowed here, in a card
93+
* that is about nothing else, for a reason that is measured rather than
94+
* stylistic:
95+
*
96+
* A fractional port **cannot ever connect**. MEASURED on Node 22:
97+
* `net.connect({ port: 587.5 })` throws `ERR_SOCKET_BAD_PORT`, and by the time
98+
* nodemailer has re-coded it the operator is shown a bare `RangeError`
99+
* (`code: 'ECONNECTION'`) reading `Port should be >= 0 and < 65536. Received
100+
* type number (587.5).` — at SEND time, naming a TCP rule and not the
101+
* Settings field they typed in. So admitting `587.5` here bought nothing: it
102+
* deferred a certain refusal by three layers and stripped the transport's name
103+
* off it on the way.
104+
*
105+
* ⭐ And the refusal this door already emitted **stated the integer rule
106+
* without enforcing it**: `587.5` is inside `1-65535` on any reading of
107+
* `(expected 1-65535)`. The sentence and the check disagreed. Exactly one of
108+
* them had to move, and the one that can never be satisfied is the value.
109+
*
110+
* ## ⚠️ This is NOT the CLI contract diverging — it is the two converging
111+
*
112+
* `packages/cli/src/utils/port-contract.ts` is often read as the precedent
113+
* *against* this, because its module header is emphatic that a door may not
114+
* narrow what boots. Read to the end of it: `parseRequestedPort` is
115+
* `if (!Number.isInteger(parsed)) return null;` and `strictPortReading` is
116+
* `Number.isInteger` behind `/^[+-]?\d+$/`. That module's width is about
117+
* **string spellings** — `3e3`, `0x0BB8`, `3000.0`, `3000abc`, `+3000`,
118+
* `08080` — every one of which `parseInt` reduces to an INTEGER before it is
119+
* ever range-checked. It governs how an operator may *spell* a port, never
120+
* whether a fractional one is admitted. On integrality the CLI has been strict
121+
* all along, in code. `smtp-port-contract.test.ts` holds that reading.
92122
*/
93123
export function isValidSmtpPort(port: number): boolean {
94-
return Number.isFinite(port) && port >= SMTP_PORT_MIN && port <= SMTP_PORT_MAX;
124+
return Number.isInteger(port) && port >= SMTP_PORT_MIN && port <= SMTP_PORT_MAX;
95125
}
96126

97127
/**
98-
* The refusal for a port outside the range, naming the value the caller
99-
* actually supplied and the range from the constants above.
128+
* The refusal for a port this transport will not connect on, naming the value
129+
* the caller actually supplied and the rule from the constants above.
100130
*
101131
* `raw` is the caller's ORIGINAL value, not the coerced number: an operator
102132
* who configured `"abc"` needs to see `abc`, not `NaN`.
133+
*
134+
* ⭐ **"an integer" is load-bearing, not decoration (#13189).** This sentence
135+
* used to read `(expected 1-65535)` while the guard admitted `587.5` — which
136+
* IS in 1-65535 — so the door stated a rule it did not enforce. Now that the
137+
* guard tests integrality, the sentence has to say so or the lie has merely
138+
* moved from the check to the message. The range itself stays GENERATED from
139+
* {@link SMTP_PORT_MIN} / {@link SMTP_PORT_MAX}: the word is prose about the
140+
* predicate, and ⛔ re-spelling either bound here would rebuild the exact
141+
* duplication #12993 deleted.
103142
*/
104143
export function formatInvalidSmtpPortNotice(raw: unknown): string {
105-
return `SmtpTransport: invalid port '${String(raw)}' (expected ${SMTP_PORT_RANGE_TEXT})`;
144+
return `SmtpTransport: invalid port '${String(raw)}' (expected an integer ${SMTP_PORT_RANGE_TEXT})`;
106145
}

0 commit comments

Comments
 (0)