mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib leak

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(

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.