From 6e2387ba896f4f7cf624031793ff0ce035e89699 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 02:01:03 -0400 Subject: [PATCH] fix(corekit): reject a bool where a maker converts an address itself (#508) - add `parse_ip_address()` to `pcapkit.corekit.fields.ipaddress`, which calls the existing `_reject_bool` before converting, and takes an optional `version` so a caller that pins the address family widens an `int` to the right one - route the seven `_make_*` sites that convert a caller-supplied address *before* the schema is built through it: `MH._make_opt_bid`, `MH._make_opt_lmaa`, `MH._make_fid_suboption`, `MH._make_opt_dmnp`, `MH._make_opt_lma_up`, `HIP._make_param_locator_set` and `TCP._make_mptcp_addaddr`. Each derives its option length or family flag from the converted address, so a bare `ipaddress.ip_address(True)` became `0.0.0.1` and #500's field-level guard could no longer tell it from a real address - leave `SwitchField` untouched: its `pre_process` delegates to the resolved field and is not reached on the pack path at all, so the guard #508 proposes putting there would be dead code - drop the now-unused `import ipaddress` from `tcp.py`, and document `parse_ip_address` on the ipaddress fields page Adds tests over three files, including one that re-derives the ten address-typed `SwitchField` declarations from `Schema.__fields__` so a new one cannot be added unnoticed. CI-equivalent unit suite: 1056 passed, with only the pre-existing `test_docstring_contract` failure that main already has. --- .../pcapkit/corekit/fields/ipaddress.rst | 5 + pcapkit/corekit/fields/ipaddress.py | 112 +++++- pcapkit/protocols/internet/hip.py | 18 +- pcapkit/protocols/internet/mh.py | 60 ++- pcapkit/protocols/transport/tcp.py | 15 +- tests/corekit/test_fields_ipaddress.py | 301 +++++++++++++++ tests/corekit/test_fields_misc.py | 352 ++++++++++++++++++ tests/protocols/internet/test_mh_unit.py | 274 ++++++++++++++ 8 files changed, 1114 insertions(+), 23 deletions(-) diff --git a/docs/source/pcapkit/corekit/fields/ipaddress.rst b/docs/source/pcapkit/corekit/fields/ipaddress.rst index d9fd8d7d4c..d14d112ebd 100644 --- a/docs/source/pcapkit/corekit/fields/ipaddress.rst +++ b/docs/source/pcapkit/corekit/fields/ipaddress.rst @@ -25,6 +25,11 @@ IP Interface :members: :show-inheritance: +Construction Helpers +~~~~~~~~~~~~~~~~~~~~ + +.. autofunction:: pcapkit.corekit.fields.ipaddress.parse_ip_address + Internal Definitions ~~~~~~~~~~~~~~~~~~~~ diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index 5cd5a8e988..27cf8dbe74 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -12,11 +12,12 @@ __all__ = [ 'IPv4AddressField', 'IPv6AddressField', 'IPv4InterfaceField', 'IPv6InterfaceField', + 'parse_ip_address', ] if TYPE_CHECKING: from ipaddress import IPv4Address, IPv4Interface, IPv6Address, IPv6Interface - from typing import Any, Callable, Iterator + from typing import Any, Callable, Iterator, Optional from typing_extensions import Literal, Self @@ -98,6 +99,12 @@ def _reject_bool(value: 'object', description: str) -> 'None': wrong position does not fire, and that placement mistake has already been made twice in this repository's history. + Guarding the field classes is necessary but not sufficient, because a + ``_make_*`` that must know the address family before it can build the + schema converts the argument itself and so never hands this module a + :obj:`bool` at all. :func:`parse_ip_address` is where those callers + reach this guard (c.f. #508). + """ if isinstance(value, bool): raise FieldValueError( @@ -105,6 +112,109 @@ def _reject_bool(value: 'object', description: str) -> 'None': f'int({value!r}) if the numeric value is what is wanted') +def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str', + description: str, + version: 'Optional[int]' = None) -> 'IPv4Address | IPv6Address': + """Convert a caller-supplied address on the **construction** path. + + Args: + value: Address as the caller gave it -- an :mod:`ipaddress` object, + which is returned unchanged, or anything :mod:`ipaddress` accepts. + description: Human-readable description of what ``value`` is, used to + build the :exc:`FieldValueError` message. Callers in a protocol + should carry their usual context into it, e.g. + ``f'{self.alias}: [OptNo {type}] care-of address'``. + version: IP version to demand, ``4`` or ``6``, or :obj:`None` to take + whichever family ``value`` describes. Pass it where the wire format + fixes the family, so that an :class:`int` is widened to the right + one -- ``258`` is ``::102`` for ``version=6`` but ``0.0.1.2`` for + :func:`ipaddress.ip_address`. + + Returns: + The converted address. + + Raises: + FieldValueError: If ``value`` is a :obj:`bool` (c.f. + :func:`_reject_bool`), is not a valid IP address, or is not of + ``version``. + + Notes: + This is the sanctioned way for a ``_make_*`` method to turn a + caller-supplied address into an :mod:`ipaddress` object, and it exists + because doing it with :func:`ipaddress.ip_address` directly is what + #508 turned out to be: a ``_make_*`` that has to know the address + *family* before it can build the schema -- to size an option whose + length is the only thing on the wire that carries the family -- must + convert the argument itself, and that conversion happens **before** the + schema, so it launders a :obj:`bool` into an + :class:`~ipaddress.IPv4Address` that #500's guard in + :meth:`_IPAddressField.pre_process` can then only see as a legitimate + address. Seven such call sites took ``True`` / ``False`` without + complaint as ``0.0.0.1`` / ``0.0.0.0`` -- or ``::1`` / ``::`` where the + wire format fixes the family as IPv6 -- and six of them went on to pack + those octets. The seventh, + :meth:`TCP._make_mptcp_addaddr + `, built an + equally corrupt schema and is only stopped from packing it by an + unrelated defect of its own. + + Routing every one of them through here rather than giving each its own + :func:`isinstance` check is the whole point: #481 added exactly such a + check to :meth:`MH._make_opt_mn_id + `, and #491 was the + same defect surviving at every site that had not been thought of. A + guard that has to be remembered per call site is a guard that will be + forgotten at the next one. + + The :obj:`bool` rejection is the **first** statement here, ahead of any + dispatch on the value's type, for the placement reason #481 gives and + :func:`_reject_bool` repeats. + + This raises :exc:`FieldValueError` and not + :exc:`~pcapkit.utilities.exceptions.ProtocolError`, which is deliberate + even though two sibling guards for the same mistake -- + :meth:`MH._make_opt_mn_id + ` from #481 and + :class:`ESP's SecurityAssociation + ` from #491 -- raise + the latter. The layer decides: this is a field-level conversion, so it + answers with what :meth:`_IPAddressField.pre_process` answers with for + the identical value, and a caller sees one exception whether the + :obj:`bool` reached the field through the schema or through a + ``_make_*``. The two protocol-level guards answer for the *option*, + alongside siblings that are not about addresses at all -- + ``_make_opt_mn_id`` refuses a :obj:`bool` for all eight MN-ID subtypes, + only one of which is address-typed -- so neither can route through here + without losing the subtype-aware message that is the point of it. Both + exception classes derive from + :exc:`~pcapkit.utilities.exceptions.BaseError` *and* :exc:`ValueError`, + so the difference is invisible to ``except BaseError`` and ``except + ValueError``, and nothing in the library catches either one + specifically. + + """ + _reject_bool(value, description) + + if isinstance(value, (ipaddress.IPv4Address, ipaddress.IPv6Address)): + ip = value # type: IPv4Address | IPv6Address + else: + with _reraise_as_field_value_error(description): + if version == 4: + ip = ipaddress.IPv4Address(value) + elif version == 6: + ip = ipaddress.IPv6Address(value) + else: + ip = ipaddress.ip_address(value) + + # NOTE: Checked outside the ``with`` block above, and after it, because an + # :mod:`ipaddress` object taken from the branch that skips the conversion + # has not been version-checked at all -- ``IPv6Address(IPv4Address(...))`` + # would have raised, but returning the object unchanged cannot. + if version is not None and ip.version != version: + raise FieldValueError(f'{description}: IP version mismatch: {ip.version} != {version}') + return ip + + class _IPField(Field[_T], Generic[_T]): """Internal IP related value for protocol fields. diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index de25570e01..0ba95de477 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -50,6 +50,7 @@ from pcapkit.const.hip.packet import Packet as Enum_Packet from pcapkit.const.hip.parameter import Parameter as Enum_Parameter from pcapkit.const.reg.transtype import TransType as Enum_TransType +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.protocols.data.internet.hip import HIP as Data_HIP from pcapkit.protocols.data.internet.hip import AckDataParameter as Data_AckDataParameter @@ -3067,14 +3068,27 @@ def _make_locator(locator: 'Optional[Data_Locator]' = None, *, preferred = locator.preferred lifetime = math.floor(locator.lifetime.total_seconds()) else: + # NOTE: Through ``parse_ip_address`` because the locator is packed + # to octets *here*, ahead of the schema, so a bare + # ``ipaddress.IPv6Address`` would launder a ``bool`` into ``::1`` + # and hand the schema's ``SwitchField`` plain bytes that its guard + # cannot question. Before this, ``ip=True`` packed a locator of + # ``::1`` with no error at all. The ``version=6`` argument is the + # *IP* version rather than the HIP one the message names, and it is + # what keeps the ``int`` widening this signature documents -- + # ``0x102`` is ``::102``, not the ``0.0.1.2`` that + # ``ipaddress.ip_address`` would give (c.f. #508). + ip_val = parse_ip_address( + ip, f'HIPv{version}: [ParamNo {code}] invalid locator', version=6) + if spi is None: length = 4 - data = ipaddress.IPv6Address(ip).packed + data = ip_val.packed else: length = 5 data = Schema_LocatorData( spi=spi, - ip=ipaddress.IPv6Address(ip).packed, + ip=ip_val.packed, ) if isinstance(lifetime, timedelta): diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 2ef8885843..5cd1918f1c 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -77,6 +77,7 @@ UpdateNotificationACKStatus as Enum_UpdateNotificationACKStatus from pcapkit.const.mh.upn_reason import UpdateNotificationReason as Enum_UpdateNotificationReason from pcapkit.const.reg.transtype import TransType as Enum_TransType +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.protocols.data.internet.mh import MH as Data_MH from pcapkit.protocols.data.internet.mh import \ @@ -8873,8 +8874,16 @@ def _make_opt_bid(self, type: 'Enum_Option', option: 'Optional[Data_BindingIdent if address is None: length = 4 else: - addr = ipaddress.ip_address(address) if not isinstance( - address, (ipaddress.IPv4Address, ipaddress.IPv6Address)) else address + # NOTE: Converted through ``parse_ip_address`` rather than with + # ``ipaddress.ip_address``, because the conversion happens *here* -- + # the option length below is derived from the family, so it cannot + # wait for the schema -- and a bare conversion therefore turns a + # ``bool`` into a perfectly ordinary ``IPv4Address`` that the + # schema's own guard can no longer tell from a real address. Before + # this, ``address=True`` packed as ``23080001000000000001``, i.e. a + # care-of address of ``0.0.0.1`` (c.f. #508). + addr = parse_ip_address( + address, f'{self.alias}: [OptNo {type}] invalid care-of address') length = 8 if addr.version == 4 else 20 address = addr @@ -9112,13 +9121,13 @@ def _make_opt_lmaa(self, type: 'Enum_Option', option: 'Optional[Data_LMAAddressO # NOTE: The address is normalised rather than passed through, so that the # schema attribute holds the same type it would after a parse. The width is # then taken from the address itself rather than from ``code``, so that the - # two cannot be emitted disagreeing. - if isinstance(address, bytes): - addr = ipaddress.ip_address(address) # type: IPv4Address | IPv6Address - elif isinstance(address, (ipaddress.IPv4Address, ipaddress.IPv6Address)): - addr = address - else: - addr = ipaddress.ip_address(address) + # two cannot be emitted disagreeing. Normalising *here*, ahead of the + # schema, is also why the conversion goes through ``parse_ip_address``: + # ``ipaddress.ip_address(True)`` is ``0.0.0.1``, and the schema's own + # guard cannot see that it was ever a ``bool``. Before this, + # ``address=True`` packed as ``2906010000000001`` (c.f. #508). + addr = parse_ip_address( + address, f'{self.alias}: [OptNo {type}] invalid address') return Schema_LMAAddressOption( type=type, @@ -9314,9 +9323,13 @@ def _make_fid_suboption(self, code: 'Enum_FlowIDSuboption', address = cast('Data_TargetCareofAddressSuboption', option).address # type: Any else: address = kwargs.get('address', '::') - addr = address if isinstance( - address, (ipaddress.IPv4Address, ipaddress.IPv6Address) - ) else ipaddress.ip_address(address) + # NOTE: Through ``parse_ip_address`` because the sub-option length + # below is derived from the family here, ahead of the schema, so a + # bare ``ipaddress.ip_address`` would launder a ``bool`` past the + # schema's guard. Before this, ``address=True`` packed as + # ``0506000000000001`` (c.f. #508). + addr = parse_ip_address( + address, f'{self.alias}: [OptNo {code}] invalid target care-of address') return Schema_TargetCareofAddressSuboption( type=code, length=6 if addr.version == 4 else 18, address=addr) @@ -9821,9 +9834,15 @@ def _make_opt_dmnp(self, type: 'Enum_Option', option: 'Optional[Data_DelegatedMN prefix_length = option.prefix_length prefix = option.prefix - addr = prefix if isinstance( - prefix, (ipaddress.IPv4Address, ipaddress.IPv6Address) - ) else ipaddress.ip_address(prefix) + # NOTE: Through ``parse_ip_address`` because the ``V`` flag and the width + # are both derived from the family here, ahead of the schema, so a bare + # ``ipaddress.ip_address`` would launder a ``bool`` past the schema's + # guard. Before this, ``prefix=True`` with an IPv4-valid + # ``prefix_length`` packed as ``3706801800000001``; the default + # ``prefix_length=64`` masked it behind the range check below, which is + # why #508's own sweep read this site as already guarded (c.f. #508). + addr = parse_ip_address( + prefix, f'{self.alias}: [OptNo {type}] invalid mobile network prefix') ipv4 = addr.version == 4 if prefix_length > (32 if ipv4 else 128): @@ -10092,9 +10111,14 @@ def _make_opt_lma_up(self, type: 'Enum_Option', option: 'Optional[Data_LMAUserPl if address is None: return Schema_LMAUserPlaneAddressOption(type=type, length=2, address=b'') - addr = address if isinstance( - address, (ipaddress.IPv4Address, ipaddress.IPv6Address) - ) else ipaddress.ip_address(address) + # NOTE: Through ``parse_ip_address`` because the option length below is + # derived from the family here, ahead of the schema, so a bare + # ``ipaddress.ip_address`` would launder a ``bool`` past the schema's + # guard. Before this, ``address=True`` packed as ``3b06000000000001``. + # ``None`` is handled above and stays an absent address, which is a + # legitimate value here and not what is being rejected (c.f. #508). + addr = parse_ip_address( + address, f'{self.alias}: [OptNo {type}] invalid LMA user-plane address') return Schema_LMAUserPlaneAddressOption( type=type, diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index 90017bcdfb..40e61ca66e 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -40,7 +40,6 @@ """ import collections import datetime -import ipaddress import math from typing import TYPE_CHECKING, cast @@ -50,6 +49,7 @@ from pcapkit.const.tcp.flags import Flags as Enum_Flags from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption from pcapkit.const.tcp.option import Option as Enum_Option +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.corekit.module import ModuleDescriptor from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.protocols.data.transport.tcp import CC as Data_CC @@ -2894,7 +2894,18 @@ def _make_mptcp_addaddr(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M addr_val = opt.addr port = opt.port else: - addr_val = ipaddress.ip_address(addr) + # NOTE: Through ``parse_ip_address`` because the ``version`` sub-field + # and the option length below are both derived from the family here, + # ahead of the schema, so a bare ``ipaddress.ip_address`` would launder + # a ``bool`` into an ``IPv4Address`` that the schema's own guard can no + # longer tell from a real address -- ``addr=True`` reached + # ``mptcp_add_address_selector`` as ``0.0.0.1`` with ``version=4`` + # (c.f. #508). This option cannot be constructed end to end at all for + # an unrelated reason, ``KeyError: 'length'`` from + # pcapkit/protocols/schema/transport/tcp.py:790, which is why the + # corruption here was only ever visible on the schema the maker returns. + addr_val = parse_ip_address( + addr, f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] invalid address') version = addr_val.version return Schema_MPTCPAddAddress( diff --git a/tests/corekit/test_fields_ipaddress.py b/tests/corekit/test_fields_ipaddress.py index 5847aade8d..36d0efdfb2 100644 --- a/tests/corekit/test_fields_ipaddress.py +++ b/tests/corekit/test_fields_ipaddress.py @@ -265,6 +265,307 @@ def test_ipv4_make_rejects_a_bool_address_through_the_public_api(self) -> None: with self.assertRaises(BaseError): proto.make(src=True, dst=False).pack() + def test_parse_ip_address_rejects_a_bool_before_converting_it(self) -> None: + """:func:`parse_ip_address` is where the construction path meets #500's guard. + + Guarding the field classes is necessary but not sufficient. A ``_make_*`` + that has to know the address *family* before it can build the schema -- + because the option length is the only thing on the wire that carries the + family -- must convert its argument itself, and that conversion runs + *before* the schema. A bare :func:`ipaddress.ip_address` therefore turns + ``True`` into an ordinary :class:`~ipaddress.IPv4Address` that + :meth:`_IPAddressField.pre_process` can only see as a legitimate address, + which is what #508 turned out to be. This function is the one place those + callers convert, so it is the one place the guard has to hold. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + # the bool rejection holds for every family the callers ask for, since + # the version argument selects a *different* stdlib constructor + for version in (None, 4, 6): + for value in (True, False): + with self.subTest(version=version, value=value): + with self.assertRaises(FieldValueError) as context: + parse_ip_address(value, 'invalid address', version) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid address', str(context.exception)) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn(f'int({value!r})', str(context.exception)) + + # the escape hatch the message points at, and the widening that makes the + # version argument necessary: 1 is 0.0.0.1 unqualified but ::1 for IPv6 + self.assertEqual(parse_ip_address(int(True), 'x'), ipaddress.IPv4Address('0.0.0.1')) + self.assertEqual(parse_ip_address(int(True), 'x', 6), ipaddress.IPv6Address('::1')) + self.assertEqual(parse_ip_address(0x102, 'x', 6), ipaddress.IPv6Address('::102')) + self.assertEqual(parse_ip_address(0x102, 'x'), ipaddress.IPv4Address('0.0.1.2')) + + # every other accepted form is passed through untouched + self.assertEqual(parse_ip_address('198.51.100.7', 'x'), + ipaddress.IPv4Address('198.51.100.7')) + self.assertEqual(parse_ip_address(b'\xc6\x33\x64\x07', 'x'), + ipaddress.IPv4Address('198.51.100.7')) + self.assertEqual(parse_ip_address(ipaddress.IPv6Address('2001:db8::1'), 'x'), + ipaddress.IPv6Address('2001:db8::1')) + + def test_parse_ip_address_version_check_survives_the_passthrough_branch(self) -> None: + """An already-converted address skips the conversion, so it needs its own check. + + ``IPv6Address(IPv4Address(...))`` would have raised, but returning an + :mod:`ipaddress` object unchanged cannot -- so the version check has to + sit after the branch rather than inside the conversion it guards. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + with self.assertRaises(FieldValueError) as context: + parse_ip_address(ipaddress.IPv4Address('198.51.100.7'), 'invalid locator', 6) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('IP version mismatch: 4 != 6', str(context.exception)) + + with self.assertRaises(FieldValueError) as context: + parse_ip_address('2001:db8::1', 'invalid locator', 4) + self.assertIsInstance(context.exception, BaseError) + + def test_parse_ip_address_malformed_value_raises_in_library_error(self) -> None: + """A malformed value must not leak :mod:`ipaddress`'s bare :exc:`ValueError`. + + The construction-path callers used to let it out verbatim -- e.g. + ``MH._make_opt_bid(address='nonsense')`` raised a plain + :exc:`ValueError`, which ``except BaseError`` cannot catch. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + for value in ('nonsense', b'\x00' * 3, None, 1 << 200, -1): + with self.subTest(value=value): + with self.assertRaises(FieldValueError) as context: + parse_ip_address(value, 'invalid address') # type: ignore[arg-type] + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid address', str(context.exception)) + + def test_parse_ip_address_pins_the_family_when_version_is_given(self) -> None: + """``version=4`` and ``version=6`` each select a *different* stdlib constructor. + + Worth stating on its own because **no caller passes ``version=4`` today** -- + only the HIP locator passes a version at all, and it passes ``6`` -- so + nothing else in the suite pins what ``version=4`` does. An unexercised + parameter is one that can be broken without a failure, and the widening it + controls is the whole reason the parameter exists. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + + # the same int is a different address in each family + self.assertEqual(parse_ip_address(0x102, 'x', 4), ipaddress.IPv4Address('0.0.1.2')) + self.assertEqual(parse_ip_address(0x102, 'x', 6), ipaddress.IPv6Address('::102')) + self.assertEqual(parse_ip_address(0x102, 'x'), ipaddress.IPv4Address('0.0.1.2')) + + # every other accepted form, with the family pinned + for version, text, packed in [ + (4, '198.51.100.7', b'\xc6\x33\x64\x07'), + (6, '2001:db8::1', bytes.fromhex('20010db8' + '00' * 10 + '0001')), + ]: + with self.subTest(version=version): + expected = ipaddress.ip_address(text) + self.assertEqual(parse_ip_address(text, 'x', version), expected) + self.assertEqual(parse_ip_address(packed, 'x', version), expected) + self.assertEqual(parse_ip_address(expected, 'x', version), expected) + self.assertEqual(parse_ip_address(int(expected), 'x', version), expected) + + def test_parse_ip_address_version_mismatch_is_reported_both_ways_round(self) -> None: + """A ``version=4`` demand must refuse an IPv6 value, not only the reverse. + + The passthrough branch returns the object untouched, so the check after it + is the only thing that can catch either direction -- and a check written for + one direction only would still pass a test that exercises one direction only. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + cases = [ + (ipaddress.IPv6Address('2001:db8::1'), 4, 'IP version mismatch: 6 != 4'), + (ipaddress.IPv4Address('198.51.100.7'), 6, 'IP version mismatch: 4 != 6'), + ] + for value, version, message in cases: + with self.subTest(value=value, version=version): + with self.assertRaises(FieldValueError) as context: + parse_ip_address(value, 'invalid locator', version) + self.assertIsInstance(context.exception, BaseError) + self.assertIn(message, str(context.exception)) + self.assertIn('invalid locator', str(context.exception)) + + # a *string* of the wrong family fails inside the conversion instead, so it + # carries ipaddress's own wording rather than the version-mismatch wording, + # and is still an in-library error + for value, version in [('2001:db8::1', 4), ('198.51.100.7', 6)]: + with self.subTest(value=value, version=version): + with self.assertRaises(FieldValueError) as context: + parse_ip_address(value, 'invalid locator', version) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid locator', str(context.exception)) + + def test_parse_ip_address_rejects_an_out_of_range_int_for_the_pinned_family(self) -> None: + """``version`` narrows what an :class:`int` may be, and the error stays in-library. + + ``2**32`` is a perfectly good IPv6 address and not an IPv4 one, so the bound + moves with ``version`` -- and :class:`ipaddress.AddressValueError`, a bare + :exc:`ValueError`, must not escape either way. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + self.assertEqual(parse_ip_address(1 << 32, 'x', 6), + ipaddress.IPv6Address('::1:0:0')) + + for value, version in [(1 << 32, 4), (1 << 128, 6), (-1, 4), (-1, 6)]: + with self.subTest(value=value, version=version): + with self.assertRaises(FieldValueError) as context: + parse_ip_address(value, 'invalid address', version) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid address', str(context.exception)) + + def test_parse_ip_address_returns_an_ipaddress_object_unchanged(self) -> None: + """The passthrough branch returns the *same object*, not a copy of it. + + The makers assign the result straight into the schema attribute, so this is + what lets ``address=IPv6Address(...)`` round-trip identically rather than + through a re-conversion that could normalise it. + """ + from pcapkit.corekit.fields.ipaddress import parse_ip_address + + for value in (ipaddress.IPv4Address('198.51.100.7'), + ipaddress.IPv6Address('2001:db8::1')): + with self.subTest(value=value): + self.assertIs(parse_ip_address(value, 'x'), value) + self.assertIs(parse_ip_address(value, 'x', value.version), value) + + # and a 16-octet bytes value is IPv6 without being told so + self.assertEqual(parse_ip_address(bytes.fromhex('20010db8' + '00' * 10 + '0001'), 'x'), + ipaddress.IPv6Address('2001:db8::1')) + + def test_both_bool_guards_stay_catchable_as_value_error_and_as_base_error(self) -> None: + """The two exception classes used for this mistake are interchangeable to callers. + + #508's seven sites answer with :exc:`FieldValueError`, because the rejection + happens in a field-level conversion; the two older hand-rolled guards for + the same mistake -- ``MH._make_opt_mn_id`` from #481 and ESP's + ``SecurityAssociation`` from #491 -- answer with + :exc:`~pcapkit.utilities.exceptions.ProtocolError`, because they answer for + the option rather than for a field. That inconsistency is deliberate and + safe *only* because both classes derive from + :exc:`~pcapkit.utilities.exceptions.BaseError` and from :exc:`ValueError`, + so neither documented handler can tell them apart. This pins that, since it + is the whole basis for leaving the two alone. + + It also pins the compatibility half of #508's two deliberate behaviour + changes: ``_make_opt_bid(address='nonsense')`` and the HIP locator's + wrong-family case used to raise a **bare** :exc:`ValueError`, so they were + catchable by ``except ValueError`` and not by ``except BaseError``. They are + now catchable by both -- a widening, not a break. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.const.mh.mn_id_subtype import MNIDSubtype + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import (BaseError, FieldValueError, + ProtocolError) + + mh = object.__new__(MH) + hip = object.__new__(HIP) + + cases = [ + # (label, callable, expected class) + ('mh bid, malformed address -- was a bare ValueError', + lambda: mh._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] + bid=1, address='nonsense'), + FieldValueError), + ('hip locator, IPv4 into a v6-only locator -- was AddressValueError', + lambda: hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, + locator_set=[{'ip': ipaddress.IPv4Address('198.51.100.7')}]), + FieldValueError), + ('mh bid, bool -- #508 site', + lambda: mh._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] + bid=1, address=True), + FieldValueError), + ('mh mn_id, bool -- #481 guard, kept as ProtocolError', + lambda: mh._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, + identifier=True), + ProtocolError), + ] + + for label, make, expected in cases: + with self.subTest(case=label): + with self.assertRaises(expected) as context: + make() + # the two handlers the library documents both work, either way round + self.assertIsInstance(context.exception, BaseError) + self.assertIsInstance(context.exception, ValueError) + + # stated as the property rather than only per case, so a future change to + # either class's bases fails here rather than at some caller + for cls in (FieldValueError, ProtocolError): + with self.subTest(cls=cls.__name__): + self.assertTrue(issubclass(cls, BaseError)) + self.assertTrue(issubclass(cls, ValueError)) + + def test_switch_backed_address_makers_reject_a_bool(self) -> None: + """#508's remaining sites, for the two protocols outside :mod:`~pcapkit.protocols.internet.mh`. + + ``HIP``'s locator and ``TCP``'s Multipath ``ADD_ADDR`` address are both + backed by a :class:`~pcapkit.corekit.fields.misc.SwitchField`, and both + makers derive the wire form from the address family before the schema + exists. Measured before the fix: ``ip=True`` packed a locator of ``::1`` + with no error at all, and ``addr=True`` gave + ``MPTCPAddAddress(test={'version': 4}, address=IPv4Address('0.0.0.1'))``. + + The five ``mh`` sites are covered next to the rest of that module, in + ``MHUnitTests.test_mh_length_derived_addresses_reject_a_bool``. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + hip = object.__new__(HIP) + tcp = object.__new__(TCP) + + for value in (True, False): + with self.subTest(site='hip.Locator.value', value=value): + with self.assertRaises(FieldValueError) as context: + hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, locator_set=[{'ip': value}]) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + + with self.subTest(site='tcp.MPTCPAddAddress.address', value=value): + with self.assertRaises(FieldValueError) as context: + tcp._make_mptcp_addaddr( # type: ignore[arg-type] + MPTCPOption.ADD_ADDR, addr_id=1, addr=value) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + + # the same two sites still take every legitimate value they took before + self.assertEqual( + hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, + locator_set=[{'ip': '2001:db8::1'}]).pack().hex(), + '00c10004000004000000000020010db800000000000000000000000100000000') + self.assertEqual( + tcp._make_mptcp_addaddr( # type: ignore[arg-type] + MPTCPOption.ADD_ADDR, addr_id=1, addr='192.0.2.1').address, + ipaddress.IPv4Address('192.0.2.1')) + # int(True) is 1, and 1 is ::1 for an IPv6-only locator -- the escape + # hatch works and still widens to the family the wire format fixes + self.assertEqual( + hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, + locator_set=[{'ip': int(True)}]).pack().hex(), + '00c1000400000400000000000000000000000000000000000000000100000000') + def test_ipv4_interface_post_process_rejects_a_non_contiguous_netmask(self) -> None: """``IPv4InterfaceField.post_process`` builds ``ipaddress.ip_interface(f'{ip}/{mask}')`` from wire bytes whose diff --git a/tests/corekit/test_fields_misc.py b/tests/corekit/test_fields_misc.py index 4979bd51b7..7eb3041bce 100644 --- a/tests/corekit/test_fields_misc.py +++ b/tests/corekit/test_fields_misc.py @@ -1,9 +1,124 @@ from __future__ import annotations +import copy import unittest from tests._support import purge_modules +#: Every address-typed branch of every address-typed +#: :class:`~pcapkit.corekit.fields.misc.SwitchField` in the schema tree, as +#: ``(module, class, attribute, label, packet)``. The ``packet`` is a selector +#: input that resolves that switch to an address field. +#: +#: This table is **enumerated rather than sampled**, and +#: :meth:`SwitchFieldBoolDispatchTests.test_the_address_typed_switch_table_is_complete` +#: holds it to that by rediscovering the switches programmatically and comparing. +#: So a new address-typed switch added to any schema module fails that test until +#: it is listed here -- which is the point, since #491 and #508 were both the same +#: defect surviving at a site nobody had thought of. +ADDRESS_TYPED_SWITCH_BRANCHES = [ + ('pcapkit.protocols.schema.internet.hip', 'Locator', 'value', + 'hip locator IPv6', {'type': 0, 'len': 4}), + + # ``tid_type`` 2 is ``TaggerID.IPv4`` and 3 is ``TaggerID.IPv6``; the + # selector also demands the matching ``len``, 3 and 15 respectively. + ('pcapkit.protocols.schema.internet.hopopt', 'SMFIdentificationBasedDPDOption', 'tid', + 'hopopt smf tid IPv4', {'info': {'type': 2, 'len': 3}}), + ('pcapkit.protocols.schema.internet.hopopt', 'SMFIdentificationBasedDPDOption', 'tid', + 'hopopt smf tid IPv6', {'info': {'type': 3, 'len': 15}}), + ('pcapkit.protocols.schema.internet.ipv6_opts', 'SMFIdentificationBasedDPDOption', 'tid', + 'ipv6_opts smf tid IPv4', {'info': {'type': 2, 'len': 3}}), + ('pcapkit.protocols.schema.internet.ipv6_opts', 'SMFIdentificationBasedDPDOption', 'tid', + 'ipv6_opts smf tid IPv6', {'info': {'type': 3, 'len': 15}}), + + ('pcapkit.protocols.schema.internet.mh', 'BindingIdentifierOption', 'address', + 'mh bid IPv4', {'length': 8}), + ('pcapkit.protocols.schema.internet.mh', 'BindingIdentifierOption', 'address', + 'mh bid IPv6', {'length': 20}), + ('pcapkit.protocols.schema.internet.mh', 'DelegatedMNPOption', 'prefix', + 'mh dmnp IPv4', {'flags': {'V': 1}}), + ('pcapkit.protocols.schema.internet.mh', 'DelegatedMNPOption', 'prefix', + 'mh dmnp IPv6', {'flags': {'V': 0}}), + ('pcapkit.protocols.schema.internet.mh', 'LMAAddressOption', 'address', + 'mh lmaa IPv4', {'length': 6}), + ('pcapkit.protocols.schema.internet.mh', 'LMAAddressOption', 'address', + 'mh lmaa IPv6', {'length': 18}), + ('pcapkit.protocols.schema.internet.mh', 'LMAUserPlaneAddressOption', 'address', + 'mh lma_up IPv4', {'length': 6}), + ('pcapkit.protocols.schema.internet.mh', 'LMAUserPlaneAddressOption', 'address', + 'mh lma_up IPv6', {'length': 18}), + ('pcapkit.protocols.schema.internet.mh', 'MNIDOption', 'identifier', + 'mh mn_id IPv6', {'subtype': 2, 'length': 17}), + ('pcapkit.protocols.schema.internet.mh', 'TargetCareofAddressSuboption', 'address', + 'mh tcoa IPv4', {'length': 6}), + ('pcapkit.protocols.schema.internet.mh', 'TargetCareofAddressSuboption', 'address', + 'mh tcoa IPv6', {'length': 18}), + + ('pcapkit.protocols.schema.transport.tcp', 'MPTCPAddAddress', 'address', + 'tcp mptcp add_addr IPv4', {'test': {'version': 4}}), + ('pcapkit.protocols.schema.transport.tcp', 'MPTCPAddAddress', 'address', + 'tcp mptcp add_addr IPv6', {'test': {'version': 6}}), +] + + +def discover_address_typed_switches() -> 'set[tuple[str, str, str]]': + """Find every address-typed :class:`SwitchField` declaration in the schema tree. + + Walks :attr:`Schema.__fields__` for every :class:`Schema` subclass rather than + grepping annotations, because three of the switches -- + ``mh.BindingIdentifierOption.address`` and both + ``SMFIdentificationBasedDPDOption.tid`` -- are wrapped in a + :class:`~pcapkit.corekit.fields.misc.ConditionalField` and so are invisible to + a grep for a ``SwitchField`` annotation. #508's own table listed seven for + exactly that reason; there are ten. + + Returns: + ``(module, class, attribute)`` for each distinct declaration. Declarations + are deduplicated by field-object identity, since a subclass inherits its + bases' ``__fields__`` entries rather than re-declaring them. + + """ + import importlib + import inspect + import pkgutil + + import pcapkit.protocols.schema as schema_pkg + from pcapkit.corekit.fields.misc import (ConditionalField, + ForwardMatchField, SwitchField) + from pcapkit.protocols.schema.schema import Schema + + for module in pkgutil.walk_packages(schema_pkg.__path__, schema_pkg.__name__ + '.'): + importlib.import_module(module.name) + + subclasses, stack = set(), [Schema] + while stack: + for sub in stack.pop().__subclasses__(): + if sub not in subclasses: + subclasses.add(sub) + stack.append(sub) + + found, seen = set(), set() + for cls in subclasses: + for attr, field in getattr(cls, '__fields__', {}).items(): + while isinstance(field, (ConditionalField, ForwardMatchField)): + field = field.field + if not isinstance(field, SwitchField) or id(field) in seen: + continue + seen.add(id(field)) + + # an address-typed switch is one that can *return* an address field; + # read off the selector's own source, since the branch actually taken + # depends on a packet this function does not have + try: + source = inspect.getsource(field._selector) + except (OSError, TypeError): # pragma: no cover + continue + if any(name in source for name in + ('IPv4AddressField', 'IPv6AddressField', + 'IPv4InterfaceField', 'IPv6InterfaceField')): + found.add((cls.__module__, cls.__qualname__, attr)) + return found + class SchemaFieldDefaultTests(unittest.TestCase): """Regression coverage for `#444 `__. @@ -44,5 +159,242 @@ def test_schema_field_accepts_a_bytes_default(self) -> None: self.assertEqual(field.default.b, 2) +class SwitchFieldBoolDispatchTests(unittest.TestCase): + """Why #508's guard is *not* in :class:`~pcapkit.corekit.fields.misc.SwitchField`. + + #508 read the defect as a dispatch problem: because + :class:`~pcapkit.corekit.fields.misc.SwitchField` picks the concrete field at + runtime, the reasoning went, a :obj:`bool` never reaches + :meth:`~pcapkit.corekit.fields.ipaddress._IPAddressField.pre_process` and so + #500's guard cannot fire -- which would put the fix in ``SwitchField``. + Measured, that is not what happens. + :meth:`SwitchField.pre_process ` + delegates straight to the resolved field, so a :obj:`bool` that actually + arrives at an address-selecting switch is already rejected, on *every* + address-typed branch of every such switch. The real cause is upstream of the + schema entirely: the ``_make_*`` converts the argument itself, to size the + option, and so destroys the :obj:`bool` before the schema is built. + + Both halves are pinned here, because both are load-bearing for the choice of + fix location: + + * a guard in ``SwitchField`` would be dead code for the address-typed + branches, since the resolved field already raises, and + * a *blanket* one would be actively wrong for the integer-typed branches, + where a :obj:`bool` legitimately means ``0``/``1`` -- the same ruling that + keeps ``NonceIndicesOption.home`` accepting one. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + @staticmethod + def _resolve(module: 'str', name: 'str', attr: 'str') -> 'tuple[object, object]': + """Return ``(declared field, inner SwitchField)`` for one table entry.""" + import importlib + + from pcapkit.corekit.fields.misc import (ConditionalField, + ForwardMatchField) + + declared = getattr(importlib.import_module(module), name).__fields__[attr] + inner = declared + while isinstance(inner, (ConditionalField, ForwardMatchField)): + inner = inner.field + return declared, inner + + def test_address_typed_switch_branches_already_reject_a_bool(self) -> None: + """Every address-typed branch of every address-typed switch already refuses a bool. + + Eighteen branches over ten switches in four protocol modules -- not just + :mod:`~pcapkit.protocols.schema.internet.mh`'s six -- because the claim + being pinned is about ``SwitchField`` in general, so a fix scoped to one + module would not settle it. Measured on ``main`` at ``27bb315d5``, i.e. + *before* #508's fix: all thirty-six probes (eighteen branches x + ``True``/``False``) raised, and none was laundered. That is what makes a + guard in ``SwitchField`` dead code. + """ + from pcapkit.corekit.fields.misc import SwitchField + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + for module, name, attr, label, packet in ADDRESS_TYPED_SWITCH_BRANCHES: + _, inner = self._resolve(module, name, attr) + self.assertIsInstance(inner, SwitchField, msg=label) + + for value in (True, False): + with self.subTest(branch=label, value=value): + field = SwitchField(selector=inner._selector) # type: ignore[attr-defined] + field.name = 'probe' + # deep-copied per probe because ``smf_i_dpd_tid_selector`` + # writes the resolved enum back into ``pkt['info']['type']`` + with self.assertRaises(FieldValueError) as context: + field(copy.deepcopy(packet)).pack(value, copy.deepcopy(packet)) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + + def test_the_address_typed_switch_table_is_complete(self) -> None: + """:data:`ADDRESS_TYPED_SWITCH_BRANCHES` must name every address-typed switch. + + The table above is hand-written, so on its own it proves nothing about + switches nobody listed -- which is the failure mode #491 and #508 share. + Rediscovering them from ``Schema.__fields__`` and comparing closes that: + ten declarations, three of which a ``SwitchField`` annotation grep cannot + see because they are ``ConditionalField``-wrapped. + """ + discovered = discover_address_typed_switches() + tabled = {(module, name, attr) + for module, name, attr, _, _ in ADDRESS_TYPED_SWITCH_BRANCHES} + + self.assertEqual( + discovered, tabled, + msg='address-typed SwitchField declarations and the table have diverged; ' + f'only discovered: {sorted(discovered - tabled)}; ' + f'only tabled: {sorted(tabled - discovered)}') + self.assertEqual(len(discovered), 10) + + # and the three that an annotation grep misses really are wrapped + from pcapkit.corekit.fields.misc import ConditionalField + + wrapped = {(module, name, attr) + for module, name, attr in discovered + if isinstance(self._resolve(module, name, attr)[0], ConditionalField)} + self.assertEqual(wrapped, { + ('pcapkit.protocols.schema.internet.hopopt', + 'SMFIdentificationBasedDPDOption', 'tid'), + ('pcapkit.protocols.schema.internet.ipv6_opts', + 'SMFIdentificationBasedDPDOption', 'tid'), + ('pcapkit.protocols.schema.internet.mh', + 'BindingIdentifierOption', 'address'), + }) + + def test_address_typed_switch_branches_still_take_a_real_address(self) -> None: + """The rejection is of :obj:`bool` specifically, and every branch still packs. + + Also pins an asymmetry that is easy to trip over, and that + :func:`~pcapkit.corekit.fields.ipaddress._reject_bool` already describes: + the ``int(...)`` escape hatch the error message advertises works on an + **IPv4** branch and not on an IPv6 one. ``_IPAddressField.pre_process`` + converts with the family-agnostic :func:`ipaddress.ip_address`, so ``1`` + becomes ``0.0.0.1`` and is then refused by an IPv6-typed field for + mismatching version -- whereas + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` called with + ``version=6``, which is how the ``_make_*`` sites reach the same guard, + widens ``1`` to ``::1`` instead. Both behaviours are deliberate; they just + are not the same behaviour, and only the maker path honours the message + for IPv6. + """ + import ipaddress + + from pcapkit.corekit.fields.misc import SwitchField + from pcapkit.utilities.exceptions import FieldValueError + + for module, name, attr, label, packet in ADDRESS_TYPED_SWITCH_BRANCHES: + _, inner = self._resolve(module, name, attr) + field = SwitchField(selector=inner._selector) # type: ignore[attr-defined] + field.name = 'probe' + resolved = field(copy.deepcopy(packet)) + + ipv4 = label.endswith('IPv4') + address = ipaddress.IPv4Address(1) if ipv4 else ipaddress.IPv6Address(1) + + with self.subTest(branch=label): + # an actual address object always packs, on either family + self.assertEqual(resolved.pack(address, copy.deepcopy(packet)), + address.packed) + self.assertEqual(resolved.pack(str(address), copy.deepcopy(packet)), + address.packed) + + if ipv4: + self.assertEqual(resolved.pack(int(True), copy.deepcopy(packet)), + ipaddress.IPv4Address('0.0.0.1').packed) + else: + # ``ip_address(1)`` is IPv4, so an IPv6-typed field refuses it + with self.assertRaises(FieldValueError) as context: + resolved.pack(int(True), copy.deepcopy(packet)) + self.assertIn('IP version mismatch: 4 != 6', str(context.exception)) + + def test_parse_ip_address_widens_an_int_where_a_bare_field_would_not(self) -> None: + """The other half of the asymmetry above, stated from the maker's side. + + This is why :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` takes + a ``version`` at all: the maker sites that pin the family need ``1`` to mean + ``::1``, which the family-agnostic conversion in the field cannot give them. + """ + import ipaddress + + from pcapkit.corekit.fields.ipaddress import (IPv6AddressField, + parse_ip_address) + from pcapkit.utilities.exceptions import FieldValueError + + self.assertEqual(parse_ip_address(1, 'x', 6), ipaddress.IPv6Address('::1')) + self.assertEqual(parse_ip_address(1, 'x', 4), ipaddress.IPv4Address('0.0.0.1')) + self.assertEqual(parse_ip_address(1, 'x'), ipaddress.IPv4Address('0.0.0.1')) + + with self.assertRaises(FieldValueError): + IPv6AddressField().pre_process(1, {}) + + def test_integer_typed_switch_branch_still_coerces_a_bool_to_zero_or_one(self) -> None: + import pcapkit.protocols.schema.internet.mh as schema_mh + from pcapkit.const.mh.binding_revocation import BindingRevocation + from pcapkit.corekit.fields.misc import SwitchField + + # ``BindingRevocationMessage.code`` is a switch too, but over two enum + # registries rather than two address families, so its branches are + # ``EnumField``s -- and a bool in an integer field means 0/1, exactly as + # ``NonceIndicesOption.home`` (a ``UInt16Field``) does. A blanket bool + # rejection in ``SwitchField`` would break this. + packet = {'br_type': BindingRevocation.Binding_Revocation_Indication} + field = SwitchField(selector=schema_mh.br_code_selector) + field.name = 'probe' + bound = field(dict(packet)) + + self.assertEqual(bound.pack(True, dict(packet)), bound.pack(1, dict(packet))) + self.assertEqual(bound.pack(True, dict(packet)), b'\x01') + self.assertEqual(bound.pack(False, dict(packet)), bound.pack(0, dict(packet))) + self.assertEqual(bound.pack(False, dict(packet)), b'\x00') + + def test_every_enum_typed_switch_branch_still_coerces_a_bool(self) -> None: + """Both halves of both enum switches, so "not a blanket rejection" is pinned whole. + + ``br_code_selector`` and ``fb_code_selector`` each pick between *two* enum + registries -- a trigger in an indication and a status code in an + acknowledgement -- so each has two branches, and a blanket :obj:`bool` + rejection in ``SwitchField`` would break all four. The ruling this follows is + the owner's on ``NonceIndicesOption.home``: a :obj:`bool` in an integer field + means ``0``/``1`` and that is correct. + """ + import pcapkit.protocols.schema.internet.mh as schema_mh + from pcapkit.const.mh.binding_revocation import BindingRevocation + from pcapkit.const.mh.fb_type import FlowBindingType + from pcapkit.corekit.fields.misc import SwitchField + from pcapkit.corekit.fields.numbers import EnumField + + cases = [ + ('br indication', schema_mh.br_code_selector, + {'br_type': BindingRevocation.Binding_Revocation_Indication}), + ('br acknowledgement', schema_mh.br_code_selector, + {'br_type': BindingRevocation.Binding_Revocation_Acknowledgement}), + ('fb indication', schema_mh.fb_code_selector, + {'fb_type': FlowBindingType.Indication}), + ('fb acknowledgement', schema_mh.fb_code_selector, + {'fb_type': FlowBindingType.Acknowledgement}), + ] + + for label, selector, packet in cases: + with self.subTest(branch=label): + self.assertIsInstance(selector(dict(packet)), EnumField) + + field = SwitchField(selector=selector) + field.name = 'probe' + bound = field(dict(packet)) + + self.assertEqual(bound.pack(True, dict(packet)), b'\x01') + self.assertEqual(bound.pack(True, dict(packet)), + bound.pack(1, dict(packet))) + self.assertEqual(bound.pack(False, dict(packet)), b'\x00') + self.assertEqual(bound.pack(False, dict(packet)), + bound.pack(0, dict(packet))) + + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index b3732f457b..10a5d24304 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2543,6 +2543,280 @@ def test_mh_length_derived_addresses_pick_their_family(self) -> None: self.assertEqual(data.ipv4, ipv4) self.assertEqual(data.prefix, ipaddress.ip_address(prefix)) + def test_mh_length_derived_addresses_reject_a_bool(self) -> None: + """A :obj:`bool` address must not be silently sized and emitted. See #508. + + These are exactly the options of the test above, and the defect is a + consequence of what that test pins: because the option length is derived + from the address family, the maker has to convert the argument *itself*, + ahead of the schema. A bare :func:`ipaddress.ip_address` therefore turned + ``True`` into ``0.0.0.1`` and ``False`` into ``0.0.0.0``, sized the option + as IPv4, and packed it with no exception and no warning -- so #500's guard + in :meth:`~pcapkit.corekit.fields.ipaddress._IPAddressField.pre_process` + never saw a :obj:`bool` at all. Measured before the fix: + + .. code-block:: text + + _make_opt_bid(address=True) -> 23080001000000000001 + _make_opt_lmaa(address=True) -> 2906010000000001 + _make_opt_lma_up(address=True) -> 3b06000000000001 + _make_fid_suboption(address=True, Target_Care_of_Address) + -> 0506000000000001 + _make_opt_dmnp(prefix=True, prefix_length=24) + -> 3706801800000001 + + ``_make_opt_dmnp`` is the reason the sweep in #508 undercounted this: at + its default ``prefix_length=64`` the bool is converted to an IPv4 address + and then rejected by the *prefix length* range check, which reads as a + guard but is not one. An IPv4-valid prefix length exposes it. + """ + import ipaddress + + from pcapkit.const.mh.flow_id_suboption import FlowIDSuboption + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + proto = object.__new__(MH) + + sites = [ + ('bid', lambda value: proto._make_opt_bid( # type: ignore[arg-type] + Option.Binding_Identifier, bid=1, address=value)), + ('lmaa', lambda value: proto._make_opt_lmaa( # type: ignore[arg-type] + Option.Local_Mobility_Anchor_Address_Option, address=value)), + ('lma_up', lambda value: proto._make_opt_lma_up( # type: ignore[arg-type] + Option.LMA_User_Plane_Address, address=value)), + ('tcoa', lambda value: proto._make_fid_suboption( # type: ignore[arg-type] + FlowIDSuboption.Target_Care_of_Address, address=value)), + ('dmnp', lambda value: proto._make_opt_dmnp( # type: ignore[arg-type] + Option.Delegated_Mobile_Network_Prefix, prefix_length=24, prefix=value)), + ] + + for name, make in sites: + for value in (True, False): + with self.subTest(option=name, value=value): + with self.assertRaises(FieldValueError) as context: + make(value) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn(f'int({value!r})', str(context.exception)) + + # the escape hatch the message points at: int(True) is 1, which is a + # legitimate -- if unusual -- IPv4 address, and still sizes as one + schema = proto._make_opt_lmaa( # type: ignore[arg-type] + Option.Local_Mobility_Anchor_Address_Option, address=int(True)) + self.assertEqual(schema.length, 6) + self.assertEqual(schema.address, ipaddress.ip_address('0.0.0.1')) + + # and a malformed address is now an in-library error rather than + # ipaddress's own bare ValueError, which ``except BaseError`` cannot catch + with self.assertRaises(FieldValueError) as context: + proto._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] + bid=1, address='nonsense') + self.assertIsInstance(context.exception, BaseError) + self.assertIn('does not appear to be an IPv4 or IPv6 address', + str(context.exception)) + + def test_mh_bool_is_still_a_valid_value_for_an_integer_field(self) -> None: + """#508 must not spread to fields where a :obj:`bool` legitimately means 0/1. + + ``NonceIndicesOption.home`` is a + :class:`~pcapkit.corekit.fields.numbers.UInt16Field` at + ``pcapkit/protocols/schema/internet/mh.py:670`` -- a nonce *index* rather + than an address -- so ``home=True`` packing as ``1`` is correct, not a + defect. ``BindingRevocationMessage.code`` makes the same point through a + :class:`~pcapkit.corekit.fields.misc.SwitchField`, which is why the guard + for #508 is not a blanket rejection there. + """ + from pcapkit.const.mh.binding_revocation import BindingRevocation + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + + proto = object.__new__(MH) + + self.assertEqual( + proto._make_opt_ni(Option.Nonce_Indices, home=True).pack(), # type: ignore[arg-type] + bytes.fromhex('040400010000')) + self.assertEqual( + proto._make_opt_ni(Option.Nonce_Indices, home=True).pack(), # type: ignore[arg-type] + proto._make_opt_ni(Option.Nonce_Indices, home=1).pack()) # type: ignore[arg-type] + self.assertEqual( + proto._make_opt_ni(Option.Nonce_Indices, home=False).pack(), # type: ignore[arg-type] + bytes.fromhex('040400000000')) + + # the same, through a SwitchField whose branches are EnumFields + self.assertEqual( + proto._make_msg_brm( # type: ignore[arg-type] + br_type=BindingRevocation.Binding_Revocation_Indication, + code=True, seq=1).pack(), + proto._make_msg_brm( # type: ignore[arg-type] + br_type=BindingRevocation.Binding_Revocation_Indication, + code=1, seq=1).pack()) + self.assertEqual( + proto._make_msg_brm( # type: ignore[arg-type] + br_type=BindingRevocation.Binding_Revocation_Indication, + code=True, seq=1).pack(), + bytes.fromhex('010100010000')) + + def test_mh_dmnp_prefix_length_range_check_survives_the_bool_guard(self) -> None: + """The prefix-length range check needs a test that does not depend on #508. + + Before #508's fix, the only thing reaching this check with an IPv4 address + was ``prefix=True`` at the default ``prefix_length=64``: the bool was + laundered into ``0.0.0.1``, and ``64 > 32`` then raised -- which is exactly + why #508's own sweep read this site as already guarded. It was not; the + ``ProtocolError`` was about the *prefix length*, not the prefix. + + Now that the bool is refused earlier, that accident no longer exercises the + range check at all, so this test reaches it with a real address instead. + Without it, the fix silently drops coverage of a branch that was only ever + covered by the defect it removes. + """ + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + proto = object.__new__(MH) + + # an IPv4 prefix caps at /32 and an IPv6 one at /128 + for prefix, prefix_length in (('198.51.100.0', 33), ('198.51.100.0', 64), + ('2001:db8:3::', 129)): + with self.subTest(prefix=prefix, prefix_length=prefix_length): + with self.assertRaises(ProtocolError) as context: + proto._make_opt_dmnp( # type: ignore[arg-type] + Option.Delegated_Mobile_Network_Prefix, + prefix_length=prefix_length, prefix=prefix) + self.assertIsInstance(context.exception, BaseError) + self.assertIn(f'invalid prefix length: {prefix_length}', + str(context.exception)) + + # and the boundary values either side are accepted + for prefix, prefix_length, length in (('198.51.100.0', 32, 6), + ('2001:db8:3::', 128, 18)): + with self.subTest(prefix=prefix, prefix_length=prefix_length): + schema = proto._make_opt_dmnp( # type: ignore[arg-type] + Option.Delegated_Mobile_Network_Prefix, + prefix_length=prefix_length, prefix=prefix) + self.assertEqual(schema.length, length) + + def test_mh_bid_rejects_an_out_of_range_binding_priority(self) -> None: + """``BID-PRI`` is a 7-bit field, so ``0x80`` cannot be packed. + + In the same maker as #508's care-of-address conversion, and immediately + above it, so it is worth pinning that the new conversion did not move the + check out from under a valid input. + """ + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + proto = object.__new__(MH) + + with self.assertRaises(ProtocolError) as context: + proto._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] + bid=1, bid_pri=0x80) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid binding priority: 128', str(context.exception)) + + # 0x7F is the largest value that fits, and it still takes an address + schema = proto._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] + bid=1, bid_pri=0x7F, address='198.51.100.7') + self.assertEqual(schema.flags['BID_PRI'], 0x7F) + self.assertEqual(schema.length, 8) + + def test_mh_lmaa_rejects_a_bytes_address_of_the_wrong_width(self) -> None: + """A ``bytes`` address must be 4 or 16 octets before the conversion sees it. + + This check sits directly above #508's conversion and is the reason a short + ``bytes`` value gets a ``ProtocolError`` naming the address rather than + whatever :mod:`ipaddress` would have said about it. + """ + import ipaddress + + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + proto = object.__new__(MH) + + for address in (b'\x00' * 3, b'\x00' * 5, b'\x00' * 15, b''): + with self.subTest(address=address): + with self.assertRaises(ProtocolError) as context: + proto._make_opt_lmaa( # type: ignore[arg-type] + Option.Local_Mobility_Anchor_Address_Option, address=address) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('invalid address', str(context.exception)) + + # the two accepted widths pick the family, and so the option length + for address, length, expected in ( + (b'\xc6\x33\x64\x07', 6, ipaddress.IPv4Address('198.51.100.7')), + (bytes.fromhex('20010db8' + '00' * 10 + '0001'), 18, + ipaddress.IPv6Address('2001:db8::1')), + ): + with self.subTest(address=address): + schema = proto._make_opt_lmaa( # type: ignore[arg-type] + Option.Local_Mobility_Anchor_Address_Option, address=address) + self.assertEqual(schema.length, length) + self.assertEqual(schema.address, expected) + + def test_mh_target_careof_address_suboption_round_trips_both_families(self) -> None: + """The sub-option length is derived from the family, so both widths need pinning. + + ``_make_fid_suboption`` is one of #508's seven sites and the only one reached + through the sub-option dispatcher rather than an option maker, so its + legitimate output is worth asserting byte for byte alongside the bool + rejection. + """ + import ipaddress + + from pcapkit.const.mh.flow_id_suboption import FlowIDSuboption + from pcapkit.protocols.internet.mh import MH + + proto = object.__new__(MH) + + # type, length, the two reserved octets, then the address + for address, length, packed in ( + ('198.51.100.7', 6, '05060000' 'c6336407'), + ('2001:db8::1', 18, '05120000' '20010db8' + '00' * 10 + '0001'), + ): + with self.subTest(address=address): + schema = proto._make_fid_suboption( # type: ignore[arg-type] + FlowIDSuboption.Target_Care_of_Address, address=address) + self.assertEqual(schema.length, length) + self.assertEqual(schema.address, ipaddress.ip_address(address)) + self.assertEqual(schema.pack().hex(), packed) + + # the documented default is an IPv6 unspecified address + default = proto._make_fid_suboption( # type: ignore[arg-type] + FlowIDSuboption.Target_Care_of_Address) + self.assertEqual(default.length, 18) + self.assertEqual(default.address, ipaddress.IPv6Address('::')) + + def test_mh_fid_suboption_takes_an_unassigned_suboption_from_its_data_model(self) -> None: + """The ``option is not None`` path of the unassigned sub-option branch. + + The same maker that carries #508's target care-of conversion ends in a + catch-all for sub-option types the library does not model, and that + catch-all's data-model path was unexercised. + """ + from pcapkit.const.mh.flow_id_suboption import FlowIDSuboption + from pcapkit.protocols.data.internet.mh import \ + UnassignedFlowIdentificationSuboption as Data_Unassigned + from pcapkit.protocols.internet.mh import MH + + proto = object.__new__(MH) + code = FlowIDSuboption(200) + + schema = proto._make_fid_suboption( # type: ignore[arg-type] + code, Data_Unassigned(type=code, length=3, data=b'\xde\xad\xbe')) + self.assertEqual(schema.length, 3) + self.assertEqual(schema.data, b'\xde\xad\xbe') + + # and the keyword path, for contrast + self.assertEqual( + proto._make_fid_suboption(code, data=b'\xde\xad\xbe').pack(), # type: ignore[arg-type] + schema.pack()) + def test_mh_mn_id_option_length_matches_packed_octets(self) -> None: """The MN-ID option's declared length must count what actually gets packed.