diff --git a/CHANGELOG.md b/CHANGELOG.md index e2578b6..da9a440 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ 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. An Octet String carries a NULL as before. +- A number with no decimal spelling — `NaN`, `Infinity`, `-Infinity` — is refused where a text field + takes one. `sendSms({ from: NaN })` put the literal sender `NaN` on the wire and reported the send + as successful; `message_id`, `source_addr` and every other text field took such a number the same + way. A finite number still writes its decimals, so `message_id: 123` is unchanged. ## 0.5.0 diff --git a/src/defs/types.ts b/src/defs/types.ts index 5a24a85..f72e8a3 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -99,6 +99,10 @@ function wantText(value: ParamValue): Result<{ text: string }> { 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); return pastLatin1(text) ?? { text }; diff --git a/test/pdu.test.ts b/test/pdu.test.ts index 7d44959..a774c3b 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -153,6 +153,10 @@ 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); + + // String(NaN) is a sender the peer reads as the three letters, and the send reports success. + 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); }); }); diff --git a/test/types.test.ts b/test/types.test.ts index d353335..51ddc51 100644 --- a/test/types.test.ts +++ b/test/types.test.ts @@ -112,13 +112,20 @@ describe('cstring (C-Octet String)', () => { assert.deepEqual(target, encoded); }); - test('coerces a numeric value to its decimal string', () => { + test('coerces a numeric value to its decimal string, and refuses one with no decimals', () => { const target = Buffer.alloc(4); types.cstring.write(123, target, 0); assert.deepEqual(target, Buffer.from([0x31, 0x32, 0x33, 0x00])); 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.tlv.string.write(value, Buffer.alloc(9), 0).err instanceof Error, String(value)); + } }); test('carries every latin1 octet, and refuses a character past it', () => { diff --git a/todo.md b/todo.md index e062053..685cf8a 100644 --- a/todo.md +++ b/todo.md @@ -189,12 +189,6 @@ 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