Refuse a non-finite number where a text field coerces one #18

Merged
lilleman merged 9 commits from worktree-non-finite-text into main 2026-09-21 10:46:52 +02:00
8 changed files with 107 additions and 19 deletions
+7
View File
@@ -25,6 +25,13 @@
will not match the sender it came from. Ids most SMSCs issue are digits or hex and are unaffected. will not match the sender it came from. Ids most SMSCs issue are digits or hex and are unaffected.
- A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is - A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is
refused. An Octet String carries a NULL as before. refused. An Octet String carries a NULL as before.
- A non-finite number — `NaN`, `Infinity`, `-Infinity` — is refused where a text field on the wire
takes one. `sendSms({ from: NaN })` put the literal sender `NaN` on the wire and resolved as a
successful send; `message_id`, `source_addr` and the string TLVs took such a number the same way.
The call now resolves with `err` naming the field — `from: Expected a finite number, got NaN` — so
a caller that reads only `smsIds` meets a failure it has not met before. A whole number in an
address or an id still spells its digits, so `message_id: 123` is unchanged. The integer fields
name a refused `NaN` too, where the refusal used to read `null`.
## 0.5.0 ## 0.5.0
+3 -2
View File
@@ -348,8 +348,9 @@ Name a later instant as a `Date`, which goes out absolute.
**What gets checked.** The library checks what it composes: an address you gave as `from` or `to`, **What gets checked.** The library checks what it composes: an address you gave as `from` or `to`,
an alphabet or a time you named, a string body under a `data_coding` you named. What you formed an alphabet or a time you named, a string body under a `data_coding` you named. What you formed
yourself, a `Buffer` body or a stamp you formatted, passes through as written. The same rule holds yourself, a `Buffer` body or a stamp you formatted, passes through as written, except that a text
for `session.send()`. field is still checked: [PDUs and the low-level API](#pdus-and-the-low-level-api). The same rule
holds for `session.send()`.
## Session ## Session
+6 -6
View File
@@ -65,8 +65,8 @@ rule and an index of the titles below.
which serve their own tables that way. Rejected: a `Result` signature on all three, which costs which serve their own tables that way. Rejected: a `Result` signature on all three, which costs
every typed consumer a narrow forever — goal 8, and the tag is the last cheap chance to spend it — every typed consumer a narrow forever — goal 8, and the tag is the last cheap chance to spend it —
to guard a state the compiler refuses. Where the domain really is open the check is already there: to guard a state the compiler refuses. Where the domain really is open the check is already there:
`sendSms()` takes its options as `unknown` and refuses `encoding` by name, which is what a caller `sendSms()` widens `encoding`, `messagingMode` and the two time options to `unknown` and refuses
without types gets. `smppTime.encode()` is where that reasoning lands the other way and is recorded each by name, which is what a caller without types gets. `smppTime.encode()` is where that reasoning lands the other way and is recorded
under [The wire](#the-wire): `Date | number | string` is not a closed set, so it is a `Result`. under [The wire](#the-wire): `Date | number | string` is not a closed set, so it is a `Result`.
- **A segment the SMSC took and named no id for is `undefined` in `smsIds`, not an empty string.** - **A segment the SMSC took and named no id for is `undefined` in `smsIds`, not an empty string.**
@@ -485,12 +485,12 @@ rule and an index of the titles below.
Goal 1 settles the write, being 3.4 as SMSCs actually run it: an operator routing an alphanumeric Goal 1 settles the write, being 3.4 as SMSCs actually run it: an operator routing an alphanumeric
sender through the upper half is traffic to keep, and Node's `ascii` write already put those octets sender through the upper half is traffic to keep, and Node's `ascii` write already put those octets
on the wire, so naming the write latin1 makes the round trip idempotent and no peer sees a change. on the wire, so naming the write latin1 makes the round trip idempotent and no peer sees a change.
Goal 2 settles the refusals, both of them a `size()` that would have agreed with a `write()` that Goal 2 settles the two latin1 refusals, each a `size()` that would have agreed with a `write()`
put something else on the wire: a character past `U+00FF` written as its low octet, and a caller's that put something else on the wire: a character past `U+00FF` written as its low octet, and a caller's
own `U+0000`, which a mandatory field's reader takes as the end of the field. Goal 4 settles them own `U+0000`, which a mandatory field's reader takes as the end of the field. Goal 4 settles them
twice over: for one character in every 256 that low octet is `0x00`, and the PDU went out malformed twice over: for one character in every 256 that low octet is `0x00`, and the PDU went out malformed
on the operator's parser. `wantText()` and `wantCstringText()` are the only two places that decide on the operator's parser. `wantText()` and `wantCstringText()` are the two places that decide the
it. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing refusals; every read and write spells `latin1` itself. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing
only by accident. Rejected: refusing the upper half on send only by accident. Rejected: refusing the upper half on send
to stay strict to 3.4's ASCII, which would be a new restriction taking away traffic this library to stay strict to 3.4's ASCII, which would be a new restriction taking away traffic this library
already sends and operators already accept, on no defect. Rejected: refusing `U+0000` in every already sends and operators already accept, on no defect. Rejected: refusing `U+0000` in every
+10 -1
View File
@@ -38,6 +38,11 @@ export function paramNumber(value: ParamValue | undefined, fallback: number): nu
return typeof value === 'number' ? value : fallback; return typeof value === 'number' ? value : fallback;
} }
/** Spells a refused value for the caller who wrote it; JSON spells NaN and the infinities `null`. */
export function valueText(value: ParamValue): string {
return typeof value === 'number' ? String(value) : JSON.stringify(value);
}
function outOfRange(buffer: Buffer, offset: number, needed: number): Error | undefined { function outOfRange(buffer: Buffer, offset: number, needed: number): Error | undefined {
if (offset < 0 || needed < 0 || offset + needed > buffer.length) { if (offset < 0 || needed < 0 || offset + needed > buffer.length) {
return new Error( return new Error(
@@ -50,7 +55,7 @@ function outOfRange(buffer: Buffer, offset: number, needed: number): Error | und
function wantInt(value: ParamValue, max: number): Result<{ int: number }> { function wantInt(value: ParamValue, max: number): Result<{ int: number }> {
if (typeof value !== 'number' || !Number.isInteger(value)) { if (typeof value !== 'number' || !Number.isInteger(value)) {
return { err: new Error(`Expected an integer, got ${JSON.stringify(value)}`) }; return { err: new Error(`Expected an integer, got ${valueText(value)}`) };
} }
if (value < 0 || value > max) { if (value < 0 || value > max) {
@@ -99,6 +104,10 @@ function wantText(value: ParamValue): Result<{ text: string }> {
return { err: new Error(`Expected a string or a number, got ${typeof value}`) }; return { err: new Error(`Expected a string or a number, got ${typeof value}`) };
} }
if (typeof value === 'number' && !Number.isFinite(value)) {
return { err: new Error(`Expected a finite number, got ${String(value)}`) };
}
const text = String(value); const text = String(value);
return pastLatin1(text) ?? { text }; return pastLatin1(text) ?? { text };
+2 -2
View File
@@ -9,7 +9,7 @@ import { cmds, commandNameById, respNameFor } from './defs/commands.ts';
import { hasUdh } from './defs/constants.ts'; import { hasUdh } from './defs/constants.ts';
import { decodeMessage, encodeBody } from './message.ts'; import { decodeMessage, encodeBody } from './message.ts';
import { errorNameById, errors, isErrorName } from './defs/errors.ts'; import { errorNameById, errors, isErrorName } from './defs/errors.ts';
import { paramNumber } from './defs/types.ts'; import { paramNumber, valueText } from './defs/types.ts';
import { tagIdOf, tlvDefault, tlvs, tlvsById, writeTlvs } from './defs/tlvs.ts'; import { tagIdOf, tlvDefault, tlvs, tlvsById, writeTlvs } from './defs/tlvs.ts';
/** The highest sequence number this library hands out; SMPP 3.4 4.7.1 reserves 0x7fffffff. */ /** The highest sequence number this library hands out; SMPP 3.4 4.7.1 reserves 0x7fffffff. */
@@ -234,7 +234,7 @@ function buildPdu(
} }
if (!Number.isInteger(seqNr) || seqNr < 0 || seqNr > maxWireSeqNr) { if (!Number.isInteger(seqNr) || seqNr < 0 || seqNr > maxWireSeqNr) {
return { err: new Error(`Invalid seqNr: ${JSON.stringify(seqNr)}`) }; return { err: new Error(`Invalid seqNr: ${valueText(seqNr)}`) };
} }
const built = buildBody(definition, cmdName, cmdStatus, params, tlvs); const built = buildBody(definition, cmdName, cmdStatus, params, tlvs);
+9 -1
View File
@@ -51,6 +51,11 @@ describe('header', () => {
assert.equal(decode(encode({ cmdName: 'enquire_link', seqNr: 0x80000001 })).seqNr, 0x80000001); assert.equal(decode(encode({ cmdName: 'enquire_link', seqNr: 0x80000001 })).seqNr, 0x80000001);
assert.equal(decode(encode({ cmdName: 'enquire_link', seqNr: 0xFFFFFFFF })).seqNr, 0xFFFFFFFF); assert.equal(decode(encode({ cmdName: 'enquire_link', seqNr: 0xFFFFFFFF })).seqNr, 0xFFFFFFFF);
assert.ok(objToPdu({ cmdName: 'submit_sm', seqNr: 0x100000000 }).err instanceof Error); assert.ok(objToPdu({ cmdName: 'submit_sm', seqNr: 0x100000000 }).err instanceof Error);
const notANumber = objToPdu({ cmdName: 'submit_sm', seqNr: NaN });
assert.ok(notANumber.err instanceof Error);
assert.match(notANumber.err.message, /NaN/);
}); });
}); });
@@ -144,7 +149,7 @@ describe('parsing real PDUs', () => {
assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); assert.equal(decode(pdu).params.source_addr, 'Kaffeé');
}); });
test('refuses an address the field cannot carry rather than truncating it', () => { test('refuses an address the field cannot carry rather than truncating or coercing it', () => {
const smuggled = objToPdu({ const smuggled = objToPdu({
cmdName: 'submit_sm', cmdName: 'submit_sm',
params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' },
@@ -153,6 +158,9 @@ describe('parsing real PDUs', () => {
assert.ok(smuggled.err instanceof Error); assert.ok(smuggled.err instanceof Error);
assert.equal(smuggled.buffer, undefined); assert.equal(smuggled.buffer, undefined);
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error);
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: NaN } }).err instanceof Error);
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: Infinity } }).err instanceof Error);
}); });
}); });
+15 -1
View File
@@ -39,6 +39,11 @@ describe('integers', () => {
assert.ok(types.int8.write(-1, Buffer.alloc(1), 0).err instanceof Error); assert.ok(types.int8.write(-1, Buffer.alloc(1), 0).err instanceof Error);
assert.ok(types.int16.write(1.5, Buffer.alloc(2), 0).err instanceof Error); assert.ok(types.int16.write(1.5, Buffer.alloc(2), 0).err instanceof Error);
assert.ok(types.int8.write('nope', Buffer.alloc(1), 0).err instanceof Error); assert.ok(types.int8.write('nope', Buffer.alloc(1), 0).err instanceof Error);
const notANumber = types.int8.write(NaN, Buffer.alloc(1), 0);
assert.ok(notANumber.err instanceof Error);
assert.match(notANumber.err.message, /NaN/);
}); });
}); });
@@ -112,13 +117,22 @@ describe('cstring (C-Octet String)', () => {
assert.deepEqual(target, encoded); assert.deepEqual(target, encoded);
}); });
test('coerces a numeric value to its decimal string', () => { test('coerces a numeric value to its decimal string, and refuses a non-finite one', () => {
const target = Buffer.alloc(4); const target = Buffer.alloc(4);
types.cstring.write(123, target, 0); types.cstring.write(123, target, 0);
assert.deepEqual(target, Buffer.from([0x31, 0x32, 0x33, 0x00])); assert.deepEqual(target, Buffer.from([0x31, 0x32, 0x33, 0x00]));
assert.deepEqual(types.cstring.size(123), { size: 4 }); assert.deepEqual(types.cstring.size(123), { size: 4 });
for (const value of [NaN, Infinity, -Infinity]) {
assert.ok(types.cstring.size(value).err instanceof Error, String(value));
assert.ok(types.cstring.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value));
assert.ok(types.string.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value));
assert.ok(types.buffer.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value));
assert.ok(types.tlv.string.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value));
assert.ok(types.tlv.cstring.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value));
}
}); });
test('carries every latin1 octet, and refuses a character past it', () => { test('carries every latin1 octet, and refuses a character past it', () => {
+55 -6
View File
@@ -189,11 +189,16 @@ and is also what the panel ranked hardest — two methods, one answer.
### Correctness, ahead of everything below ### Correctness, ahead of everything below
- [ ] **Refuse a non-finite number where a text field coerces one.** `wantText()` in `defs/types.ts` - [ ] **Take the maintainer's call on whether goal 2 covers a value we could not send as given.**
stringifies a number so `message_id: 123` writes `"123"`, which is deliberate and tested. It Goal 2's four clauses are one family — an undeterminable outcome, a non-final report, a
takes `NaN` and `Infinity` on the same path, so `sendSms({ from: NaN })` puts the literal sender re-send, dropped work — and none of them covers *the wire carried a value the caller did not
`NaN` on the wire and reports success — goal 2. Gate the numeric branch on `Number.isFinite`. write, and the call reported success*, which is the `NaN` sender, the `sm_length: 0` body and
Predates the latin1 guard; found by the stability review of #16. `1e+21`. Items below cite goal 2 for exactly that, and the DLR-merge item concedes it "is
stated in no file today either way". Proposed clause, after "…is not dropped": "; a call that
reports a message as sent asserts that the wire carried what the caller wrote, so a value we
cannot send as given is refused before anything goes out rather than coerced into one the
caller never wrote." A goal is the maintainer's, so nothing edits README until that is
answered. From the prose pass of #18.
- [ ] **Answer `alert_notification` and `outbind` by not answering them.** Both are response-less in - [ ] **Answer `alert_notification` and `outbind` by not answering them.** Both are response-less in
SMPP 3.4, both fall through `route()`'s default into `unhandled()`, which calls SMPP 3.4, both fall through `route()`'s default into `unhandled()`, which calls
@@ -222,6 +227,37 @@ and is also what the panel ranked hardest — two methods, one answer.
send — and not this one. If goal 2 forbids reporting on a message we cannot fully account for, send — and not this one. If goal 2 forbids reporting on a message we cannot fully account for,
that is the answer; it is stated in no file today either way. that is the answer; it is stated in no file today either way.
- [ ] **Return an `err` where `message` is not a string, rather than throwing.**
`sendSms({ message: undefined })` — a forgotten property — reaches `value.replace()` in
`defs/encodings.ts` through the alphabet detection `checkOptions()` runs, and the `TypeError`
escapes `submitSms()` into the caller's process; `NaN` and `12345` do the same. README promises
"Never throws. Every fallible call resolves to `{ err?, … }`" and AGENTS.md hard rule 1 says it
again, so the docs are false for the likeliest caller mistake there is. From the stability
review of #18.
- [ ] **Derive `sm_length` for a numeric body, or refuse one.** `resolveBody()` in `pdu.ts` reads the
length only where the body is a Buffer or a string, so
`objToPdu({ cmdName: 'submit_sm', params: { short_message: 12345 } })` writes `sm_length: 0`,
then five octets after it, and reports success — and this library's own parser refuses what it
built, as "TLV 12594 runs past the end of the PDU". Goals 1 and 2. From the stability review
of #18.
- [ ] **Settle which numbers may spell a text field, refuse the rest, and say so where a consumer
reads it.** `wantText()` takes every finite number through `String()`, so `message_id: 1e21`
writes `1e+21`, `from: 0.1 + 0.2` writes `0.30000000000000004` and `source_addr: -5` writes
`-5` — none of them is the id or the address the caller meant, and all three are reported as
sent. The numeric branch exists for a digit sequence (`message_id: 123`); the product-owner
review of #18 recommends `Number.isSafeInteger(value) && value >= 0` with the refusal naming
the fix, since a 64-bit SMSC id loses digits to a JS number before this library ever sees it.
Goals 2 then 3: `from: 1e21` is reported as sent to an address that reaches nobody, which is
the wrong answer about what happened before it is laxness in what we send. That a number is
accepted at all reaches a consumer in no sentence either: only the type comment at
`defs/commands.ts:239`, and one CHANGELOG line that stops being visible when
0.7.0 is cut, while README's Building bullet reads as the whole rule for a text field. Whether
this is a supported spelling or 0.4.0 tolerance decides whether that sentence lands in
README.md or in MIGRATION.md — write it in the same change as the rule, so it is worded once.
From the stability and product-owner reviews of #18.
### Throughput — goal 6, and the default window is where we are slowest ### Throughput — goal 6, and the default window is where we are slowest
- [ ] **Close the gap to jsmpp at `maxOutstanding: 10`.** Measured 2026-09-20 against the same sink, - [ ] **Close the gap to jsmpp at `maxOutstanding: 10`.** Measured 2026-09-20 against the same sink,
@@ -386,6 +422,18 @@ and is also what the panel ranked hardest — two methods, one answer.
creation rule and the FIFO one, which nothing else states, and drop "CI's ten-minute cap" — creation rule and the FIFO one, which nothing else states, and drop "CI's ten-minute cap" —
that number lives in `.gitea/workflows/test.yaml`. Raised by the prose pass, 2026-09-20. that number lives in `.gitea/workflows/test.yaml`. Raised by the prose pass, 2026-09-20.
- [ ] **Leave AGENTS.md hard rule 1 the rule, and the decision log its reasoning.** Rule 1's fourth
sentence — "a function whose argument types are a closed set is guarded by the compiler and
stays total, which is why the encoding helpers return plainly, and the check belongs at
whichever boundary the argument arrives untyped at" — is the reasoning of the
`bitCount()`/`encodeMessage()`/`splitMessage()` entry in `docs/decisions.md`, which AGENTS.md's
own Documentation section makes a defect: it scopes AGENTS.md to an index of the decisions. It
also reads two ways — "wherever the types admit one" as an exemption for a typed field,
"the check belongs at whichever boundary the argument arrives untyped at" as a requirement at
`sendSms()` — and the `message` `TypeError` item sits exactly between them, so one rewrite
settles both. Maintainer's call, since it changes what a hard rule asks. From the prose pass
of #18.
- [ ] **Refuse a delay Node's timers cannot hold, in `checkLimits`.** `idleTimeout`, - [ ] **Refuse a delay Node's timers cannot hold, in `checkLimits`.** `idleTimeout`,
`reassemblyTimeout`, `responseTimeout` and `shutdownTimeout` take any integer, and `setTimeout` `reassemblyTimeout`, `responseTimeout` and `shutdownTimeout` take any integer, and `setTimeout`
fires after 1 ms for anything above 2147483647 — so a value in the wrong unit gets the inverse fires after 1 ms for anything above 2147483647 — so a value in the wrong unit gets the inverse
@@ -394,7 +442,8 @@ and is also what the panel ranked hardest — two methods, one answer.
`idleTimeout: '5000'` is refused with `got 5000` — a value the reader reads as correct — where `idleTimeout: '5000'` is refused with `got 5000` — a value the reader reads as correct — where
`connectTimeout` quotes it. `namedValue()`'s four sites — `messagingMode`, `encoding`, the time `connectTimeout` quotes it. `namedValue()`'s four sites — `messagingMode`, `encoding`, the time
options and `smsIdFormat` — are the same defect once more: there `true` and `'true'` both print options and `smsIdFormat` — are the same defect once more: there `true` and `'true'` both print
as `true`. One fix closes all three. Raised by review, 2026-09-20. as `true`. One fix closes all three, and `valueText()` in `defs/types.ts` is the quoted
spelling to take it from. Raised by review, 2026-09-20.
- [ ] **A send the codec will refuse waits for a link and a window slot first.** `refuse()` in - [ ] **A send the codec will refuse waits for a link and a window slot first.** `refuse()` in
`outgoing-requests.ts` runs `misuse()` and the abort check before the wait, precisely so a call `outgoing-requests.ts` runs `misuse()` and the abort check before the wait, precisely so a call