From 7db242e37559e33fee7a50f79e004042fe5b16ab Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 07:54:25 +0200 Subject: [PATCH 1/6] Read and write every text field on the wire as latin1 --- AGENTS.md | 1 + CHANGELOG.md | 5 ++++ README.md | 3 +- docs/decisions.md | 22 ++++++++++++-- src/defs/types.ts | 46 ++++++++++++++++++----------- test/pdu.test.ts | 18 ++++++++++++ test/types.test.ts | 72 +++++++++++++++++++++++++++++++++++++++++++++- todo.md | 9 ------ 8 files changed, 147 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 42b42d0..ded3adb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -292,6 +292,7 @@ the file. - A string body is written in the alphabet its own `data_coding` names, and one that alphabet cannot carry is refused by the codec — `message_payload` on the same terms as `short_message`. - A GSM 03.38 message declares `data_coding` 0x00, and an inbound 0x01 is still read as GSM. +- Every text field on the wire is latin1, and a character past `U+00FF` is refused. ### [The session's life](docs/decisions.md#the-sessions-life) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dc91e2..60d1055 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ one that expires as an ordinary connect failure, so `reconnect` retries it on its usual backoff. A connect previously waited the operating system out, around 130 s on Linux against a host that drops SYNs. `connectTimeout` retunes the bound, and `connectTimeout: false` restores the old wait. +- Addresses, ids and every other text field on the wire are read and written as latin1. A + `source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote + the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, + `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` + in one of those fields is now refused, where it used to go out as its low octet. ## 0.5.0 diff --git a/README.md b/README.md index b78ac6c..8c95f4b 100644 --- a/README.md +++ b/README.md @@ -288,7 +288,8 @@ await session.sendSms({ ``` **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address -and 1 for a numeric one; the NPI fields default to 0. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` goes out and +comes back as its own octets; a character past `U+00FF` is refused rather than sent truncated. **Encoding.** diff --git a/docs/decisions.md b/docs/decisions.md index 376e871..16066c4 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -443,8 +443,8 @@ rule and an index of the titles below. the guard would be a second spelling that disagrees about correctness. Rejected: refusing a string `message_payload` outright and demanding octets, which contradicts `short_message` on the same PDU. Rejected: guarding every string-valued - field, which `data_coding` says nothing about — an address is a C-Octet String and ASCII by 3.4's - own definition. + field against `data_coding`, which says nothing about them — a text field on the wire has an + alphabet of its own. - **A GSM 03.38 message declares `data_coding` 0x00, and an inbound 0x01 is still read as GSM.** Maintainer's call, 2026-09-09: `dataCodingFor()` and `encodeBody()` both resolved an alphabet @@ -478,6 +478,24 @@ rule and an index of the titles below. concatenation reference rather than on `data_coding` — so a receipt or a segment that crossed the change reads exactly as it did. +- **Every text field on the wire is latin1, and a character past `U+00FF` is refused.** Maintainer's + call, 2026-09-21, from the 0.6.0 correctness list: the codec read these fields with + `toString('ascii')`, which masks bit 7, and wrote them with `write(text, 'ascii')`, which does not, + so an inbound `source_addr` of `Kaffeé` reached the application as `Kaffei` and + `objToPdu(pduToObj(x))` was idempotent for none of `source_addr`, `destination_addr`, `system_id`, + `message_id`, `service_type` or the cstring TLVs. Goal 3 settles the read: 3.4 calls these fields + ASCII, and an operator routing an alphanumeric sender through the upper half is traffic to keep. + The write is named latin1 to match, which is what makes the round trip idempotent and is already + the octets Node put on the wire, so no peer sees a change. `wantText()` is the single place that + says so — which is why the `dest_address` and `unsuccess_sme` structures write their embedded + addresses through `cstring.write()` rather than reaching past it — and a character past `U+00FF` + is refused there rather than truncated to its low octet: a `size()` that agreed with a `write()` + that dropped half a character is a wrong answer about what went out (goal 2), and it is how an + alphabet that cannot carry a message body is already handled. Rejected: reading latin1 and leaving + the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the + upper half on send to stay strict to 3.4's ASCII, which would drop exactly the traffic the read + keeps. + ## The session's life - **A close arriving after our own `unbind` is a clean unbind, not an error.** Maintainer's call, diff --git a/src/defs/types.ts b/src/defs/types.ts index 9f8a323..e7589d4 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -28,7 +28,7 @@ export type WireType = { export function paramText(value: ParamValue | undefined): string { if (typeof value === 'string') return value; if (typeof value === 'number') return value.toString(); - if (Buffer.isBuffer(value)) return value.toString('ascii'); + if (Buffer.isBuffer(value)) return value.toString('latin1'); return ''; } @@ -79,11 +79,25 @@ function writeInt32(value: ParamValue, buf: Buffer, offset: number): VoidResult return {}; } -function wantText(value: ParamValue): Result<{ text: string }> { - if (typeof value === 'string') return { text: value }; - if (typeof value === 'number') return { text: value.toString() }; +function pastLatin1(text: string): { err: Error } | undefined { + const index = text.search(/[\u0100-\uFFFF]/); - return { err: new Error(`Expected a string, got ${typeof value}`) }; + if (index === -1) return undefined; + + const code = text.charCodeAt(index).toString(16).toUpperCase().padStart(4, '0'); + + return { + err: new Error( + `Character U+${code} at index ${String(index)} is past latin1, which every text field on the wire is written in`, + ), + }; +} + +function wantText(value: ParamValue): Result<{ text: string }> { + if (typeof value === 'number') return { text: value.toString() }; + if (typeof value !== 'string') return { err: new Error(`Expected a string, got ${typeof value}`) }; + + return pastLatin1(value) ?? { text: value }; } function wantBytes(value: ParamValue): Result<{ bytes: Buffer }> { @@ -91,7 +105,7 @@ function wantBytes(value: ParamValue): Result<{ bytes: Buffer }> { const { err, text } = wantText(value); - return err ? { err } : { bytes: Buffer.from(text, 'ascii') }; + return err ? { err } : { bytes: Buffer.from(text, 'latin1') }; } function isDestAddress(value: unknown): value is DestAddress { @@ -167,7 +181,7 @@ function readCstring(buffer: Buffer, offset: number): Result<{ bytesRead: number } } - return { bytesRead: length + 1, value: buffer.toString('ascii', offset, offset + length) }; + return { bytesRead: length + 1, value: buffer.toString('latin1', offset, offset + length) }; } function writeCstring(text: string, buffer: Buffer, offset: number): VoidResult { @@ -175,7 +189,7 @@ function writeCstring(text: string, buffer: Buffer, offset: number): VoidResult if (err) return { err }; - buffer.write(text, offset, 'ascii'); + buffer.write(text, offset, 'latin1'); buffer[offset + text.length] = 0; return {}; @@ -248,7 +262,7 @@ export const string: WireType = { if (err) return { err }; - return { bytesRead: length + 1, value: buffer.toString('ascii', offset + 1, offset + 1 + length) }; + return { bytesRead: length + 1, value: buffer.toString('latin1', offset + 1, offset + 1 + length) }; }, size(value) { const { err, text } = wantText(value); @@ -271,7 +285,7 @@ export const string: WireType = { if (rangeErr) return { err: rangeErr }; buffer.writeUInt8(text.length, offset); - buffer.write(text, offset + 1, 'ascii'); + buffer.write(text, offset + 1, 'latin1'); return {}; }, @@ -401,7 +415,7 @@ export const dest_address_array: WireType = { if ('dl_name' in dest) { buf.writeUInt8(2, offset++); - const name = writeCstring(dest.dl_name, buf, offset); + const name = cstring.write(dest.dl_name, buf, offset); if (name.err) return { err: name.err }; @@ -417,7 +431,7 @@ export const dest_address_array: WireType = { if (npi.err) return { err: npi.err }; - const addr = writeCstring(dest.destination_addr, buf, offset); + const addr = cstring.write(dest.destination_addr, buf, offset); if (addr.err) return { err: addr.err }; @@ -505,7 +519,7 @@ export const unsuccess_sme_array: WireType = { if (npi.err) return { err: npi.err }; - const addr = writeCstring(sme.destination_addr, buf, offset); + const addr = cstring.write(sme.destination_addr, buf, offset); if (addr.err) return { err: addr.err }; @@ -538,7 +552,7 @@ export const tlv = { ? offset + length : terminator; - return { bytesRead: length, value: buf.toString('ascii', offset, end) }; + return { bytesRead: length, value: buf.toString('latin1', offset, end) }; }, size(value: ParamValue) { const { err, text } = wantText(value); @@ -559,7 +573,7 @@ export const tlv = { read(buf: Buffer, offset: number, length = 0) { const err = outOfRange(buf, offset, length); - return err ? { err } : { bytesRead: length, value: buf.toString('ascii', offset, offset + length) }; + return err ? { err } : { bytesRead: length, value: buf.toString('latin1', offset, offset + length) }; }, size(value: ParamValue) { const { err, text } = wantText(value); @@ -575,7 +589,7 @@ export const tlv = { if (rangeErr) return { err: rangeErr }; - buf.write(text, offset, 'ascii'); + buf.write(text, offset, 'latin1'); return {}; }, diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 999337b..8d8a296 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -127,6 +127,24 @@ describe('parsing real PDUs', () => { assert.equal(pduObj.params.short_message, 'hej 一'); assert.equal(pduObj.tlvs.message_state?.tagValue, 2); }); + + // 'ascii' masks bit 7 on the way in, so this address used to come back Kaffei. + test('keeps an alphanumeric sender whole through the wire and back', () => { + const pdu = encode({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46709771337', + short_message: 'hej', + source_addr: 'Kaffeé', + source_addr_ton: 5, + }, + seqNr: 9, + }); + + assert.ok(pdu.includes(Buffer.from('Kaffeé', 'latin1'))); + assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); + assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); + }); }); describe('encoding submit_sm', () => { diff --git a/test/types.test.ts b/test/types.test.ts index 4d8f0a4..1da0051 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -1,8 +1,8 @@ import assert from 'node:assert/strict'; import test, { describe } from 'node:test'; import type { DestAddress, UnsuccessSme } from '../src/defs/types.ts'; +import { paramText, types } from '../src/defs/types.ts'; import { tlvs } from '../src/defs/tlvs.ts'; -import { types } from '../src/defs/types.ts'; describe('integers', () => { test('int8 reads, sizes and writes one octet', () => { @@ -69,6 +69,20 @@ describe('string (Octet String)', () => { assert.deepEqual(target, encoded); }); + + test('carries every latin1 octet, and refuses a character past it', () => { + const target = Buffer.alloc(4); + + assert.deepEqual(types.string.read(Buffer.from([3, 0xE9, 0x80, 0xFF]), 0), { + bytesRead: 4, + value: 'é\u0080ÿ', + }); + assert.deepEqual(types.string.write('é\u0080ÿ', target, 0), {}); + assert.deepEqual(target, Buffer.from([3, 0xE9, 0x80, 0xFF])); + + assert.ok(types.string.size('一').err instanceof Error); + assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error); + }); }); describe('cstring (C-Octet String)', () => { @@ -100,6 +114,20 @@ describe('cstring (C-Octet String)', () => { assert.deepEqual(types.cstring.size(123), { size: 4 }); }); + // 'ascii' masks bit 7 on the way in and keeps it on the way out, so an address a peer wrote as + // Kaffeé came back Kaffei and objToPdu(pduToObj(x)) stopped being idempotent. + test('carries every latin1 octet, and refuses a character past it', () => { + const address = Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9, 0x00]); + const target = Buffer.alloc(7); + + assert.deepEqual(types.cstring.read(address, 0), { bytesRead: 7, value: 'Kaffeé' }); + assert.deepEqual(types.cstring.write('Kaffeé', target, 0), {}); + assert.deepEqual(target, address); + + assert.ok(types.cstring.size('一').err instanceof Error); + assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); + }); + test('refuses a string with no terminator rather than running off the end', () => { assert.ok(types.cstring.read(Buffer.from('abcd'), 0).err instanceof Error); }); @@ -142,6 +170,32 @@ describe('integer TLVs', () => { }); }); +describe('text TLVs', () => { + const encoded = Buffer.from([0xE9, 0x80, 0xFF]); + + test('carry every latin1 octet, and refuse a character past it', () => { + const target = Buffer.alloc(3); + + assert.deepEqual(types.tlv.string.read(encoded, 0, 3), { bytesRead: 3, value: 'é\u0080ÿ' }); + assert.deepEqual(types.tlv.string.write('é\u0080ÿ', target, 0), {}); + assert.deepEqual(target, encoded); + + assert.ok(types.tlv.string.size('一').err instanceof Error); + assert.ok(types.tlv.string.write('一', Buffer.alloc(3), 0).err instanceof Error); + }); + + test('carry them through a cstring tag too, terminator or none', () => { + const target = Buffer.alloc(4); + + assert.deepEqual(types.tlv.cstring.read(encoded, 0, 3), { bytesRead: 3, value: 'é\u0080ÿ' }); + assert.deepEqual(types.tlv.cstring.write('é\u0080ÿ', target, 0), {}); + assert.deepEqual(target, Buffer.from([0xE9, 0x80, 0xFF, 0x00])); + + assert.ok(types.tlv.cstring.size('一').err instanceof Error); + assert.ok(types.tlv.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); + }); +}); + describe('buffer', () => { const expected = Buffer.from('abcd1234'); @@ -170,6 +224,22 @@ describe('buffer', () => { assert.deepEqual(target, expected); }); + + test('takes a string as the latin1 octets it stands for', () => { + const target = Buffer.alloc(3); + + assert.deepEqual(types.buffer.size('é\u0080ÿ'), { size: 3 }); + assert.deepEqual(types.buffer.write('é\u0080ÿ', target, 0), {}); + assert.deepEqual(target, Buffer.from([0xE9, 0x80, 0xFF])); + }); +}); + +describe('paramText()', () => { + // The one reader of a receipt body that arrived with no octets of its own, so masking bit 7 + // here loses the same characters the wire types used to. + test('renders a Buffer parameter as the latin1 text its octets spell', () => { + assert.equal(paramText(Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9])), 'Kaffeé'); + }); }); describe('dest_address_array', () => { diff --git a/todo.md b/todo.md index e34ff08..473e856 100644 --- a/todo.md +++ b/todo.md @@ -189,15 +189,6 @@ and is also what the panel ranked hardest — two methods, one answer. ### Correctness, ahead of everything below -- [ ] **Read C-Octet Strings as `latin1`, so an address survives the wire.** `defs/types.ts` reads - with `toString('ascii')` at four sites and writes with `write(text, 'ascii')` at three. Node - masks bit 7 when decoding and not when encoding, so the codec writes `0xE9` and reads back - `0x69`: an inbound `source_addr` of `Kaffeé` reaches the application as `Kaffei`, with no raw - escape hatch as `short_message` has in `shortMessageOctets`. Affects `source_addr`, - `destination_addr`, `system_id`, `message_id`, `service_type` and the cstring TLVs, and makes - `objToPdu(pduToObj(x))` non-idempotent for them. Goals 1 and 3. Verified in the container: - `Buffer.from([0xE9]).toString('ascii')` is `'i'`. - - [ ] **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 `sendReturn(pduObj, 'ESME_RINVCMDID')`; `pduReturn()` then finds no response command, and the -- 2.52.0 From c1e0407942950af493899e7f1247c09c728a2e9a Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 07:59:08 +0200 Subject: [PATCH 2/6] Refuse a NULL inside a C-Octet String, which ends the field on the peer --- AGENTS.md | 3 ++- CHANGELOG.md | 4 ++++ README.md | 3 ++- docs/decisions.md | 13 ++++++++++--- src/defs/types.ts | 34 +++++++++++++++++++++++++++------- test/pdu.test.ts | 12 ++++++++++++ test/types.test.ts | 17 +++++++++++++++++ 7 files changed, 74 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ded3adb..06540eb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -292,7 +292,8 @@ the file. - A string body is written in the alphabet its own `data_coding` names, and one that alphabet cannot carry is refused by the codec — `message_payload` on the same terms as `short_message`. - A GSM 03.38 message declares `data_coding` 0x00, and an inbound 0x01 is still read as GSM. -- Every text field on the wire is latin1, and a character past `U+00FF` is refused. +- Every text field on the wire is latin1, and what the field cannot carry is refused rather than + truncated. ### [The session's life](docs/decisions.md#the-sessions-life) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60d1055..9ee8857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` in one of those fields is now refused, where it used to go out as its low octet. +- A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is + refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every + mandatory field behind it while `command_length` still counted the whole string. An Octet String + carries a NULL as before; its length octet is what ends it. ## 0.5.0 diff --git a/README.md b/README.md index 8c95f4b..3fad325 100644 --- a/README.md +++ b/README.md @@ -289,7 +289,8 @@ await session.sendSms({ **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` goes out and -comes back as its own octets; a character past `U+00FF` is refused rather than sent truncated. +comes back as its own octets; a character past `U+00FF`, or a `U+0000` that would end the field +early, is refused rather than sent truncated. **Encoding.** diff --git a/docs/decisions.md b/docs/decisions.md index 16066c4..70748b0 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -478,8 +478,9 @@ rule and an index of the titles below. concatenation reference rather than on `data_coding` — so a receipt or a segment that crossed the change reads exactly as it did. -- **Every text field on the wire is latin1, and a character past `U+00FF` is refused.** Maintainer's - call, 2026-09-21, from the 0.6.0 correctness list: the codec read these fields with +- **Every text field on the wire is latin1, and what the field cannot carry is refused rather than + truncated.** Maintainer's call, 2026-09-21, from the 0.6.0 correctness list: the codec read these + fields with `toString('ascii')`, which masks bit 7, and wrote them with `write(text, 'ascii')`, which does not, so an inbound `source_addr` of `Kaffeé` reached the application as `Kaffei` and `objToPdu(pduToObj(x))` was idempotent for none of `source_addr`, `destination_addr`, `system_id`, @@ -494,7 +495,13 @@ rule and an index of the titles below. alphabet that cannot carry a message body is already handled. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send to stay strict to 3.4's ASCII, which would drop exactly the traffic the read - keeps. + keeps. The security pass over this chunk found the field's other end open the same way: a caller's + own `U+0000` was written verbatim, and since the peer reads a C-Octet String to its first NULL, + every mandatory field behind it shifted under a `command_length` that had counted the whole string + — so an application forwarding a customer's sender id could have a PDU rewritten from inside one. + `wantCstringText()` refuses that where `wantText()` refuses the upper end. Rejected: refusing NULL + in every text field, which would buy one spelling by taking a legitimate octet away from the + length-prefixed Octet String, whose length octet is what ends it. ## The session's life diff --git a/src/defs/types.ts b/src/defs/types.ts index e7589d4..24c0b4b 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -94,10 +94,30 @@ function pastLatin1(text: string): { err: Error } | undefined { } function wantText(value: ParamValue): Result<{ text: string }> { - if (typeof value === 'number') return { text: value.toString() }; - if (typeof value !== 'string') return { err: new Error(`Expected a string, got ${typeof value}`) }; + if (typeof value !== 'number' && typeof value !== 'string') { + return { err: new Error(`Expected a string, got ${typeof value}`) }; + } - return pastLatin1(value) ?? { text: value }; + const text = String(value); + + return pastLatin1(text) ?? { text }; +} + +/** A C-Octet String ends at its first NULL, so one inside the value truncates the field on the peer. */ +function wantCstringText(value: ParamValue): Result<{ text: string }> { + const { err, text } = wantText(value); + + if (err) return { err }; + + const index = text.indexOf('\u0000'); + + if (index === -1) return { text }; + + return { + err: new Error( + `U+0000 at index ${String(index)} would end the C-Octet String there, ${String(text.length - index - 1)} characters early`, + ), + }; } function wantBytes(value: ParamValue): Result<{ bytes: Buffer }> { @@ -302,12 +322,12 @@ export const cstring: WireType = { default: '', read: readCstring, size(value) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : { size: text.length + 1 }; }, write(value, buffer, offset) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : writeCstring(text, buffer, offset); }, @@ -555,12 +575,12 @@ export const tlv = { return { bytesRead: length, value: buf.toString('latin1', offset, end) }; }, size(value: ParamValue) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : { size: text.length + 1 }; }, write(value: ParamValue, buf: Buffer, offset: number) { - const { err, text } = wantText(value); + const { err, text } = wantCstringText(value); return err ? { err } : writeCstring(text, buf, offset); }, diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 8d8a296..1981dab 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -145,6 +145,18 @@ describe('parsing real PDUs', () => { assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); }); + + // The peer reads source_addr to the first NULL and every mandatory field behind it shifts, so an + // application forwarding a customer's sender id could have a PDU rewritten under it. + test('refuses an address carrying its own terminator', () => { + const smuggled = objToPdu({ + cmdName: 'submit_sm', + params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, + }); + + assert.ok(smuggled.err instanceof Error); + assert.equal(smuggled.buffer, undefined); + }); }); describe('encoding submit_sm', () => { diff --git a/test/types.test.ts b/test/types.test.ts index 1da0051..d755ea4 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -83,6 +83,14 @@ describe('string (Octet String)', () => { assert.ok(types.string.size('一').err instanceof Error); assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error); }); + + // Its length octet is what ends it, so unlike a C-Octet String it carries a NULL like any other. + test('carries a NULL octet, which its length octet already bounds', () => { + const target = Buffer.alloc(4); + + assert.deepEqual(types.string.write('a\u0000b', target, 0), {}); + assert.deepEqual(target, Buffer.from([3, 0x61, 0x00, 0x62])); + }); }); describe('cstring (C-Octet String)', () => { @@ -128,6 +136,14 @@ describe('cstring (C-Octet String)', () => { assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); }); + // The field ends at its first NULL, so writing one smuggles a field boundary into the peer's + // parse: every mandatory field behind it shifts, under a command_length that counted the whole + // string. + test('refuses a NULL of its own rather than ending the field early', () => { + assert.ok(types.cstring.size('46701113311\u0000EVIL').err instanceof Error); + assert.ok(types.cstring.write('46701113311\u0000EVIL', Buffer.alloc(17), 0).err instanceof Error); + }); + test('refuses a string with no terminator rather than running off the end', () => { assert.ok(types.cstring.read(Buffer.from('abcd'), 0).err instanceof Error); }); @@ -193,6 +209,7 @@ describe('text TLVs', () => { assert.ok(types.tlv.cstring.size('一').err instanceof Error); assert.ok(types.tlv.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); + assert.ok(types.tlv.cstring.write('a\u0000b', Buffer.alloc(4), 0).err instanceof Error); }); }); -- 2.52.0 From f870b7530f88373672afb6a9943169814431ec5c Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 08:08:42 +0200 Subject: [PATCH 3/6] Cover the guarded address writers, and cut what the code already says --- CHANGELOG.md | 3 ++- README.md | 6 +++--- docs/decisions.md | 39 ++++++++++++++++----------------------- src/defs/types.ts | 7 ++----- test/pdu.test.ts | 7 ++----- test/types.test.ts | 19 +++++++++++-------- test/unsendable.test.ts | 24 ++++++++++++++++++++++++ todo.md | 8 ++++++++ 8 files changed, 68 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ee8857..3027ec5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ `source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` - in one of those fields is now refused, where it used to go out as its low octet. + in one of those fields is now refused, where it used to go out as its low octet — which for `一`, + ` ` and most emoji is `0x00`, ending the field there. - A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every mandatory field behind it while `command_length` still counted the whole string. An Octet String diff --git a/README.md b/README.md index 3fad325..1cc3dc7 100644 --- a/README.md +++ b/README.md @@ -288,9 +288,9 @@ await session.sendSms({ ``` **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address -and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` goes out and -comes back as its own octets; a character past `U+00FF`, or a `U+0000` that would end the field -early, is refused rather than sent truncated. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` reaches the +peer as its own octets; a character past `U+00FF`, or a `U+0000`, is refused rather than sent +truncated. **Encoding.** diff --git a/docs/decisions.md b/docs/decisions.md index 70748b0..e911f74 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -479,29 +479,22 @@ rule and an index of the titles below. change reads exactly as it did. - **Every text field on the wire is latin1, and what the field cannot carry is refused rather than - truncated.** Maintainer's call, 2026-09-21, from the 0.6.0 correctness list: the codec read these - fields with - `toString('ascii')`, which masks bit 7, and wrote them with `write(text, 'ascii')`, which does not, - so an inbound `source_addr` of `Kaffeé` reached the application as `Kaffei` and - `objToPdu(pduToObj(x))` was idempotent for none of `source_addr`, `destination_addr`, `system_id`, - `message_id`, `service_type` or the cstring TLVs. Goal 3 settles the read: 3.4 calls these fields - ASCII, and an operator routing an alphanumeric sender through the upper half is traffic to keep. - The write is named latin1 to match, which is what makes the round trip idempotent and is already - the octets Node put on the wire, so no peer sees a change. `wantText()` is the single place that - says so — which is why the `dest_address` and `unsuccess_sme` structures write their embedded - addresses through `cstring.write()` rather than reaching past it — and a character past `U+00FF` - is refused there rather than truncated to its low octet: a `size()` that agreed with a `write()` - that dropped half a character is a wrong answer about what went out (goal 2), and it is how an - alphabet that cannot carry a message body is already handled. Rejected: reading latin1 and leaving - the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the - upper half on send to stay strict to 3.4's ASCII, which would drop exactly the traffic the read - keeps. The security pass over this chunk found the field's other end open the same way: a caller's - own `U+0000` was written verbatim, and since the peer reads a C-Octet String to its first NULL, - every mandatory field behind it shifted under a `command_length` that had counted the whole string - — so an application forwarding a customer's sender id could have a PDU rewritten from inside one. - `wantCstringText()` refuses that where `wantText()` refuses the upper end. Rejected: refusing NULL - in every text field, which would buy one spelling by taking a legitimate octet away from the - length-prefixed Octet String, whose length octet is what ends it. + truncated.** Maintainer's call, 2026-09-21, the refusals from the security and stability passes on + [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). Goal 3 settles the alphabet: 3.4 calls + these fields ASCII, and an operator routing an alphanumeric sender through the upper half is + traffic to keep, so the read is latin1 and the write is named latin1 to match — which is what + makes the round trip idempotent, and is already the octets Node put on the wire, so no peer sees a + change. Goal 2 settles the refusals, both of them a `size()` that would have agreed with a + `write()` 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 the peer reads as the end of the field, shifting every mandatory + field behind it under a `command_length` that counted the whole string. `wantText()` and + `wantCstringText()` are the only two places that decide it, which is why the `dest_address` and + `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than + reaching past it into `writeCstring()`. Rejected: reading latin1 and leaving the write spelled + ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send + to stay strict to 3.4's ASCII, which would drop exactly the traffic the read keeps. Rejected: + refusing `U+0000` in every text field, which would buy one spelling by taking a legitimate octet + away from the length-prefixed Octet String, whose length octet is what ends it. ## The session's life diff --git a/src/defs/types.ts b/src/defs/types.ts index 24c0b4b..b12dd6a 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -84,7 +84,7 @@ function pastLatin1(text: string): { err: Error } | undefined { if (index === -1) return undefined; - const code = text.charCodeAt(index).toString(16).toUpperCase().padStart(4, '0'); + const code = (text.codePointAt(index) ?? 0).toString(16).toUpperCase().padStart(4, '0'); return { err: new Error( @@ -103,7 +103,6 @@ function wantText(value: ParamValue): Result<{ text: string }> { return pastLatin1(text) ?? { text }; } -/** A C-Octet String ends at its first NULL, so one inside the value truncates the field on the peer. */ function wantCstringText(value: ParamValue): Result<{ text: string }> { const { err, text } = wantText(value); @@ -114,9 +113,7 @@ function wantCstringText(value: ParamValue): Result<{ text: string }> { if (index === -1) return { text }; return { - err: new Error( - `U+0000 at index ${String(index)} would end the C-Octet String there, ${String(text.length - index - 1)} characters early`, - ), + err: new Error(`U+0000 at index ${String(index)} would end the C-Octet String there`), }; } diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 1981dab..7d44959 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -128,7 +128,6 @@ describe('parsing real PDUs', () => { assert.equal(pduObj.tlvs.message_state?.tagValue, 2); }); - // 'ascii' masks bit 7 on the way in, so this address used to come back Kaffei. test('keeps an alphanumeric sender whole through the wire and back', () => { const pdu = encode({ cmdName: 'deliver_sm', @@ -143,12 +142,9 @@ describe('parsing real PDUs', () => { assert.ok(pdu.includes(Buffer.from('Kaffeé', 'latin1'))); assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); - assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); }); - // The peer reads source_addr to the first NULL and every mandatory field behind it shifts, so an - // application forwarding a customer's sender id could have a PDU rewritten under it. - test('refuses an address carrying its own terminator', () => { + test('refuses an address the field cannot carry rather than truncating it', () => { const smuggled = objToPdu({ cmdName: 'submit_sm', params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, @@ -156,6 +152,7 @@ describe('parsing real PDUs', () => { assert.ok(smuggled.err instanceof Error); assert.equal(smuggled.buffer, undefined); + assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error); }); }); diff --git a/test/types.test.ts b/test/types.test.ts index d755ea4..d353335 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -84,7 +84,6 @@ describe('string (Octet String)', () => { assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error); }); - // Its length octet is what ends it, so unlike a C-Octet String it carries a NULL like any other. test('carries a NULL octet, which its length octet already bounds', () => { const target = Buffer.alloc(4); @@ -122,8 +121,6 @@ describe('cstring (C-Octet String)', () => { assert.deepEqual(types.cstring.size(123), { size: 4 }); }); - // 'ascii' masks bit 7 on the way in and keeps it on the way out, so an address a peer wrote as - // Kaffeé came back Kaffei and objToPdu(pduToObj(x)) stopped being idempotent. test('carries every latin1 octet, and refuses a character past it', () => { const address = Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9, 0x00]); const target = Buffer.alloc(7); @@ -136,9 +133,6 @@ describe('cstring (C-Octet String)', () => { assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); }); - // The field ends at its first NULL, so writing one smuggles a field boundary into the peer's - // parse: every mandatory field behind it shifts, under a command_length that counted the whole - // string. test('refuses a NULL of its own rather than ending the field early', () => { assert.ok(types.cstring.size('46701113311\u0000EVIL').err instanceof Error); assert.ok(types.cstring.write('46701113311\u0000EVIL', Buffer.alloc(17), 0).err instanceof Error); @@ -252,8 +246,6 @@ describe('buffer', () => { }); describe('paramText()', () => { - // The one reader of a receipt body that arrived with no octets of its own, so masking bit 7 - // here loses the same characters the wire types used to. test('renders a Buffer parameter as the latin1 text its octets spell', () => { assert.equal(paramText(Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9])), 'Kaffeé'); }); @@ -293,6 +285,11 @@ describe('dest_address_array', () => { types.dest_address_array.write(expected, target, 0); assert.deepEqual(target, encoded); + + const smuggled: DestAddress[] = [{ dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: '46\u0000EVIL' }]; + + assert.ok(types.dest_address_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error); + assert.ok(types.dest_address_array.write([{ dl_name: '一' }], Buffer.alloc(16), 0).err instanceof Error); }); test('refuses a field value the wire cannot hold instead of throwing', () => { @@ -332,6 +329,12 @@ describe('unsuccess_sme_array', () => { types.unsuccess_sme_array.write(expected, target, 0); assert.deepEqual(target, encoded); + + const smuggled: UnsuccessSme[] = [ + { dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: 'a\u0000b', error_status_code: 0 }, + ]; + + assert.ok(types.unsuccess_sme_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error); }); test('refuses a field value the wire cannot hold instead of throwing', () => { diff --git a/test/unsendable.test.ts b/test/unsendable.test.ts index cdcda44..07f88e5 100644 --- a/test/unsendable.test.ts +++ b/test/unsendable.test.ts @@ -345,6 +345,30 @@ describe('a body the PDU\'s own data_coding cannot carry', () => { }); }); +describe('an address the field cannot carry', () => { + test('refuses it through sendSms(), with nothing reaching the socket', async t => { + const smsc = await dummySmsc(t); + const session = await bindToSmsc(t, smsc.port, { reconnect: false }); + + const refusals: [string, RegExp][] = [ + ['46701113311\u0000EVIL', /U\+0000 at index 11/], + ['Kaffe一', /U\+4E00 at index 5/], + ['😀', /U\+1F600 at index 0/], + ]; + + for (const [sender, names] of refusals) { + const sent = await session.sendSms({ from: sender, message: 'Hello world', to }); + + assert.ok(sent.err instanceof Error, sender); + assert.match(sent.err.message, /source_addr/); + assert.match(sent.err.message, names); + assert.deepEqual(sent.smsIds, [], sender); + } + + assert.deepEqual(smsc.octets, [], 'an address the field cannot carry never reaches the socket'); + }); +}); + describe('a time no peer can read', () => { const invalid = new Date('nope'); diff --git a/todo.md b/todo.md index 473e856..685cf8a 100644 --- a/todo.md +++ b/todo.md @@ -259,6 +259,14 @@ and is also what the panel ranked hardest — two methods, one answer. budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the current group survived. Named by 6 of 9 readers. +- [ ] **Let the two address arrays size a C-Octet String through `cstring.size()`.** + `sizeDestAddresses()` and `sizeUnsuccessSmes()` spell "len + 1" themselves, and each `offset +=` + after a write spells it a third time, so `dest_address_array` and `unsuccess_sme_array` each + know the cost in three places. Route both through `cstring.size()` and advance the offset by + what it returns. That also makes `size()` refuse where `write()` already does, so the error + arrives from the first call rather than the second; today the pair only fails closed because + `writeParams()` and `writeTlvs()` both bail on the write. From the stability review of #16. + - [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it twice for opposite conclusions — "nothing to drain, success" and "the link died under us, failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7 -- 2.52.0 From dc690b912fe73eade6082773504524fb4479bde9 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 08:14:08 +0200 Subject: [PATCH 4/6] Name the option a refused address came from, and the data a 0.5.0 consumer stored --- CHANGELOG.md | 5 +++++ README.md | 19 +++++++++++++------ docs/decisions.md | 20 +++++++++++++------- src/defs/types.ts | 9 ++++++--- src/send-sms.ts | 17 ++++++++++++++++- test/unsendable.test.ts | 6 +++--- 6 files changed, 56 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3027ec5..1ecf03a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` in one of those fields is now refused, where it used to go out as its low octet — which for `一`, ` ` and most emoji is `0x00`, ending the field there. + + **Check what you stored before you roll this out.** Values your application persisted under 0.5.0 + were read with bit 7 masked, so an address or a `message_id` carrying an octet above `0x7F` is + spelled differently now: a stored id will not match the receipt it belongs to, and a stored address + 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 refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every mandatory field behind it while `command_length` still counted the whole string. An Octet String diff --git a/README.md b/README.md index 1cc3dc7..53a467e 100644 --- a/README.md +++ b/README.md @@ -288,9 +288,11 @@ await session.sendSms({ ``` **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address -and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` reaches the -peer as its own octets; a character past `U+00FF`, or a `U+0000`, is refused rather than sent -truncated. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1: `Kaffeé` goes out as the +six octets that spell it, `4B 61 66 66 65 E9`, and an address you received always sends back. One +outside `/^[\u0001-ÿ]*$/` is refused, naming the character and its index — strip or +transliterate it first. An SMSC may still refuse a non-ASCII sender of its own accord, which reaches +you as `ESME_RINVSRCADR`. **Encoding.** @@ -344,8 +346,8 @@ you formatted. Refused before anything goes out: an invalid `Date`, `NaN`, `Infi count, and a count past 99 days 23:59:59, since a count in seconds is spelled in days and below. Name a later instant as a `Date`, which goes out absolute. -**What gets checked.** The library checks what it composes: 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 +**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 yourself, a `Buffer` body or a stamp you formatted, passes through as written. The same rule holds for `session.send()`. ## Session @@ -610,6 +612,9 @@ if (isCommand(pduObj, 'submit_sm')) { - A string `short_message` or `message_payload` is encoded in the alphabet the PDU's `data_coding` names, detected from the text where you name none. One that alphabet cannot carry is refused, naming the character, its code point and where it is. +- Every text field is latin1: addresses, `system_id`, `message_id`, `service_type` and the C-Octet + String TLVs. A character past `U+00FF` is refused, as is a `U+0000` in a C-Octet String, which the + peer reads as the end of the field. - A `Buffer` goes out exactly as given under any `data_coding`: binary payloads, hand-built user data headers, deliberately malformed bodies. - `session.send()` and `session.sendReturn()` build through the same codec and refuse the same bodies. @@ -707,7 +712,9 @@ Who depends on this library, and what they may rely on. spooling, scheduling, retry policy and billing belong to whatever this is the edge of, and state shared between instances goes through the store in goal 9. - **Pre-1.0, so the minor is the breaking unit** and a patch never breaks. What a 0.4.0 consumer has - to change is in [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md). + to change is in [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md); + what each later minor changes is in + [CHANGELOG.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/CHANGELOG.md). Personas this README serves, in order: diff --git a/docs/decisions.md b/docs/decisions.md index e911f74..b7c7bc7 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -480,19 +480,25 @@ rule and an index of the titles below. - **Every text field on the wire is latin1, and what the field cannot carry is refused rather than truncated.** Maintainer's call, 2026-09-21, the refusals from the security and stability passes on - [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). Goal 3 settles the alphabet: 3.4 calls - these fields ASCII, and an operator routing an alphanumeric sender through the upper half is - traffic to keep, so the read is latin1 and the write is named latin1 to match — which is what - makes the round trip idempotent, and is already the octets Node put on the wire, so no peer sees a - change. Goal 2 settles the refusals, both of them a `size()` that would have agreed with a + [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). 3.4 calls these fields ASCII, so goal 3 + settles the read alone — its generous clause is scoped to reading, and its sender clause is strict. + 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 emitted the low + octet, so `Kaffeé` went out as `4B 61 66 66 65 E9` before this change and goes out as the same + octets after it. Naming the write latin1 is what 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 put something else on the wire: a character past `U+00FF` written as its low octet, and a caller's own `U+0000`, which the peer reads as the end of the field, shifting every mandatory - field behind it under a `command_length` that counted the whole string. `wantText()` and + field behind it under a `command_length` that counted the whole string. Goal 4 settles them twice + over, since for `一`, ` ` and most emoji that low octet is `0x00` and the field went out malformed + on the operator's parser. `wantText()` and `wantCstringText()` are the only two places that decide it, which is why the `dest_address` and `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than reaching past it into `writeCstring()`. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send - to stay strict to 3.4's ASCII, which would drop exactly the traffic the read keeps. Rejected: + 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; whether an SMSC wants a non-ASCII sender + stays its own call, answered as `ESME_RINVSRCADR`. Rejected: refusing `U+0000` in every text field, which would buy one spelling by taking a legitimate octet away from the length-prefixed Octet String, whose length octet is what ends it. diff --git a/src/defs/types.ts b/src/defs/types.ts index b12dd6a..dfc70ef 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -1,4 +1,5 @@ import type { Result, VoidResult } from '../result.ts'; +import { unencodableText } from './encodings.ts'; export type DestAddress = | { dest_addr_npi: number; dest_addr_ton: number; destination_addr: string } @@ -84,11 +85,11 @@ function pastLatin1(text: string): { err: Error } | undefined { if (index === -1) return undefined; - const code = (text.codePointAt(index) ?? 0).toString(16).toUpperCase().padStart(4, '0'); + const char = String.fromCodePoint(text.codePointAt(index) ?? 0); return { err: new Error( - `Character U+${code} at index ${String(index)} is past latin1, which every text field on the wire is written in`, + `latin1 cannot carry ${unencodableText({ char, index })}, and every text field on the wire is written in it; strip or transliterate it`, ), }; } @@ -113,7 +114,9 @@ function wantCstringText(value: ParamValue): Result<{ text: string }> { if (index === -1) return { text }; return { - err: new Error(`U+0000 at index ${String(index)} would end the C-Octet String there`), + err: new Error( + `U+0000 at index ${String(index)} would end the C-Octet String there, so the peer would read every field behind it shifted`, + ), }; } diff --git a/src/send-sms.ts b/src/send-sms.ts index 2223d5b..0224c33 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -7,10 +7,10 @@ import type { SmppLog } from './log.ts'; import type { SmsIdNotation } from './sms-id.ts'; import { UnansweredError } from './unanswered-error.ts'; import { consts, defaultMessagingMode, isMessagingMode, isSubmitMessagingMode, submitMessagingModes } from './defs/constants.ts'; +import { cstring, paramText } from './defs/types.ts'; import { dataCodingByEncoding, detect, encodingNames, isEncodingName, unencodable, unencodableText } from './defs/encodings.ts'; import { namedValue } from './error-from.ts'; import { normaliseSmsId } from './sms-id.ts'; -import { paramText } from './defs/types.ts'; import { maxSegments, smppTime, splitMessage } from './message.ts'; export type SendSmsOptions = { @@ -211,8 +211,23 @@ function checkFlash(encoding: EncodingName, flash: boolean): Error | undefined { return new Error('flash has no Latin-1 spelling: a message class carries GSM 7-bit, 8-bit data or UCS2, and 8-bit data is not text a handset will display, so send it as UCS2 or drop flash'); } +/** Asked of the wire type itself, so the codec cannot refuse an address this let through. */ +function checkAddresses(sms: SendSmsInput): Error | undefined { + for (const option of ['from', 'to'] as const) { + const { err } = cstring.size(sms[option]); + + if (err) return new Error(`${option}: ${err.message}`); + } + + return undefined; +} + /** Every option a send can be refused for, so nothing is built for a message that will not go. */ function checkOptions(sms: SendSmsInput): Result { + const unwritable = checkAddresses(sms); + + if (unwritable) return { err: unwritable }; + const mode = checkMessagingMode(sms.messagingMode, sms.dlr === true); if (mode.err) return { err: mode.err }; diff --git a/test/unsendable.test.ts b/test/unsendable.test.ts index 07f88e5..5fae834 100644 --- a/test/unsendable.test.ts +++ b/test/unsendable.test.ts @@ -352,15 +352,15 @@ describe('an address the field cannot carry', () => { const refusals: [string, RegExp][] = [ ['46701113311\u0000EVIL', /U\+0000 at index 11/], - ['Kaffe一', /U\+4E00 at index 5/], - ['😀', /U\+1F600 at index 0/], + ['Kaffe一', /"一" \(U\+4E00\) at index 5/], + ['😀', /"😀" \(U\+1F600\) at index 0/], ]; for (const [sender, names] of refusals) { const sent = await session.sendSms({ from: sender, message: 'Hello world', to }); assert.ok(sent.err instanceof Error, sender); - assert.match(sent.err.message, /source_addr/); + assert.match(sent.err.message, /^from: /, 'names the option the caller wrote, not the wire field'); assert.match(sent.err.message, names); assert.deepEqual(sent.smsIds, [], sender); } -- 2.52.0 From a7eb4923f5d341023a5b5598739deff53afb6566 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 08:20:59 +0200 Subject: [PATCH 5/6] Say what is true for a TLV too, and cover the to half of the check --- README.md | 17 +++++++++-------- docs/decisions.md | 25 +++++++++++-------------- src/defs/types.ts | 6 ++---- test/unsendable.test.ts | 19 ++++++++++--------- todo.md | 6 ++++++ 5 files changed, 38 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 53a467e..5305de7 100644 --- a/README.md +++ b/README.md @@ -288,11 +288,11 @@ await session.sendSms({ ``` **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address -and 1 for a numeric one; the NPI fields default to 0. An address is latin1: `Kaffeé` goes out as the -six octets that spell it, `4B 61 66 66 65 E9`, and an address you received always sends back. One -outside `/^[\u0001-ÿ]*$/` is refused, naming the character and its index — strip or -transliterate it first. An SMSC may still refuse a non-ASCII sender of its own accord, which reaches -you as `ESME_RINVSRCADR`. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1: `Kaffeé` goes out as +the six octets that spell it, `4B 61 66 66 65 E9`, and an address you received always sends back. +One outside `/^[\u0001-\u00FF]*$/` is refused, naming the character and its index — strip or +transliterate it first. An SMSC may still refuse a non-ASCII sender of its own accord, which +reaches you as a refusal such as `ESME_RINVSRCADR`. **Encoding.** @@ -346,9 +346,10 @@ you formatted. Refused before anything goes out: an invalid `Date`, `NaN`, `Infi count, and a count past 99 days 23:59:59, since a count in seconds is spelled in days and below. 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`, 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 for `session.send()`. +**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 +yourself, a `Buffer` body or a stamp you formatted, passes through as written. The same rule holds +for `session.send()`. ## Session diff --git a/docs/decisions.md b/docs/decisions.md index b7c7bc7..061277d 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -483,24 +483,21 @@ rule and an index of the titles below. [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). 3.4 calls these fields ASCII, so goal 3 settles the read alone — its generous clause is scoped to reading, and its sender clause is strict. 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 emitted the low - octet, so `Kaffeé` went out as `4B 61 66 66 65 E9` before this change and goes out as the same - octets after it. Naming the write latin1 is what 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 put something else on the wire: a character past `U+00FF` written as its low octet, - and a caller's own `U+0000`, which the peer reads as the end of the field, shifting every mandatory - field behind it under a `command_length` that counted the whole string. Goal 4 settles them twice - over, since for `一`, ` ` and most emoji that low octet is `0x00` and the field went out malformed - on the operator's parser. `wantText()` and - `wantCstringText()` are the only two places that decide it, which is why the `dest_address` and + 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. + Goal 2 settles the refusals, both of them a `size()` that would have agreed with a `write()` 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 + twice over: for a great many characters 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 + it, which is why the `dest_address` and `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than reaching past it into `writeCstring()`. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing 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 - already sends and operators already accept, on no defect; whether an SMSC wants a non-ASCII sender - stays its own call, answered as `ESME_RINVSRCADR`. Rejected: - refusing `U+0000` in every text field, which would buy one spelling by taking a legitimate octet - away from the length-prefixed Octet String, whose length octet is what ends it. + already sends and operators already accept, on no defect. Rejected: refusing `U+0000` in every + text field, which would buy one spelling by taking a legitimate octet away from the + length-prefixed Octet String, whose length octet is what ends it. ## The session's life diff --git a/src/defs/types.ts b/src/defs/types.ts index dfc70ef..5a24a85 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -96,7 +96,7 @@ function pastLatin1(text: string): { err: Error } | undefined { function wantText(value: ParamValue): Result<{ text: string }> { if (typeof value !== 'number' && typeof value !== 'string') { - return { err: new Error(`Expected a string, got ${typeof value}`) }; + return { err: new Error(`Expected a string or a number, got ${typeof value}`) }; } const text = String(value); @@ -114,9 +114,7 @@ function wantCstringText(value: ParamValue): Result<{ text: string }> { if (index === -1) return { text }; return { - err: new Error( - `U+0000 at index ${String(index)} would end the C-Octet String there, so the peer would read every field behind it shifted`, - ), + err: new Error(`U+0000 at index ${String(index)} would end the C-Octet String there`), }; } diff --git a/test/unsendable.test.ts b/test/unsendable.test.ts index 5fae834..3c8c7e3 100644 --- a/test/unsendable.test.ts +++ b/test/unsendable.test.ts @@ -350,19 +350,20 @@ describe('an address the field cannot carry', () => { const smsc = await dummySmsc(t); const session = await bindToSmsc(t, smsc.port, { reconnect: false }); - const refusals: [string, RegExp][] = [ - ['46701113311\u0000EVIL', /U\+0000 at index 11/], - ['Kaffe一', /"一" \(U\+4E00\) at index 5/], - ['😀', /"😀" \(U\+1F600\) at index 0/], + const refusals: ['from' | 'to', string, RegExp][] = [ + ['from', '46701113311\u0000EVIL', /U\+0000 at index 11/], + ['from', 'Kaffe一', /"一" \(U\+4E00\) at index 5/], + ['from', '😀', /"😀" \(U\+1F600\) at index 0/], + ['to', 'Kaffe一', /"一" \(U\+4E00\) at index 5/], ]; - for (const [sender, names] of refusals) { - const sent = await session.sendSms({ from: sender, message: 'Hello world', to }); + for (const [option, address, names] of refusals) { + const sent = await session.sendSms({ from, message: 'Hello world', to, [option]: address }); - assert.ok(sent.err instanceof Error, sender); - assert.match(sent.err.message, /^from: /, 'names the option the caller wrote, not the wire field'); + assert.ok(sent.err instanceof Error, address); + assert.match(sent.err.message, new RegExp(`^${option}: `), 'names the option the caller wrote'); assert.match(sent.err.message, names); - assert.deepEqual(sent.smsIds, [], sender); + assert.deepEqual(sent.smsIds, [], address); } assert.deepEqual(smsc.octets, [], 'an address the field cannot carry never reaches the socket'); diff --git a/todo.md b/todo.md index 685cf8a..e062053 100644 --- a/todo.md +++ b/todo.md @@ -189,6 +189,12 @@ and is also what the panel ranked hardest — two methods, one answer. ### Correctness, ahead of everything below +- [ ] **Refuse a non-finite number where a text field coerces one.** `wantText()` in `defs/types.ts` + stringifies a number so `message_id: 123` writes `"123"`, which is deliberate and tested. It + takes `NaN` and `Infinity` on the same path, so `sendSms({ from: NaN })` puts the literal sender + `NaN` on the wire and reports success — goal 2. Gate the numeric branch on `Number.isFinite`. + Predates the latin1 guard; found by the stability review of #16. + - [ ] **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 `sendReturn(pduObj, 'ESME_RINVCMDID')`; `pduReturn()` then finds no response command, and the -- 2.52.0 From 36a1c6ea7b3c23524a43da5668d24afe2c6cf53e Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 21 Sep 2026 08:25:39 +0200 Subject: [PATCH 6/6] Cut the prose the code, the error and the record already carry --- CHANGELOG.md | 7 ++----- README.md | 13 ++++++------- docs/decisions.md | 8 +++----- test/unsendable.test.ts | 2 +- 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ecf03a..8d78acf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,17 +10,14 @@ `source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` - in one of those fields is now refused, where it used to go out as its low octet — which for `一`, - ` ` and most emoji is `0x00`, ending the field there. + in one of those fields is now refused, where it used to go out as its low octet. **Check what you stored before you roll this out.** Values your application persisted under 0.5.0 were read with bit 7 masked, so an address or a `message_id` carrying an octet above `0x7F` is spelled differently now: a stored id will not match the receipt it belongs to, and a stored address 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 - refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every - mandatory field behind it while `command_length` still counted the whole string. An Octet String - carries a NULL as before; its length octet is what ends it. + refused. An Octet String carries a NULL as before. ## 0.5.0 diff --git a/README.md b/README.md index 5305de7..59d280b 100644 --- a/README.md +++ b/README.md @@ -288,11 +288,11 @@ await session.sendSms({ ``` **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address -and 1 for a numeric one; the NPI fields default to 0. An address is latin1: `Kaffeé` goes out as -the six octets that spell it, `4B 61 66 66 65 E9`, and an address you received always sends back. -One outside `/^[\u0001-\u00FF]*$/` is refused, naming the character and its index — strip or -transliterate it first. An SMSC may still refuse a non-ASCII sender of its own accord, which -reaches you as a refusal such as `ESME_RINVSRCADR`. +and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `é` is one octet on +the wire and an address you received always sends back. One outside `/^[\u0001-\u00FF]*$/` is +refused, naming the character and its index — strip or transliterate it first. An SMSC may still +refuse a non-ASCII sender of its own accord, which reaches you as a refusal such as +`ESME_RINVSRCADR`. **Encoding.** @@ -614,8 +614,7 @@ if (isCommand(pduObj, 'submit_sm')) { names, detected from the text where you name none. One that alphabet cannot carry is refused, naming the character, its code point and where it is. - Every text field is latin1: addresses, `system_id`, `message_id`, `service_type` and the C-Octet - String TLVs. A character past `U+00FF` is refused, as is a `U+0000` in a C-Octet String, which the - peer reads as the end of the field. + String TLVs. A character past `U+00FF` is refused, as is a `U+0000` in a C-Octet String. - A `Buffer` goes out exactly as given under any `data_coding`: binary payloads, hand-built user data headers, deliberately malformed bodies. - `session.send()` and `session.sendReturn()` build through the same codec and refuse the same bodies. diff --git a/docs/decisions.md b/docs/decisions.md index 061277d..118e9a7 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -488,12 +488,10 @@ rule and an index of the titles below. Goal 2 settles the refusals, both of them a `size()` that would have agreed with a `write()` 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 - twice over: for a great many characters 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 - it, which is why the `dest_address` and - `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than - reaching past it into `writeCstring()`. Rejected: reading latin1 and leaving the write spelled - ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send + it. Rejected: reading latin1 and leaving the write spelled ASCII, which leaves two halves agreeing + 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 already sends and operators already accept, on no defect. Rejected: refusing `U+0000` in every text field, which would buy one spelling by taking a legitimate octet away from the diff --git a/test/unsendable.test.ts b/test/unsendable.test.ts index 3c8c7e3..f01aef4 100644 --- a/test/unsendable.test.ts +++ b/test/unsendable.test.ts @@ -366,7 +366,7 @@ describe('an address the field cannot carry', () => { assert.deepEqual(sent.smsIds, [], address); } - assert.deepEqual(smsc.octets, [], 'an address the field cannot carry never reaches the socket'); + assert.deepEqual(smsc.octets, []); }); }); -- 2.52.0