protocols: reject an int MN-ID identifier for every subtype but IPv6_Address

pcapkit/protocols/internet/mh.py

@@ -7645,12 +7645,30 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption
             subtype_default: MN-ID subtype default value.
             subtype_namespace: MN-ID subtype namespace.
             subtype_reversed: MN-ID subtype reversed flag.
-            identifier: Identifier.
+            identifier: Identifier. An :obj:`int` is accepted for every
+                subtype except ``NAI``. 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
+                EUI-48/64 address, a GUTI, a DUID) -- it is converted to its
+                own minimal big-endian octets, at least one. ``NAI`` is text
+                (RFC 4283's ``user@realm`` form) rather than a numeric
+                identifier, so there is no non-arbitrary int-to-text mapping
+                the way there is int-to-address or int-to-octets, and an
+                :obj:`int` is rejected there (c.f. #467, #468).
             **kwargs: Arbitrary keyword arguments.
 
         Returns:
             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
+                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).
+
         """
         if option is not None:
             subtype_val = option.subtype
@@ -7659,6 +7677,31 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption
             subtype_val = self._make_index(subtype, subtype_default, namespace=subtype_namespace,  # type: ignore[assignment]
                                            reversed=subtype_reversed, pack=False)
 
+        if isinstance(identifier, int) and identifier < 0:
+            # NOTE: checked before the subtype dispatch below, not inside it,
+            # because *no* subtype has a wire form for a negative identifier and
+            # each one fails differently on its own: ``int.to_bytes`` raises
+            # ``OverflowError`` and ``ipaddress.IPv6Address`` an
+            # ``AddressValueError`` -- itself a bare :exc:`ValueError`, which is
+            # exactly the class of leak this handler exists to stop, and which a
+            # guard living inside the ``elif isinstance(identifier, int)`` branch
+            # could not catch, since the ``IPv6_Address`` dispatch never reaches
+            # it (c.f. #467, #468).
+            try:
+                # ``Enum_MNIDSubtype(subtype_val)`` round-trips a plain int back
+                # into a named member for the message below -- but its own
+                # ``_missing_`` only auto-extends 9-15 and 16-255, so 0,
+                # negatives and anything above 255 make the constructor itself
+                # raise a bare ``ValueError``, which would defeat the point of
+                # this guard (c.f. #468 review). Caught here and the raw value
+                # used instead rather than let it propagate.
+                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 a non-negative int, not {identifier!r}')
+
         # 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
@@ -7667,11 +7710,59 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption
         # form here as well, keeping the packed bytes and the declared length derived
         # from one value instead of two independent computations (c.f. #448).
         if subtype_val == Enum_MNIDSubtype.IPv6_Address:
+            if isinstance(identifier, int) and identifier >= 1 << 128:
+                # NOTE: the upper-bound mirror of the negative-int guard above, and
+                # it belongs here rather than up there because this bound is
+                # subtype-*dependent*: ``2**140`` is a perfectly good identifier for
+                # the six ``BytesField`` subtypes -- it simply packs into more
+                # octets -- and only ``IPv6_Address`` caps at 128 bits. Left
+                # unguarded, :class:`ipaddress.IPv6Address` raises
+                # ``AddressValueError``, itself a bare :exc:`ValueError`, so this
+                # handler would otherwise ship with its lower bound guarded and its
+                # upper bound leaking (c.f. #467, #468). Checked explicitly rather
+                # than by wrapping the construction below, because that would also
+                # swallow the wrong-*type* ``AddressValueError`` -- a ``str`` or
+                # ``None`` reaching here -- which is #469's subject, not this one's.
+                raise ProtocolError(
+                    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)
             id_len = 16
         elif isinstance(identifier, int):
-            id_len = math.ceil(identifier.bit_length() / 8)
+            if subtype_val == Enum_MNIDSubtype.NAI:
+                # NOTE: NAI's field is a StringField (c.f. mn_id_selector), so an
+                # int has to become text -- and unlike the numeric subtypes below,
+                # there is no non-arbitrary way to do that. str(identifier) packs
+                # and round-trips fine, but an NAI is a network access identifier
+                # ('user@realm', RFC 4283), and a bare decimal-digit string is not
+                # one: it is mechanically valid and semantically nonsense, exactly
+                # the "silently accepting a value that cannot pack" #467 removed,
+                # just relocated to "silently accepting a value that packs into
+                # the wrong thing". Rejected instead, with the explicit spelling
+                # a caller who really wants a decimal-digit NAI can use.
+                raise ProtocolError(
+                    f'{self.alias}: [OptNo {type}] MN-ID subtype NAI identifier '
+                    f'must be str, not int -- pass str({identifier!r}) if a '
+                    f'decimal-digit NAI is really what is wanted')
+            # NOTE: every other subtype's field is a
+            # BytesField(length=pkt['length'] - 1) (c.f. mn_id_selector) -- a
+            # numeric identifier, so unlike NAI there IS a non-arbitrary wire
+            # form: its own minimal big-endian encoding. That is self-consistent
+            # with the declared length by construction and round-trips exactly.
+            # ``id_len = math.ceil(identifier.bit_length() / 8)`` was the right
+            # width all along -- the pre-#467 defect was never the sizing, it
+            # was that ``identifier`` itself stayed an ``int`` afterwards and
+            # was handed to ``BytesField`` unconverted, which ``struct.pack()``
+            # cannot do anything with. #468 initially rejected outright instead
+            # of noticing that; converting is what this revision does (c.f.
+            # #467, #468). ``bit_length()`` is 0 for 0 itself, which would
+            # otherwise declare a zero-octet identifier -- collapsing "the
+            # identifier's value is 0" into "there is no identifier" -- so the
+            # width is floored at one octet, matching what any reasonable
+            # encoder would produce.
+            id_len = max(1, math.ceil(identifier.bit_length() / 8))
+            identifier = identifier.to_bytes(id_len, 'big')
         else:
             id_len = len(identifier)
 

pcapkit/protocols/schema/internet/mh.py

@@ -724,7 +724,25 @@ class MNIDOption(Option, code=Enum_Option.MN_ID_OPTION_TYPE):
     identifier: 'bytes | str | IPv6Address' = SwitchField(selector=mn_id_selector)
 
     if TYPE_CHECKING:
-        def __init__(self, type: 'Enum_Option', length: 'int', subtype: 'Enum_MNIDSubtype', identifier: 'bytes | str | IPv6Address | int') -> 'None': ...
+        # NOTE: No ``int`` here, and deliberately so even though
+        # :meth:`MH._make_opt_mn_id <pcapkit.protocols.internet.mh.MH._make_opt_mn_id>`
+        # *does* accept one. The two annotations describe different boundaries:
+        # the maker's is what a **caller** may pass, while this one is what the
+        # schema can **hold**, and the maker converts between them before ever
+        # constructing this class -- an ``int`` becomes :obj:`bytes` via
+        # :meth:`int.to_bytes` for the octet subtypes and an
+        # :class:`~ipaddress.IPv6Address` for ``IPv6_Address``. Measured through
+        # the maker: ``identifier=0x1234`` arrives here as ``b'\\x124'`` for
+        # ``IMSI``/``DUID`` and as ``IPv6Address('::1234')`` for
+        # ``IPv6_Address``, never as an ``int``. Widening this stub to admit one
+        # would therefore document a value the schema can never hold, and would
+        # positively mislead: ``mn_id_selector`` resolves every subtype but
+        # ``IPv6_Address`` to a
+        # :class:`~pcapkit.corekit.fields.strings.StringField` or
+        # :class:`~pcapkit.corekit.fields.strings.BytesField`, and handing either
+        # a raw ``int`` is precisely the #467 defect -- ``struct.pack()`` cannot
+        # consume it (c.f. #467, #468).
+        def __init__(self, type: 'Enum_Option', length: 'int', subtype: 'Enum_MNIDSubtype', identifier: 'bytes | str | IPv6Address') -> 'None': ...
 
 
 @schema_final

tests/protocols/internet/test_mh_unit.py

@@ -2330,6 +2330,156 @@ def test_mh_mn_id_option_length_matches_packed_octets(self) -> None:
                 self.assertEqual(schema.length, 17)
                 self.assertEqual(len(schema.pack()), schema.length + 2)
 
+    def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None:
+        """An ``int`` identifier converts to each subtype's own wire form.
+
+        ``_make_opt_mn_id`` used to size an ``int`` identifier from the
+        integer's own :meth:`int.bit_length` regardless of ``subtype``, but
+        never actually turned it into the octets that width described --
+        ``BytesField`` received the ``int`` itself, and ``struct.pack()``
+        cannot do anything with that (#467). An earlier revision of this fix
+        (9b26fa387) rejected ``int`` outright for every subtype but
+        ``IPv6_Address``, on the reasoning that there was no non-arbitrary
+        width to convert it to. That reasoning held for ``NAI`` (its field is
+        a ``StringField``, and NAI is text, not a number) but was wrong for
+        the other six: ``id_len = math.ceil(identifier.bit_length() / 8)``
+        *was* the right, non-arbitrary width all along, self-consistent with
+        the declared ``length`` by construction -- what was missing was
+        actually converting ``identifier`` to those octets via
+        :meth:`int.to_bytes` before handing it to the schema. See #467, #468.
+        """
+        import io
+
+        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.protocols.schema.internet.mh import \
+            MNIDOption as Schema_MNIDOption
+        from pcapkit.utilities.exceptions import BaseError, ProtocolError
+
+        proto = object.__new__(MH)
+
+        # every ``BytesField`` subtype (c.f. ``mn_id_selector``'s fallback
+        # ``return BytesField(length=pkt['length'] - 1)``) converts an ``int``
+        # to its own minimal big-endian octets, at least one -- tested through
+        # the maker itself, not a hand-built schema with a self-consistent
+        # ``length`` the maker would never produce, and round-tripped through
+        # the wire (packed, then unpacked back into a fresh schema), not just
+        # packed once and trusted.
+        cases = (
+            (0x1234, 2, b'\x12\x34'),
+            (0x0, 1, b'\x00'),  # bit_length() is 0 for 0 itself; floored at 1
+            (0x1, 1, b'\x01'),
+            (0xff, 1, b'\xff'),
+            (0x100000000, 5, b'\x01\x00\x00\x00\x00'),
+        )
+        for subtype in ('IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address',
+                        'GUTI', 'DUID'):
+            for identifier, id_len, octets in cases:
+                with self.subTest(subtype=subtype, identifier=hex(identifier)):
+                    schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+                        Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype),
+                        identifier=identifier)
+                    self.assertEqual(schema.length, 1 + id_len)
+                    self.assertEqual(schema.identifier, octets)
+                    packed = schema.pack()
+                    self.assertEqual(len(packed), schema.length + 2)
+
+                    unpacked = Schema_MNIDOption.unpack(io.BytesIO(packed), len(packed), {})
+                    self.assertEqual(unpacked.identifier, octets)
+
+        # ``NAI`` is the one subtype with no non-arbitrary int-to-wire mapping
+        # -- its field is text (RFC 4283's ``user@realm``), not a number -- so
+        # an ``int`` is still rejected there, with a message naming the
+        # decimal-string alternative, which is itself checked to actually work.
+        with self.assertRaises(ProtocolError) as ctx:
+            proto._make_opt_mn_id(  # type: ignore[arg-type]
+                Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier=0x1234)
+        self.assertIn('str(4660)', str(ctx.exception))
+        schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+            Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier=str(0x1234))
+        self.assertEqual(schema.pack()[3:].decode(), '4660')
+
+        # a negative ``int`` has no wire form under any subtype, and each one
+        # fails differently on its own: ``int.to_bytes()`` raises a bare
+        # ``OverflowError`` and ``ipaddress.IPv6Address`` an
+        # ``AddressValueError`` -- itself a bare ``ValueError``. So the guard
+        # sits before the subtype dispatch rather than inside the ``int``
+        # branch, and ``IPv6_Address`` is covered here too: an earlier revision
+        # of this fix guarded only inside that branch, which the
+        # ``IPv6_Address`` dispatch never reaches, leaving
+        # ``identifier=-5, subtype=IPv6_Address`` leaking
+        # ``AddressValueError: -5 (< 0) is not permitted as an IPv6 address``
+        # out of the very handler that exists to stop bare stdlib exceptions
+        # escaping. Confirmed to fail against that revision.
+        for subtype in ('NAI', 'IPv6_Address', 'IMSI', 'P_TMSI',
+                        'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'):
+            with self.subTest(subtype=subtype, identifier=-5):
+                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=-5)
+                self.assertIsInstance(ctx.exception, BaseError)
+
+        # the ``IPv6_Address`` subtype is unaffected -- an ``int`` identifier
+        # still converts to its fixed 16-octet wire form via
+        # :class:`ipaddress.IPv6Address`, which both converts and validates,
+        # as #448 fixed and neither revision of this fix has touched.
+        schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+            Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, identifier=0x1234)
+        self.assertEqual(schema.length, 17)
+        self.assertEqual(len(schema.pack()), schema.length + 2)
+
+        # ``IPv6_Address`` is the one subtype with an *upper* bound as well, and it
+        # is checked on both sides of the boundary rather than at some large value,
+        # which is how this gap survived the first pass: the earlier probe stopped
+        # at ``2**128 - 1``, exactly one below where the answer changes. Above the
+        # bound ``ipaddress.IPv6Address`` raises ``AddressValueError`` -- a bare
+        # ``ValueError`` -- so it must be rejected in-library instead. The bound is
+        # subtype-dependent: the ``BytesField`` subtypes have no ceiling and simply
+        # produce more octets, which is asserted here too so that a future guard
+        # cannot be hoisted to cover them by mistake.
+        schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+            Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address,
+            identifier=2 ** 128 - 1)
+        self.assertEqual(schema.length, 17)
+        for identifier in (2 ** 128, 2 ** 140):
+            with self.subTest(subtype='IPv6_Address', identifier=identifier):
+                with self.assertRaises(ProtocolError) as ctx:
+                    proto._make_opt_mn_id(  # type: ignore[arg-type]
+                        Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address,
+                        identifier=identifier)
+                self.assertIn('below 2**128', str(ctx.exception))
+        for subtype in ('IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address',
+                        'GUTI', 'DUID'):
+            for identifier, id_len in ((2 ** 128, 17), (2 ** 140, 18)):
+                with self.subTest(subtype=subtype, identifier=identifier):
+                    schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+                        Option.MN_ID_OPTION_TYPE,
+                        subtype=getattr(MNIDSubtype, subtype), identifier=identifier)
+                    self.assertEqual(schema.length, 1 + id_len)
+                    self.assertEqual(len(schema.pack()), schema.length + 2)
+
+        # 0, a negative value, and anything above 255 are all outside what
+        # ``MNIDSubtype._missing_`` extends. Naming the subtype in a rejection
+        # message must not let that enum round-trip's own bare ``ValueError``
+        # escape in its place -- but paired with a non-negative int, an
+        # out-of-range subtype is not itself an error: ``mn_id_selector``
+        # resolves it to the same generic ``BytesField`` fallback as any
+        # unassigned subtype, so only the negative-identifier combination is
+        # expected to raise here.
+        for subtype in (0, -1, 300, 999):
+            with self.subTest(subtype=subtype, identifier=0x1234):
+                schema = proto._make_opt_mn_id(  # type: ignore[arg-type]
+                    Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=0x1234)
+                self.assertEqual(schema.length, 3)
+                self.assertEqual(len(schema.pack()), schema.length + 2)
+            with self.subTest(subtype=subtype, identifier=-5):
+                with self.assertRaises(BaseError) as ctx:
+                    proto._make_opt_mn_id(  # type: ignore[arg-type]
+                        Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=-5)
+                self.assertIsInstance(ctx.exception, BaseError)
+
     def test_mh_redirect_option_rejects_contradictory_flags(self) -> None:
         """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags.