From 9b26fa3875b70b321d8f92d0df705a660009be6e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 01:26:04 -0400 Subject: [PATCH 1/6] protocols: reject an int MN-ID identifier for every subtype but IPv6_Address _make_opt_mn_id's `elif isinstance(identifier, int)` branch sized the identifier from the int's own bit_length() regardless of subtype, the same type-vs-subtype confusion #464 fixed for IPv6_Address. mn_id_selector resolves NAI to a StringField and every other non-IPv6 subtype to a BytesField, and neither field type converts an int, so the declared length was wrong and pack() always failed -- AttributeError for NAI ('int' has no .encode()), struct.error for the rest. This is the other half of #448, pre-existing on main and unrelated to #464. See #467. - pcapkit/protocols/internet/mh.py: the int branch now raises ProtocolError naming the resolved subtype and the type it actually accepts (str for NAI, bytes for the rest), instead of silently building an unpackable schema. IPv6_Address is unaffected -- #464 already normalises it via ipaddress.IPv6Address independent of identifier's Python type, and int stays valid there. - pcapkit/protocols/schema/internet/mh.py: MNIDOption's TYPE_CHECKING __init__ stub drops int from the identifier union, matching the class attribute's own already-narrower annotation two lines above it. - tests/protocols/internet/test_mh_unit.py: new test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes, one subTest per rejected subtype (NAI, IMSI, P_TMSI, EUI_48_address, EUI_64_address, GUTI, DUID) plus a check that IPv6_Address still converts. Full suite (PYTHONSAFEPATH=1, interpreter 3.14.7): 1007 passed, 17 skipped, 1560 subtests passed. Baseline at fa128959e: 1006 passed, 17 skipped, 1553 subtests passed. mypy: 125 errors before and after, byte-identical modulo line numbers. --- pcapkit/protocols/internet/mh.py | 33 +++++++++++++++++-- pcapkit/protocols/schema/internet/mh.py | 9 +++++- tests/protocols/internet/test_mh_unit.py | 40 ++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index b81b0a93b1..76ea66647b 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7645,12 +7645,23 @@ 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` remains accepted for the + ``IPv6_Address`` subtype (converted the same way as any other + value :class:`ipaddress.IPv6Address` accepts), but is rejected + for every other subtype: their fields are variable-length -- + :obj:`str` for ``NAI``, :obj:`bytes` for the rest -- sized from + the wire ``length`` rather than from anything ``subtype`` fixes + on its own, so there is no non-arbitrary width to convert an + :obj:`int` into (c.f. #467). **kwargs: Arbitrary keyword arguments. Returns: Constructed option schema. + Raises: + ProtocolError: If ``identifier`` is an :obj:`int` and ``subtype`` + is not ``IPv6_Address``. + """ if option is not None: subtype_val = option.subtype @@ -7671,7 +7682,25 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption identifier = ipaddress.IPv6Address(identifier) id_len = 16 elif isinstance(identifier, int): - id_len = math.ceil(identifier.bit_length() / 8) + # NOTE: Every other subtype's field is inherently variable-length in + # the schema (c.f. ``mn_id_selector``): a + # :class:`~pcapkit.corekit.fields.strings.StringField` for ``NAI``, a + # :class:`~pcapkit.corekit.fields.strings.BytesField` for the rest -- + # both sized from the packed ``length`` header, not from anything + # ``subtype_val`` fixes on its own. That is unlike ``IPv6_Address``, + # whose 16-octet width is a spec-fixed constant independent of the + # identifier's value. Neither field type converts an ``int`` -- + # ``BytesField`` packs the value as-is and ``StringField`` calls + # ``.encode()`` on it -- so there is no non-arbitrary width to take + # from ``subtype_val`` here: picking one (e.g. from the int's own + # ``bit_length()``, as this branch used to) would just reintroduce + # the type-vs-subtype confusion that produced this defect, only + # without the crash (c.f. #467). Reject instead of silently + # accepting a value that cannot pack. + expected = 'str' if subtype_val == Enum_MNIDSubtype.NAI else 'bytes' + raise ProtocolError(f'{self.alias}: [OptNo {type}] MN-ID subtype ' + f'{Enum_MNIDSubtype(subtype_val)!r} identifier must be ' + f'{expected}, not int') else: id_len = len(identifier) diff --git a/pcapkit/protocols/schema/internet/mh.py b/pcapkit/protocols/schema/internet/mh.py index c6ba345fb7..4ddb701713 100644 --- a/pcapkit/protocols/schema/internet/mh.py +++ b/pcapkit/protocols/schema/internet/mh.py @@ -724,7 +724,14 @@ 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, matching ``identifier``'s own field annotation + # above -- ``mn_id_selector`` only ever resolves to an + # :class:`~pcapkit.corekit.fields.ipaddress.IPv6AddressField` (which + # would accept one) for the ``IPv6_Address`` subtype; every other + # subtype resolves to a :class:`~pcapkit.corekit.fields.strings.StringField` + # or :class:`~pcapkit.corekit.fields.strings.BytesField`, neither of + # which accepts an ``int`` (c.f. #467). + 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..1b64d8d7d9 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2330,6 +2330,46 @@ 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_rejects_int_identifier_for_non_ipv6_subtypes(self) -> None: + """An ``int`` identifier is only meaningful for the ``IPv6_Address`` subtype. + + ``_make_opt_mn_id`` used to size an ``int`` identifier from the + integer's own :meth:`int.bit_length` regardless of ``subtype`` -- the + same type-vs-subtype confusion #448 fixed for ``IPv6_Address`` -- which + produced a schema that could not be packed for any of the other seven + subtypes, with a declared ``length`` that was wrong either way. Unlike + ``IPv6_Address`` (a spec-fixed 16-octet width, independent of the + identifier's value), every other subtype's field is variable-length + and sized from the packed ``length`` header rather than from anything + ``subtype`` fixes on its own, so there is no non-arbitrary width to + convert an ``int`` into -- it is rejected instead. See #467. + """ + 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) + + # ``NAI``'s field is a ``StringField`` (``str``); the other six are all + # ``BytesField`` (``bytes``) via the same generic fallback in + # ``mn_id_selector``. Both groups reject ``int``, but for a different + # reason, so both are exercised rather than assuming the fix generalises. + for subtype in ('NAI', 'IMSI', 'P_TMSI', 'EUI_48_address', + 'EUI_64_address', 'GUTI', 'DUID'): + with self.subTest(subtype): + with self.assertRaises(ProtocolError): + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=0x1234) + + # the ``IPv6_Address`` subtype is unaffected -- an ``int`` identifier + # still converts to its fixed 16-octet wire form, as #448 fixed. + 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) + def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags. From 1ea8b56ab0b1027d9fdbc5c057ec25615efa3266 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 02:04:41 -0400 Subject: [PATCH 2/6] protocols: don't let MN-ID's own rejection message raise a bare ValueError _make_opt_mn_id's new int-identifier rejection (9b26fa387) named the resolved subtype by round-tripping it through Enum_MNIDSubtype(subtype_val) for a friendly repr. MNIDSubtype._missing_ only auto-extends 9-15 and 16-255, so an out-of-range subtype (0, negative, or above 255) made that constructor call itself raise a bare ValueError, escaping in place of the ProtocolError this branch exists to raise instead. #468 review. - pcapkit/protocols/internet/mh.py: wrap the round-trip in try/except ValueError and fall back to the raw subtype_val for the message on failure, so the branch always raises ProtocolError regardless of what subtype_val holds. Every real subtype (and the Reserved_N/Unassigned_N extensions in 9-255) still gets the named repr; only genuinely unrepresentable values fall back to the bare int. - tests/protocols/internet/test_mh_unit.py: extends test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes with subtype 0, -1, 300, 999, asserting isinstance(e, BaseError). Checked mh.py for the same pattern elsewhere (an enum constructor called while formatting a raised message): none found; the only occurrence was the one being fixed here. _make_index (protocol.py) is unrelated -- for an int or Enum argument it returns the value/`.value` unvalidated, by design, for every enum it is used with, not specific to MNIDSubtype. Full suite (PYTHONSAFEPATH=1, interpreter 3.14.7): 37 passed in test_mh_unit.py, 283 subtests. mypy: 125 errors, unchanged. --- pcapkit/protocols/internet/mh.py | 14 +++++++++++++- tests/protocols/internet/test_mh_unit.py | 22 +++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 76ea66647b..7eb11098f5 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7698,8 +7698,20 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # without the crash (c.f. #467). Reject instead of silently # accepting a value that cannot pack. expected = 'str' if subtype_val == Enum_MNIDSubtype.NAI else 'bytes' + 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``. That would defeat the point of this branch, + # which exists to stop a bare stdlib exception from escaping + # ``_make_opt_mn_id`` in the first place, so it is caught here and + # the raw value is 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 ' - f'{Enum_MNIDSubtype(subtype_val)!r} identifier must be ' + f'{subtype_repr} identifier must be ' f'{expected}, not int') else: id_len = len(identifier) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 1b64d8d7d9..952384d268 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2343,11 +2343,19 @@ def test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes(self) -> N and sized from the packed ``length`` header rather than from anything ``subtype`` fixes on its own, so there is no non-arbitrary width to convert an ``int`` into -- it is rejected instead. See #467. + + The rejection message also has to survive an out-of-range ``subtype``: + it round-trips ``subtype_val`` through ``Enum_MNIDSubtype`` for a + friendly name, and that constructor itself raises a bare ``ValueError`` + for a value :meth:`MNIDSubtype._missing_` does not auto-extend (only + 9-15 and 16-255 are). No input to ``_make_opt_mn_id`` may produce a + non-:exc:`~pcapkit.utilities.exceptions.BaseError` exception, so that + is exercised too rather than assumed from the seven real subtypes. """ 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 + from pcapkit.utilities.exceptions import BaseError, ProtocolError proto = object.__new__(MH) @@ -2370,6 +2378,18 @@ def test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes(self) -> N self.assertEqual(schema.length, 17) self.assertEqual(len(schema.pack()), schema.length + 2) + # 0, a negative value, and anything above 255 are all outside what + # ``MNIDSubtype._missing_`` extends, so the enum constructor itself + # raises for them -- this must still come out as an in-library + # ``BaseError`` (a ``ProtocolError``, here), never the bare + # ``ValueError`` the naming round-trip would otherwise leak. + for subtype in (0, -1, 300, 999): + with self.subTest(subtype=subtype): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=0x1234) + 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. From a8c214ab45631c70266c8e7f401eeab2706aa94b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 09:55:59 -0400 Subject: [PATCH 3/6] protocols: convert an int MN-ID identifier instead of rejecting it (#467) The owner asked for int to stay accepted rather than be turned away, and the reasoning behind the earlier rejection was wrong. `id_len = math.ceil(identifier.bit_length() / 8)` was the right, non-arbitrary width all along -- self-consistent with the declared `length` by construction. The pre-#467 defect was never the sizing; it was that `identifier` stayed an `int` afterwards and reached `BytesField` unconverted, which `struct.pack()` cannot do anything with. So the six `BytesField` subtypes (IMSI, P_TMSI, EUI_48/64_address, GUTI, DUID) now convert via `identifier.to_bytes(id_len, 'big')`, with the width floored at one octet so that identifier 0 does not collapse into "no identifier" (`bit_length()` is 0 for 0). NAI still rejects an int, and that is a judgement call rather than a mechanical limit: its field is a `StringField`, and `str(identifier)` packs and round-trips perfectly well -- but an NAI is a network access identifier (`user@realm`, RFC 4283), so a bare decimal-digit string is mechanically valid and semantically nonsense. That is the same "accept a value that means the wrong thing" #467 removed, only relocated. The message names `str(...)` so a caller who really wants that can spell it. A negative int is rejected for *every* subtype, and the guard sits before the subtype dispatch rather than inside the int branch. Inside it, the `IPv6_Address` dispatch never reaches the guard, so `identifier=-5, subtype=IPv6_Address` leaked `AddressValueError: -5 (< 0) is not permitted as an IPv6 address` -- a bare ValueError escaping the handler whose whole purpose is to stop that. Measured before the hoist, not inferred. `IPv6_Address` itself is untouched: still `ipaddress.IPv6Address(...)`, which converts and validates, at a fixed 16 octets (#448). Verified through the maker, not a hand-built schema: every one of the eight subtypes x {0, 1, 0xff, 0x1234, 0x100000000, 2**128-1, True} run maker -> pack() -> unpack(), all round-tripping identically, and -5 raising ProtocolError on all eight. Wrong-*type* identifiers (bytes for NAI, str for the octet subtypes, and so on) still escape as bare stdlib exceptions -- unchanged by this PR, tracked separately as #469. tests/protocols/internet/test_mh_unit.py: 37 passed, 318 subtests, 0 failed. Merged origin/main (5182ad0ce) first. --- pcapkit/protocols/internet/mh.py | 113 +++++++++++++------- tests/protocols/internet/test_mh_unit.py | 130 +++++++++++++++++------ 2 files changed, 167 insertions(+), 76 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 7eb11098f5..aee331135e 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7645,22 +7645,26 @@ 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. An :obj:`int` remains accepted for the - ``IPv6_Address`` subtype (converted the same way as any other - value :class:`ipaddress.IPv6Address` accepts), but is rejected - for every other subtype: their fields are variable-length -- - :obj:`str` for ``NAI``, :obj:`bytes` for the rest -- sized from - the wire ``length`` rather than from anything ``subtype`` fixes - on its own, so there is no non-arbitrary width to convert an - :obj:`int` into (c.f. #467). + 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 an :obj:`int` and ``subtype`` - is not ``IPv6_Address``. + ProtocolError: If ``identifier`` is a negative :obj:`int` (no + subtype has a wire form for one), or an :obj:`int` of any value + with the ``NAI`` subtype. """ if option is not None: @@ -7670,6 +7674,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 @@ -7682,37 +7711,39 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption identifier = ipaddress.IPv6Address(identifier) id_len = 16 elif isinstance(identifier, int): - # NOTE: Every other subtype's field is inherently variable-length in - # the schema (c.f. ``mn_id_selector``): a - # :class:`~pcapkit.corekit.fields.strings.StringField` for ``NAI``, a - # :class:`~pcapkit.corekit.fields.strings.BytesField` for the rest -- - # both sized from the packed ``length`` header, not from anything - # ``subtype_val`` fixes on its own. That is unlike ``IPv6_Address``, - # whose 16-octet width is a spec-fixed constant independent of the - # identifier's value. Neither field type converts an ``int`` -- - # ``BytesField`` packs the value as-is and ``StringField`` calls - # ``.encode()`` on it -- so there is no non-arbitrary width to take - # from ``subtype_val`` here: picking one (e.g. from the int's own - # ``bit_length()``, as this branch used to) would just reintroduce - # the type-vs-subtype confusion that produced this defect, only - # without the crash (c.f. #467). Reject instead of silently - # accepting a value that cannot pack. - expected = 'str' if subtype_val == Enum_MNIDSubtype.NAI else 'bytes' - 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``. That would defeat the point of this branch, - # which exists to stop a bare stdlib exception from escaping - # ``_make_opt_mn_id`` in the first place, so it is caught here and - # the raw value is 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 ' - f'{subtype_repr} identifier must be ' - f'{expected}, not int') + 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/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 952384d268..83035a9db6 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2330,64 +2330,124 @@ 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_rejects_int_identifier_for_non_ipv6_subtypes(self) -> None: - """An ``int`` identifier is only meaningful for the ``IPv6_Address`` subtype. + 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`` -- the - same type-vs-subtype confusion #448 fixed for ``IPv6_Address`` -- which - produced a schema that could not be packed for any of the other seven - subtypes, with a declared ``length`` that was wrong either way. Unlike - ``IPv6_Address`` (a spec-fixed 16-octet width, independent of the - identifier's value), every other subtype's field is variable-length - and sized from the packed ``length`` header rather than from anything - ``subtype`` fixes on its own, so there is no non-arbitrary width to - convert an ``int`` into -- it is rejected instead. See #467. - - The rejection message also has to survive an out-of-range ``subtype``: - it round-trips ``subtype_val`` through ``Enum_MNIDSubtype`` for a - friendly name, and that constructor itself raises a bare ``ValueError`` - for a value :meth:`MNIDSubtype._missing_` does not auto-extend (only - 9-15 and 16-255 are). No input to ``_make_opt_mn_id`` may produce a - non-:exc:`~pcapkit.utilities.exceptions.BaseError` exception, so that - is exercised too rather than assumed from the seven real subtypes. + 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) - # ``NAI``'s field is a ``StringField`` (``str``); the other six are all - # ``BytesField`` (``bytes``) via the same generic fallback in - # ``mn_id_selector``. Both groups reject ``int``, but for a different - # reason, so both are exercised rather than assuming the fix generalises. - for subtype in ('NAI', 'IMSI', 'P_TMSI', 'EUI_48_address', - 'EUI_64_address', 'GUTI', 'DUID'): - with self.subTest(subtype): - with self.assertRaises(ProtocolError): + # 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=0x1234) + 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, as #448 fixed. + # 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) # 0, a negative value, and anything above 255 are all outside what - # ``MNIDSubtype._missing_`` extends, so the enum constructor itself - # raises for them -- this must still come out as an in-library - # ``BaseError`` (a ``ProtocolError``, here), never the bare - # ``ValueError`` the naming round-trip would otherwise leak. + # ``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): + 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=0x1234) + Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=-5) self.assertIsInstance(ctx.exception, BaseError) def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: From 6a9e4e427d145427c53ae9e940535217ce17fc06 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 10:20:01 -0400 Subject: [PATCH 4/6] protocols: guard the IPv6_Address identifier's upper bound too (#467) The review of a8c214ab4 found the upper-bound mirror of the leak that sha's own guard had just closed: an int identifier at or above 2**128 with subtype=IPv6_Address still reached ipaddress.IPv6Address() and raised AddressValueError, which subclasses ValueError -- a bare stdlib exception escaping the handler whose purpose is to stop that. Verified before fixing: 2**128 and 2**140 both leaked, while 2**128-1 packed normally at length 17. The bound is subtype-*dependent*, so the check goes inside the IPv6_Address branch rather than into the shared negative-int guard above it: 2**140 is a perfectly good identifier for the six BytesField subtypes, which simply produce more octets (length 19 for 2**140, 18 for 2**128). The test now asserts that too, so a future guard cannot be hoisted to cover them by mistake. Checked explicitly rather than by wrapping the IPv6Address construction, because wrapping would also swallow the wrong-TYPE AddressValueError a str or None produces there -- that is #469's subject and not this PR's to annex. Pre-existing since #448, not a regression from this branch: the construction line is byte-for-byte unchanged from origin/main. How the gap survived the first pass, since it is the more useful part: my probe swept every subtype but stopped at 2**128-1, exactly one value below where the answer changes. Probing a large value is not probing the boundary. Verified: 2**128 and 2**140 now raise ProtocolError for IPv6_Address and still pack for the six BytesField subtypes. Disabling only the new guard condition fails the two new subtests and nothing else; restoring it passes. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed (318 before, +14 new). --- pcapkit/protocols/internet/mh.py | 16 +++++++++++++ tests/protocols/internet/test_mh_unit.py | 30 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index aee331135e..fe96a75156 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7707,6 +7707,22 @@ 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 diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 83035a9db6..24044c926a 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2430,6 +2430,36 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: 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`` From 5cf1740ad49d212810d856bafa62129e3bdb29cf Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 10:50:19 -0400 Subject: [PATCH 5/6] protocols: list the third raising condition in _make_opt_mn_id (#467) Docstring only; no executable line changes. The review of 6a9e4e427 caught that its Raises: clause was left stale by that same commit. It enumerated two conditions -- a negative int, and an int of any value with NAI -- while the guard added three lines below introduces a third: an int of 2**128 or above with IPv6_Address. Three ways to raise ProtocolError, two documented. Verified all three raise ProtocolError (a BaseError) at this sha, rather than reading the code and assuming. The clause now also says why the ceiling exists only for IPv6_Address -- its wire form is a fixed 16 octets, where the other subtypes have no ceiling and simply pack into more -- because the asymmetry is the part a caller would otherwise read as an inconsistency. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed, unchanged. --- pcapkit/protocols/internet/mh.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index fe96a75156..49fb6b1b7f 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7663,8 +7663,11 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption Raises: ProtocolError: If ``identifier`` is a negative :obj:`int` (no - subtype has a wire form for one), or an :obj:`int` of any value - with the ``NAI`` subtype. + 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: From 4bebfd4c0c1563624aca85eed58acc4307964a69 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 11:59:36 -0400 Subject: [PATCH 6/6] protocols: say why the MN-ID schema stub excludes int, since the maker takes one (#467) Comment only; no executable line changes and no annotation changes. The owner asked, reasonably, why the __init__ type stub does not accept int now that the maker does. The answer is that the two annotations describe different boundaries and the old comment did not say so: the maker's `identifier: 'bytes | str | IPv6Address | int'` is what a caller may pass, while the schema's is what the schema can hold, and the maker converts between them before constructing the schema at all. Measured through the maker rather than read off the code: identifier 0x1234 arrives at the schema as b'\x124' for IMSI and DUID and as IPv6Address('::1234') for IPv6_Address. No int ever reaches the constructor, so admitting one in the stub would document a value the schema cannot hold -- and would mislead, because handing a raw int to the StringField or BytesField that mn_id_selector resolves for every subtype but IPv6_Address is exactly the #467 defect: struct.pack() cannot consume it. The previous comment said only "neither of which accepts an int", which was true but read as though int were rejected outright, which is no longer the case anywhere the caller can see. Merged origin/main (f7b5cc5cd, #470) first -- clean, touching only ipaddress.py and its tests, neither owned by this branch. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed, unchanged. --- pcapkit/protocols/schema/internet/mh.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/pcapkit/protocols/schema/internet/mh.py b/pcapkit/protocols/schema/internet/mh.py index 4ddb701713..08b7a64378 100644 --- a/pcapkit/protocols/schema/internet/mh.py +++ b/pcapkit/protocols/schema/internet/mh.py @@ -724,13 +724,24 @@ class MNIDOption(Option, code=Enum_Option.MN_ID_OPTION_TYPE): identifier: 'bytes | str | IPv6Address' = SwitchField(selector=mn_id_selector) if TYPE_CHECKING: - # NOTE: No ``int`` here, matching ``identifier``'s own field annotation - # above -- ``mn_id_selector`` only ever resolves to an - # :class:`~pcapkit.corekit.fields.ipaddress.IPv6AddressField` (which - # would accept one) for the ``IPv6_Address`` subtype; every other - # subtype resolves to a :class:`~pcapkit.corekit.fields.strings.StringField` - # or :class:`~pcapkit.corekit.fields.strings.BytesField`, neither of - # which accepts an ``int`` (c.f. #467). + # 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': ...