Refuse a TLV input that is no { tagValue }, and the alternate tag names, keep numbers out of octet TLVs, and pin that a TLV is copied off its chunk

CHANGELOG.md

@@ -66,7 +66,9 @@
   **A TLV input is keyed by its name, or by its decimal id where the table names none, and `tagId`
   is refused.** Write `{ 5142: { tagValue } }` for a vendor tag, not `{ vendor: { tagId: 5142, … } }`.
   A name and a `tagId` could disagree, and `{ message_state: { tagId: 5, … } }` went out as tag 5.
-  A decimal key naming a tag the table knows, `{ 1063: … }`, is refused in favour of the name.
+  A decimal key naming a tag the table knows, `{ 1063: … }`, is refused in favour of the name, and
+  so are `alert_on_msg_delivery` and `failed_broadcast_area_identifier` in favour of
+  `alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back under.
 - `cmds.broadcast_sm_resp.tlvMap` is removed; nothing read it.
 
 ## 0.5.0

MIGRATION.md

@@ -32,7 +32,8 @@ shape is the same, connect, send, listen for delivery reports, with callbacks re
   `consts.ESM_CLASS.STORE_FORWARD` reads `undefined`, which OR-s into an `esm_class` carrying no mode.
 - **A TLV is keyed by its name, or by its decimal id where the table names none**, and `tagId` on
   an input is refused: `{ 5142: { tagValue } }`, not `{ vendor: { tagId: 5142, tagValue } }`.
-  Unknown tags read back under the same decimal key.
+  Write `alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back
+  under, for `alert_on_msg_delivery` and `failed_broadcast_area_identifier`.
 - **The `error` event is `sessionError`**, and `serverError` on the server handle.
 - **`log`** takes any object with `debug`, `error`, `info`, `verbose` and `warn` methods instead of
   a `larvitutils` one, and is silent by default: [README](README.md#logging).

docs/decisions.md

@@ -434,9 +434,7 @@ rule and an index of the titles below.
   `writeParams()` ignores it, so an empty one, an absent one and one the wire cannot carry are the
   same input rather than three. A `data_coding` on a command that declares no such field is honoured
   the other way round, since it is `replace_sm`'s only way to name the alphabet its octets are in.
-  Every entry carrying the payload tag is resolved, by tag id rather
+  Rejected: refusing a string
-  than by record key, since `tagIdOf()` lets a caller name it anything and a spelling that escaped
-  the guard would be a second spelling that disagrees about correctness. Rejected: refusing a string
   `message_payload` outright and demanding
   octets, which contradicts `short_message` on the same PDU. Rejected: guarding every string-valued
   field against `data_coding`, which says nothing about them — a text field on the wire has an

src/defs/tlvs.ts

@@ -110,7 +110,7 @@ type ReadValue<K extends TlvName> = Repeated<K, WireValue<K>>;
 type WriteValue<K extends TlvName> = Specs[K] extends { multiple: true } ? ReadValue<K>
 	: WireValue<K> extends number ? number
 	: WireValue<K> extends string ? number | string
-	: Buffer | number | string;
+	: Buffer | string;
 
 type KnownTlv<K extends TlvName> = { tagId: number; tagName: K; tagValue: ReadValue<K> };
 
@@ -122,22 +122,34 @@ export type Tlvs = { [K in TlvName]?: KnownTlv<K> } & Record<`${number}`, Unknow
 
 export type Tlv = { [K in TlvName]: KnownTlv<K> }[TlvName] | UnknownTlv;
 
-type Alternate = keyof typeof alternates;
-
-type Canonical<K extends Alternate | TlvName> = K extends Alternate ? (typeof alternates)[K]['tag'] : K;
-
 /** Keyed like `Tlvs`, by tag name or by the decimal id of a tag the table does not define. */
-export type TlvInputs = { [K in Alternate | TlvName]?: { tagValue: WriteValue<Canonical<K>> } }
+export type TlvInputs = { [K in TlvName]?: { tagValue: WriteValue<K> } }
 	& Record<`${number}`, { tagValue: Buffer | number | string }>;
 
-function tagIdOf(name: string, input: object): Result<{ tagId: number }> {
+function isTlvInput(input: unknown): input is { tagValue: TlvValue } {
+	if (typeof input !== 'object' || input === null || !('tagValue' in input)) return false;
+
+	const value = input.tagValue;
+
+	if (!Array.isArray(value)) return Buffer.isBuffer(value) || typeof value === 'number' || typeof value === 'string';
+
+	return value.every(one => Buffer.isBuffer(one)) || value.every(one => typeof one === 'number');
+}
+
+function entryOf(name: string, input: unknown): Result<{ tagId: number; tagValue: TlvValue }> {
+	if (!isTlvInput(input)) {
+		return { err: new Error(`TLV "${name}": give it as { tagValue }, holding a Buffer, a number, a string, or an array of Buffers or of numbers`) };
+	}
+
 	if ('tagId' in input) {
 		return { err: new Error(`TLV "${name}": key it by its name, or a tag the table does not define by its decimal id, instead of giving a tagId`) };
 	}
 
-	const named = Object.hasOwn(tlvs, name) ? tlvs[name] : undefined;
+	if (isTlvName(name)) return { tagId: specs[name].id, tagValue: input.tagValue };
 
-	if (named) return { tagId: named.id };
+	const alternate = Object.hasOwn(tlvs, name) ? tlvs[name] : undefined;
+
+	if (alternate) return { err: new Error(`TLV "${name}": key it ${alternate.tag}, the name it reads back under`) };
 
 	if (!/^(0|[1-9]\d*)$/.test(name)) {
 		return { err: new Error(`TLV "${name}": unknown tag name; key a tag the table does not define by its decimal id`) };
@@ -149,20 +161,22 @@ function tagIdOf(name: string, input: object): Result<{ tagId: number }> {
 
 	const known = tlvsById[tagId];
 
-	return known ? { err: new Error(`TLV "${name}": the table names this tag ${known.tag}, key it by that`) } : { tagId };
+	return known
+		? { err: new Error(`TLV "${name}": the table names this tag ${known.tag}, key it by that`) }
+		: { tagId, tagValue: input.tagValue };
 }
 
 /** Each TLV as its four octet header and the value the tag's own wire type writes. */
 export function writeTlvs(inputs: TlvInputs | undefined): Result<{ chunks: Buffer[] }> {
 	const chunks: Buffer[] = [];
 
-	for (const [name, input] of Object.entries(inputs ?? {})) {
+	for (const [name, input] of Object.entries<unknown>(inputs ?? {})) {
-		const tag = tagIdOf(name, input);
+		const tag = entryOf(name, input);
 
 		if (tag.err) return { err: tag.err };
 
 		const definition = tlvsById[tag.tagId];
-		const values = occurrences(input.tagValue, definition?.multiple === true);
+		const values = occurrences(tag.tagValue, definition?.multiple === true);
 
 		if (values.err) return { err: new Error(`TLV "${name}": ${values.err.message}`) };
 

test/pdu.test.ts

@@ -392,7 +392,7 @@ describe('TLVs', () => {
 		assert.deepEqual(unknown.tagValue, Buffer.from('blajfoo', 'ascii'));
 	});
 
-	test('keeps a binary TLV byte for byte through pduToObj and back', () => {
+	test('keeps a binary TLV byte for byte through pduToObj and back, copied off the chunk it arrived in', () => {
 		const payload = Buffer.from('deadbeef00ff', 'hex');
 		const params = {
 			destination_addr: '46709771337',
@@ -400,16 +400,21 @@ describe('TLVs', () => {
 			short_message: 'binary payload follows',
 			source_addr: '46701113311',
 		};
-		const parsed = decode(encode({
+		const pdu = encode({
 			cmdName: 'deliver_sm',
 			params,
 			seqNr: 7,
 			tlvs: { message_payload: { tagValue: payload } },
-		}));
+		});
-		const carried = parsed.tlvs.message_payload;
+		const chunk = Buffer.alloc(64 * 1024);
+
+		pdu.copy(chunk, 100);
+
+		const carried = decode(chunk.subarray(100, 100 + pdu.length)).tlvs.message_payload;
 
 		assert.ok(carried);
 		assert.deepEqual(carried.tagValue, payload);
+		assert.notEqual(carried.tagValue.buffer, chunk.buffer, 'a TLV holds its own octets, not the chunk it arrived in');
 
 		const rebuilt = decode(encode({
 			cmdName: 'deliver_sm',
@@ -442,6 +447,8 @@ describe('TLVs', () => {
 			{ built: objToPdu({ cmdName: 'deliver_sm', params, tlvs: { 65536: { tagValue: 'blajfoo' } } }), reason: /out of range/ },
 			{ built: objToPdu({ cmdName: 'deliver_sm', params, tlvs: { '05142': { tagValue: 'blajfoo' } } }), reason: /decimal id/ },
 			{ built: objToPdu({ cmdName: 'deliver_sm', params, tlvs: { 1063: { tagValue: 2 } } }), reason: /message_state/ },
+			// @ts-expect-error the value goes in a { tagValue } wrapper
+			{ built: objToPdu({ cmdName: 'deliver_sm', params, tlvs: { message_state: 2 } }), reason: /give it as \{ tagValue \}/ },
 		];
 
 		for (const { built: { buffer, err }, reason } of refusals) {
@@ -481,15 +488,17 @@ describe('TLVs', () => {
 		assert.deepEqual(pduObj.tlvs.callback_num_pres_ind?.tagValue, [1]);
 	});
 
-	test('reads the failed areas of a broadcast_sm_resp as broadcast_area_identifier', () => {
+	test('writes and 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({
+		const params = { message_id: '01a0d051-b588-76eb-a5c5-a8cb8b854e68' };
-			cmdName: 'broadcast_sm_resp',
+		const pduObj = decode(encode({ cmdName: 'broadcast_sm_resp', params, tlvs: { broadcast_area_identifier: { tagValue: areas } } }));
-			params: { message_id: '01a0d051-b588-76eb-a5c5-a8cb8b854e68' },
-			tlvs: { failed_broadcast_area_identifier: { tagValue: areas } },
-		}));
 
 		assert.deepEqual(pduObj.tlvs.broadcast_area_identifier?.tagValue, areas);
+
+		// @ts-expect-error the alternate spelling is read back under the name, so only the name is written
+		const { err } = objToPdu({ cmdName: 'broadcast_sm_resp', params, tlvs: { failed_broadcast_area_identifier: { tagValue: areas } } });
+
+		assert.match(err?.message ?? '', /key it broadcast_area_identifier/);
 	});
 
 	test('refuses a repeatable TLV given one value, and a lone TLV given several', () => {
@@ -544,6 +553,8 @@ describe('TLVs', () => {
 
 		assert.deepEqual([id, state, unknown], ['0199d8a4-5e2c-7b3f-9a61-c4e07f2d8b15', 2, Buffer.from('01', 'hex')]);
 
+		// @ts-expect-error an octet field takes no number, which would go out as its digits
+		assert.ok(objToPdu({ cmdName: 'deliver_sm', params: {}, tlvs: { network_error_code: { tagValue: 5 } } }));
 		// @ts-expect-error message_state is an integer
 		assert.ok(objToPdu({ cmdName: 'deliver_sm', params: {}, tlvs: { message_state: { tagValue: 'ENROUTE' } } }).err);
 	});