diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 49fb6b1b7f..7b6d588c88 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7646,7 +7646,9 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption subtype_namespace: MN-ID subtype namespace. subtype_reversed: MN-ID subtype reversed flag. identifier: Identifier. An :obj:`int` is accepted for every - subtype except ``NAI``. For ``IPv6_Address`` it is converted + subtype except ``NAI``, with the sole exception of a + :obj:`bool`, which is rejected for every subtype -- see + ``Raises`` below. For ``IPv6_Address`` it is converted and validated the same way as any other value :class:`ipaddress.IPv6Address` accepts. For the other six subtypes -- all numeric identifiers (an IMSI, a P-TMSI, an @@ -7662,12 +7664,24 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption Constructed option schema. Raises: - ProtocolError: If ``identifier`` is a negative :obj:`int` (no - subtype has a wire form for one), an :obj:`int` of any value - with the ``NAI`` subtype, or an :obj:`int` of ``2**128`` or + ProtocolError: If ``identifier`` is a :obj:`bool`, for any subtype + -- :obj:`bool` is an :obj:`int` subclass, so ``True`` would + otherwise be converted by two different paths below, to + ``::1`` for ``IPv6_Address`` and to a one-octet identifier for + the other six, neither of which a caller passing a flag can + plausibly have meant; pass ``int(...)`` to get the numeric + value (c.f. #469). If ``identifier`` is a negative :obj:`int` + (no subtype has a wire form for one), an :obj:`int` of any value + with the ``NAI`` subtype, an :obj:`int` of ``2**128`` or above with the ``IPv6_Address`` subtype (whose wire form is a fixed 16 octets, unlike the other subtypes, which have no - ceiling and simply pack into more). + ceiling and simply pack into more), or ``identifier`` is of a + type its subtype's field cannot hold at all: anything but + :obj:`str` for ``NAI``, anything but :obj:`bytes`/ + :obj:`bytearray`/:obj:`int` for the other six -- an :obj:`int` + is converted rather than rejected there, per #468 -- or anything + :class:`ipaddress.IPv6Address` itself does not accept for + ``IPv6_Address`` (c.f. #469). """ if option is not None: @@ -7702,6 +7716,21 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption f'{self.alias}: [OptNo {type}] MN-ID subtype {subtype_repr} ' f'identifier must be a non-negative int, not {identifier!r}') + if isinstance(identifier, bool): + # NOTE: checked before the subtype dispatch, because ``bool`` is an + # ``int`` subclass and so would otherwise be converted by *two* + # different paths below -- ``IPv6Address(1)``, that is ``::1``, for + # ``IPv6_Address``, and a one-octet identifier for the six + # ``BytesField`` subtypes. An MN-ID of ``True`` is a caller mistake + # in every case rather than a value anyone means, so it is refused + # before either path can give it a plausible-looking wire form. A + # caller who genuinely wants the integer should pass ``int(flag)`` + # (c.f. #469 review). + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID identifier must not be a ' + f'bool, not {identifier!r} -- pass int({identifier!r}) if the ' + f'numeric value is what is wanted') + # NOTE: The wire format is chosen by ``subtype_val`` (c.f. ``mn_id_selector``), # not by the Python type of ``identifier``, so the width has to be taken from # the former. For the ``IPv6_Address`` subtype the schema always packs a fixed @@ -7727,7 +7756,23 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption f'{self.alias}: [OptNo {type}] MN-ID subtype IPv6_Address ' f'identifier must be an int below 2**128, not {identifier!r}') if not isinstance(identifier, ipaddress.IPv6Address): - identifier = ipaddress.IPv6Address(identifier) + # NOTE: catches ipaddress.AddressValueError -- itself a bare + # ValueError -- for every identifier ipaddress.IPv6Address + # cannot turn into an address: bytes of the wrong length, a + # str that is not an IPv6 literal, or a type it does not + # accept at all (float, None, list, dict, bytearray, + # memoryview -- ipaddress.IPv6Address only ever dispatches on + # bytes, int or str). The ``try`` wraps only this call, not + # the whole branch, so it cannot swallow the ProtocolError + # raised above for an out-of-range int, which is also a + # ValueError subclass (c.f. #467, #468, #469). + try: + identifier = ipaddress.IPv6Address(identifier) + except ValueError as error: + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype IPv6_Address ' + f'identifier must be an ipaddress.IPv6Address, an int, or ' + f'bytes/str it accepts, not {identifier!r}') from error id_len = 16 elif isinstance(identifier, int): if subtype_val == Enum_MNIDSubtype.NAI: @@ -7763,7 +7808,52 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # encoder would produce. id_len = max(1, math.ceil(identifier.bit_length() / 8)) identifier = identifier.to_bytes(id_len, 'big') + elif subtype_val == Enum_MNIDSubtype.NAI: + # NOTE: NAI's field is a StringField (c.f. mn_id_selector), which + # calls ``identifier.encode(...)`` to pack -- so anything but a + # genuine str leaks a bare stdlib exception: AttributeError for + # bytes/list/dict (no ``.encode``), TypeError for float/None/an + # ipaddress.IPv6Address (no ``__len__`` either, so this branch's + # own ``len()`` call below would be the one to raise). Guarded + # here, before ``len()``, rather than relying on whichever of + # those two happens to fire first (c.f. #469). Deliberately not + # decoding a ``bytes`` identifier here: an NAI that happens to be + # ASCII-encodable is still the caller handing over the wrong + # representation, the same "accepts a value that means the wrong + # thing" #467 removed for int, just relocated to bytes. + if not isinstance(identifier, str): + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype NAI ' + f'identifier must be str, not {identifier!r}') + id_len = len(identifier) else: + # NOTE: every other subtype's field is a + # BytesField(length=pkt['length'] - 1) (c.f. mn_id_selector), + # which hands ``identifier`` to ``struct.pack('Ns', ...)`` + # unconverted -- so anything but bytes leaks a bare stdlib + # exception: struct.error for str/list/dict (struct's ``s`` + # format demands a bytes object), TypeError for float/None/an + # ipaddress.IPv6Address (no ``__len__``, so this branch's own + # ``len()`` call below would raise instead). Guarded here, + # before ``len()``, for the same reason as the NAI branch above + # (c.f. #469). + # + # bytearray is accepted alongside bytes -- unlike every other + # wrong type here, it already round-trips correctly through + # ``struct.pack('Ns', ...)`` (measured), so rejecting it would + # be a gratuitous behaviour change to a type nothing here is + # actually broken for. memoryview looks equally bytes-like but + # does *not* survive struct's ``s`` format (measured: same + # ``struct.error`` as str/list/dict), so it is rejected with + # everything else rather than let through to leak anyway. + if not isinstance(identifier, (bytes, bytearray)): + try: + subtype_repr = repr(Enum_MNIDSubtype(subtype_val)) + except ValueError: + subtype_repr = repr(subtype_val) + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype {subtype_repr} ' + f'identifier must be bytes, not {identifier!r}') id_len = len(identifier) return Schema_MNIDOption( diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 42ffe9bbe3..462c4a0ef6 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2482,6 +2482,152 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=-5) self.assertIsInstance(ctx.exception, BaseError) + def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None: + """Every MN-ID subtype rejects the *other* documented type in-library. + + ``_make_opt_mn_id`` annotates ``identifier`` as + ``bytes | str | IPv6Address | int``, and #467/#468 (see + ``test_mh_mn_id_option_converts_int_identifier_per_subtype`` above) + closed the ``int`` half of that union. But each subtype's field + (c.f. ``mn_id_selector``) accepts exactly *one* of ``bytes``/``str`` + -- a ``StringField`` for ``NAI``, a ``BytesField`` for the other six + -- so handing the wrong one, or a value of neither type, used to + leak a bare stdlib exception instead of an in-library one: + + * ``bytes``/list/dict for ``NAI`` -- ``AttributeError`` (``StringField`` + calls ``identifier.encode(...)`` to pack) + * ``str``/list/dict for the six octet subtypes -- ``struct.error`` + (struct's ``s`` format demands a bytes object) + * anything without ``__len__`` (``float``, ``None``, an + :class:`~ipaddress.IPv6Address`) for any of the seven -- + ``TypeError: object of type '...' has no len()`` + * anything :class:`ipaddress.IPv6Address` itself cannot parse, for + ``IPv6_Address`` -- ``ipaddress.AddressValueError``, itself a bare + ``ValueError`` + + Every one of those must become a ``ProtocolError`` naming the subtype + and the type it actually accepts. See #469. + """ + from ipaddress import IPv6Address + + from pcapkit.const.mh.mn_id_subtype import MNIDSubtype + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + proto = object.__new__(MH) + + octet_subtypes = ('IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address', + 'GUTI', 'DUID') + + # values that are the *wrong* type for every subtype below -- none of + # bytes, str, an IPv6Address instance, a float, None, a list, or a + # dict is what any of these subtypes' fields can hold except the one + # matching type tested separately further down. + wrong_for_nai = (b'\x01\x02', [1, 2], {'a': 1}, IPv6Address('::1'), 1.5, None, b'') + wrong_for_octets = ('user@realm', [1, 2], {'a': 1}, IPv6Address('::1'), 1.5, None, '', memoryview(b'\x01\x02')) + wrong_for_ipv6 = (b'\x01\x02', 'user@realm', [1, 2], {'a': 1}, 1.5, None, b'', + bytearray(b'\x00' * 16), memoryview(b'\x00' * 16)) + + for value in wrong_for_nai: + with self.subTest(subtype='NAI', identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('NAI', str(ctx.exception)) + self.assertIn('str', str(ctx.exception)) + + for subtype in octet_subtypes: + for value in wrong_for_octets: + with self.subTest(subtype=subtype, identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('bytes', str(ctx.exception)) + + for value in wrong_for_ipv6: + with self.subTest(subtype='IPv6_Address', identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, + identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('IPv6_Address', str(ctx.exception)) + + # the matching type, including the empty-value boundary, still works + # for every subtype -- this fix must reject the wrong type, not + # tighten what already worked. + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier='') + self.assertEqual(schema.length, 1) + self.assertEqual(len(schema.pack()), schema.length + 2) + + for subtype in octet_subtypes: + with self.subTest(subtype=subtype, identifier=b''): + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=b'') + self.assertEqual(schema.length, 1) + self.assertEqual(len(schema.pack()), schema.length + 2) + + # ``bytearray`` is accepted alongside ``bytes`` for the six octet + # subtypes: unlike every value in ``wrong_for_octets`` above, it + # already round-trips correctly through + # ``struct.pack('Ns', ...)`` (measured separately), so rejecting + # it would tighten behaviour that was never broken. ``memoryview`` + # looks equally bytes-like but does *not* survive that same pack + # call, so it stays in ``wrong_for_octets`` above rather than + # being let through to leak a bare ``struct.error`` anyway. + with self.subTest(subtype=subtype, identifier='bytearray'): + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=bytearray(b'\x01\x02')) + self.assertEqual(schema.identifier, bytearray(b'\x01\x02')) + self.assertEqual(schema.pack()[3:], b'\x01\x02') + + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, + identifier=IPv6Address('2001:db8::1')) + self.assertEqual(schema.length, 17) + self.assertEqual(len(schema.pack()), schema.length + 2) + + def test_mh_mn_id_option_rejects_a_bool_identifier_for_every_subtype(self) -> None: + """A ``bool`` identifier is a caller mistake, not a one-octet integer. + + ``bool`` is an :class:`int` subclass, so before #469's review flagged it + ``True``/``False`` fell through to the #468 int-conversion path and + silently produced a plausible-looking wire form -- ``IPv6Address(1)``, + that is ``::1``, for ``IPv6_Address``, and a one-octet identifier for the + six ``BytesField`` subtypes. Refused for every subtype now, before the + subtype dispatch, so neither numeric path can reach it. A caller who + genuinely wants the integer passes ``int(flag)``, which the message says. + """ + from pcapkit.const.mh.mn_id_subtype import MNIDSubtype + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import ProtocolError + + proto = object.__new__(MH) + for subtype in ('NAI', 'IPv6_Address', 'IMSI', 'P_TMSI', + 'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'): + for identifier in (True, False): + with self.subTest(subtype=subtype, identifier=identifier): + with self.assertRaises(ProtocolError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, + subtype=getattr(MNIDSubtype, subtype), + identifier=identifier) + self.assertIn('must not be a bool', str(ctx.exception)) + + # ``int(flag)`` is the documented escape hatch and still converts. + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IMSI, + identifier=int(True)) + self.assertEqual(schema.identifier, b'\x01') + def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags.