diff --git a/AGENTS.md b/AGENTS.md index 52c2cde..dfd4751 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -198,6 +198,14 @@ exactly 140. dependencies. `@larvit/log` implements it structurally and stays a devDependency, where `test/tls.test.ts` passing a real `Log` as the server's logger keeps that compatibility compiled. +- **`esm_class` decides what a `deliver_sm` is, and the body is only read when it names nothing.** + Message type `MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a + receipt in a format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of + arriving as an inbound SMS. Any other named type — delivery or user acknowledgement, conversation + abort, intermediate notification — is not a receipt and its body is not scraped. Message type 0 + keeps the scrape: SMSCs that send text-only receipts leave `esm_class` at 0, and reading that as + the spec's "default message type" would lose every one of them. + - **The TLS tests build their own self-signed certificate in DER** (`test/tls.test.ts`) instead of adding a devDependency or shelling out to openssl. Maintainer's call, 2026-08-26: the dev image `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI diff --git a/README.md b/README.md index 8fa98e2..3b8c9e1 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,8 @@ session.on('sms', async sms => { ``` Delivery receipts travel on the same SMPP command but reach you as `dlr`, so nothing you write has -to tell the two apart. +to tell the two apart. `esm_class` is what tells them apart; a peer that marks no message type +there has the message body read for the standard `id:` and `stat:` receipt fields instead. ## Server @@ -294,8 +295,8 @@ TypeScript users can import `SmppLog` to have the compiler check one. | Event | Fires when | | --- | --- | | `sms` | An SMS arrives, reassembled if it was multipart. Carries `sendResp()`, `sendDlr()` and the `smsId` it was answered with. | -| `dlr` | A delivery report arrives, one per segment. | -| `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. | +| `dlr` | A delivery report arrives, one per segment. `smsId` is undefined when the peer marked a receipt whose body carries no readable id. | +| `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. | | `close` | The connection closed. | | `reconnected` | The client re-bound after a drop (only with `reconnect` configured). | | `sessionError` | Something failed on a live session, including a hook or listener that threw. | diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 3778faf..a33b62d 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -93,6 +93,8 @@ export class DlrMerger { collect(dlr: Dlr): MessageDlr | undefined { this.sweep(); + if (dlr.smsId === undefined) return undefined; + const match = numbered.exec(dlr.smsId); const base = match?.[1]; const part = match?.[2]; diff --git a/src/dlr.ts b/src/dlr.ts index 0a6c7bd..80d84aa 100644 --- a/src/dlr.ts +++ b/src/dlr.ts @@ -1,4 +1,5 @@ import type { MessageState } from './defs/constants.ts'; +import type { ParamValue } from './defs/types.ts'; import type { PduObject } from './pdu.ts'; import { consts, constsById } from './defs/constants.ts'; @@ -46,7 +47,7 @@ export type Dlr = { doneDate: Date | undefined; errorCode: string | undefined; receipt: Receipt | undefined; - smsId: string; + smsId: string | undefined; statusId: number; statusMsg: string; }; @@ -120,34 +121,59 @@ export function parseReceipt(message: string): Receipt { }; } +/** esm_class bits 5-2 name the message type; the rest are the messaging mode and the GSM features. */ +const messageTypeBits = 0x3c; + +type MessageType = 'other' | 'receipt' | 'unmarked'; + +function messageType(pduObj: PduObject): MessageType { + const esmClass = pduObj.params.esm_class; + + if (typeof esmClass !== 'number') return 'unmarked'; + + const type = esmClass & messageTypeBits; + + if (type === consts.ESM_CLASS.MC_DELIVERY_RECEIPT) return 'receipt'; + + return type === 0 ? 'unmarked' : 'other'; +} + +function receiptId(tlvId: ParamValue | undefined, receipt: Receipt | undefined): string | undefined { + if (typeof tlvId === 'string' && tlvId !== '') return tlvId; + + return receipt?.id === '' ? undefined : receipt?.id; +} + +function receiptStatus( + tlvState: ParamValue | undefined, + receipt: Receipt | undefined, +): { statusId: number; statusMsg: string | undefined } { + if (typeof tlvState === 'number') { + return { statusId: tlvState, statusMsg: constsById.MESSAGE_STATE?.[tlvState] }; + } + + const state = receiptStates[receipt?.stat?.toUpperCase() ?? '']; + + return { statusId: consts.MESSAGE_STATE[state ?? 'UNKNOWN'], statusMsg: state }; +} + /** - * Builds a delivery report from a deliver_sm. The message_state and receipted_message_id TLVs are - * authoritative when present; otherwise the receipt text is parsed, which is the only thing Kannel - * and several other SMSCs send. + * Builds a delivery report from a deliver_sm, or nothing if the PDU carries a message rather than a + * receipt. `esm_class` decides that where the peer sets a message type; where it sets none, the body + * is read for the standard receipt fields, which is the only thing Kannel and several other SMSCs + * send. The message_state and receipted_message_id TLVs are authoritative over the body. */ export function dlrFromPdu(pduObj: PduObject): Dlr | undefined { + const type = messageType(pduObj); + + if (type === 'other') return undefined; + const message = pduObj.params.short_message; const receipt = typeof message === 'string' ? parseReceipt(message) : undefined; - const receiptState = receiptStates[receipt?.stat?.toUpperCase() ?? '']; + const smsId = receiptId(pduObj.tlvs.receipted_message_id?.tagValue, receipt); + const { statusId, statusMsg } = receiptStatus(pduObj.tlvs.message_state?.tagValue, receipt); - const tlvState = pduObj.tlvs.message_state?.tagValue; - const tlvId = pduObj.tlvs.receipted_message_id?.tagValue; - - const smsId = typeof tlvId === 'string' && tlvId !== '' - ? tlvId - : receipt?.id; - - if (smsId === undefined || smsId === '') return undefined; - - const statusMsg = typeof tlvState === 'number' - ? constsById.MESSAGE_STATE?.[tlvState] - : receiptState; - - if (statusMsg === undefined) return undefined; - - const statusId = typeof tlvState === 'number' - ? tlvState - : consts.MESSAGE_STATE[receiptState ?? 'UNKNOWN']; + if (type === 'unmarked' && (smsId === undefined || statusMsg === undefined)) return undefined; return { doneDate: receiptDate(receipt?.doneDate), @@ -155,6 +181,6 @@ export function dlrFromPdu(pduObj: PduObject): Dlr | undefined { receipt, smsId, statusId, - statusMsg, + statusMsg: statusMsg ?? 'UNKNOWN', }; } diff --git a/test/dlr.test.ts b/test/dlr.test.ts index c7bb2cb..153803d 100644 --- a/test/dlr.test.ts +++ b/test/dlr.test.ts @@ -1,17 +1,22 @@ import assert from 'node:assert/strict'; import test, { describe } from 'node:test'; +import { consts } from '../src/defs/constants.ts'; import { dlrFromPdu, parseReceipt, receiptCodes } from '../src/dlr.ts'; import { objToPdu, pduToObj } from '../src/pdu.ts'; import type { PduObject, TlvInput } from '../src/pdu.ts'; const receiptText = 'id:0195f0c7 sub:001 dlvrd:001 submit date:2508251430 done date:2508251431 stat:DELIVRD err:000 text:hello'; -function deliverSm(message: string, tlvs?: Record): PduObject { +function deliverSm( + message: string, + tlvs?: Record, + esmClass: number = consts.ESM_CLASS.MC_DELIVERY_RECEIPT, +): PduObject { const { buffer } = objToPdu({ cmdName: 'deliver_sm', params: { destination_addr: '46701113311', - esm_class: 4, + esm_class: esmClass, short_message: message, source_addr: '46709771337', }, @@ -112,8 +117,40 @@ describe('dlrFromPdu()', () => { assert.equal(dlrFromPdu(deliverSm('id:x stat:DELIVRD done date:2501012560'))?.doneDate, undefined); }); - test('returns nothing when the PDU identifies no message', () => { - assert.equal(dlrFromPdu(deliverSm('just a normal sms')), undefined); + test('returns nothing when an unmarked deliver_sm identifies no message', () => { + assert.equal(dlrFromPdu(deliverSm('just a normal sms', undefined, 0)), undefined); + }); + + test('still reads the body when the peer marks no message type', () => { + const dlr = dlrFromPdu(deliverSm(receiptText, undefined, 0)); + + assert.ok(dlr); + assert.equal(dlr.smsId, '0195f0c7'); + assert.equal(dlr.statusMsg, 'DELIVERED'); + }); + + test('reports a marked receipt whose body it cannot read, rather than an inbound message', () => { + const dlr = dlrFromPdu(deliverSm('a receipt in a format nobody documented')); + + assert.ok(dlr); + assert.equal(dlr.smsId, undefined); + assert.equal(dlr.statusMsg, 'UNKNOWN'); + assert.equal(dlr.statusId, 7); + + const withUdh = consts.ESM_CLASS.MC_DELIVERY_RECEIPT | consts.ESM_CLASS.UDH_INDICATOR; + + assert.ok(dlrFromPdu(deliverSm(receiptText, undefined, withUdh))); + }); + + test('leaves a message the peer marked as another type to arrive as an SMS', () => { + for (const esmClass of [ + consts.ESM_CLASS.CONVERSATION_ABORT, + consts.ESM_CLASS.DELIVERY_ACKNOWLEDGEMENT, + consts.ESM_CLASS.INTERMEDIATE_DELIVERY, + consts.ESM_CLASS.USER_ACKNOWLEDGEMENT, + ]) { + assert.equal(dlrFromPdu(deliverSm(receiptText, undefined, esmClass)), undefined); + } }); test('exposes the raw receipt alongside the resolved fields', () => { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index c0d1e78..1f37467 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -57,7 +57,7 @@ describe('merged delivery reports', () => { const merged = once(resolve => { session.on('messageDlr', resolve); }); const perSegment: string[] = []; - session.on('dlr', dlr => perSegment.push(dlr.smsId)); + session.on('dlr', dlr => perSegment.push(dlr.smsId ?? '')); const [sms] = await Promise.all([ incoming.then(async received => { diff --git a/test/session.test.ts b/test/session.test.ts index f8607c8..16f9cc5 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -711,6 +711,34 @@ describe('receiving', () => { assert.equal(answered.pduObj.params.message_id, 'inbound-id'); }); + test('hands a client a receipt it cannot read as a dlr rather than as an sms', async t => { + const { peer, session } = await inbound(t); + const reported = once(resolve => { session.on('dlr', resolve); }); + let messages = 0; + + session.on('sms', () => { messages++; }); + + const delivered = peer.send({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46709771337', + esm_class: consts.ESM_CLASS.MC_DELIVERY_RECEIPT, + short_message: 'a receipt in a format nobody documented', + source_addr: '46701113311', + }, + }); + const dlr = await raceWithin(2000, reported); + + assert.ok(dlr, 'esm_class marks it a receipt, so nothing else may claim it'); + assert.equal(dlr.smsId, undefined); + assert.equal(messages, 0); + + const answered = await delivered; + + assert.ok(answered.pduObj); + assert.equal(answered.pduObj.cmdName, 'deliver_sm_resp'); + }); + test('reassembles a multipart inbound SMS before the sms event', async t => { const message = 'Inbound lorem ipsum dolor sit amet consectetur, '.repeat(6); const { peer, session } = await inbound(t); @@ -757,7 +785,7 @@ describe('delivery reports', () => { assert.ok(session); - const dlr = once<[{ smsId: string; statusMsg: string }, PduObject]>(resolve => { + const dlr = once<[Dlr, PduObject]>(resolve => { session.on('dlr', (report, pduObj) => { resolve([report, pduObj]); }); }); @@ -873,7 +901,7 @@ describe('delivery reports', () => { const perSegment: string[] = []; let merged = 0; - session.on('dlr', dlr => perSegment.push(dlr.smsId)); + session.on('dlr', dlr => perSegment.push(dlr.smsId ?? '')); session.on('messageDlr', () => { merged++; }); const [sms] = await Promise.all([ diff --git a/todo.md b/todo.md index 9d59b91..1582cc5 100644 --- a/todo.md +++ b/todo.md @@ -5,7 +5,7 @@ rules there constrain every item below. ## Status -The rewrite is **feature complete and green**: 223 tests, lint and typecheck clean, verified on Node +The rewrite is **feature complete and green**: 227 tests, lint and typecheck clean, verified on Node 18, 20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0. ```bash @@ -56,6 +56,7 @@ Rules the API follows: | Session, client, server: bind, auth, send, reassembly, DLRs, timeouts, abort, send window | `test/session.test.ts` | | Merged multipart DLRs, reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | +| Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | | Cross-checked against node-smpp both ways and over a live session | `test/interop.test.ts` | | CI on Node 18/20/22/24, Renovate, tag-triggered publish | `.github/workflows/` | @@ -100,10 +101,6 @@ session message is a change to every call site. ## Before publishing 1.0.0 -- [ ] **`messageDlr` is documented without its precondition.** The README event table says it fires - once every segment of a multipart message has been reported on. `DlrMerger` only merges ids - shaped `-`, which is this library's own server's convention, so against most SMSCs it - never fires at all. `src/dlr-merger.ts` states the precondition; the README must too. - [ ] Create the `@larvit/smpp` package on npm and add `NPM_TOKEN` to the repository secrets, which `.github/workflows/release.yaml` needs. - [ ] Tag `v1.0.0` to publish. @@ -164,13 +161,6 @@ session message is a change to every call site. function) applied to both ids before they are compared covers the whole class. The smallest change on this list for the most real-world breakage removed. -- [ ] **Detect a receipt by `esm_class`, not by what happens to parse.** `dlrFromPdu()` treats a - `deliver_sm` as a receipt exactly when it can scrape an id and a state out of it, and never - reads `esm_class` — `MC_DELIVERY_RECEIPT` (0x04) sits in the constants table unused. That - misclassifies both ways: a receipt in a format we cannot parse arrives as an inbound `sms`, - and a mobile-originated message whose text happens to contain `id:… stat:DELIVRD` arrives as a - `dlr`. Read the bits first and keep the scrape as the fallback for a peer that sets none. - - [ ] **An `onReceipt` hook.** Receipt text is only loosely specified and operators disagree on it, but `dlrFromPdu()` is wired into `IncomingRequests` with no way past it: an application facing a format we do not parse has to listen on `incomingPduObj` and reimplement the dispatch.