From a47d2318664b1f6a333e865b7e2f53cd3f4d9b51 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 28 Sep 2026 19:35:22 -0400 Subject: [PATCH] fix(mh): stop the four RFC-inline helper enums from minting on unassigned bytes Per the owner's ruling on #877, FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LocalizedRoutingStatus and LMAAddressCode stay immutable: none of their RFCs keep an IANA registry, so an unassigned byte is closed rather than pending assignment. - `_missing_` raises a new `EnumValueError` (`pcapkit.utilities.exceptions`, paired with `EnumError` the way `FieldValueError` pairs with `FieldError`) instead of minting an `Unassigned_N` member. Over IPv4 that costs one MH message, via the `@beholder`-decorated `_import_next_layer`. Over IPv6, where Mobility Header chains as one of IPv6's own extension headers, it costs the whole IPv6 packet: the walk's `proto = info.next` (`ipv6.py:338`) has nothing to read off the `Raw` fallback, and that second, unrelated `AttributeError` is what a further-out `@beholder` actually catches. That walk defect is pre-existing -- a truncated or malformed extension header hits it on unmodified `main` too -- but a well-formed packet naming merely an unassigned byte is a far likelier way to reach it. Neither case is a capture-level failure; sibling frames are unaffected either way. - `get()`'s `default=-1` mint path is removed for the two enums that keep `get()`. It now raises the same `EnumValueError` for an unknown key, closing the defect where a second unknown name aliased the first at the shared sentinel `-1` and bypassed `_missing_`'s own range guard. - `LocalizedRoutingStatus.get`/`LMAAddressCode.get` are deleted outright: zero callers repo-wide, tests included. - Two existing tests pinning the old minting/aliasing behaviour are re-pointed to the new contract. A new test proves the aliasing defect, the range invariant and the IPv4 degrade path, each shown to fail without this fix; it also pins the IPv6 whole-packet defect as it stands today, so whoever fixes ipv6.py:338 sees this test and knows to revisit it. tests/protocols/internet/test_mh_unit.py: 51 run, 0 failures, 0 errors. Refs #880 (mh.py half only; ngap.py's ProcedureCode/ProtocolIE are separate, per-key work tracked there) --- pcapkit/protocols/internet/mh.py | 218 +++++++++++++++------- pcapkit/utilities/exceptions.py | 6 +- tests/protocols/internet/test_mh_unit.py | 221 ++++++++++++++++++++--- 3 files changed, 354 insertions(+), 91 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 713bcb7ac..edfec040a 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -27,7 +27,7 @@ import math from typing import TYPE_CHECKING, cast, overload -from aenum import IntEnum, extend_enum +from aenum import IntEnum from pcapkit.const.mh.access_type import AccessType as Enum_AccessType from pcapkit.const.mh.ack_status_code import ACKStatusCode as Enum_ACKStatusCode @@ -487,7 +487,7 @@ UpdateNotificationMessage as Schema_UpdateNotificationMessage from pcapkit.protocols.schema.internet.mh import VendorSpecificOption as Schema_VendorSpecificOption from pcapkit.protocols.schema.schema import Schema -from pcapkit.utilities.exceptions import ProtocolError, UnsupportedCall +from pcapkit.utilities.exceptions import EnumValueError, ProtocolError, UnsupportedCall from pcapkit.utilities.warnings import ProtocolWarning, RegistryWarning, warn if TYPE_CHECKING: @@ -582,6 +582,25 @@ class FastBindingAcknowledgmentStatus(IntEnum): *incorrect interface identifier length* here but *home registration not supported* there. + The enumeration is **closed**: an in-range value :rfc:`5568` leaves + unassigned is not minted a placeholder member. Per the owner's ruling + on GitHub issue #877, this RFC-inline value set stays immutable, so + :meth:`_missing_` raises :exc:`~pcapkit.utilities.exceptions.EnumValueError` + instead of extending the class. That is not a capture-level failure + -- sibling frames are unaffected -- but the cost is bigger than one + message. Mobility Header is itself one of IPv6's own chained + extension headers, and IPv6's own extension-header walk then does + ``proto = info.next`` (:mod:`pcapkit.protocols.internet.ipv6`, line + 338) on whatever :meth:`_import_next_layer` handed back; a ``Raw`` + fallback's info carries no ``.next``. That :exc:`AttributeError`, + not this exception, is what a further-out ``@beholder`` actually + catches -- degrading the **whole IPv6 packet**, header fields + included, to :class:`~pcapkit.protocols.misc.raw.Raw`, rather than + just this one MH message. The walk defect predates this change and + already fires on a malformed extension header; a well-formed packet + naming merely an unassigned byte is simply a more likely way to + reach it. See GitHub issue #880. + """ #: Fast Binding Update accepted [:rfc:`5568#section-6.2.3`] @@ -604,32 +623,47 @@ class FastBindingAcknowledgmentStatus(IntEnum): Incorrect_interface_identifier_length = 131 @staticmethod - def get(key: 'int | str', default: 'int' = -1) -> 'FastBindingAcknowledgmentStatus': + def get(key: 'int | str') -> 'FastBindingAcknowledgmentStatus': """Backport support for original codes. Args: key: Key to get enum item. - default: Default value if not found. + + Returns: + The matching member. + + Raises: + EnumValueError: If ``key`` names no member -- raised for an + unknown name the same way :meth:`_missing_` raises for an + unknown value, so the two ways of getting this wrong do not + report differently. There is no ``default`` parameter: this + enumeration cannot grow, so a default that could only ever be + ignored would be worse than one that is absent. """ if isinstance(key, int): return FastBindingAcknowledgmentStatus(key) - if key not in FastBindingAcknowledgmentStatus._member_map_: # pylint: disable=no-member - extend_enum(FastBindingAcknowledgmentStatus, key, default) - return FastBindingAcknowledgmentStatus[key] # type: ignore[misc] + try: + return FastBindingAcknowledgmentStatus[key] # type: ignore[misc] + except KeyError: + raise EnumValueError('%r is not a valid %s' % + (key, FastBindingAcknowledgmentStatus.__name__)) from None @classmethod - def _missing_(cls, value: 'int') -> 'FastBindingAcknowledgmentStatus': + def _missing_(cls, value: 'int') -> 'NoReturn': """Lookup function used when value is not found. Args: value: Value to get enum item. + Raises: + EnumValueError: Always. :rfc:`5568#section-6.2.3` names this value + set inline with no IANA registry behind it, and the owner's + ruling on GitHub issue #877 is that it stays immutable rather + than minting an ``Unassigned_N`` placeholder member. + """ - if not (isinstance(value, int) and 0 <= value <= 255): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - #: Unassigned - return extend_enum(cls, 'Unassigned_%d' % value, value) + raise EnumValueError('%r is not a valid %s' % (value, cls.__name__)) class IPv6AddressPrefixCode(IntEnum): @@ -646,6 +680,25 @@ class IPv6AddressPrefixCode(IntEnum): discovery IP address/prefix option of :rfc:`5568#section-6.4.1` is likewise unregistered. + The enumeration is **closed**: an in-range value :rfc:`5568` leaves + unassigned is not minted a placeholder member. Per the owner's ruling + on GitHub issue #877, this RFC-inline value set stays immutable, so + :meth:`_missing_` raises :exc:`~pcapkit.utilities.exceptions.EnumValueError` + instead of extending the class. That is not a capture-level failure + -- sibling frames are unaffected -- but the cost is bigger than one + message. Mobility Header is itself one of IPv6's own chained + extension headers, and IPv6's own extension-header walk then does + ``proto = info.next`` (:mod:`pcapkit.protocols.internet.ipv6`, line + 338) on whatever :meth:`_import_next_layer` handed back; a ``Raw`` + fallback's info carries no ``.next``. That :exc:`AttributeError`, + not this exception, is what a further-out ``@beholder`` actually + catches -- degrading the **whole IPv6 packet**, header fields + included, to :class:`~pcapkit.protocols.misc.raw.Raw`, rather than + just this one MH message. The walk defect predates this change and + already fires on a malformed extension header; a well-formed packet + naming merely an unassigned byte is simply a more likely way to + reach it. See GitHub issue #880. + """ #: Old Care-of Address [:rfc:`5568#section-6.4.2`] @@ -662,32 +715,47 @@ class IPv6AddressPrefixCode(IntEnum): NAR_Prefix = 4 @staticmethod - def get(key: 'int | str', default: 'int' = -1) -> 'IPv6AddressPrefixCode': + def get(key: 'int | str') -> 'IPv6AddressPrefixCode': """Backport support for original codes. Args: key: Key to get enum item. - default: Default value if not found. + + Returns: + The matching member. + + Raises: + EnumValueError: If ``key`` names no member -- raised for an + unknown name the same way :meth:`_missing_` raises for an + unknown value, so the two ways of getting this wrong do not + report differently. There is no ``default`` parameter: this + enumeration cannot grow, so a default that could only ever be + ignored would be worse than one that is absent. """ if isinstance(key, int): return IPv6AddressPrefixCode(key) - if key not in IPv6AddressPrefixCode._member_map_: # pylint: disable=no-member - extend_enum(IPv6AddressPrefixCode, key, default) - return IPv6AddressPrefixCode[key] # type: ignore[misc] + try: + return IPv6AddressPrefixCode[key] # type: ignore[misc] + except KeyError: + raise EnumValueError('%r is not a valid %s' % + (key, IPv6AddressPrefixCode.__name__)) from None @classmethod - def _missing_(cls, value: 'int') -> 'IPv6AddressPrefixCode': + def _missing_(cls, value: 'int') -> 'NoReturn': """Lookup function used when value is not found. Args: value: Value to get enum item. + Raises: + EnumValueError: Always. :rfc:`5568#section-6.4.2` names this value + set inline with no IANA registry behind it, and the owner's + ruling on GitHub issue #877 is that it stays immutable rather + than minting an ``Unassigned_N`` placeholder member. + """ - if not (isinstance(value, int) and 0 <= value <= 255): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - #: Unassigned - return extend_enum(cls, 'Unassigned_%d' % value, value) + raise EnumValueError('%r is not a valid %s' % (value, cls.__name__)) class LocalizedRoutingStatus(IntEnum): @@ -705,6 +773,31 @@ class LocalizedRoutingStatus(IntEnum): registered :class:`~pcapkit.const.mh.status_code.StatusCode`, whose ``128`` and ``129`` mean something else entirely. + The enumeration is **closed**: an in-range value :rfc:`6705` leaves + unassigned is not minted a placeholder member. Per the owner's ruling + on GitHub issue #877, this RFC-inline value set stays immutable, so + :meth:`_missing_` raises :exc:`~pcapkit.utilities.exceptions.EnumValueError` + instead of extending the class. That is not a capture-level failure + -- sibling frames are unaffected -- but the cost is bigger than one + message. Mobility Header is itself one of IPv6's own chained + extension headers, and IPv6's own extension-header walk then does + ``proto = info.next`` (:mod:`pcapkit.protocols.internet.ipv6`, line + 338) on whatever :meth:`_import_next_layer` handed back; a ``Raw`` + fallback's info carries no ``.next``. That :exc:`AttributeError`, + not this exception, is what a further-out ``@beholder`` actually + catches -- degrading the **whole IPv6 packet**, header fields + included, to :class:`~pcapkit.protocols.misc.raw.Raw`, rather than + just this one MH message. The walk defect predates this change and + already fires on a malformed extension header; a well-formed packet + naming merely an unassigned byte is simply a more likely way to + reach it. See GitHub issue #880. + + There is no ``get()`` backport here, unlike + :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode`: it had zero callers repo-wide -- tests + included -- so GitHub issue #880 deleted it outright rather than + rebuilding it on the immutable contract. + """ #: Success [:rfc:`6705#section-10.2`] @@ -716,33 +809,21 @@ class LocalizedRoutingStatus(IntEnum): #: MN Not Attached [:rfc:`6705#section-10.2`] MN_Not_Attached = 129 - @staticmethod - def get(key: 'int | str', default: 'int' = -1) -> 'LocalizedRoutingStatus': - """Backport support for original codes. - - Args: - key: Key to get enum item. - default: Default value if not found. - - """ - if isinstance(key, int): - return LocalizedRoutingStatus(key) - if key not in LocalizedRoutingStatus._member_map_: # pylint: disable=no-member - extend_enum(LocalizedRoutingStatus, key, default) - return LocalizedRoutingStatus[key] # type: ignore[misc] - @classmethod - def _missing_(cls, value: 'int') -> 'LocalizedRoutingStatus': + def _missing_(cls, value: 'int') -> 'NoReturn': """Lookup function used when value is not found. Args: value: Value to get enum item. + Raises: + EnumValueError: Always. :rfc:`6705#section-10.2` names this value + set inline with no IANA registry behind it, and the owner's + ruling on GitHub issue #877 is that it stays immutable rather + than minting an ``Unassigned_N`` placeholder member. + """ - if not (isinstance(value, int) and 0 <= value <= 255): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - #: Unassigned - return extend_enum(cls, 'Unassigned_%d' % value, value) + raise EnumValueError('%r is not a valid %s' % (value, cls.__name__)) class LMAAddressCode(IntEnum): @@ -756,6 +837,31 @@ class LMAAddressCode(IntEnum): registry of them, so the enumeration lives here rather than in :mod:`pcapkit.const.mh`. + The enumeration is **closed**: an in-range value :rfc:`5949` leaves + unassigned is not minted a placeholder member. Per the owner's ruling + on GitHub issue #877, this RFC-inline value set stays immutable, so + :meth:`_missing_` raises :exc:`~pcapkit.utilities.exceptions.EnumValueError` + instead of extending the class. That is not a capture-level failure + -- sibling frames are unaffected -- but the cost is bigger than one + message. Mobility Header is itself one of IPv6's own chained + extension headers, and IPv6's own extension-header walk then does + ``proto = info.next`` (:mod:`pcapkit.protocols.internet.ipv6`, line + 338) on whatever :meth:`_import_next_layer` handed back; a ``Raw`` + fallback's info carries no ``.next``. That :exc:`AttributeError`, + not this exception, is what a further-out ``@beholder`` actually + catches -- degrading the **whole IPv6 packet**, header fields + included, to :class:`~pcapkit.protocols.misc.raw.Raw`, rather than + just this one MH message. The walk defect predates this change and + already fires on a malformed extension header; a well-formed packet + naming merely an unassigned byte is simply a more likely way to + reach it. See GitHub issue #880. + + There is no ``get()`` backport here, unlike + :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode`: it had zero callers repo-wide -- tests + included -- so GitHub issue #880 deleted it outright rather than + rebuilding it on the immutable contract. + """ #: Reserved [:rfc:`5949#section-6.2.2`] @@ -767,33 +873,21 @@ class LMAAddressCode(IntEnum): #: IPv4 local mobility anchor address [:rfc:`5949#section-6.2.2`] IPv4_LMAA = 2 - @staticmethod - def get(key: 'int | str', default: 'int' = -1) -> 'LMAAddressCode': - """Backport support for original codes. - - Args: - key: Key to get enum item. - default: Default value if not found. - - """ - if isinstance(key, int): - return LMAAddressCode(key) - if key not in LMAAddressCode._member_map_: # pylint: disable=no-member - extend_enum(LMAAddressCode, key, default) - return LMAAddressCode[key] # type: ignore[misc] - @classmethod - def _missing_(cls, value: 'int') -> 'LMAAddressCode': + def _missing_(cls, value: 'int') -> 'NoReturn': """Lookup function used when value is not found. Args: value: Value to get enum item. + Raises: + EnumValueError: Always. :rfc:`5949#section-6.2.2` names this value + set inline with no IANA registry behind it, and the owner's + ruling on GitHub issue #877 is that it stays immutable rather + than minting an ``Unassigned_N`` placeholder member. + """ - if not (isinstance(value, int) and 0 <= value <= 255): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - #: Unassigned - return extend_enum(cls, 'Unassigned_%d' % value, value) + raise EnumValueError('%r is not a valid %s' % (value, cls.__name__)) class MH(Internet[Data_MH, Schema_MH], diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 633485f98..dace8303b 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -43,7 +43,7 @@ 'FileNotFound', # FileNotFoundError 'ProtocolNotFound', # IndexError 'VersionError', 'IndexNotFound', 'ProtocolError', # ValueError - 'EndianError', 'KeyExists', 'NoDefaultValue', # ValueError + 'EndianError', 'KeyExists', 'NoDefaultValue', 'EnumValueError', # ValueError 'FieldValueError', 'SchemaError', 'SeekError', 'TruncateError', # ValueError 'VendorPathNotFound', # ValueError 'ProtocolNotImplemented', 'VendorNotImplemented', # NotImplementedError @@ -371,6 +371,10 @@ class NoDefaultValue(BaseError, ValueError): """No default value.""" +class EnumValueError(BaseError, ValueError): + """No member of a closed enumeration carries this value.""" + + class FieldValueError(BaseError, ValueError): """Invalid field value.""" diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 10a5d2430..2365e9843 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -1362,13 +1362,6 @@ def test_mh_fmipv6_round_trip_is_byte_identical(self) -> None: }, {'status': FastBindingAcknowledgmentStatus.Incorrect_interface_identifier_length, 'key_mngt': True, 'seq': 0x1234, 'lifetime': datetime.timedelta(seconds=40)}), - # ... and as a bare integer, for a value the RFC leaves unassigned - ('FBack, unassigned status', Packet.Fast_Binding_Acknowledgment, { - 'status': 77, 'key_mngt': False, 'seq': 1, 'lifetime': 4, - 'options': [(acoa_type, {'address': '2001:db8:1::3'}), - (badf_type, {'spi': 1, 'data': bytes(range(12))})], - }, {'status': 77, 'key_mngt': False, 'seq': 1, - 'lifetime': datetime.timedelta(seconds=4)}), ('FNA', Packet.Fast_Neighbor_Advertisement, { 'options': [(mhlla_type, {'address': b'\x00\x11\x22\x33\x44'})], }, {}), @@ -1415,13 +1408,42 @@ def test_mh_fmipv6_round_trip_is_byte_identical(self) -> None: chksum=parsed.chksum, data=parsed)) self.assertEqual(rebuilt, raw) + with self.subTest('FBack, unassigned status now raises on construction'): + # This used to be in ``cases`` above, round-tripping a bare int for + # a status value RFC 5568 leaves unassigned. Constructing from + # ``data=`` builds the wire bytes and then reads them straight + # back to populate ``.info`` -- that read-back is what used to + # mint ``Unassigned_77`` harmlessly. Per the owner's ruling on + # GitHub issue #877 (see + # ``test_mh_rfc5568_local_enums_cover_unregistered_value_sets``), + # the closed :class:`FastBindingAcknowledgmentStatus` now raises + # instead, and it does so on construction itself, before there is + # any wire-format bytes object to round-trip at all. + from pcapkit.utilities.exceptions import EnumValueError + + with self.assertRaises(EnumValueError): + MH(next=TransType.UDP, chksum=b'\x12\x34', + type=Packet.Fast_Binding_Acknowledgment, + data={'status': 77, 'key_mngt': False, 'seq': 1, 'lifetime': 4, + 'options': [(acoa_type, {'address': '2001:db8:1::3'}), + (badf_type, {'spi': 1, 'data': bytes(range(12))})]}) + def test_mh_rfc5568_local_enums_cover_unregistered_value_sets(self) -> None: """:rfc:`5568` defines two value sets inline, with no IANA registry. Both are therefore enumerated in :mod:`pcapkit.protocols.internet.mh` - itself rather than in :mod:`pcapkit.const.mh`, and both have to tolerate - the values the RFC leaves unassigned -- a capture in the wild carries - whatever it carries, so an unassigned code must parse rather than raise. + itself rather than in :mod:`pcapkit.const.mh`. Per the owner's ruling + on GitHub issue #877, both stay **closed**: GitHub issue #880 found + that a value the RFC leaves unassigned used to be minted a + ``Unassigned_N`` placeholder member -- through a path that bypassed + ``_missing_``'s own ``0 <= value <= 255`` guard and, for an unknown + *name*, aliased a second unknown key onto the first at a shared ``-1`` + sentinel. Both enums now raise instead of minting, so an unassigned + code degrades the MH parse to :class:`~pcapkit.protocols.misc.raw.Raw` + via ``@beholder`` rather than silently growing the class -- one + message's worth of damage over IPv4, the whole packet's over IPv6 -- + see ``test_mh_local_enums_raise_and_do_not_alias`` for that + distinction and the other two RFC-inline enums. """ from pcapkit.const.mh.option import Option from pcapkit.const.mh.packet import Packet @@ -1429,14 +1451,17 @@ def test_mh_rfc5568_local_enums_cover_unregistered_value_sets(self) -> None: from pcapkit.const.reg.transtype import TransType from pcapkit.protocols.internet.mh import (MH, FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode) + from pcapkit.utilities.exceptions import EnumValueError apfx_type = Option.Mobility_Header_IPv6_Address_Prefix badf_type = Option.Binding_Authorization_Data_for_FMIPv6 acoa_type = Option.Alternate_Care_of_Address mhlla_type = Option.Mobility_Header_Link_Layer_Address_option - # The membership assertions come first: ``_missing_`` extends the class in - # place, so any unassigned lookup below would otherwise show up here. + # The membership assertions come first, before any lookup that raises + # gets a chance to run: ``_missing_`` is closed now, but pinning the + # declared set up front keeps this test honest about exactly what is + # -- and is not -- a member, independent of the raise behaviour below. with self.subTest('FBack status members, RFC 5568 section 6.2.3'): self.assertEqual( {int(member): member.name for member in FastBindingAcknowledgmentStatus}, @@ -1500,45 +1525,43 @@ def handover_initiate(code: int) -> object: ]: self.assertIs(handover_initiate(wire).options[apfx_type].code, member, wire) - with self.subTest('unassigned values parse without raising'): + with self.subTest('unassigned values raise instead of minting'): # 2..127 is the unassigned half of the "accepted" range, 132..255 of - # the "rejected" one; both must survive a parse. + # the "rejected" one; both must now raise rather than parse. for wire in (2, 77, 127, 132, 255): - status = fback(wire).status - self.assertIsInstance(status, FastBindingAcknowledgmentStatus) - self.assertEqual(status, wire) - self.assertEqual(status.name, 'Unassigned_%d' % wire) + with self.assertRaises(EnumValueError): + fback(wire) # RFC 5568 assigns 1 through 4 only, so 0 and 5..255 are unassigned. for wire in (0, 5, 200, 255): - code = handover_initiate(wire).options[apfx_type].code - self.assertIsInstance(code, IPv6AddressPrefixCode) - self.assertEqual(code, wire) - self.assertEqual(code.name, 'Unassigned_%d' % wire) + with self.assertRaises(EnumValueError): + handover_initiate(wire) with self.subTest('out-of-octet values are still rejected'): for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): for value in (-1, 256): - with self.assertRaises(ValueError): + with self.assertRaises(EnumValueError): enum_cls(value) - with self.subTest('get() backports string and integer lookups'): + with self.subTest('get() backports string and integer lookups, and no longer mints'): self.assertIs(FastBindingAcknowledgmentStatus.get(130), FastBindingAcknowledgmentStatus.Insufficient_resources) self.assertIs(FastBindingAcknowledgmentStatus.get('Reason_unspecified'), FastBindingAcknowledgmentStatus.Reason_unspecified) - self.assertEqual(FastBindingAcknowledgmentStatus.get('Vendor_specific', 200), 200) + with self.assertRaises(EnumValueError): + FastBindingAcknowledgmentStatus.get('Vendor_specific') self.assertIs(IPv6AddressPrefixCode.get(4), IPv6AddressPrefixCode.NAR_Prefix) self.assertIs(IPv6AddressPrefixCode.get('New_Care_of_Address'), IPv6AddressPrefixCode.New_Care_of_Address) - self.assertEqual(IPv6AddressPrefixCode.get('Vendor_specific', 201), 201) + with self.assertRaises(EnumValueError): + IPv6AddressPrefixCode.get('Vendor_specific') with self.subTest('make round trips both enums byte-identically'): for status, code in [ (FastBindingAcknowledgmentStatus.Insufficient_resources, IPv6AddressPrefixCode.NAR_Prefix), - # unassigned on the wire, and handed in as a bare integer - (99, 250), + (FastBindingAcknowledgmentStatus.Fast_Binding_Update_accepted_but_NCoA_is_invalid, + IPv6AddressPrefixCode.Old_Care_of_Address), ]: fback_raw = bytes(MH(next=TransType.UDP, chksum=b'\x12\x34', type=Packet.Fast_Binding_Acknowledgment, @@ -1570,6 +1593,148 @@ def handover_initiate(code: int) -> object: self.assertEqual(bytes(MH(next=parsed.next, type=parsed.type, chksum=parsed.chksum, data=parsed)), hi_raw) + def test_mh_local_enums_raise_and_do_not_alias(self) -> None: + """GitHub issue #880: the four RFC-inline helper enums stay immutable. + + Companion to :meth:`test_mh_rfc5568_local_enums_cover_unregistered_value_sets`, + which covers :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode`. This one covers the two enums that + never had wire-level coverage in that test -- + :class:`LocalizedRoutingStatus` (:rfc:`6705#section-10.2`) and + :class:`LMAAddressCode` (:rfc:`5949#section-6.2.2`) -- plus three + properties the issue measured that are not specific to any one of the + four: the ``get()``-level aliasing defect, the ``_missing_`` range + invariant, and the consequence of the raise once it reaches a real + capture -- which turns out to differ by wrapping protocol, see the + last two subtests. + """ + import socket + import struct + + from pcapkit.const.mh.packet import Packet + from pcapkit.const.reg.transtype import TransType + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.internet.mh import (MH, FastBindingAcknowledgmentStatus, + IPv6AddressPrefixCode, LMAAddressCode, + LocalizedRoutingStatus) + from pcapkit.protocols.link.ethernet import Ethernet + from pcapkit.protocols.misc.raw import Raw + from pcapkit.utilities.exceptions import EnumValueError + + with self.subTest('LocalizedRoutingStatus and LMAAddressCode raise and have no get()'): + # Both had zero callers repo-wide (tests included), so GitHub issue + # #880 deleted ``get()`` outright rather than rebuilding it. + for enum_cls, unassigned in ((LocalizedRoutingStatus, 50), (LMAAddressCode, 50)): + with self.assertRaises(EnumValueError): + enum_cls(unassigned) + self.assertFalse(hasattr(enum_cls, 'get')) + + with self.subTest('the shared -1 aliasing defect is gone'): + # Before the fix, ``get('bogus_one')`` minted a member at the + # shared sentinel -1, and ``get('bogus_two')`` then resolved to + # that same member -- a name that lies about itself. Both calls + # must now raise independently, and neither may touch the + # class's own lookup tables while doing so. + for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): + before_members = dict(enum_cls._member_map_) # pylint: disable=no-member + before_values = dict(enum_cls._value2member_map_) # pylint: disable=no-member + with self.assertRaises(EnumValueError): + enum_cls.get('bogus_one') + with self.assertRaises(EnumValueError): + enum_cls.get('bogus_two') + self.assertEqual(enum_cls._member_map_, before_members) # pylint: disable=no-member + self.assertEqual(enum_cls._value2member_map_, before_values) # pylint: disable=no-member + + with self.subTest("_missing_'s range invariant holds for all four"): + # ``extend_enum`` used to install a member at an explicit value + # without consulting ``_missing_`` at all, which is how a value + # the class's own guard rejects (e.g. -1) ended up a member of it. + # With no minting path left, no lookup -- in range or out of it -- + # may change the declared member set. + for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, + LocalizedRoutingStatus, LMAAddressCode): + before = len(list(enum_cls)) + for bad_value in (-1, 50, 256): + with self.assertRaises(EnumValueError): + enum_cls(bad_value) + self.assertEqual(len(list(enum_cls)), before) + self.assertTrue(all(0 <= int(member) <= 255 for member in enum_cls)) + + with self.subTest('over IPv4, an unassigned wire byte degrades just that one MH layer'): + # All four are read by direct construction from an unvalidated + # wire byte under ``_import_next_layer``, which is + # ``@beholder``-decorated and re-parses the layer as ``Raw`` on + # any exception. Over IPv4, which dispatches to Mobility Header + # through the base, non-chain-walking ``_import_next_layer``, + # that is the whole story: the raise costs one MH message, not + # the packet or the capture. IPv4 is deliberately the wrapping + # protocol in *this* subtest -- the next one shows IPv6 does not + # get off this lightly. + def wrap_in_ipv4(mh_payload: bytes) -> IPv4: + header = struct.pack( + '>BBHHHBBH4s4s', 0x45, 0, 20 + len(mh_payload), 0, 0, 64, + int(TransType.Mobility_Header), 0, + socket.inet_aton('127.0.0.1'), socket.inet_aton('127.0.0.1'), + ) + raw = header + mh_payload + return IPv4(io.BytesIO(raw), len(raw)) + + fback_raw = bytearray(bytes(MH( + next=TransType.UDP, chksum=b'\x12\x34', type=Packet.Fast_Binding_Acknowledgment, + data={'status': FastBindingAcknowledgmentStatus.Insufficient_resources, + 'key_mngt': True, 'seq': 0x1234, 'lifetime': 40, 'options': []}))) + fback_raw[6] = 50 # unassigned FastBindingAcknowledgmentStatus byte + ip = wrap_in_ipv4(bytes(fback_raw)) + self.assertIsInstance(ip.payload, Raw) + self.assertEqual(bytes(ip.payload), bytes(fback_raw)) + + lra_raw = bytearray(bytes(MH( + next=TransType.UDP, chksum=b'\x12\x34', type=Packet.Localized_Routing_Acknowledgment, + data={'seq': 4, 'unsolicited': True, + 'status': LocalizedRoutingStatus.Localized_Routing_Not_Allowed, + 'lifetime': 600}))) + lra_raw[9] = 50 # unassigned LocalizedRoutingStatus byte + ip = wrap_in_ipv4(bytes(lra_raw)) + self.assertIsInstance(ip.payload, Raw) + self.assertEqual(bytes(ip.payload), bytes(lra_raw)) + + with self.subTest('over IPv6, the same raise costs the whole packet (pre-existing defect)'): + # Mobility Header is *also* one of IPv6's own chained extension + # headers (``Enum_ExtensionHeader.Mobility_Header``, value 135 -- + # ``pcapkit/const/ipv6/extension_header.py:42``), so IPv6 never + # reaches the fallback through the base ``_import_next_layer`` at + # all. It walks its own ``@beholder``-decorated override in + # ``_decode_next_layer``, gets a ``Raw`` fallback back for the MH + # slot exactly as IPv4 does, and then does + # ``proto = info.next`` (``pcapkit/protocols/internet/ipv6.py:338``) + # on it regardless. ``Raw``'s info carries no ``.next``, so *that* + # line raises ``AttributeError`` inside ``IPv6.read`` -- and it is + # this second, unrelated exception that a further-out + # ``@beholder`` (Ethernet's, here) actually catches, degrading + # the whole IPv6 packet -- header fields and the MH message + # both -- to ``Raw``. + # + # This walk defect predates this fix and already fires on a + # truncated or otherwise malformed extension header on + # unmodified ``main``; what this fix adds is a route to it from + # a *well-formed* packet that merely names a byte its RFC leaves + # unassigned, which is a far likelier way to arrive here than a + # malformed header. This subtest pins that as it stands today, + # deliberately, so that whoever fixes ipv6.py:338 sees this + # assertion fail and knows to revisit it -- it is not a claim + # that losing the whole packet is the desired behaviour. + def wrap_in_ethernet_ipv6(mh_payload: bytes) -> Ethernet: + ipv6_header = struct.pack( + '>IHBB', 6 << 28, len(mh_payload), int(TransType.Mobility_Header), 64, + ) + (b'\x00' * 15 + b'\x01') * 2 + eth_header = bytes.fromhex('001122334455' '667788990011') + struct.pack('>H', 0x86DD) + raw = eth_header + ipv6_header + mh_payload + return Ethernet(io.BytesIO(raw), len(raw)) + + eth = wrap_in_ethernet_ipv6(bytes(fback_raw)) + self.assertIsInstance(eth.payload, Raw) + self.assertEqual(str(eth.protochain), 'Ethernet:Internet_Protocol_version_6') + def test_mh_message_flags_pack_each_bit_independently(self) -> None: """Regression test: a cleared flag must not be emitted as a set bit.