diff --git a/AGENTS.md b/AGENTS.md index 06540eb..86266bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,7 +12,7 @@ not for structure or style. ## Goals -The nine goals, in priority order, live in +The goals, in priority order, live in [README.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/README.md#goals) — they say where this library is heading, which an outside reader judges it by. The README states the audience alongside them. Everything below cites a goal by number. @@ -82,12 +82,13 @@ src/ constants.ts consts + constsById, and the SMPP version constants encodings.ts GSM 03.38, LATIN1, UCS2, detection, data_coding resolution errors.ts errors + errorsById (ESME_*) - tlvs.ts TLV definitions, tlvsById, the input shape, and writing a TLV stream + tlvs.ts TLV definitions, tlvsById, the input shape, and reading and writing a TLV stream types.ts Wire types: int8/int16/int32/string/cstring/buffer/arrays ``` -Dependency direction is one way: `defs` knows nothing above it, `pdu` uses `defs`, `session` uses -`pdu`, and `client`/`server` use `session`. Nothing reaches back up. +Imports point one way: `defs` knows nothing above it but `result.ts`, `pdu` uses `defs`, `session` +uses `pdu`, and `client`/`server` use `session`. The one way back up is the `Session` handed to +`IncomingRequests` and `createSms()`, imported as a type only. **Parameter order is wire order.** The key order inside `cmds.*.params` is the order the fields are written to and read from the buffer. Never sort those alphabetically — the alphabetical-ordering @@ -103,20 +104,12 @@ docker compose run --rm node npm test docker compose run --rm node npm run build ``` -- Tests are `.ts` and run directly under Node's type stripping — no build step in the dev loop. - Source imports use `.ts` extensions; `rewriteRelativeImportExtensions` emits `.js` into `dist`. - `erasableSyntaxOnly` is on, so no enums, no namespaces, no parameter properties. Use `as const` objects plus union types. -- The published floor is Node 18, but the dev container runs Node 24 (type stripping needs it). CI - compiles the tests and runs them on 18, every LTS above it, and current, so the floor is - verified rather than asserted. +- The published floor is Node 18; the dev container runs Node 24 because type stripping needs it. - `typescript` is pinned to the 6.x line because `typescript-eslint` peer-requires `<6.1.0`. Move to TypeScript 7 once that constraint lifts. -- GitHub mirrors Gitea through `.gitea/workflows/mirror.yaml`, which never prunes, and - `mirror-delete.yaml`, one run per deleted ref. A delete run that fails or outlives Gitea's queue - timeout, or a push run that cloned before the delete, leaves the ref on GitHub until the delete - run is re-run. Accepted: a stale ref there is harmless, and refs only GitHub has must survive. - Maintainer's call, 2026-09-14; valid while nothing deploys from GitHub. ## Defects found in 0.4.0 @@ -152,13 +145,6 @@ Confirmed by reading the 0.4.0 source; each row has a regression test naming the | `ESME_RINVBCASTCHANIND` typo | Defined as `0x011`, three hex digits; the spec value is `0x0112` | | Every response carries a message id | `session.js` builds `params = {'message_id': …}` for every response it sends, `deliver_sm_resp` included; SMPP 3.4 4.6.2 makes that field unused and NULL, and Jasmin closes the connection on one | -## Multipart sends - -`sendSms` puts every segment of a message on the wire together instead of waiting for each response -in turn, so a long message costs one round trip rather than one per segment. Nothing on the -receiving side forces the order either way: this library answers each inbound segment as it arrives, -so a peer that dispatches one request at a time is never left waiting on us. - ## GSM 7-bit is sent unpacked Over SMPP the ESME puts one GSM character per octet in `short_message` and the SMSC packs it into @@ -249,7 +235,6 @@ the file. - `Session` is publicly constructible, which is what makes `SessionOptions` and `ReconnectOptions` public too. - `acceptsOptionalParams()` and `bindAllows()` are predicates, not chokepoints. -- `session.sock` is a getter over `PduTransport`. - Both emitters re-declare their listener methods to accept a promise. - `PduRefusedError` is exported, and `sessionError` names it in the event's type. - `bitCount()`, `encodeMessage()` and `splitMessage()` keep their total signatures, because @@ -308,6 +293,7 @@ the file. `false` is the one way to turn it off. - A stream this library cannot frame is a dead link; one PDU it cannot parse is not. - A deliberate shutdown drains; an unusable link and an abort do not. +- `sendSms()` puts every segment of a message on the wire together. - Every segment of a concatenated message is answered as it arrives, so `sendResp()` on one is the application's own signal rather than the peer's answer. - `server()` composes the application's `onRequest` after its own bind handling, and offers it every @@ -331,3 +317,5 @@ the file. - `test/` stays flat too, and a file there is named for the question it answers rather than for the module it covers. - CI tests on Linux only; `src/` keeps off what is known to break on macOS or Windows. +- GitHub mirrors Gitea without pruning, and a ref deleted on Gitea is deleted on GitHub by a run of + its own. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1606b0b..77f326f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,16 @@ response command`. - `server()` refuses a `maxOctets` below 1 or not a whole number, `Infinity` included, like its other limits. `server({ maxOctets: 0 })` used to start and then refuse every multipart message. +- `callback_num`, `callback_num_atag`, `callback_num_pres_ind`, `broadcast_area_identifier` and + `broadcast_error_status`, the TLVs SMPP allows more than once in a PDU, keep every occurrence in + wire order. A PDU carrying two of one used to keep only the last. + + **Reading one of these now needs an index.** `pduObj.tlvs.callback_num?.tagValue` is a `Buffer[]` + even where one arrived (a `number[]` for `callback_num_pres_ind` and `broadcast_error_status`), so a + `Buffer.isBuffer()` or `typeof` check written for 0.5.0 now reads it as absent. Read `tagValue[0]` + for the first occurrence. `objToPdu()`, `session.send()` and `session.sendReturn()` take + `{ tagValue: [value] }` for them and refuse a lone value before anything goes out. +- `cmds.broadcast_sm_resp.tlvMap` is removed; nothing read it. ## 0.5.0 diff --git a/MIGRATION.md b/MIGRATION.md index 4eaf30d..35b8b09 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -70,6 +70,9 @@ have for these: - Binary TLVs (`message_payload`, `network_error_code`, `callback_num` and the rest) were parsed into a hex string and written back as the ASCII of that string, so every round trip corrupted them. They are `Buffer`s in both directions now; drop any hex encoding of your own. +- `callback_num`, `callback_num_atag`, `callback_num_pres_ind`, `broadcast_area_identifier` and + `broadcast_error_status` may repeat within a PDU, and each is an array of every occurrence in both + directions. 0.4.0 kept only the last one it read. - A body carried in the `message_payload` TLV was ignored, so the message arrived empty, and a `data_sm` was answered `ESME_RINVCMDID`, so a receipt thrown on one was lost silently. Both reach the application now: a receipt as `dlr`, answered for you, and a message as `sms` for you to answer. diff --git a/README.md b/README.md index fb5ce7f..d93ca5e 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,6 @@ await smpp.close(); // stop listening, then drain and close every live sess `sendDlr('UNDELIVERABLE')` any other state: [Server in depth](#server-in-depth). - A message that arrived in several segments was answered as they arrived, so `sendResp()` there takes no `smsId` or refusing `status`. `sms.answeredOnArrival` says which case you are in. -- `smpp.close()` stops listening, then drains and closes every live session. ## Errors @@ -262,7 +261,7 @@ All optional. Timeouts are milliseconds. | `tls` | `false` | A `tls.TlsOptions` object with your certificate and key. A bare `true` is refused. | | `idleTimeout` | `40000` | Drop a peer that has been silent this long. | | `maxReassembly` | `1000` | Incomplete multipart messages held per session. | -| `maxOctets` | `67108864` | Bytes of incomplete multipart messages held per session. | +| `maxOctets` | `67108864` | Roughly the memory incomplete multipart messages may hold per session. | | `reassemblyTimeout` | `300000` | How long a late segment can still join an incomplete message. | | `responseTimeout`, `shutdownTimeout`, `maxOutstanding`, `log`, `signal` | as for the client | | @@ -608,6 +607,10 @@ if (isCommand(pduObj, 'submit_sm')) { and hands back the UDH where the PDU carries one. - `concatOf(pduObj)`: the `part`, `total` and `reference` a PDU declares and the `spelling` that carried them, `'udh'` or `'sar'`, or `undefined` for a whole message. +- `callback_num`, `callback_num_atag`, `callback_num_pres_ind`, `broadcast_area_identifier` and + `broadcast_error_status` may repeat in one PDU, so each reads as an array of every occurrence in wire + order: `number[]` for `callback_num_pres_ind` and `broadcast_error_status`, `Buffer[]` for the rest. + A `broadcast_sm_resp`'s `failed_broadcast_area_identifier` reads as `broadcast_area_identifier`. - `messageClassOf(dataCoding)`: `0` for the flash class, `1`, `2` and `3` for the ME-, SIM- and TE-specific ones, `undefined` where that `data_coding`'s coding group carries no class. @@ -618,6 +621,8 @@ if (isCommand(pduObj, 'submit_sm')) { 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. +- The five repeatable TLVs take an array, written as one TLV per element; a lone value or an empty + array is refused. - 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. @@ -651,10 +656,10 @@ See [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRAT ## Goals In priority order, and the order is the point: where two of them pull against each other, the earlier -one wins. They do not override the hard rules below. +one wins. They do not override the [hard rules](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/AGENTS.md#hard-rules). 1. **Correct on the wire.** SMPP 3.4 as SMSCs actually run it. Every other goal yields to this one; - the defect table below is what the alternative costs. + the [defect table](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/AGENTS.md#defects-found-in-040) is what the alternative costs. 2. **Never give the application a wrong answer about what happened.** An outcome we cannot determine is reported as undetermined rather than guessed; a report the peer marked as not final settles nothing, so nothing the library concludes may rest on one; a request the peer may already have @@ -695,7 +700,7 @@ one wins. They do not override the hard rules below. to the application to persist. Coordinating processes any other way is declined without a fresh argument each time. 10. **It builds, tests and runs the same everywhere.** Container-only toolchain, no runtime - dependencies, the Node 18 floor verified in CI rather than asserted, every README example executed + dependencies, the Node 18 floor verified in CI, every README example executed by the suite. ## Audience @@ -704,8 +709,7 @@ Who depends on this library, and what they may rely on. - **The public npm audience.** Only what `src/index.ts` exports is public; everything behind it is reshaped freely. -- **Node 18 and newer, ESM only, no runtime dependencies.** The floor is verified in CI rather than - asserted, so the library drops into a service or a container without pulling a tree behind it. +- **Node 18 and newer, ESM only, no runtime dependencies.** - **Real SMSCs and ESMEs as operators actually run them**, not a reference implementation. Jasmin, SMPPSim, Kannel, jsmpp, Cloudhopper, python-smpplib and php-smpp are the interop targets, and what they do in practice outranks what the specification says they should do. diff --git a/docs/decisions.md b/docs/decisions.md index b3b7e33..5034230 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -17,9 +17,6 @@ rule and an index of the titles below. sending them. Only `submit_sm`, `deliver_sm` and `data_sm` are policed by bind direction — the three the library dispatches by it, of which it sends the first two. -- **`session.sock` is a getter over `PduTransport`.** Reading it is unchanged; assigning it no longer - compiles, which never rewired the handlers and so never worked. - - **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call, 2026-08-27: `EventEmitter` types every listener as void-returning, so the `session.on('sms', async sms => …)` README documents reads as a misused promise in any strict @@ -133,8 +130,7 @@ rule and an index of the titles below. rule purely additive: no PDU that parsed before reads differently now. Rejected: preferring the TLV, which re-reads every message a peer echoes into both. Rejected: refusing a PDU carrying both, which discards a message that is almost certainly present twice over, where goal 3 keeps the - traffic. The reassembler's octet cap already counts TLV values, so a 64 KB payload is bounded like - any other segment. + traffic. - **A segment's concatenation is read from its UDH, or from the `sar_*` TLVs where it declares none, and each spelling groups in a reference space of its own.** Maintainer's call, 2026-09-06, from @@ -359,8 +355,8 @@ rule and an index of the titles below. it. There is one budget, 140 less the UDH, and the alphabet decides only what it is counted in, so Latin-1 and UCS2 both take those 134 octets — 134 characters and 67 — and it is GSM 7-bit's 153 that is the odd number rather than the other way round. `Record` is what makes - a fourth alphabet state its own. Rejected: 134 for GSM 7-bit too, which is the mistake the - unpacked-alphabet section above exists to stop. Accepted: a Latin-1 message past the 140 characters + a fourth alphabet state its own. Rejected: 134 for GSM 7-bit too, which is the mistake + [GSM 7-bit is sent unpacked](../AGENTS.md#gsm-7-bit-is-sent-unpacked) exists to stop. Accepted: a Latin-1 message past the 140 characters one SMS holds now costs more segments than it did, and `smsIds` is that much longer. - **An alphabet the caller named has to carry the message, and a time the format cannot express is @@ -588,6 +584,10 @@ rule and an index of the titles below. reports each session's unfinished drain through `serverError`, because its own result says nothing but that the listener stopped. +- **`sendSms()` puts every segment of a message on the wire together.** Goal 6: a long message costs + one round trip rather than one per segment. Rejected: sending each segment once the last is + answered, which a receiver waiting for the whole message before answering would deadlock. + - **Every segment of a concatenated message is answered as it arrives, so `sendResp()` on one is the application's own signal rather than the peer's answer.** Maintainer's call, 2026-09-06, from the Jasmin interoperability phase: Jasmin dispatches one `submit_sm` per connector at a time and will @@ -789,7 +789,7 @@ rule and an index of the titles below. dev image has no openssl. - **`src/` stays flat until a module has to move for another reason.** Architecture review, - 2026-09-06: the grouping the file map above already implies — `wire/` for `pdu*` and `defs`, + 2026-09-06: the grouping the [file map](../AGENTS.md#architecture) already implies — `wire/` for `pdu*` and `defs`, `link/` for `link-*`, `reconnect-*`, `pdu-transport` and `send-window`, `messages/` for `sms*`, `dlr*`, `message*`, `reassembly` and `udh` — rewrites every import for no change to `dist/index.js`, the one published entry. Valid while that map is what a reader navigates by. @@ -809,3 +809,10 @@ rule and an index of the titles below. That binds what `dist/` runs; the container tooling, `interop-tests/` and the `package.json` scripts run on Linux by goal 10. Rejected: macOS and Windows runners, on GitHub's mirror or as Gitea host-mode runners on a Windows VM and a Mac. + +- **GitHub mirrors Gitea without pruning, and a ref deleted on Gitea is deleted on GitHub by a run of + its own.** Maintainer's call, 2026-09-14; valid while nothing deploys from GitHub. + `.gitea/workflows/mirror.yaml` never prunes, and `mirror-delete.yaml` runs once per deleted ref. A + delete run that fails or outlives Gitea's queue timeout, or a push run that cloned before the + delete, leaves the ref on GitHub until the delete run is re-run. Accepted: a stale ref there is + harmless, and refs only GitHub has must survive. diff --git a/src/defs/commands.ts b/src/defs/commands.ts index 900a789..13ebf82 100644 --- a/src/defs/commands.ts +++ b/src/defs/commands.ts @@ -4,7 +4,6 @@ import { buffer, cstring, dest_address_array, int8, unsuccess_sme_array } from ' type CommandSpec = { id: number; params?: Record; - tlvMap?: Record; }; const bindParams = { @@ -59,7 +58,6 @@ const specs = { broadcast_sm_resp: { id: 0x80000111, params: { message_id: cstring }, - tlvMap: { broadcast_area_identifier: 'failed_broadcast_area_identifier' }, }, cancel_broadcast_sm: { id: 0x00000113, diff --git a/src/defs/tlvs.ts b/src/defs/tlvs.ts index 79ff8c2..7959625 100644 --- a/src/defs/tlvs.ts +++ b/src/defs/tlvs.ts @@ -1,18 +1,15 @@ -import type { ParamValue, WireType } from './types.ts'; +import type { ParamValue, TlvValue, WireType } from './types.ts'; import type { Result } from '../result.ts'; import { tlv } from './types.ts'; -export type TlvDefinition = { - id: number; - multiple?: boolean; - tag: string; - type: WireType; -}; +/** Only a tag read as octets or as a number may repeat, since its occurrences are listed as one of those. */ +type Definition = { id: number; multiple?: false; tag: Tag; type: WireType } + | { id: number; multiple: true; tag: Tag; type: WireType | WireType }; + +export type TlvDefinition = Definition; /** The constraint keys every definition to its own name, so a `tag` that drifts fails to compile. */ -const tlvSpecs = ( - definitions: T, -): T => definitions; +const tlvSpecs = }>(definitions: T): T => definitions; // Ordered by tag id, mirroring the SMPP 5.0 TLV table. const specs = tlvSpecs({ @@ -98,18 +95,18 @@ for (const definition of Object.values(specs)) { } /** Fallback for tags this table does not know: keep the raw octets. */ -export const tlvDefault: WireType = tlv.buffer; +export const tlvDefault: WireType = tlv.buffer; export type Tlv = { tagId: number; tagName: string | undefined; - tagValue: ParamValue; + tagValue: TlvValue; }; export type TlvInput = { /** Resolved from the record key; pass it for a tag the TLV table does not define. */ tagId?: number | undefined; - tagValue: ParamValue; + tagValue: TlvValue; }; export function tagIdOf(name: string, input: TlvInput): Result<{ tagId: number }> { @@ -135,30 +132,115 @@ export function writeTlvs(inputs: Record | undefined): Result< if (tag.err) return { err: tag.err }; - const type = tlvsById[tag.tagId]?.type ?? tlvDefault; - const sized = type.size(input.tagValue); + const definition = tlvsById[tag.tagId]; + const values = occurrences(input.tagValue, definition?.multiple === true); - if (sized.err) { - return { err: new Error(`TLV "${name}": ${sized.err.message}`) }; + if (values.err) return { err: new Error(`TLV "${name}": ${values.err.message}`) }; + + for (const value of values.values) { + const chunk = writeTlv(tag.tagId, definition?.type ?? tlvDefault, value); + + if (chunk.err) return { err: new Error(`TLV "${name}": ${chunk.err.message}`) }; + + chunks.push(chunk.chunk); } - - if (sized.size > 0xffff) { - return { err: new Error(`TLV "${name}": ${String(sized.size)} octets overflow the two octet length`) }; - } - - const chunk = Buffer.alloc(sized.size + 4); - - chunk.writeUInt16BE(tag.tagId, 0); - chunk.writeUInt16BE(sized.size, 2); - - const written = type.write(input.tagValue, chunk, 4); - - if (written.err) { - return { err: new Error(`TLV "${name}": ${written.err.message}`) }; - } - - chunks.push(chunk); } return { chunks }; } + +function occurrences(value: TlvValue, multiple: boolean): Result<{ values: ParamValue[] }> { + if (!multiple) { + return Array.isArray(value) ? { err: new Error('takes one value, not an array') } : { values: [value] }; + } + + if (!Array.isArray(value)) return { err: new Error('is repeatable, wrap it in an array: [value]') }; + + if (value.length === 0) return { err: new Error('holds no values, omit it instead') }; + + return { values: value }; +} + +function writeTlv(tagId: number, type: WireType, value: ParamValue): Result<{ chunk: Buffer }> { + const sized = type.size(value); + + if (sized.err) return { err: sized.err }; + + if (sized.size > 0xffff) { + return { err: new Error(`${String(sized.size)} octets overflow the two octet length`) }; + } + + const chunk = Buffer.alloc(sized.size + 4); + + chunk.writeUInt16BE(tagId, 0); + chunk.writeUInt16BE(sized.size, 2); + + const written = type.write(value, chunk, 4); + + return written.err ? { err: written.err } : { chunk }; +} + +type Occurrence = { definition: TlvDefinition | undefined; tagId: number; value: Buffer | number | string }; + +function readTlv(pdu: Buffer, offset: number): Result<{ octets: number; occurrence: Occurrence }> { + const tagId = pdu.readUInt16BE(offset); + const tagLength = pdu.readUInt16BE(offset + 2); + + if (offset + 4 + tagLength > pdu.length) { + return { err: new Error(`TLV ${String(tagId)} runs past the end of the PDU`) }; + } + + const definition = tlvsById[tagId]; + const read = (definition?.type ?? tlvDefault).read(pdu, offset + 4, tagLength); + + if (read.err) return { err: read.err }; + + return { occurrence: { definition, tagId, value: read.value }, octets: 4 + tagLength }; +} + +/** Keyed by tag name, a repeatable tag listing every occurrence in wire order and any other keeping its last. */ +function keyedTlvs(occurrences: Occurrence[]): Record { + const repeated = new Map(); + const tlvs: Record = {}; + + for (const { definition, tagId, value } of occurrences) { + const key = definition?.tag ?? tagId.toString(); + + if (definition?.multiple === true) { + const entry = repeated.get(key) ?? { tagId, values: [] }; + + entry.values.push(value); + repeated.set(key, entry); + } else { + tlvs[key] = { tagId, tagName: definition?.tag, tagValue: value }; + } + } + + for (const [key, { tagId, values }] of repeated) { + const buffers = values.filter(value => Buffer.isBuffer(value)); + + tlvs[key] = { + tagId, + tagName: key, + tagValue: buffers.length === values.length ? buffers : values.filter(value => typeof value === 'number'), + }; + } + + return tlvs; +} + +export function parseTlvs(pdu: Buffer, start: number): Result<{ offset: number; tlvs: Record }> { + const occurrences: Occurrence[] = []; + let offset = start; + + while (offset + 4 <= pdu.length) { + const read = readTlv(pdu, offset); + + if (read.err) return { err: read.err }; + + occurrences.push(read.occurrence); + offset += read.octets; + } + + return { offset, tlvs: keyedTlvs(occurrences) }; +} diff --git a/src/defs/types.ts b/src/defs/types.ts index cd601d8..7f3acf5 100644 --- a/src/defs/types.ts +++ b/src/defs/types.ts @@ -14,6 +14,34 @@ export type UnsuccessSme = { export type ParamValue = Buffer | DestAddress[] | UnsuccessSme[] | number | string; +/** A tag defined `multiple` holds every occurrence, in wire order; any other tag holds one value. */ +export type TlvValue = Buffer | Buffer[] | number | number[] | string; + +/** Octets a value holds, counting a string by its length. */ +export function tlvOctets(value: TlvValue): number { + if (Buffer.isBuffer(value)) return value.length; + if (typeof value === 'string') return value.length; + if (typeof value === 'number') return 0; + + let octets = 0; + + for (const one of value) { + octets += typeof one === 'number' ? 0 : one.length; + } + + return octets; +} + +/** A value holding no view into the PDU it was read from. */ +export function detachedTlv(value: TlvValue): TlvValue { + if (Buffer.isBuffer(value)) return Buffer.from(value); + if (!Array.isArray(value)) return value; + + const buffers = value.filter(one => Buffer.isBuffer(one)); + + return buffers.length === value.length ? buffers.map(one => Buffer.from(one)) : value; +} + /** * One field on the wire. `read` reports how many octets it consumed so callers never have to * re-derive a length that could disagree with what was actually written. @@ -26,7 +54,7 @@ export type WireType = { }; /** Renders a parameter as text without ever falling back to "[object Object]". */ -export function paramText(value: ParamValue | undefined): string { +export function paramText(value: ParamValue | TlvValue | undefined): string { if (typeof value === 'string') return value; if (typeof value === 'number') return value.toString(); if (Buffer.isBuffer(value)) return value.toString('latin1'); diff --git a/src/dlr.ts b/src/dlr.ts index 1ef5b46..27ab4dd 100644 --- a/src/dlr.ts +++ b/src/dlr.ts @@ -1,5 +1,5 @@ import type { MessageState } from './defs/constants.ts'; -import type { ParamValue } from './defs/types.ts'; +import type { TlvValue } from './defs/types.ts'; import type { PduObject } from './pdu.ts'; import type { SmsIdFormat } from './sms-id.ts'; import { consts, constsById, hasUdh, messageTypeOf } from './defs/constants.ts'; @@ -150,7 +150,7 @@ const smeMessageTypes: readonly number[] = [ consts.ESM_CLASS.USER_ACKNOWLEDGEMENT, ]; -function nonEmptyText(value: ParamValue | undefined): string | undefined { +function nonEmptyText(value: TlvValue | undefined): string | undefined { return typeof value === 'string' && value !== '' ? value : undefined; } @@ -178,7 +178,7 @@ function receiptBody(pduObj: PduObject): string { } function receiptId( - tlvId: ParamValue | undefined, + tlvId: TlvValue | undefined, receipt: Receipt | undefined, format: SmsIdFormat, ): string | undefined { @@ -198,7 +198,7 @@ function isMessageState(name: string | undefined): name is MessageState { /** The state TLV wins where it names a state we know; an unnameable one leaves the body to say. */ function receiptStatus( - tlvState: ParamValue | undefined, + tlvState: TlvValue | undefined, receipt: Receipt | undefined, ): { statusId: number; statusMsg: MessageState | undefined } { const scraped = receiptStates[receipt?.stat?.toUpperCase() ?? '']; diff --git a/src/index.ts b/src/index.ts index eea6081..d24f9d0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -68,7 +68,7 @@ export type { PduObject, PduObjectInput, TlvInput } from './pdu.ts'; export type { PduHeader } from './pdu-refusal.ts'; export type { SplitOptions } from './message.ts'; export type { Tlv, TlvDefinition, TlvName } from './defs/tlvs.ts'; -export type { DestAddress, ParamValue, UnsuccessSme, WireType } from './defs/types.ts'; +export type { DestAddress, ParamValue, TlvValue, UnsuccessSme, WireType } from './defs/types.ts'; /** The spec tables, grouped the way `larvitsmpp.defs` was in 0.4.0. */ export { defs } from './defs/index.ts'; diff --git a/src/pdu.ts b/src/pdu.ts index 48d686a..8ba806f 100644 --- a/src/pdu.ts +++ b/src/pdu.ts @@ -10,7 +10,7 @@ import { hasUdh } from './defs/constants.ts'; import { decodeMessage, encodeBody } from './message.ts'; import { errorNameById, errors, isErrorName } from './defs/errors.ts'; import { paramNumber, valueText } from './defs/types.ts'; -import { tagIdOf, tlvDefault, tlvs, tlvsById, writeTlvs } from './defs/tlvs.ts'; +import { parseTlvs, tagIdOf, tlvs, writeTlvs } from './defs/tlvs.ts'; /** The highest sequence number this library hands out; SMPP 3.4 4.7.1 reserves 0x7fffffff. */ export const maxSeqNr = 2147483646; @@ -262,35 +262,6 @@ export function objToPdu(obj: PduObjectInput): Result< ); } -function parseTlvs(pdu: Buffer, start: number): Result<{ offset: number; tlvs: Record }> { - const tlvs: Record = {}; - let offset = start; - - while (offset + 4 <= pdu.length) { - const tagId = pdu.readUInt16BE(offset); - const tagLength = pdu.readUInt16BE(offset + 2); - - if (offset + 4 + tagLength > pdu.length) { - return { err: new Error(`TLV ${String(tagId)} runs past the end of the PDU`) }; - } - - const definition = tlvsById[tagId]; - const read = (definition?.type ?? tlvDefault).read(pdu, offset + 4, tagLength); - - if (read.err) return { err: read.err }; - - tlvs[definition?.tag ?? tagId.toString()] = { - tagId, - tagName: definition?.tag, - tagValue: read.value, - }; - - offset += 4 + tagLength; - } - - return { offset, tlvs }; -} - function readParams( cmdName: CommandName, pdu: Buffer, diff --git a/src/reassembly.ts b/src/reassembly.ts index 98a7abc..a40eccc 100644 --- a/src/reassembly.ts +++ b/src/reassembly.ts @@ -6,7 +6,7 @@ import type { Tlv } from './defs/tlvs.ts'; import { ExpiringGroups } from './expiring-groups.ts'; import { decodeMessage } from './message.ts'; import { messageOctets } from './message-body.ts'; -import { paramNumber, paramText } from './defs/types.ts'; +import { detachedTlv, paramNumber, paramText, tlvOctets } from './defs/types.ts'; import { uuidv7 } from './uuid.ts'; /** A concatenated message given up on, whose segments the peer has already been answered for. */ @@ -62,9 +62,7 @@ function detach(pduObj: PduObject): PduObject { } for (const [name, tlv] of Object.entries(pduObj.tlvs)) { - tlvs[name] = Buffer.isBuffer(tlv.tagValue) - ? { ...tlv, tagValue: Buffer.from(tlv.tagValue) } - : tlv; + tlvs[name] = { ...tlv, tagValue: detachedTlv(tlv.tagValue) }; } // short_message holds the same octets wherever it was not decoded, so one copy covers both. @@ -75,8 +73,11 @@ function detach(pduObj: PduObject): PduObject { return { ...pduObj, params, shortMessageOctets: octets, tlvs }; } +// Roughly the heap a listed occurrence costs beyond its value, so a PDU of empty repeats is not free. +const listedTlvOverhead = 200; + // A cstring param arrives as a string, and source_addr alone can carry most of a 1 MiB PDU. -function sizeOf(value: unknown): number { +function sizeOf(value: ParamValue): number { if (Buffer.isBuffer(value)) return value.length; return typeof value === 'string' ? value.length : 0; @@ -90,7 +91,9 @@ function octetsOf(pduObj: PduObject): number { } for (const tlv of Object.values(pduObj.tlvs)) { - octets += sizeOf(tlv.tagValue); + const listed = Array.isArray(tlv.tagValue) ? tlv.tagValue.length : 0; + + octets += tlvOctets(tlv.tagValue) + listed * listedTlvOverhead; } return octets; diff --git a/src/send-sms.ts b/src/send-sms.ts index 0224c33..2e70932 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -314,8 +314,7 @@ export async function submitSms(deps: SendSmsDeps, sms: SendSmsInput): Promise deps.send({ cmdName: 'submit_sm', params: submitSmParams(sms, segment, { diff --git a/test/pdu.test.ts b/test/pdu.test.ts index cab91b7..c2de212 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -463,6 +463,48 @@ describe('TLVs', () => { assert.ok(err instanceof Error); }); + test('keeps every occurrence of a repeatable TLV, in wire order', () => { + const first = Buffer.from('0146709771337', 'hex'); + const second = Buffer.from('0146701113311', 'hex'); + const pduObj = decode(encode({ + cmdName: 'submit_sm', + params: { destination_addr: '46709771337', short_message: 'hi', source_addr: '46701113311' }, + tlvs: { + callback_num: { tagValue: [first, second] }, + callback_num_pres_ind: { tagValue: [1] }, + }, + })); + + assert.deepEqual(pduObj.tlvs.callback_num?.tagValue, [first, second]); + assert.deepEqual(pduObj.tlvs.callback_num_pres_ind?.tagValue, [1]); + }); + + test('reads the failed areas of a broadcast_sm_resp as broadcast_area_identifier', () => { + const areas = [Buffer.from('0001', 'hex'), Buffer.from('0002', 'hex')]; + const pduObj = decode(encode({ + cmdName: 'broadcast_sm_resp', + params: { message_id: '01a0d051-b588-76eb-a5c5-a8cb8b854e68' }, + tlvs: { failed_broadcast_area_identifier: { tagValue: areas } }, + })); + + assert.deepEqual(pduObj.tlvs.broadcast_area_identifier?.tagValue, areas); + }); + + test('refuses a repeatable TLV given one value, and a lone TLV given several', () => { + const params = { destination_addr: '46709771337', short_message: 'hi', source_addr: '46701113311' }; + + for (const tlvs of [ + { callback_num: { tagValue: Buffer.from('01', 'hex') } }, + { callback_num: { tagValue: [] } }, + { source_port: { tagValue: [1234, 1235] } }, + ]) { + const { buffer, err } = objToPdu({ cmdName: 'submit_sm', params, tlvs }); + + assert.equal(buffer, undefined); + assert.ok(err instanceof Error); + } + }); + test('round-trips a receipt with message_state and receipted_message_id', () => { const receipt = 'id:450 sub:001 dlvrd:1 submit date:1504031342 done date:1504031342 stat:DELIVRD err:0 text:xxx'; const pduObj = decode(encode({ diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 36b0bdb..3456adf 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1824,6 +1824,32 @@ describe('reassembly bounds', () => { assert.equal(collectPayload(40).kept, true); }); + test('counts every occurrence of a repeatable TLV against the octet cap, empty ones included', () => { + function collectCallbacks(maxOctets: number, tagValue: Buffer[]): Collected { + const reassembler = new Reassembler({ + log: silentLog, + max: 10, + maxOctets, + now: () => 0, + onLost: () => undefined, + timeout: 60_000, + }); + const carried = segment(9, 1, 2); + + return collectPdu(reassembler, { ...carried, tlvs: { callback_num: { tagId: 0x0381, tagName: 'callback_num', tagValue } } }); + } + + const numbers = [Buffer.alloc(10_000, 0x31), Buffer.alloc(10_000, 0x32)]; + + assert.equal(collectCallbacks(20_000, numbers).kept, false); + assert.equal(collectCallbacks(30_000, numbers).kept, true); + assert.equal( + collectCallbacks(30_000, Array.from({ length: 10_000 }, () => Buffer.alloc(0))).kept, + false, + 'an empty occurrence still holds an object', + ); + }); + // The segments before it were answered ESME_ROK, so dropping those is not the same as refusing one. test('reports the answered segments of a group that overruns the cap mid-message', () => { const lost: LostGroup[] = []; diff --git a/todo.md b/todo.md index 1276023..7b83a5f 100644 --- a/todo.md +++ b/todo.md @@ -6,6 +6,14 @@ hard rules first — they constrain every item below. This is a working file that sets its own rules. The documentation conventions in AGENTS.md do not govern it, and nothing here is a source anything else may cite. +## Security + +- [ ] **Charge a held segment's TLVs for the objects they keep, not only their value octets.** A + peer sending segments that carry thousands of distinct unknown tags with empty values makes + this library hold megabytes of heap per segment that `maxOctets` counts as nothing, up to + 255 segments per group. Repeatable tags are already charged per occurrence. From the stability + review of #25. + ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18 @@ -189,12 +197,22 @@ and is also what the panel ranked hardest — two methods, one answer. ### Correctness, ahead of everything below -- [ ] **Read `multiple` in `parseTlvs()` and `writeTlvs()`, or delete it and `tlvMap`.** Five TLVs - declare `multiple: true` (`callback_num`, `callback_num_atag`, `callback_num_pres_ind`, - `broadcast_area_identifier`, `broadcast_error_status`) and nothing reads it; `parseTlvs()` keys - by tag name, so a peer sending two `callback_num` TLVs silently keeps the last. `tlvMap` on - `broadcast_sm_resp` is declared, set once and read nowhere. This is the "Dormant filters" row - of the 0.4.0 defect table in a new spelling — metadata that reads as a guarantee. +- [ ] **Type each known TLV's value by its tag, on read and on write, before 0.6.0 is cut.** Every + `tagValue` is `TlvValue`, so `{ callback_num: { tagValue: buf } }` compiles and is refused + only at runtime, and reading `receipted_message_id` has to narrow out arrays it can never + hold. Derive the types from the specs' own wire types and `multiple` flag. It has to land in + the same minor as the arrays, or narrowing the types is a second break. A test compiles the + CHANGELOG's `tagValue[0]` advice, which the union does not type-check today. From the + product-owner review of #25. + +- [ ] **Settle what a repeated tag not marked `multiple` reads as, and pin it in a test.** A vendor + tag or a known single-value tag a peer sends twice keeps the last occurrence and drops the + rest silently, which goal 3 argues against; listing it would change every such tag's shape. + From the architecture review of #25. + +- [ ] **Refuse a TLV input naming one tag under both its spellings.** `broadcast_area_identifier` + and `failed_broadcast_area_identifier` in one `tlvs` record both write, so the peer receives + the union of two lists the caller may have meant as one. From the architecture review of #25. - [ ] **Test that a multipart send which errors never fires `messageDlr`.** Goal 2 now says so and README promises it; `session-extras.test.ts` covers a drop *after* the send, not one during it. @@ -244,8 +262,8 @@ and is also what the panel ranked hardest — two methods, one answer. - [ ] **Give `IncomingRequests` a port instead of the `Session` it drives.** It holds its owner and calls eight members of it 18 times, including `this.session.close()` on an inbound `unbind` — a collaborator ending its owner's life. `OutgoingRequests` is the mirror half of the same - boundary and takes no session at all. AGENTS.md's "Nothing reaches back up" is false because of - this, and `docs/decisions.md` already states the rule under The session's life: "a collaborator + boundary and takes no session at all. AGENTS.md names this as the one way back up; the port + removes that exception, and `docs/decisions.md` already states the rule under The session's life: "a collaborator that has to ask does not own its decision". It is also the missing test seam — inbound routing, reassembly dispatch, `onRequest` ordering and bind-direction refusal have no unit test because the class cannot be built without a live socket. Carry the eight members as `IncomingDeps`, @@ -362,6 +380,18 @@ and is also what the panel ranked hardest — two methods, one answer. ### Doc claims this review falsified +- [ ] **Make `LinkGate.isUp()`'s doc true or its state match it.** It says a link attached but not + yet bound cannot carry a request, while `up` starts `true`, so the first link and a server + session are up before any bind. From the comprehension panel of #25. + +- [ ] **Move `checkSessionOptions()`'s doc comment to what it describes.** It explains why a count + below 1 is refused, which is `checkLimits`' job, and says nothing of the function it heads. + From the comprehension panel of #25. + +- [ ] **Log why `DlrMerger.expect()` registered no merge.** Ids with no common `-` + numbering return silently, the likeliest cause of a `messageDlr` that never fires and the one + that leaves no trace. From the comprehension panel of #25. + - [ ] **Make "every README example is executed by the suite" true, or stop claiming it.** Goal 10 and the Done table both promise it; `test/readme.test.ts` transcribes the examples by hand and has drifted — 15 fenced `javascript` blocks in the README against 10 tests, and the test named "the @@ -369,10 +399,6 @@ and is also what the panel ranked hardest — two methods, one answer. the fenced blocks at test time and assert each appears verbatim in the executed source, so an edit to either fails the gate. -- [ ] **Correct AGENTS.md's "Nothing reaches back up".** False while `IncomingRequests` holds a - `Session`: either the first Locality item makes it true, or the sentence names the exception - until it does. - - [ ] **Narrow the `src/defs/*` lint exemption to the four table files.** Its stated reason — "the spec tables are data: their length tracks the specification, not any complexity" — is false for `defs/types.ts`, which is 595 lines of wire codec with 25 functions and is the file that parses