From 6d2ceed0bbffdd764a05f238abc47451f4de09eb Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 00:28:25 -0400 Subject: [PATCH] fix(link,internet): reject a bool at the four remaining maker call sites (#540) Follow-up to #508/#539/#552: bool is an int subclass, so a bare ipaddress.IPv4Address/IPv6Address/ip_address call in a _make_* helper silently laundered True/False into 0.0.0.1/::1 instead of raising. Four sites were deliberately left out of #539 because their files were owned by other work at the time; all four are now routed through the existing parse_ip_address helper, the same pattern #539 and #552 used. - ARP._make_proto_resolve (pcapkit/protocols/link/arp.py): addr=True packed as 00000001 (IPv4) or ::1 (IPv6) with no exception. - IPv6_Route._make_data_type_rpl (pcapkit/protocols/internet/ipv6_route.py): worse than a packed-address defect, since cmpr_i/cmpr_e are derived from the laundered value -- ip=[True] packed with cmpr_e=0 and an address of 00000001 instead of raising. - IPv6_Route.make's dst parameter: the most reachable of the four, on the public make() entry point; dst=True converted to ::1 silently. - OSPF._make_id_numbers: latent, no production caller today, fixed anyway so it does not resurface the defect the moment one is added. ARP and OSPF use self.__class__.__name__ rather than self.alias in the FieldValueError message, because their alias properties read state (_acnm, _version) that read() only assigns -- unavailable to a construction-only instance that never went through read(). Beyond bool rejection, pinning version=6 on the two IPv6_Route sites is a second, smaller behaviour change: both previously converted a plain integer through the bare, family-inferring ipaddress.ip_address, so ip=[258] packed a 4-octet IPv4 address (0.0.1.2) inside an IPv6-only header; it now packs the 16-octet IPv6 form (::102) instead, which is what an IPv6-only header should hold regardless of what an int happens to fit as an IPv4 address. Overlaps PR #561 (open, unmerged) in ipv6_route.py, which touches _read_data_type_rpl; this change stays inside make()/_make_data_type_rpl so the conflict on merge should be trivial. Four new tests, each shown to fail without its fix (FieldValueError not raised). Full unit tier green: 1120 passed, 8 skipped, 2673 subtests passed. CHANGELOG.md regenerated via util/changelog_md.py from docs/source/changelog/1.5.0.rst. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 17 ++++ pcapkit/protocols/internet/ipv6_route.py | 38 +++++++-- pcapkit/protocols/link/arp.py | 31 ++++++- pcapkit/protocols/link/ospf.py | 24 +++++- .../internet/test_ipv6_extension_unit.py | 80 +++++++++++++++++++ tests/protocols/link/test_link_unit.py | 64 +++++++++++++++ 7 files changed, 244 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc99bac9d..143a075998 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- a truncated or under-declared area no longer parses "successfully", and no longer wedges the process. The option and list loops could spin forever with no exception on a truncated area, reachable from untrusted input through HOPOPT, IPv6-Opts, MH, HIP and SCTP; each iteration must now advance the stream by at least one octet, and the error names the option, the offset and the octets remaining (#431, #432). Separately, wire-derived lengths in `ipv6_opts`, CALIPSO, MPL, REG_INFO and four HIP list callbacks underflowed below zero, which `ListField`'s own `while length > 0` then turned into a silent empty list; they are floored and raise instead (#449, #456, #460, #463). - **Fixed** -- field widths and units, each measured against the specification rather than inferred: HIP's `TRANSPORT_FORMAT_LIST`, `NAT_TRAVERSAL_MODE` and `ESP_TRANSFORM` list entries are two octets, not one [[RFC 7401](https://datatracker.ietf.org/doc/html/rfc7401), [RFC 5770](https://datatracker.ietf.org/doc/html/rfc5770), [RFC 7402](https://datatracker.ietf.org/doc/html/rfc7402)] (#463, #472); the MN-ID option sizes from its subtype, not from `identifier`'s Python type (#448, #464, #467); IPv6-Route's `Hdr Ext Len` is computed in 8-octet units on both sides [[RFC 8200](https://datatracker.ietf.org/doc/html/rfc8200)] (#487, #489); and the Fast Binding Update and Acknowledgment Lifetimes are plain seconds [[RFC 5568](https://datatracker.ietf.org/doc/html/rfc5568)], not [RFC 6275](https://datatracker.ietf.org/doc/html/rfc6275)'s four-second units (#502). - **Fixed** -- stdlib exceptions leaking out where the library's own were promised: a malformed IP field value raised a bare `ValueError` instead of `FieldValueError` (#465); a `bool` address was silently packed as `0.0.0.1` or `0.0.0.0`, `bool` being an `int` subclass (#491, #500); and `@prepare` discarded extra arguments silently and treated a *declared* zero length as end of stream, which is now distinguished from a genuinely exhausted one and raises `StreamEOFError` (#454, #458). +- **Fixed** -- the last four call sites where a `_make_*` method converts an address itself, ahead of the schema, so the same `bool`-as-`int` mistake reached them too, unguarded by the schema-layer fix above: `ARP._make_proto_resolve`, `IPv6_Route._make_data_type_rpl` (which also derives its RPL compression lengths from the laundered value, corrupting the address list alongside the exception it should have raised), the `dst` parameter of `IPv6_Route.make`, and the latent `OSPF._make_id_numbers`, which nothing calls yet but would have inherited the defect regardless. All four now go through `parse_ip_address`, the helper #539 and #552 route their own sites through, completing the programme #481 and #508 started. Pinning `version=6` on the two `IPv6_Route` sites is a second, smaller behaviour change beyond bool rejection: both previously converted a plain integer through the bare, family-inferring `ipaddress.ip_address`, which let `ip=[258]` pack a 4-octet IPv4 address (`0.0.1.2`) inside an IPv6-only header; it now packs the 16-octet IPv6 form (`::102`) instead, which is what an IPv6-only header should hold regardless of what an integer argument happens to fit as an IPv4 address (#540). - **Fixed** -- the engine adapters, which were quietly wrong rather than loud. The `dpkt` toolkit split TCP and IPv4 headers at their fixed struct size instead of their real length, so option octets overwrote payload in the sequence-indexed reassembly buffer; it also read an `ipv6_frag.nh` that `dpkt` does not have, and passed fragment offsets unscaled (#351, #370, #385, #395). `scapy` never loaded its layer registry, so that engine did not dissect at all (#409), and its IPv4 fragment offset reached reassembly unscaled (#483, #484). The four IPv6 adapters disagreed about whether the 8-octet Fragment header belongs to `ihl`, `header` and `tl`; per [RFC 8200](https://datatracker.ietf.org/doc/html/rfc8200) section 4.5 it belongs to none of them, and all four now agree (#415, #424). - **Fixed** -- PCAP and PCAP-NG output and parsing: `bytes(frame)` returned the *next* frame's octets, `files=True` wrote names like `Frame 1..json`, and a PCAP-NG `timestamp_epoch` was shifted by the reading host's timezone (#403); seven further parser defects (#341--#347, #371) and, on the write path, four more block-parsing ones (#388); `BitField` packed every named bit as set (#359, #374); the extension-header walk failed to advance past the last IPv6 extension header (#348, #373); and IPv6 fragment offsets went unscaled, with reassembly keyed on the flow label -- optional, and routinely zero, so distinct datagrams collapsed together -- rather than on the fragment identification (#389). - **Fixed** -- TCP reassembly mixed absolute sequence numbers with buffer-relative slicing, so on any capture carrying a SYN with a realistic initial sequence number incomplete datagrams were dropped silently, and the `completed=False` branch of the public API was unreachable (#349, #376). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 4e230831a2..656f60cf98 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -242,6 +242,23 @@ pull requests between #326 and #509. and ``@prepare`` discarded extra arguments silently and treated a *declared* zero length as end of stream, which is now distinguished from a genuinely exhausted one and raises ``StreamEOFError`` (#454, #458). +* **Fixed** -- the last four call sites where a ``_make_*`` method converts an + address itself, ahead of the schema, so the same ``bool``-as-``int`` mistake + reached them too, unguarded by the schema-layer fix above: + ``ARP._make_proto_resolve``, ``IPv6_Route._make_data_type_rpl`` (which also + derives its RPL compression lengths from the laundered value, corrupting the + address list alongside the exception it should have raised), the ``dst`` + parameter of ``IPv6_Route.make``, and the latent ``OSPF._make_id_numbers``, + which nothing calls yet but would have inherited the defect regardless. All + four now go through ``parse_ip_address``, the helper #539 and #552 route + their own sites through, completing the programme #481 and #508 started. + Pinning ``version=6`` on the two ``IPv6_Route`` sites is a second, smaller + behaviour change beyond bool rejection: both previously converted a plain + integer through the bare, family-inferring ``ipaddress.ip_address``, which + let ``ip=[258]`` pack a 4-octet IPv4 address (``0.0.1.2``) inside an + IPv6-only header; it now packs the 16-octet IPv6 form (``::102``) instead, + which is what an IPv6-only header should hold regardless of what an integer + argument happens to fit as an IPv4 address (#540). * **Fixed** -- the engine adapters, which were quietly wrong rather than loud. The ``dpkt`` toolkit split TCP and IPv4 headers at their fixed struct size instead of their real length, so option octets overwrote payload in the diff --git a/pcapkit/protocols/internet/ipv6_route.py b/pcapkit/protocols/internet/ipv6_route.py index 3f1a5ba0a5..c25c03c716 100644 --- a/pcapkit/protocols/internet/ipv6_route.py +++ b/pcapkit/protocols/internet/ipv6_route.py @@ -23,13 +23,13 @@ """ import collections -import ipaddress import math import os.path as os_path from typing import TYPE_CHECKING, cast, overload from pcapkit.const.ipv6.routing import Routing as Enum_Routing from pcapkit.const.reg.transtype import TransType as Enum_TransType +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.protocols.data.internet.ipv6_route import RPL as Data_RPL from pcapkit.protocols.data.internet.ipv6_route import IPv6_Route as Data_IPv6_Route from pcapkit.protocols.data.internet.ipv6_route import SourceRoute as Data_SourceRoute @@ -286,6 +286,10 @@ def make(self, Returns: Constructed packet data. + Raises: + FieldValueError: If ``dst`` is a :obj:`bool` (c.f. + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). + """ next_val = cast('Enum_TransType', self._make_index(next, next_default, namespace=next_namespace, @@ -311,7 +315,13 @@ def make(self, else: meth = name[1] - dst_val = cast('IPv6Address', ipaddress.ip_address(dst)) if dst is not None else None + # NOTE: Through ``parse_ip_address`` rather than + # ``ipaddress.ip_address`` directly, because ``bool`` is an + # ``int`` subclass the latter accepts without complaint. Before + # this, ``dst=True`` converted to ``::1`` with no exception at + # all (c.f. #508, #540). + dst_val = cast('IPv6Address', parse_ip_address( + dst, f'{self.alias}: invalid destination address', version=6)) if dst is not None else None if isinstance(data, dict): data_val = meth(type_val, dst=dst_val, **data) else: @@ -718,6 +728,10 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' Returns: Constructed route data schema. + Raises: + FieldValueError: If an entry of ``ip`` is a :obj:`bool` (c.f. + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). + """ if route is not None: cmpr_i = route.cmpr_i @@ -729,12 +743,22 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' else: ip = [] if ip is None else ip + # NOTE: Through ``parse_ip_address`` rather than + # ``ipaddress.ip_address`` directly, because ``bool`` is an + # ``int`` subclass the latter accepts without complaint -- and + # here the laundered value would not just pack wrong, it would + # feed ``cmpr_i``/``cmpr_e`` below, corrupting the compression + # metadata alongside the address list. Before this, + # ``ip=[True]`` packed with ``cmpr_e=0`` and an address of + # ``00000001`` instead of raising (c.f. #508, #540). + descr = f'{self.alias}: invalid RPL source address' + if dst is None: pad = 0 cmpr_i = 0 cmpr_e = 0 ip_val = [ - cast('IPv6Address', ipaddress.ip_address(addr)).packed for addr in ip + cast('IPv6Address', parse_ip_address(addr, descr, version=6)).packed for addr in ip ] else: test_list = [dst.packed] # type: list[bytes] @@ -742,7 +766,7 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' if isinstance(item, bytes): test_list.append(item) else: - test_list.append(cast('IPv6Address', ipaddress.ip_address(item)).packed) + test_list.append(cast('IPv6Address', parse_ip_address(item, descr, version=6)).packed) prefix_i = os_path.commonprefix(test_list) cmpr_i = len(prefix_i) @@ -750,7 +774,7 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' if isinstance(ip[-1], bytes): test_list.append(ip[-1]) else: - test_list.append(cast('IPv6Address', ipaddress.ip_address(ip[-1])).packed) + test_list.append(cast('IPv6Address', parse_ip_address(ip[-1], descr, version=6)).packed) prefix_e = os_path.commonprefix(test_list) cmpr_e = len(prefix_e) @@ -761,11 +785,11 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' if isinstance(item, bytes): ip_val.append(item[cmpr_i:]) else: - ip_val.append(cast('IPv6Address', ipaddress.ip_address(item)).packed[cmpr_i:]) + ip_val.append(cast('IPv6Address', parse_ip_address(item, descr, version=6)).packed[cmpr_i:]) if isinstance(ip[-1], bytes): ip_val.append(ip[-1][cmpr_e:]) else: - ip_val.append(cast('IPv6Address', ipaddress.ip_address(ip[-1])).packed[cmpr_e:]) + ip_val.append(cast('IPv6Address', parse_ip_address(ip[-1], descr, version=6)).packed[cmpr_e:]) return Schema_RPL( cmpr_i=cmpr_i, diff --git a/pcapkit/protocols/link/arp.py b/pcapkit/protocols/link/arp.py index b37f34eb31..a06b66c8f9 100644 --- a/pcapkit/protocols/link/arp.py +++ b/pcapkit/protocols/link/arp.py @@ -41,11 +41,12 @@ import re import sys import textwrap -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from pcapkit.const.arp.hardware import Hardware as Enum_Hardware from pcapkit.const.arp.operation import Operation as Enum_Operation from pcapkit.const.reg.ethertype import EtherType as Enum_EtherType +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.protocols.data.link.arp import ARP as Data_ARP from pcapkit.protocols.data.link.arp import Address as Data_Address from pcapkit.protocols.data.link.arp import Type as Data_Type @@ -395,11 +396,35 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p object; otherwise, returns a raw :data:`str` representing the protocol address. + Raises: + FieldValueError: If ``addr`` is a :obj:`bool` (c.f. + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). + + Notes: + Through :func:`parse_ip_address` rather than + :class:`~ipaddress.IPv4Address`/:class:`~ipaddress.IPv6Address` + directly, because :obj:`bool` is an :class:`int` subclass that + either constructor accepts without complaint. Before this, + ``addr=True`` packed as ``00000001`` (IPv4) or ``::1`` (IPv6) with + no exception and no warning at all (c.f. #508, #540). + + The description below uses :attr:`self.__class__.__name__ + ` rather than :attr:`self.alias + `, even though the + latter is this project's usual choice: :attr:`alias` here reads + ``self._acnm``, which :meth:`read` only assigns once the operation + code is known, and this method runs from :meth:`make` -- before + :meth:`read` has ever executed on a construction-only instance. + Using :attr:`alias` would turn every call into an + :exc:`AttributeError`, bool or not. + """ if ptype == Enum_EtherType.Internet_Protocol_version_4: - return ipaddress.IPv4Address(addr).packed + return cast('IPv4Address', parse_ip_address( + addr, f'{self.__class__.__name__}: invalid protocol address', version=4)).packed if ptype == Enum_EtherType.Internet_Protocol_version_6: - return ipaddress.IPv6Address(addr).packed + return cast('IPv6Address', parse_ip_address( + addr, f'{self.__class__.__name__}: invalid protocol address', version=6)).packed if isinstance(addr, str): return addr.encode() diff --git a/pcapkit/protocols/link/ospf.py b/pcapkit/protocols/link/ospf.py index e5f8ee4227..02d67a6f8b 100644 --- a/pcapkit/protocols/link/ospf.py +++ b/pcapkit/protocols/link/ospf.py @@ -42,6 +42,7 @@ from pcapkit.const.ospf.authentication import Authentication as Enum_Authentication from pcapkit.const.ospf.packet import Packet as Enum_Packet from pcapkit.const.reg.transtype import TransType as Enum_TransType +from pcapkit.corekit.fields.ipaddress import parse_ip_address from pcapkit.protocols.data.link.ospf import OSPF as Data_OSPF from pcapkit.protocols.data.link.ospf import \ CrytographicAuthentication as Data_CrytographicAuthentication @@ -325,8 +326,29 @@ def _make_id_numbers(self, id: 'IPv4Address | str | bytes | bytearray') -> 'byte Returns: ID bytes. + Raises: + FieldValueError: If ``id`` is a :obj:`bool` (c.f. + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`). + + Notes: + Latent rather than live: nothing in this module calls this + method today (:meth:`make` builds ``router_id``/``area_id`` + straight from its own arguments), so the only caller is a unit + test. It is routed through :func:`parse_ip_address` anyway, so + that it does not resurface the defect the moment a caller + reaches it -- the same kind of omission is how #481's single-site + fix survived to become #491 and then #508 (c.f. #540). + + The description below uses :attr:`self.__class__.__name__ + ` rather than :attr:`self.alias + `, because ``alias`` + here reads ``self._version``, which :meth:`read` only assigns + from the wire -- unavailable to a construction-only instance that + never went through :meth:`read`. + """ - return ipaddress.ip_address(id).packed + return cast('IPv4Address', parse_ip_address( + id, f'{self.__class__.__name__}: invalid ID', version=4)).packed def _read_encrypt_auth(self, schema: 'Schema_CrytographicAuthentication') -> 'Data_CrytographicAuthentication': """Read Authentication field when Cryptographic Authentication is employed, diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 71699e8c61..9ff8fc0d9f 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -377,6 +377,86 @@ def test_ipv6_route_readers_and_constructors_cover_registered_types(self) -> Non ) self.assertEqual(rpl_from_data.cmpr_i, 1) + def test_ipv6_route_make_dst_rejects_a_bool(self) -> None: + """A :obj:`bool` destination address must not be silently converted. See #540. + + ``IPv6_Route.make``'s ``dst`` is converted to an :class:`~ipaddress.IPv6Address` + ahead of dispatching to the per-type constructor, via a bare + ``ipaddress.ip_address`` before this fix -- so ``dst=True`` converted to + ``::1`` with no exception at all. This is the most reachable of the four + #540 sites, being on the public ``make`` entry point rather than a private + helper. + """ + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + proto = object.__new__(IPv6_Route) + + with self.assertRaises(FieldValueError) as context: + proto.make(dst=True, data={'ip': ['2001:db8::1']}) # type: ignore[arg-type] + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn('int(True)', str(context.exception)) + + # a real destination still converts and dispatches normally -- default + # ``type`` is Source Route, whose constructor accepts but ignores ``dst``, + # so this exercises exactly the conversion above without depending on any + # other routing type's own behaviour + schema = proto.make(dst='2001:db8::ffff', data={'ip': ['2001:db8::1']}) + self.assertEqual(str(schema.data.ip[0]), '2001:db8::1') + + def test_ipv6_route_rpl_source_addresses_reject_a_bool(self) -> None: + """A :obj:`bool` entry in ``ip`` must not corrupt the RPL compression metadata. See #540. + + Worse than a plain packed-address defect: because ``cmpr_i``/``cmpr_e`` are + *derived* from the (previously unguarded) converted addresses, a bool + corrupted the compression metadata alongside the address list. Measured + before the fix: + + .. code-block:: text + + _make_data_type_rpl(ip=[True]) -> cmpr_i=0 cmpr_e=0 addresses=['00000001'] + _make_data_type_rpl(dst=2001:db8::1, ip=[True]) -> cmpr_i=16 cmpr_e=0 addresses=['00000001'] + + Both the ``dst is None`` branch and the ``dst`` branch (which separately + converts the ``cmpr_i``-prefix items and the last, ``cmpr_e``-suffix item) + are checked here, since each is its own call site of the same bare + conversion. + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + proto = object.__new__(IPv6_Route) + + sites = [ + ('dst=None', lambda: proto._make_data_type_rpl( + Routing.RPL_Source_Route_Header, ip=[True])), + ('dst given, bool is the only (cmpr_e) address', lambda: proto._make_data_type_rpl( + Routing.RPL_Source_Route_Header, dst=ip_address('2001:db8::1'), ip=[True])), + ('dst given, bool is a cmpr_i-prefix address', lambda: proto._make_data_type_rpl( + Routing.RPL_Source_Route_Header, dst=ip_address('2001:db8::1'), + ip=[True, '2001:db8::2'])), + ] + + for name, make in sites: + with self.subTest(site=name): + with self.assertRaises(FieldValueError) as context: + make() + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn('int(True)', str(context.exception)) + + # real addresses still compress and pack normally, with the metadata this + # defect would have corrupted intact + rpl = proto._make_data_type_rpl( + Routing.RPL_Source_Route_Header, + dst=ip_address('2001:db8::ffff'), + ip=['2001:db8::1', '2001:db8::2'], + ) + self.assertGreaterEqual(rpl.cmpr_i, 0) + self.assertGreaterEqual(rpl.cmpr_e, 0) + def test_ipv6_route_read_data_type_errors_report_real_routing_type(self) -> None: """Regression test for GH-442. diff --git a/tests/protocols/link/test_link_unit.py b/tests/protocols/link/test_link_unit.py index 49ea3f60fa..f70e41d312 100644 --- a/tests/protocols/link/test_link_unit.py +++ b/tests/protocols/link/test_link_unit.py @@ -256,6 +256,42 @@ def test_arp_make_builds_schema_with_resolved_addresses(self) -> None: self.assertEqual(schema.tha, b'112233445566') self.assertEqual(schema.tpa, b'\xc6\x33\x64\x02') + def test_arp_proto_resolve_rejects_a_bool(self) -> None: + """A :obj:`bool` protocol address must not be silently packed. See #540. + + ``bool`` is an :class:`int` subclass, so before this fix + ``ipaddress.IPv4Address(True)``/``ipaddress.IPv6Address(True)`` converted + without complaint. Measured before the fix: + + .. code-block:: text + + ARP._make_proto_resolve(True, IPv4) -> 00000001 (i.e. 0.0.0.1) + OSPF._make_id_numbers(True) -> 00000001 (i.e. 0.0.0.1) + + Both IPv4 and IPv6 branches share the same guard, so both are checked here. + """ + from pcapkit.const.reg.ethertype import EtherType + from pcapkit.protocols.link.arp import ARP + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + proto = object.__new__(ARP) + + for ptype in (EtherType.Internet_Protocol_version_4, EtherType.Internet_Protocol_version_6): + with self.subTest(ptype=ptype): + with self.assertRaises(FieldValueError) as context: + proto._make_proto_resolve(True, ptype) # type: ignore[arg-type] + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn('int(True)', str(context.exception)) + + # the fix must not disturb a real address on either family + self.assertEqual( + proto._make_proto_resolve('198.51.100.9', EtherType.Internet_Protocol_version_4), + b'\xc6\x33\x64\x09') + self.assertEqual( + proto._make_proto_resolve('2001:db8::9', EtherType.Internet_Protocol_version_6), + b'\x20\x01\x0d\xb8' + b'\x00' * 11 + b'\x09') + def test_vlan_make_data_preserves_tci_and_type(self) -> None: from pcapkit.const.reg.ethertype import EtherType from pcapkit.const.vlan.priority_level import PriorityLevel @@ -860,6 +896,34 @@ def test_ospf_properties_read_make_and_auth_helpers(self) -> None: with self.assertRaises(ProtocolError): maker._make_encrypt_auth(object()) + def test_ospf_id_numbers_rejects_a_bool(self) -> None: + """A :obj:`bool` router/area ID must not be silently packed. See #540. + + Latent rather than live: nothing in this module calls ``_make_id_numbers`` + today -- ``OSPF.make`` builds ``router_id``/``area_id`` from its own + arguments rather than through this helper -- so only a unit test (this one, + and the pre-existing one above) reaches it. It is fixed alongside the three + live sites anyway, so that it does not resurface the moment a future caller + reaches it. Measured before the fix: + + .. code-block:: text + + OSPF._make_id_numbers(True) -> 00000001 (i.e. 0.0.0.1) + """ + from pcapkit.protocols.link.ospf import OSPF + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + proto = object.__new__(OSPF) + + with self.assertRaises(FieldValueError) as context: + proto._make_id_numbers(True) # type: ignore[arg-type] + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn('int(True)', str(context.exception)) + + # a real ID still converts normally + self.assertEqual(proto._make_id_numbers('192.0.2.6'), b'\xc0\x00\x02\x06') + if __name__ == '__main__': unittest.main()