Cover the guarded address writers, and cut what the code already says
Mirror / push (push) Successful in 5s
Test / lint (pull_request) Successful in 22s
Test / test (18) (pull_request) Successful in 29s
Test / test (20) (pull_request) Successful in 30s
Test / test (22) (pull_request) Successful in 37s
Test / test (24) (pull_request) Successful in 29s
Test / test (26) (pull_request) Successful in 30s

This commit is contained in:
2026-09-21 08:08:42 +02:00
parent c1e0407942
commit f870b7530f
8 changed files with 68 additions and 45 deletions
+2 -1
View File
@@ -10,7 +10,8 @@
`source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote `source_addr` of `Kaffeé` previously reached the application as `Kaffei`, because the codec wrote
the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`, the octet and then masked bit 7 reading it back; `destination_addr`, `system_id`, `message_id`,
`service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF` `service_type` and the C-Octet String TLVs were affected the same way. A character past `U+00FF`
in one of those fields is now refused, where it used to go out as its low octet. in one of those fields is now refused, where it used to go out as its low octet — which for `一`,
` ` and most emoji is `0x00`, ending the field there.
- A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is - A `U+0000` inside a C-Octet String — `source_addr`, `message_id`, `system_id` and the rest — is
refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every refused. The peer reads such a field to its first NULL, so one sent inside the value shifted every
mandatory field behind it while `command_length` still counted the whole string. An Octet String mandatory field behind it while `command_length` still counted the whole string. An Octet String
+3 -3
View File
@@ -288,9 +288,9 @@ await session.sendSms({
``` ```
**Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address **Addresses.** `sourceAddrTon` and `destinationAddrTon` default to 5 for an alphanumeric address
and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` goes out and and 1 for a numeric one; the NPI fields default to 0. An address is latin1, so `Kaffeé` reaches the
comes back as its own octets; a character past `U+00FF`, or a `U+0000` that would end the field peer as its own octets; a character past `U+00FF`, or a `U+0000`, is refused rather than sent
early, is refused rather than sent truncated. truncated.
**Encoding.** **Encoding.**
+16 -23
View File
@@ -479,29 +479,22 @@ rule and an index of the titles below.
change reads exactly as it did. change reads exactly as it did.
- **Every text field on the wire is latin1, and what the field cannot carry is refused rather than - **Every text field on the wire is latin1, and what the field cannot carry is refused rather than
truncated.** Maintainer's call, 2026-09-21, from the 0.6.0 correctness list: the codec read these truncated.** Maintainer's call, 2026-09-21, the refusals from the security and stability passes on
fields with [#16](https://gitea.larvit.se/larvit/smpp-js/pulls/16). Goal 3 settles the alphabet: 3.4 calls
`toString('ascii')`, which masks bit 7, and wrote them with `write(text, 'ascii')`, which does not, these fields ASCII, and an operator routing an alphanumeric sender through the upper half is
so an inbound `source_addr` of `Kaffeé` reached the application as `Kaffei` and traffic to keep, so the read is latin1 and the write is named latin1 to match — which is what
`objToPdu(pduToObj(x))` was idempotent for none of `source_addr`, `destination_addr`, `system_id`, makes the round trip idempotent, and is already the octets Node put on the wire, so no peer sees a
`message_id`, `service_type` or the cstring TLVs. Goal 3 settles the read: 3.4 calls these fields change. Goal 2 settles the refusals, both of them a `size()` that would have agreed with a
ASCII, and an operator routing an alphanumeric sender through the upper half is traffic to keep. `write()` that put something else on the wire: a character past `U+00FF` written as its low octet,
The write is named latin1 to match, which is what makes the round trip idempotent and is already and a caller's own `U+0000`, which the peer reads as the end of the field, shifting every mandatory
the octets Node put on the wire, so no peer sees a change. `wantText()` is the single place that field behind it under a `command_length` that counted the whole string. `wantText()` and
says so — which is why the `dest_address` and `unsuccess_sme` structures write their embedded `wantCstringText()` are the only two places that decide it, which is why the `dest_address` and
addresses through `cstring.write()` rather than reaching past it — and a character past `U+00FF` `unsuccess_sme` structures write their embedded addresses through `cstring.write()` rather than
is refused there rather than truncated to its low octet: a `size()` that agreed with a `write()` reaching past it into `writeCstring()`. Rejected: reading latin1 and leaving the write spelled
that dropped half a character is a wrong answer about what went out (goal 2), and it is how an ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the upper half on send
alphabet that cannot carry a message body is already handled. Rejected: reading latin1 and leaving to stay strict to 3.4's ASCII, which would drop exactly the traffic the read keeps. Rejected:
the write spelled ASCII, which leaves two halves agreeing only by accident. Rejected: refusing the refusing `U+0000` in every text field, which would buy one spelling by taking a legitimate octet
upper half on send to stay strict to 3.4's ASCII, which would drop exactly the traffic the read away from the length-prefixed Octet String, whose length octet is what ends it.
keeps. The security pass over this chunk found the field's other end open the same way: a caller's
own `U+0000` was written verbatim, and since the peer reads a C-Octet String to its first NULL,
every mandatory field behind it shifted under a `command_length` that had counted the whole string
— so an application forwarding a customer's sender id could have a PDU rewritten from inside one.
`wantCstringText()` refuses that where `wantText()` refuses the upper end. Rejected: refusing NULL
in every text field, which would buy one spelling by taking a legitimate octet away from the
length-prefixed Octet String, whose length octet is what ends it.
## The session's life ## The session's life
+2 -5
View File
@@ -84,7 +84,7 @@ function pastLatin1(text: string): { err: Error } | undefined {
if (index === -1) return undefined; if (index === -1) return undefined;
const code = text.charCodeAt(index).toString(16).toUpperCase().padStart(4, '0'); const code = (text.codePointAt(index) ?? 0).toString(16).toUpperCase().padStart(4, '0');
return { return {
err: new Error( err: new Error(
@@ -103,7 +103,6 @@ function wantText(value: ParamValue): Result<{ text: string }> {
return pastLatin1(text) ?? { text }; return pastLatin1(text) ?? { text };
} }
/** A C-Octet String ends at its first NULL, so one inside the value truncates the field on the peer. */
function wantCstringText(value: ParamValue): Result<{ text: string }> { function wantCstringText(value: ParamValue): Result<{ text: string }> {
const { err, text } = wantText(value); const { err, text } = wantText(value);
@@ -114,9 +113,7 @@ function wantCstringText(value: ParamValue): Result<{ text: string }> {
if (index === -1) return { text }; if (index === -1) return { text };
return { return {
err: new Error( err: new Error(`U+0000 at index ${String(index)} would end the C-Octet String there`),
`U+0000 at index ${String(index)} would end the C-Octet String there, ${String(text.length - index - 1)} characters early`,
),
}; };
} }
+2 -5
View File
@@ -128,7 +128,6 @@ describe('parsing real PDUs', () => {
assert.equal(pduObj.tlvs.message_state?.tagValue, 2); assert.equal(pduObj.tlvs.message_state?.tagValue, 2);
}); });
// 'ascii' masks bit 7 on the way in, so this address used to come back Kaffei.
test('keeps an alphanumeric sender whole through the wire and back', () => { test('keeps an alphanumeric sender whole through the wire and back', () => {
const pdu = encode({ const pdu = encode({
cmdName: 'deliver_sm', cmdName: 'deliver_sm',
@@ -143,12 +142,9 @@ describe('parsing real PDUs', () => {
assert.ok(pdu.includes(Buffer.from('Kaffeé', 'latin1'))); assert.ok(pdu.includes(Buffer.from('Kaffeé', 'latin1')));
assert.equal(decode(pdu).params.source_addr, 'Kaffeé'); assert.equal(decode(pdu).params.source_addr, 'Kaffeé');
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error);
}); });
// The peer reads source_addr to the first NULL and every mandatory field behind it shifts, so an test('refuses an address the field cannot carry rather than truncating it', () => {
// application forwarding a customer's sender id could have a PDU rewritten under it.
test('refuses an address carrying its own terminator', () => {
const smuggled = objToPdu({ const smuggled = objToPdu({
cmdName: 'submit_sm', cmdName: 'submit_sm',
params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' }, params: { destination_addr: '46709771337', source_addr: '46701113311\u0000EVIL' },
@@ -156,6 +152,7 @@ describe('parsing real PDUs', () => {
assert.ok(smuggled.err instanceof Error); assert.ok(smuggled.err instanceof Error);
assert.equal(smuggled.buffer, undefined); assert.equal(smuggled.buffer, undefined);
assert.ok(objToPdu({ cmdName: 'deliver_sm', params: { source_addr: '一' } }).err instanceof Error);
}); });
}); });
+11 -8
View File
@@ -84,7 +84,6 @@ describe('string (Octet String)', () => {
assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error); assert.ok(types.string.write('一', Buffer.alloc(4), 0).err instanceof Error);
}); });
// Its length octet is what ends it, so unlike a C-Octet String it carries a NULL like any other.
test('carries a NULL octet, which its length octet already bounds', () => { test('carries a NULL octet, which its length octet already bounds', () => {
const target = Buffer.alloc(4); const target = Buffer.alloc(4);
@@ -122,8 +121,6 @@ describe('cstring (C-Octet String)', () => {
assert.deepEqual(types.cstring.size(123), { size: 4 }); assert.deepEqual(types.cstring.size(123), { size: 4 });
}); });
// 'ascii' masks bit 7 on the way in and keeps it on the way out, so an address a peer wrote as
// Kaffeé came back Kaffei and objToPdu(pduToObj(x)) stopped being idempotent.
test('carries every latin1 octet, and refuses a character past it', () => { test('carries every latin1 octet, and refuses a character past it', () => {
const address = Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9, 0x00]); const address = Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9, 0x00]);
const target = Buffer.alloc(7); const target = Buffer.alloc(7);
@@ -136,9 +133,6 @@ describe('cstring (C-Octet String)', () => {
assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error); assert.ok(types.cstring.write('一', Buffer.alloc(4), 0).err instanceof Error);
}); });
// The field ends at its first NULL, so writing one smuggles a field boundary into the peer's
// parse: every mandatory field behind it shifts, under a command_length that counted the whole
// string.
test('refuses a NULL of its own rather than ending the field early', () => { test('refuses a NULL of its own rather than ending the field early', () => {
assert.ok(types.cstring.size('46701113311\u0000EVIL').err instanceof Error); assert.ok(types.cstring.size('46701113311\u0000EVIL').err instanceof Error);
assert.ok(types.cstring.write('46701113311\u0000EVIL', Buffer.alloc(17), 0).err instanceof Error); assert.ok(types.cstring.write('46701113311\u0000EVIL', Buffer.alloc(17), 0).err instanceof Error);
@@ -252,8 +246,6 @@ describe('buffer', () => {
}); });
describe('paramText()', () => { describe('paramText()', () => {
// The one reader of a receipt body that arrived with no octets of its own, so masking bit 7
// here loses the same characters the wire types used to.
test('renders a Buffer parameter as the latin1 text its octets spell', () => { test('renders a Buffer parameter as the latin1 text its octets spell', () => {
assert.equal(paramText(Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9])), 'Kaffeé'); assert.equal(paramText(Buffer.from([0x4B, 0x61, 0x66, 0x66, 0x65, 0xE9])), 'Kaffeé');
}); });
@@ -293,6 +285,11 @@ describe('dest_address_array', () => {
types.dest_address_array.write(expected, target, 0); types.dest_address_array.write(expected, target, 0);
assert.deepEqual(target, encoded); assert.deepEqual(target, encoded);
const smuggled: DestAddress[] = [{ dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: '46\u0000EVIL' }];
assert.ok(types.dest_address_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error);
assert.ok(types.dest_address_array.write([{ dl_name: '一' }], Buffer.alloc(16), 0).err instanceof Error);
}); });
test('refuses a field value the wire cannot hold instead of throwing', () => { test('refuses a field value the wire cannot hold instead of throwing', () => {
@@ -332,6 +329,12 @@ describe('unsuccess_sme_array', () => {
types.unsuccess_sme_array.write(expected, target, 0); types.unsuccess_sme_array.write(expected, target, 0);
assert.deepEqual(target, encoded); assert.deepEqual(target, encoded);
const smuggled: UnsuccessSme[] = [
{ dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: 'a\u0000b', error_status_code: 0 },
];
assert.ok(types.unsuccess_sme_array.write(smuggled, Buffer.alloc(16), 0).err instanceof Error);
}); });
test('refuses a field value the wire cannot hold instead of throwing', () => { test('refuses a field value the wire cannot hold instead of throwing', () => {
+24
View File
@@ -345,6 +345,30 @@ describe('a body the PDU\'s own data_coding cannot carry', () => {
}); });
}); });
describe('an address the field cannot carry', () => {
test('refuses it through sendSms(), with nothing reaching the socket', async t => {
const smsc = await dummySmsc(t);
const session = await bindToSmsc(t, smsc.port, { reconnect: false });
const refusals: [string, RegExp][] = [
['46701113311\u0000EVIL', /U\+0000 at index 11/],
['Kaffe一', /U\+4E00 at index 5/],
['😀', /U\+1F600 at index 0/],
];
for (const [sender, names] of refusals) {
const sent = await session.sendSms({ from: sender, message: 'Hello world', to });
assert.ok(sent.err instanceof Error, sender);
assert.match(sent.err.message, /source_addr/);
assert.match(sent.err.message, names);
assert.deepEqual(sent.smsIds, [], sender);
}
assert.deepEqual(smsc.octets, [], 'an address the field cannot carry never reaches the socket');
});
});
describe('a time no peer can read', () => { describe('a time no peer can read', () => {
const invalid = new Date('nope'); const invalid = new Date('nope');
+8
View File
@@ -259,6 +259,14 @@ and is also what the panel ranked hardest — two methods, one answer.
budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the
current group survived. Named by 6 of 9 readers. current group survived. Named by 6 of 9 readers.
- [ ] **Let the two address arrays size a C-Octet String through `cstring.size()`.**
`sizeDestAddresses()` and `sizeUnsuccessSmes()` spell "len + 1" themselves, and each `offset +=`
after a write spells it a third time, so `dest_address_array` and `unsuccess_sme_array` each
know the cost in three places. Route both through `cstring.size()` and advance the offset by
what it returns. That also makes `size()` refuse where `write()` already does, so the error
arrives from the first call rather than the second; today the pair only fails closed because
`writeParams()` and `writeTlvs()` both bail on the write. From the stability review of #16.
- [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it - [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it
twice for opposite conclusions — "nothing to drain, success" and "the link died under us, twice for opposite conclusions — "nothing to drain, success" and "the link died under us,
failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7 failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7