Refuse a PDU whose optional parameters do not end on command_length (#87)

* Regression tests for a truncated TLV tail refused rather than accepted

* Refuse a PDU whose optional parameters do not end on command_length

* Assert the bare TLV header refusal against jsmpp instead of recording it as a defect

* Note the truncated TLV tail defect as fixed in the java-client findings

* Derive the padding position, share the bare TLV fixture and trim the decision record

* Regression tests for a PDU whose trailing C-Octet String a peer left out

* An absent trailing C-Octet String consumes no octet, so a bodyless PDU still parses

* Bound the TLV loop by the buffer it was given rather than a second spelling of its length

* Answer the stability review's questions in the record and pin the array contract

test/pdu.test.ts

@@ -87,6 +87,46 @@ describe('parsing real PDUs', () => {
 		assert.equal(pduObj.params.short_message, 'test');
 		assert.equal(pduObj.cmdLength, 59);
 	});
+
+	// SMPP 3.4 4.6.2 leaves deliver_sm_resp's message_id unused and peers send the response with no
+	// body at all, so the last C-Octet String of a PDU is one a peer may leave out entirely.
+	test('reads a PDU whose trailing C-Octet String was left out altogether', () => {
+		const bind = encode({
+			cmdName: 'bind_transceiver',
+			params: { password: 'secret08', system_id: 'SMPP3TEST' },
+			seqNr: 1,
+		});
+		const withoutRange = bind.subarray(0, bind.length - 1);
+
+		withoutRange.writeUInt32BE(withoutRange.length, 0);
+
+		const bound = decode(withoutRange);
+
+		assert.equal(bound.params.system_id, 'SMPP3TEST');
+		assert.equal(bound.params.address_range, '');
+		assert.equal(decode(Buffer.from('00000010800000050000000000000007', 'hex')).params.message_id, '');
+		assert.equal(decode(Buffer.from('00000010800000040000000000000007', 'hex')).params.message_id, '');
+	});
+
+	// The padded read skips one NULL octet and no more: the body's own last octet is 0x00 here, and
+	// the optional parameters behind the padding still have to be read.
+	test('reads past that NULL octet to the optional parameters behind it', () => {
+		const padded = Buffer.concat([
+			encode({
+				cmdName: 'deliver_sm',
+				params: { destination_addr: '46709771337', short_message: 'hej 一', source_addr: '46701113311' },
+				seqNr: 41,
+			}),
+			Buffer.from('000427000102', 'hex'),
+		]);
+
+		padded.writeUInt32BE(padded.length, 0);
+
+		const pduObj = decode(padded);
+
+		assert.equal(pduObj.params.short_message, 'hej 一');
+		assert.equal(pduObj.tlvs.message_state?.tagValue, 2);
+	});
 });
 
 describe('encoding submit_sm', () => {
@@ -532,6 +572,37 @@ describe('malformed input', () => {
 		});
 	});
 
+	// interop-tests/findings/05-java-clients.md's reproducer, octet for octet.
+	test('refuses a bare TLV header the same way it refuses a truncated value', () => {
+		const refused = pduToObj(Buffer.from(
+			'000000460000000500000000000000630000007261772d66726f6d0000007261772d746f0000000000000000000013'
+			+ '7472756e636174656420746c762070726f6265001d00c8',
+			'hex',
+		)).err;
+
+		assert.ok(refused instanceof PduRefusedError);
+		assert.equal(refused.reason, 'tlvs');
+		assert.equal(refused.header.seqNr, 99);
+		assert.deepEqual(refusalAnswer(refused), {
+			cmdName: 'deliver_sm_resp',
+			cmdStatus: 'ESME_RINVTLVSTREAM',
+		});
+	});
+
+	test('refuses octets left over after the optional parameters', () => {
+		const slack = Buffer.concat([
+			encode({ cmdName: 'deliver_sm', params: { short_message: 'hello' }, seqNr: 13 }),
+			Buffer.from('4142', 'hex'),
+		]);
+
+		slack.writeUInt32BE(slack.length, 0);
+
+		const refused = pduToObj(slack).err;
+
+		assert.ok(refused instanceof PduRefusedError);
+		assert.equal(refused.reason, 'tlvs');
+	});
+
 	test('answers a command with no response of its own with generic_nack', () => {
 		const outbind = encode({ cmdName: 'outbind', params: { system_id: 'smsc' }, seqNr: 12 });
 		// Both C-Octet Strings lose their terminator, so system_id runs off the end of the PDU.

test/raw-pdus.ts

@@ -38,3 +38,12 @@ export function truncatedTlv(input: PduObjectInput): Buffer {
 
 	return appended;
 }
+
+/** The same, ending in a bare TLV header: a tag, a declared length, and no value octets at all. */
+export function bareTlvHeader(input: PduObjectInput): Buffer {
+	const appended = Buffer.concat([pduBytes(input), Buffer.from('001d00c8', 'hex')]);
+
+	appended.writeUInt32BE(appended.length, 0);
+
+	return appended;
+}

test/session-error.test.ts

@@ -3,8 +3,8 @@ import test, { describe } from 'node:test';
 import type { PduHeader, Session } from '../src/index.ts';
 import type { TestContext } from 'node:test';
 import { PduRefusedError, client, server } from '../src/index.ts';
+import { bareTlvHeader, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts';
 import { closeAfter } from './teardown.ts';
-import { shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts';
 
 const receipt = {
 	destination_addr: '46709771337',
@@ -85,11 +85,12 @@ describe('telling a refused PDU from a failed session', () => {
 		peer.sock.write(withUnknownCmdId({ cmdName: 'enquire_link', seqNr: 9 }));
 		peer.sock.write(shortened({ cmdName: 'deliver_sm', params: receipt, seqNr: 6 }, 3));
 		peer.sock.write(truncatedTlv({ cmdName: 'deliver_sm', params: receipt, seqNr: 7 }));
+		peer.sock.write(bareTlvHeader({ cmdName: 'deliver_sm', params: receipt, seqNr: 8 }));
 
-		assert.ok(await waitFor(() => seen.length === 3), 'one sessionError per refused PDU');
+		assert.ok(await waitFor(() => seen.length === 4), 'one sessionError per refused PDU');
 		assert.deepEqual(
 			seen.map(err => (err instanceof PduRefusedError ? err.reason : err.message)),
-			['command', 'body', 'tlvs'],
+			['command', 'body', 'tlvs', 'tlvs'],
 		);
 	});
 

test/session.test.ts

@@ -19,7 +19,7 @@ import { PduRefusedError } from '../src/pdu-refusal.ts';
 import { isCommand, objToPdu, pduReturn, pduToObj } from '../src/pdu.ts';
 import { paramText } from '../src/defs/types.ts';
 import { server } from '../src/server.ts';
-import { pduBytes, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts';
+import { bareTlvHeader, pduBytes, shortened, truncatedTlv, withUnknownCmdId } from './raw-pdus.ts';
 import { silentLog } from '../src/log.ts';
 import { splitMessage } from '../src/message.ts';
 
@@ -1568,6 +1568,24 @@ describe('a PDU the codec cannot read', () => {
 		assert.equal((await answerTo(peer)).cmdName, 'enquire_link_resp');
 	});
 
+	test('answers a deliver_sm ending in a bare TLV header with ESME_RINVTLVSTREAM', async t => {
+		const { peer, session } = await bound(t);
+		let reports = 0;
+
+		session.on('dlr', () => { reports++; });
+		peer.writeRaw(bareTlvHeader({ cmdName: 'deliver_sm', params: receipt, seqNr: 55 }));
+
+		const answered = await answerTo(peer);
+
+		assert.equal(answered.cmdName, 'deliver_sm_resp');
+		assert.equal(answered.cmdStatus, 'ESME_RINVTLVSTREAM');
+		assert.equal(answered.seqNr, 55);
+		assert.equal(reports, 0, 'a PDU whose optional parameters were never read is no report');
+
+		peer.writeRaw(pduBytes({ cmdName: 'enquire_link', seqNr: 79 }));
+		assert.equal((await answerTo(peer)).cmdName, 'enquire_link_resp');
+	});
+
 	test('answers a deliver_sm whose body is shorter than it declares with ESME_RINVCMDLEN', async t => {
 		const { peer } = await bound(t);
 

test/types.test.ts

@@ -108,8 +108,9 @@ describe('cstring (C-Octet String)', () => {
 		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: '' });
+		// At the end exactly the field is absent, not corrupt, and consumes no octet: counting one
+		// puts every later offset past the declared end.
+		assert.deepEqual(types.cstring.read(encoded, encoded.length), { bytesRead: 0, value: '' });
 	});
 });
 
@@ -187,6 +188,12 @@ describe('dest_address_array', () => {
 			bytesRead: 13,
 			value: expected,
 		});
+
+		// A structure that ran out counts the octets that were there, never the terminator that was not.
+		assert.deepEqual(types.dest_address_array.read(Buffer.from([0x01, 0x01, 0x00, 0x00]), 0), {
+			bytesRead: 4,
+			value: [{ dest_addr_npi: 0, dest_addr_ton: 0, destination_addr: '' }],
+		});
 	});
 
 	test('sizes every dest_address structure', () => {