Address the CodeRabbit review: inbound SMS, binary payloads and untrusted input

test/dlr.test.ts

@@ -104,6 +104,14 @@ describe('dlrFromPdu()', () => {
 		}
 	});
 
+	test('leaves an impossible receipt date undefined rather than rolling it over', () => {
+		const rolled = dlrFromPdu(deliverSm('id:x stat:DELIVRD done date:9902310000'));
+
+		assert.ok(rolled);
+		assert.equal(rolled.doneDate, undefined);
+		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/encodings.test.ts

@@ -50,6 +50,12 @@ describe('LATIN1', () => {
 			assert.equal(encodings.LATIN1.decode(Buffer.from(bytes)), str);
 		}
 	});
+
+	test('carries every octet through unchanged, which is what makes it the binary codec', () => {
+		const every = Buffer.from(Array.from({ length: 256 }, (_, byte) => byte));
+
+		assert.deepEqual(encodings.LATIN1.encode(encodings.LATIN1.decode(every)), every);
+	});
 });
 
 describe('UCS2', () => {
@@ -78,6 +84,15 @@ describe('UCS2', () => {
 
 		assert.deepEqual(buffer, Buffer.from([0x00, 0x20]));
 	});
+
+	// swap16() throws ERR_INVALID_BUFFER_SIZE on an odd octet count, and sm_length is peer-controlled.
+	test('drops an incomplete trailing octet instead of throwing', () => {
+		const odd = Buffer.from([0x00, 0x41, 0x00, 0x42, 0x00]);
+
+		assert.equal(encodings.UCS2.decode(odd), 'AB');
+		assert.equal(encodings.UCS2.decode(Buffer.from([0x41])), '');
+		assert.deepEqual(odd, Buffer.from([0x00, 0x41, 0x00, 0x42, 0x00]));
+	});
 });
 
 describe('detect()', () => {
@@ -111,6 +126,12 @@ describe('encodingByDataCoding()', () => {
 		assert.equal(encodingByDataCoding(0xF0), 'ASCII');
 	});
 
+	test('resolves the 8-bit binary codings to the codec that keeps every octet', () => {
+		for (const dataCoding of [0x02, 0x04, 0x14, 0xF4, 0xF7]) {
+			assert.equal(encodingByDataCoding(dataCoding), 'LATIN1');
+		}
+	});
+
 	test('falls back to ASCII for alphabets it has no codec for', () => {
 		assert.equal(encodingByDataCoding(0x05), 'ASCII');
 		assert.equal(encodingByDataCoding(0x0E), 'ASCII');

test/interop.test.ts

@@ -204,7 +204,7 @@ describe('the reference encoder against our parser', () => {
 });
 
 describe('a live session against the reference implementation', () => {
-	test('our client binds to a reference server and delivers an SMS', async () => {
+	test('our client binds to a reference server and delivers an SMS', async t => {
 		const received: { from: string; message: string }[] = [];
 		const refServer = reference.createServer({}, (session: ReferenceSession) => {
 			session.on('bind_transceiver', pdu => {
@@ -226,11 +226,14 @@ describe('a live session against the reference implementation', () => {
 			});
 		});
 
+		t.after(() => new Promise<void>(resolve => { refServer.close(() => { resolve(); }); }));
 		await new Promise<void>(resolve => { refServer.listen(0, () => { resolve(); }); });
 
 		const port = refServer.address()?.port ?? 0;
 		const { err, session } = await client({ port });
 
+		t.after(() => { session?.close(); });
+
 		assert.equal(err, undefined);
 		assert.ok(session);
 
@@ -243,14 +246,13 @@ describe('a live session against the reference implementation', () => {
 		assert.equal(sent.err, undefined);
 		assert.deepEqual(sent.smsIds, ['ref-id']);
 		assert.deepEqual(received, [{ from: 'MyBrand', message: 'interop check' }]);
-
-		session.close();
-		await new Promise<void>(resolve => { refServer.close(() => { resolve(); }); });
 	});
 
-	test('a reference client binds to our server and delivers an SMS', async () => {
+	test('a reference client binds to our server and delivers an SMS', async t => {
 		const { err: serverErr, server: smpp } = await server({ port: 0 });
 
+		t.after(async () => { await smpp?.close(); });
+
 		assert.equal(serverErr, undefined);
 		assert.ok(smpp);
 
@@ -262,6 +264,8 @@ describe('a live session against the reference implementation', () => {
 			url: `smpp://localhost:${String(smpp.port)}`,
 		});
 
+		t.after(() => { refSession.close(); });
+
 		await new Promise<void>(resolve => {
 			refSession.bind_transceiver({ password: 'bar', system_id: 'foo' }, () => { resolve(); });
 		});
@@ -277,8 +281,5 @@ describe('a live session against the reference implementation', () => {
 		assert.equal(sms.from, '46701113311');
 		assert.equal(sms.message, 'from the reference client');
 		await sms.sendResp();
-
-		refSession.close();
-		await smpp.close();
 	});
 });

test/message.test.ts

@@ -110,6 +110,16 @@ describe('encodeMessage() and decodeMessage()', () => {
 		assert.equal(decodeMessage(buffer, 0x08).message, 'hej 一');
 	});
 
+	test('keeps a binary payload octet for octet instead of running it through GSM 03.38', () => {
+		const payload = Buffer.from([0x00, 0x1B, 0x60, 0x80, 0xFF]);
+
+		assert.deepEqual(Buffer.from(decodeMessage(payload, 0x04).message, 'latin1'), payload);
+	});
+
+	test('decodes the whole characters of a UCS2 payload cut in half by sm_length', () => {
+		assert.equal(decodeMessage(Buffer.from([0x00, 0x68, 0x00, 0x65, 0x00]), 0x08).message, 'he');
+	});
+
 	test('strips a UDH when the esm_class says one is present', () => {
 		const withUdh = Buffer.concat([
 			Buffer.from([0x05, 0x00, 0x03, 0x01, 0x02, 0x01]),

test/session-extras.test.ts

@@ -28,8 +28,18 @@ async function startServer(options: Parameters<typeof server>[0] = {}): Promise<
 	return smpp;
 }
 
+/** An event that never fires would otherwise block until the CI job limit, asserting nothing. */
 function once<T>(register: (resolve: (value: T) => void) => void): Promise<T> {
-	return new Promise<T>(resolve => { register(resolve); });
+	return new Promise<T>((resolve, reject) => {
+		const timer = setTimeout(() => {
+			reject(new Error('waited 5000 ms for an event that never fired'));
+		}, 5000);
+
+		register(value => {
+			clearTimeout(timer);
+			resolve(value);
+		});
+	});
 }
 
 describe('merged delivery reports', () => {
@@ -345,6 +355,16 @@ describe('reassembly bounds', () => {
 		assert.equal(reassembler.size, 0);
 	});
 
+	// The UDH is peer-controlled, and the default authenticate() accepts every peer.
+	test('refuses a segment whose concatenation metadata cannot be honoured', () => {
+		const reassembler = new Reassembler({ log: silentLog, max: 10, now: () => 0, timeout: 60_000 });
+
+		assert.equal(collect(reassembler, 1, 1, 0), undefined);
+		assert.equal(collect(reassembler, 2, 0, 3), undefined);
+		assert.equal(collect(reassembler, 3, 4, 3), undefined);
+		assert.equal(reassembler.size, 0);
+	});
+
 	// 0.4.0 held incomplete groups without limit and swept them only when other traffic arrived.
 	test('drops the oldest incomplete message once the cap is reached', () => {
 		const reassembler = new Reassembler({ log: silentLog, max: 2, now: () => 0, timeout: 60_000 });

test/session.test.ts

@@ -5,15 +5,19 @@ import type { Dlr } from '../src/dlr.ts';
 import type { PduObject, PduObjectInput } from '../src/pdu.ts';
 import type { Sms } from '../src/sms.ts';
 import type { SmppServer } from '../src/server.ts';
+import type { TestContext } from 'node:test';
+import type { VoidResult } from '../src/result.ts';
 import { DlrMerger } from '../src/dlr-merger.ts';
 import { PduFramer } from '../src/pdu-framer.ts';
 import { ReconnectLoop } from '../src/reconnect-loop.ts';
 import { Session, bindCommands } from '../src/session.ts';
 import { client } from '../src/client.ts';
+import { consts } from '../src/defs/constants.ts';
 import { isCommand, objToPdu, pduReturn, pduToObj } from '../src/pdu.ts';
 import { paramText } from '../src/defs/types.ts';
 import { server } from '../src/server.ts';
 import { silentLog } from '../src/log.ts';
+import { splitMessage } from '../src/message.ts';
 
 async function startServer(options: Parameters<typeof server>[0] = {}): Promise<SmppServer> {
 	const { err, server: smpp } = await server({ ...options, port: 0 });
@@ -391,6 +395,33 @@ describe('bind', () => {
 		peer.close();
 		await smpp.close();
 	});
+
+	test('records the version the SMSC declared in its bind response', async t => {
+		const smpp = await startServer({ interfaceVersion: 0x50 });
+
+		t.after(() => smpp.close());
+
+		const { session } = await connect(smpp);
+
+		assert.ok(session);
+		t.after(() => { session.close(); });
+		assert.equal(session.peerInterfaceVersion, 0x50);
+		assert.ok(session.acceptsOptionalParams());
+	});
+
+	// The spec: an absent sc_interface_version means the SMSC supports no optional parameters.
+	test('takes an SMSC that declares no version as older than 3.4', async t => {
+		const peer = await bindOnlyPeer();
+
+		t.after(() => peer.close());
+
+		const { session } = await client({ port: peer.port });
+
+		assert.ok(session);
+		t.after(() => { session.close(); });
+		assert.equal(session.peerInterfaceVersion, 0x00);
+		assert.equal(session.acceptsOptionalParams(), false);
+	});
 });
 
 describe('sending', () => {
@@ -517,6 +548,84 @@ describe('sending', () => {
 	});
 });
 
+describe('receiving', () => {
+	async function inbound(t: TestContext): Promise<{ peer: Session; session: Session }> {
+		const smpp = await startServer();
+
+		t.after(() => smpp.close());
+
+		const bound = once<Session>(resolve => { smpp.on('session', resolve); });
+		const { session } = await connect(smpp);
+
+		assert.ok(session);
+		t.after(() => { session.close(); });
+
+		return { peer: await bound, session };
+	}
+
+	test('hands a client a deliver_sm that is not a delivery receipt', async t => {
+		const { peer, session } = await inbound(t);
+		const incoming = once<Sms>(resolve => { session.on('sms', resolve); });
+		const delivered = peer.send({
+			cmdName: 'deliver_sm',
+			params: {
+				destination_addr: '46709771337',
+				short_message: 'inbound hello',
+				source_addr: '46701113311',
+			},
+		});
+		const sms = await raceWithin(2000, incoming);
+
+		assert.ok(sms, 'a deliver_sm that carries no receipt is an inbound SMS');
+		assert.equal(sms.from, '46701113311');
+		assert.equal(sms.to, '46709771337');
+		assert.equal(sms.message, 'inbound hello');
+
+		await sms.sendResp({ smsId: 'inbound-id' });
+
+		const answered = await delivered;
+
+		assert.ok(answered.pduObj);
+		assert.equal(answered.pduObj.cmdName, 'deliver_sm_resp');
+		assert.equal(answered.pduObj.params.message_id, 'inbound-id');
+	});
+
+	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);
+		const incoming = once<Sms>(resolve => { session.on('sms', resolve); });
+		const segments = splitMessage(message, { reference: 42 });
+
+		assert.equal(segments.length, 2);
+
+		const delivered = Promise.all(segments.map(segment => peer.send({
+			cmdName: 'deliver_sm',
+			params: {
+				destination_addr: '46709771337',
+				esm_class: consts.ESM_CLASS.UDH_INDICATOR,
+				short_message: segment,
+				source_addr: '46701113311',
+			},
+		})));
+		const sms = await raceWithin(2000, incoming);
+
+		assert.ok(sms, 'both segments should reassemble into one message');
+		assert.equal(sms.message, message);
+		assert.equal(sms.pduObjs.length, 2);
+
+		await sms.sendResp({ smsId: 'inbound-long' });
+
+		const ids: string[] = [];
+
+		for (const answered of await delivered) {
+			assert.ok(answered.pduObj);
+			ids.push(paramText(answered.pduObj.params.message_id));
+		}
+
+		assert.deepEqual(ids, ['inbound-long-1', 'inbound-long-2']);
+	});
+});
+
 describe('delivery reports', () => {
 	test('reaches the sender as a dlr event', async () => {
 		const smpp = await startServer();
@@ -860,6 +969,75 @@ describe('robustness', () => {
 		assert.ok(await closed);
 		await smpp.close();
 	});
+
+	// An aborted send that still reaches the SMSC bills a message the caller believes never went out.
+	test('puts nothing on the wire for a signal that is already aborted', async t => {
+		const smpp = await startServer();
+
+		t.after(() => smpp.close());
+
+		const bound = once<Session>(resolve => { smpp.on('session', resolve); });
+		const { session } = await connect(smpp);
+
+		assert.ok(session);
+		t.after(() => { session.close(); });
+
+		const peer = await bound;
+		const controller = new AbortController();
+		const seen: string[] = [];
+
+		peer.on('incomingPduObj', pduObj => { seen.push(pduObj.cmdName); });
+		controller.abort();
+
+		const sent = await session.sendSms({
+			from: '46701113311',
+			message: 'must never reach the peer',
+			to: '46709771337',
+		}, { signal: controller.signal });
+
+		assert.ok(sent.err instanceof Error);
+		await delay(50);
+		assert.deepEqual(seen, []);
+	});
+
+	// A socket the loop opened and never handed over is one leaked per retry, forever.
+	test('leaves no socket open when coming back up fails', async () => {
+		const opened: net.Socket[] = [];
+
+		function onConnected(): Promise<VoidResult> {
+			if (opened.length === 1) return Promise.resolve({ err: new Error('bind refused') });
+
+			throw new Error('bind exploded');
+		}
+
+		const loop = new ReconnectLoop({
+			connect: () => {
+				const sock = new net.Socket();
+
+				opened.push(sock);
+
+				return Promise.resolve({ sock });
+			},
+			log: silentLog,
+			maxDelay: 10,
+			minDelay: 1,
+			onConnected,
+		});
+
+		loop.schedule();
+
+		const destroyed = await waitFor(() => opened.length >= 2
+			&& opened[0]?.destroyed === true
+			&& opened[1]?.destroyed === true);
+
+		loop.stop();
+
+		for (const sock of opened) {
+			sock.destroy();
+		}
+
+		assert.ok(destroyed, 'a failed setup should leave no socket open');
+	});
 });
 
 describe('application hooks that throw', () => {

test/types.test.ts

@@ -1,6 +1,7 @@
 import assert from 'node:assert/strict';
 import test, { describe } from 'node:test';
 import type { DestAddress, UnsuccessSme } from '../src/defs/types.ts';
+import { tlvs } from '../src/defs/tlvs.ts';
 import { types } from '../src/defs/types.ts';
 
 describe('integers', () => {
@@ -102,6 +103,42 @@ describe('cstring (C-Octet String)', () => {
 	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);
 	});
+
+	test('refuses one that starts past the end instead of inventing an empty value', () => {
+		assert.ok(types.cstring.read(encoded, encoded.length + 1).err instanceof Error);
+		assert.ok(types.cstring.read(encoded, -1).err instanceof Error);
+
+		// At the end exactly the field is absent, not corrupt: peers truncate a NULL-only body.
+		assert.deepEqual(types.cstring.read(encoded, encoded.length), { bytesRead: 1, value: '' });
+	});
+});
+
+describe('integer TLVs', () => {
+	const encoded = Buffer.from([0x00, 0x00, 0x01, 0x02]);
+
+	// The TLV header's length is what the parser skips, so it is what the value must be read at.
+	test('read the width the TLV header declares', () => {
+		assert.deepEqual(types.tlv.int8.read(encoded, 0, 4), { bytesRead: 4, value: 0x00000102 });
+		assert.deepEqual(types.tlv.int16.read(encoded, 2, 2), { bytesRead: 2, value: 0x0102 });
+		assert.deepEqual(types.tlv.int32.read(encoded, 3, 1), { bytesRead: 1, value: 0x02 });
+		assert.deepEqual(types.tlv.int16.read(encoded, 2), { bytesRead: 2, value: 0x0102 });
+	});
+
+	test('refuse a length no integer field can have', () => {
+		assert.ok(types.tlv.int8.read(encoded, 0, 0).err instanceof Error);
+		assert.ok(types.tlv.int16.read(encoded, 0, 3).err instanceof Error);
+		assert.ok(types.tlv.int32.read(encoded, 0, 8).err instanceof Error);
+	});
+
+	test('stay bounds-checked at the declared width', () => {
+		assert.ok(types.tlv.int8.read(Buffer.alloc(2), 0, 4).err instanceof Error);
+	});
+
+	// SMPP 3.4 5.3.2.7-8: the two telematics ids are deliberately different widths.
+	test('are the width the spec gives each tag', () => {
+		assert.equal(tlvs.source_telematics_id.type, types.tlv.int8);
+		assert.equal(tlvs.dest_telematics_id.type, types.tlv.int16);
+	});
 });
 
 describe('buffer', () => {