From 435fa427083667eda2b7180d88629f4eae7954db Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Wed, 26 Aug 2026 22:59:39 +0200 Subject: [PATCH] Decompose the codec, document the session surface and record the review decisions --- AGENTS.md | 14 ++++ README.md | 7 +- eslint.config.js | 10 +-- src/dlr.ts | 5 +- src/pdu.ts | 188 ++++++++++++++++++++++++++++++----------------- src/server.ts | 4 +- src/session.ts | 4 +- test/dlr.test.ts | 29 ++++---- test/pdu.test.ts | 15 ++++ todo.md | 6 +- 10 files changed, 186 insertions(+), 96 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3a30f9d..4c81610 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,6 +154,20 @@ exactly 140. ## Decisions +- **The published surface is frozen at what `src/index.ts` exports today.** `Session` is exported and + publicly constructible, which is why `SessionOptions` and `ReconnectOptions` are public too — that + is correct, not a leak, and it has been raised twice. The collaborators `session.ts` delegates to + (`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `DlrMerger`, + `submitSms`) stay unpublished so they can be reshaped. +- **The sub-3.4 optional-parameter rule is a predicate, not a chokepoint.** `acceptsOptionalParams()` + is consulted by the library's own senders; `session.send({ tlvs })` is passed through as written, + because silently stripping a caller's explicit TLVs off a deliberately public low-level surface + would be worse than sending them. The guarantee is "what this library sends honours the rule", + never "the session cannot send optional parameters to an old peer". +- **Only the server feeds `peerInterfaceVersion`.** `acceptBind()` records what the peer declared; + the client never reads `sc_interface_version` out of its bind response, so a client session is + permissive. That is not a defect today — this library's ESME direction sends no TLVs at all — but + anyone adding a client-side TLV owes the other half of the feed. - **The library speaks SMPP 3.4 on the wire, and `defs/` keeps the 5.0 tables as a superset.** Maintainer's call, 2026-08-26: 3.4 is what SMSCs actually run, while the wider tables let the codec parse and build whatever a peer sends. The declared version is an option on both `client()` and diff --git a/README.md b/README.md index 4cc2cf2..511f822 100644 --- a/README.md +++ b/README.md @@ -171,7 +171,7 @@ await smpp.close(); // stop listening and close every live session | `tls` | `false` | A `tls.TlsOptions` object with your certificate and key. | | `idleTimeout` | `40000` | Drop a peer that has been silent this long. | | `maxReassembly` | `1000` | Incomplete multipart messages held per session. | -| `reassemblyTimeout` | `300000` | How long an incomplete multipart message is held. | +| `reassemblyTimeout` | `300000` | How long a late segment can still join an incomplete message. | | `responseTimeout`, `maxOutstanding`, `log`, `signal` | as for the client | | ## Errors @@ -218,6 +218,11 @@ const { err, pduObj } = await session.send({ }); ``` +`acceptsOptionalParams()` answers whether the peer declared SMPP 3.4 or later, which is the version +at and above which the spec allows optional parameters to be sent to it; `peerInterfaceVersion` is +the raw value it declared. The library's own senders consult the first before attaching a TLV — a +`send()` you build yourself is passed through as written, so consult it too when you attach TLVs. + ## Working with PDUs directly The codec is exported, synchronous, and never throws — handy for inspecting captured traffic: diff --git a/eslint.config.js b/eslint.config.js index 0216ded..755c9c0 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -39,13 +39,9 @@ export default tseslint.config( rules: { 'max-lines': 'off' }, }, { - // The codec branches per wire type and per parameter; splitting it scatters the wire format - // across files instead. These two keep the ceiling they have today. - files: ['src/dlr.ts', 'src/pdu.ts'], - rules: { - complexity: ['error', 22], - 'max-lines-per-function': ['error', { max: 75, skipBlankLines: true, skipComments: true }], - }, + // ESLint counts every ?. and ?? in dlrFromPdu as a branch; the 19 is 26 lines of flat field resolution. + files: ['src/dlr.ts'], + rules: { complexity: ['error', 19] }, }, { // ESC (0x1B) is the GSM 03.38 escape character, so it belongs in these patterns. diff --git a/src/dlr.ts b/src/dlr.ts index c58fb83..19b63bb 100644 --- a/src/dlr.ts +++ b/src/dlr.ts @@ -120,6 +120,7 @@ export function parseReceipt(message: string): Receipt { export function dlrFromPdu(pduObj: PduObject): Dlr | undefined { const message = pduObj.params.short_message; const receipt = typeof message === 'string' ? parseReceipt(message) : undefined; + const receiptState = receiptStates[receipt?.stat?.toUpperCase() ?? '']; const tlvState = pduObj.tlvs.message_state?.tagValue; const tlvId = pduObj.tlvs.receipted_message_id?.tagValue; @@ -132,13 +133,13 @@ export function dlrFromPdu(pduObj: PduObject): Dlr | undefined { const statusMsg = typeof tlvState === 'number' ? constsById.MESSAGE_STATE?.[tlvState] - : receiptStates[receipt?.stat?.toUpperCase() ?? '']; + : receiptState; if (statusMsg === undefined) return undefined; const statusId = typeof tlvState === 'number' ? tlvState - : consts.MESSAGE_STATE[receiptStates[receipt?.stat?.toUpperCase() ?? ''] ?? 'UNKNOWN']; + : consts.MESSAGE_STATE[receiptState ?? 'UNKNOWN']; return { doneDate: receiptDate(receipt?.doneDate), diff --git a/src/pdu.ts b/src/pdu.ts index 1ead29a..aa5af95 100644 --- a/src/pdu.ts +++ b/src/pdu.ts @@ -1,7 +1,7 @@ -import type { CommandName, PduParams, PduParamsInput } from './defs/commands.ts'; +import type { CommandDefinition, CommandName, PduParams, PduParamsInput } from './defs/commands.ts'; import type { ErrorName } from './defs/errors.ts'; import type { ParamValue } from './defs/types.ts'; -import type { Result } from './result.ts'; +import type { Result, VoidResult } from './result.ts'; import type { Tlv } from './defs/tlvs.ts'; import { cmds, commandNameById, isCommandName } from './defs/commands.ts'; import { consts } from './defs/constants.ts'; @@ -79,46 +79,31 @@ function tagIdOf(name: string, input: TlvInput): Result<{ tagId: number }> { return { tagId }; } -function buildPdu( - cmdName: CommandName, - cmdStatus: ErrorName, - seqNr: number, +/** Encoding a string short_message also settles data_coding and sm_length. */ +function resolveShortMessage( params: Record, - tlvs: Record | undefined, -): Result<{ buffer: Buffer }> { - const definition = cmds[cmdName]; +): Record { + const message = params.short_message; - if (!definition) { - return { err: new Error(`Invalid cmdName: ${JSON.stringify(cmdName)}`) }; - } + if (typeof message !== 'string') return { ...params }; - if (!isErrorName(cmdStatus)) { - return { err: new Error(`Invalid cmdStatus: ${JSON.stringify(cmdStatus)}`) }; - } + const dataCoding = params.data_coding; + const encoding = typeof dataCoding === 'number' ? encodingByDataCoding(dataCoding) : detect(message); + const encoded = encodeMessage(message, encoding); - if (!Number.isInteger(seqNr) || seqNr < 0 || seqNr > maxSeqNr) { - return { err: new Error(`Invalid seqNr: ${JSON.stringify(seqNr)}`) }; - } - - const resolved = { ...params }; - const message = resolved.short_message; - - // A string short_message is encoded here, which also settles data_coding and sm_length. - if (typeof message === 'string') { - const dataCoding = resolved.data_coding; - const encoding = typeof dataCoding === 'number' - ? encodingByDataCoding(dataCoding) - : detect(message); - const encoded = encodeMessage(message, encoding); - - resolved.short_message = encoded.buffer; - resolved.sm_length = encoded.buffer.length; - - if (typeof dataCoding !== 'number') { - resolved.data_coding = consts.ENCODING[encoded.encoding]; - } - } + return { + ...params, + data_coding: typeof dataCoding === 'number' ? dataCoding : consts.ENCODING[encoded.encoding], + short_message: encoded.buffer, + sm_length: encoded.buffer.length, + }; +} +function writeParams( + definition: CommandDefinition, + resolved: Record, + cmdName: CommandName, +): Result<{ chunks: Buffer[] }> { const chunks: Buffer[] = []; for (const [name, type] of Object.entries(definition.params ?? {})) { @@ -139,6 +124,12 @@ function buildPdu( chunks.push(chunk); } + return { chunks }; +} + +function writeTlvs(tlvs: Record | undefined): Result<{ chunks: Buffer[] }> { + const chunks: Buffer[] = []; + for (const [name, tlv] of Object.entries(tlvs ?? {})) { const tag = tagIdOf(name, tlv); @@ -169,7 +160,39 @@ function buildPdu( chunks.push(chunk); } - const body = Buffer.concat(chunks); + return { chunks }; +} + +function buildPdu( + cmdName: CommandName, + cmdStatus: ErrorName, + seqNr: number, + params: Record, + tlvs: Record | undefined, +): Result<{ buffer: Buffer }> { + const definition = cmds[cmdName]; + + if (!definition) { + return { err: new Error(`Invalid cmdName: ${JSON.stringify(cmdName)}`) }; + } + + if (!isErrorName(cmdStatus)) { + return { err: new Error(`Invalid cmdStatus: ${JSON.stringify(cmdStatus)}`) }; + } + + if (!Number.isInteger(seqNr) || seqNr < 0 || seqNr > maxSeqNr) { + return { err: new Error(`Invalid seqNr: ${JSON.stringify(seqNr)}`) }; + } + + const written = writeParams(definition, resolveShortMessage(params), cmdName); + + if (written.err) return { err: written.err }; + + const writtenTlvs = writeTlvs(tlvs); + + if (writtenTlvs.err) return { err: writtenTlvs.err }; + + const body = Buffer.concat([...written.chunks, ...writtenTlvs.chunks]); const header = Buffer.alloc(16); header.writeUInt32BE(body.length + 16, 0); @@ -223,22 +246,11 @@ function parseTlvs( return { offset, tlvs }; } -function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolean; pduObj: PduObject }> { - const cmdLength = pdu.readUInt32BE(0); - const cmdId = pdu.readUInt32BE(4); - const cmdName = commandNameById(cmdId); - - if (!cmdName) { - return { err: new Error(`Unknown PDU command id: ${String(cmdId)}`) }; - } - - const cmdStatusId = pdu.readUInt32BE(8); - const seqNr = pdu.readUInt32BE(12); - - if (seqNr > maxSeqNr) { - return { err: new Error(`Invalid seqNr, exceeds ${String(maxSeqNr)}: ${String(seqNr)}`) }; - } - +function readParams( + cmdName: CommandName, + pdu: Buffer, + trailingNull: boolean, +): Result<{ offset: number; params: Record }> { const params: Record = {}; let offset = 16; @@ -255,10 +267,34 @@ function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolea if (name === 'short_message' && trailingNull) offset++; } - const parsed = parseTlvs(pdu, offset, cmdLength); + return { offset, params }; +} + +function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolean; pduObj: PduObject }> { + const cmdLength = pdu.readUInt32BE(0); + const cmdId = pdu.readUInt32BE(4); + const cmdName = commandNameById(cmdId); + + if (!cmdName) { + return { err: new Error(`Unknown PDU command id: ${String(cmdId)}`) }; + } + + const cmdStatusId = pdu.readUInt32BE(8); + const seqNr = pdu.readUInt32BE(12); + + if (seqNr > maxSeqNr) { + return { err: new Error(`Invalid seqNr, exceeds ${String(maxSeqNr)}: ${String(seqNr)}`) }; + } + + const read = readParams(cmdName, pdu, trailingNull); + + if (read.err) return { err: read.err }; + + const parsed = parseTlvs(pdu, read.offset, cmdLength); if (parsed.err) return { err: parsed.err }; + const params = read.params; const message = params.short_message; const esmClass = numberOr(params.esm_class, 0); @@ -282,7 +318,7 @@ function parseOnce(pdu: Buffer, trailingNull: boolean): Result<{ aligned: boolea }; } -export function pduToObj(pdu: Buffer): Result<{ pduObj: PduObject }> { +function checkFraming(pdu: Buffer): VoidResult { if (pdu.length < 16) { return { err: new Error(`PDU is too short, minimum is 16 octets, got ${String(pdu.length)}`) }; } @@ -297,6 +333,14 @@ export function pduToObj(pdu: Buffer): Result<{ pduObj: PduObject }> { return { err: new Error(`cmd_length ${String(cmdLength)} exceeds the ${String(pdu.length)} octets given`) }; } + return {}; +} + +export function pduToObj(pdu: Buffer): Result<{ pduObj: PduObject }> { + const framing = checkFraming(pdu); + + if (framing.err) return { err: framing.err }; + const plain = parseOnce(pdu, false); if (!plain.err && plain.aligned) return { pduObj: plain.pduObj }; @@ -311,6 +355,25 @@ export function pduToObj(pdu: Buffer): Result<{ pduObj: PduObject }> { return { err: plain.err }; } +/** Fields the response shares with the request are echoed back unless the caller overrode them. */ +function echoParams( + respName: CommandName, + pdu: PduObject, + params: Record, +): Record { + const respParams: Record = { ...params }; + + for (const name of Object.keys(cmds[respName]?.params ?? {})) { + const value = pdu.params[name]; + + if (respParams[name] === undefined && value !== undefined) { + respParams[name] = value; + } + } + + return respParams; +} + export function pduReturn( pdu: Buffer | PduObject, status: ErrorName = 'ESME_ROK', @@ -329,16 +392,5 @@ export function pduReturn( return { err: new Error(`"${pdu.cmdName}" has no response command`) }; } - const respParams: Record = { ...params }; - - // Fields the response shares with the request are echoed back unless the caller overrode them. - for (const name of Object.keys(cmds[respName]?.params ?? {})) { - const value = pdu.params[name]; - - if (respParams[name] === undefined && value !== undefined) { - respParams[name] = value; - } - } - - return buildPdu(respName, status, pdu.seqNr, respParams, tlvs); + return buildPdu(respName, status, pdu.seqNr, echoParams(respName, pdu, params), tlvs); } diff --git a/src/server.ts b/src/server.ts index 7b5cf7e..fbbf70e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -4,7 +4,7 @@ import type { Result } from './result.ts'; import type { Server as NetServer, Socket } from 'node:net'; import type { Server as TlsServer, TlsOptions } from 'node:tls'; import { EventEmitter } from 'node:events'; -import { Session, bindCommands } from './session.ts'; +import { Session, bindCommands, defaultSystemId } from './session.ts'; import { createServer as createNetServer } from 'node:net'; import { createServer as createTlsServer } from 'node:tls'; import { defaultInterfaceVersion } from './defs/constants.ts'; @@ -45,7 +45,7 @@ const defaults = { idleTimeout: 40_000, interfaceVersion: defaultInterfaceVersion, port: 2775, - systemId: '', + systemId: defaultSystemId, }; /** A listening SMPP server. Sessions arrive as `session` events; `close()` stops listening. */ diff --git a/src/session.ts b/src/session.ts index 83837e1..2cd377f 100644 --- a/src/session.ts +++ b/src/session.ts @@ -72,6 +72,8 @@ export type SessionOptions = { systemId?: string | undefined; }; +export const defaultSystemId = ''; + const defaults = { maxDelay: 30_000, maxOutstanding: 10, @@ -79,7 +81,7 @@ const defaults = { minDelay: 1000, reassemblyTimeout: 300_000, responseTimeout: 30_000, - systemId: '', + systemId: defaultSystemId, }; export const bindCommands: readonly string[] = [ diff --git a/test/dlr.test.ts b/test/dlr.test.ts index 0bbadc6..886bb67 100644 --- a/test/dlr.test.ts +++ b/test/dlr.test.ts @@ -84,20 +84,23 @@ describe('dlrFromPdu()', () => { assert.equal(dlr.doneDate?.toISOString(), '2025-08-25T14:31:00.000Z'); }); - test('maps every spec status code back to its message state', () => { - for (const [code, expected] of [ - ['DELIVRD', 'DELIVERED'], - ['UNDELIV', 'UNDELIVERABLE'], - ['EXPIRED', 'EXPIRED'], - ['DELETED', 'DELETED'], - ['ACCEPTD', 'ACCEPTED'], - ['REJECTD', 'REJECTED'], - ['ENROUTE', 'ENROUTE'], - ['UNKNOWN', 'UNKNOWN'], - ]) { - const dlr = dlrFromPdu(deliverSm(`id:x stat:${String(code)} err:0`)); + test('maps every spec status code back to its message state and id', () => { + for (const [code, expected, statusId] of [ + ['DELIVRD', 'DELIVERED', 2], + ['UNDELIV', 'UNDELIVERABLE', 5], + ['EXPIRED', 'EXPIRED', 3], + ['DELETED', 'DELETED', 4], + ['ACCEPTD', 'ACCEPTED', 6], + ['REJECTD', 'REJECTED', 8], + ['ENROUTE', 'ENROUTE', 1], + ['UNKNOWN', 'UNKNOWN', 7], + ['delivrd', 'DELIVERED', 2], + ] as const) { + const dlr = dlrFromPdu(deliverSm(`id:x stat:${code} err:0`)); - assert.equal(dlr?.statusMsg, expected); + assert.ok(dlr); + assert.equal(dlr.statusMsg, expected); + assert.equal(dlr.statusId, statusId); } }); diff --git a/test/pdu.test.ts b/test/pdu.test.ts index f99810b..7355d9e 100644 --- a/test/pdu.test.ts +++ b/test/pdu.test.ts @@ -120,6 +120,21 @@ describe('encoding submit_sm', () => { ); }); + test('encodes a string short_message with the given data_coding instead of a detected one', () => { + const pdu = encode({ + cmdName: 'submit_sm', + params: { + data_coding: 0x08, + destination_addr: '46709771337', + short_message: 'hi', + source_addr: '46701113311', + }, + seqNr: 12, + }); + + assert.equal(pdu.subarray(-7).toString('hex'), '08000400680069'); + }); + test('keeps a UDH-carrying short_message as a buffer', () => { const message = Buffer.concat([ Buffer.from('050003010101', 'hex'), diff --git a/todo.md b/todo.md index 5ad63d2..3b3d428 100644 --- a/todo.md +++ b/todo.md @@ -77,8 +77,10 @@ Every defect listed in the AGENTS.md table has a regression test naming the beha - [ ] **`session.ts` is 459 lines.** The one seam left in it is a socket-to-PDU transport, which would move the deliberately public `sock` field out of `Session` or turn it into a getter — a public-surface change, so it waits for a decision. -- [ ] **`buildPdu` and `dlrFromPdu` carry a complexity of 22.** `eslint.config.js` holds them at - that ceiling rather than below the repo-wide 10, so neither can grow but neither shrinks. +- [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports + `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `reconnect-loop`, `pending-requests` + and `send-sms`, so the directory would make that boundary visible. Do it on the next + extraction out of `session.ts`, not as a move of its own. - [ ] **`submit_multi` and the broadcast commands** encode and decode, but nothing exercises them end to end. The interop suite is the natural place. - [ ] **Move to TypeScript 7** once `typescript-eslint` supports it; `renovate.json` pins TypeScript