diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index b81b0a93b1..49fb6b1b7f 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/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) diff --git a/pcapkit/protocols/schema/internet/mh.py b/pcapkit/protocols/schema/internet/mh.py index c6ba345fb7..08b7a64378 100644 --- a/pcapkit/protocols/schema/internet/mh.py +++ b/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 ` + # *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 diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index f6f563c9a1..24044c926a 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/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.