fix(arp): reject bool and non-bytes addresses for non-IP ptypes (#2166)

pcapkit/protocols/link/arp.py

@@ -52,7 +52,7 @@
 from pcapkit.protocols.link.link import Link
 from pcapkit.protocols.schema.link.arp import ARP as Schema_ARP
 from pcapkit.utilities.compat import cached_property
-from pcapkit.utilities.exceptions import ProtocolError
+from pcapkit.utilities.exceptions import FieldValueError, ProtocolError
 
 if TYPE_CHECKING:
     from enum import IntEnum as StdlibEnum
@@ -403,15 +403,31 @@ def _make_addr_resolve(self, addr: 'str | bytes', htype: 'int') -> 'bytes':
             is validated and packed, one octet per group (an empty string packs
             as no octets); otherwise, a :class:`str` is
             taken as the *hex* encoded address that :meth:`_read_addr_resolve`
-            returns, and :obj:`bytes` are returned as given.
+            returns, and :class:`bytes`/:class:`bytearray` are returned as
+            :class:`bytes`.
 
         Raises:
+            FieldValueError: If ``addr`` is a :obj:`bool`, whatever ``htype``
+                is.
             ProtocolError: If ``htype`` is ``1`` and ``addr`` is not a
-                well-formed MAC address, or if ``htype`` is not ``1`` and
-                ``addr`` is a :class:`str` that is not *hex* encoded.
+                well-formed MAC address given as :class:`str`,
+                :class:`bytes` or :class:`bytearray`, or if ``htype`` is not
+                ``1`` and ``addr`` is either a :class:`str` that is not *hex*
+                encoded or none of :class:`str`, :class:`bytes` and
+                :class:`bytearray`.
 
         """
+        # NOTE: checked before the ``htype`` dispatch, for the same reason as
+        # in ``_make_proto_resolve``: without it a ``bool`` reaches the MAC
+        # pattern or falls through unchanged, and fails later as a
+        # ``TypeError`` (#2166).
+        if isinstance(addr, bool):
+            raise FieldValueError(
+                f'{self.__class__.__name__}: invalid hardware address: must not be a bool, '
+                f'not {addr!r} -- pass int({addr!r}) if the numeric value is what is wanted')
         if htype == Enum_Hardware.Ethernet:
+            if not isinstance(addr, (str, bytes, bytearray)):
+                raise ProtocolError(f'Invalid MAC address: {addr!r}')
             _addr = addr.encode() if isinstance(addr, str) else addr
             if PAT_MAC_ADDR.fullmatch(_addr) is not None:
                 return bytes.fromhex(_addr.replace(b':', b'').replace(b'-', b'').decode())
@@ -421,7 +437,9 @@ def _make_addr_resolve(self, addr: 'str | bytes', htype: 'int') -> 'bytes':
                 return bytes.fromhex(addr.replace(':', '').replace('-', ''))
             except ValueError:
                 raise ProtocolError(f'Invalid hardware address: {addr!r}') from None
-        return addr
+        if isinstance(addr, (bytes, bytearray)):
+            return bytes(addr)
+        raise ProtocolError(f'Invalid hardware address: {addr!r}')
 
     def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', ptype: 'int',
                             plen: 'Optional[int]' = None) -> 'bytes':
@@ -439,15 +457,18 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p
             octets; otherwise, a :class:`str` is taken as the *hex* encoded
             address that :meth:`_read_proto_resolve` returns, an
             :class:`~ipaddress.IPv4Address`/:class:`~ipaddress.IPv6Address` is
-            packed, and anything else is returned as given.
+            packed, and :class:`bytes`/:class:`bytearray` are returned as
+            :class:`bytes`.
 
         Raises:
-            FieldValueError: If ``addr`` is packed as an IPv4 or IPv6 address
-                and is a :obj:`bool`, not a valid address, or not of that
-                version (see
+            FieldValueError: If ``addr`` is a :obj:`bool`, whatever ``ptype``
+                is, or is packed as an IPv4 or IPv6 address and is not a
+                valid address or not of that version (see
                 :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`).
-            ProtocolError: If ``ptype`` is neither IPv4 nor IPv6 and ``addr``
-                is a :class:`str` that is not *hex* encoded.
+            ProtocolError: If the address is not packed as IPv4 or IPv6, and
+                ``addr`` is either a :class:`str` that is not *hex* encoded or
+                none of :class:`str`, an IP address, :class:`bytes` and
+                :class:`bytearray`.
 
         Notes:
             Through :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` rather than
@@ -468,6 +489,15 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p
             :exc:`AttributeError`, bool or not.
 
         """
+        # NOTE: checked before the ``ptype`` dispatch, for the placement reason
+        # ``_reject_bool`` in ``pcapkit.corekit.fields.ipaddress`` gives: the
+        # non-IP branches below never reach ``parse_ip_address``, so without it
+        # a ``bool`` falls through them unchanged and fails later as a
+        # ``TypeError`` from ``len()`` (#2166).
+        if isinstance(addr, bool):
+            raise FieldValueError(
+                f'{self.__class__.__name__}: invalid protocol address: must not be a bool, '
+                f'not {addr!r} -- pass int({addr!r}) if the numeric value is what is wanted')
         if ptype == Enum_EtherType.Internet_Protocol_version_4 and plen in (None, 4):
             return cast('IPv4Address', parse_ip_address(
                 addr, f'{self.__class__.__name__}: invalid protocol address', version=4)).packed
@@ -483,7 +513,9 @@ def _make_proto_resolve(self, addr: 'IPv4Address | IPv6Address | str | bytes', p
                                     f'{addr!r}') from None
         if isinstance(addr, (ipaddress.IPv4Address, ipaddress.IPv6Address)):
             return addr.packed
-        return addr
+        if isinstance(addr, (bytes, bytearray)):
+            return bytes(addr)
+        raise ProtocolError(f'{self.__class__.__name__}: invalid protocol address: {addr!r}')
 
 
 class InARP(ARP, schema=Schema_ARP, data=Data_ARP):

tests/protocols/link/test_arp_proto_addr_length_unit.py

@@ -17,7 +17,7 @@
 from pcapkit.const.arp.hardware import Hardware
 from pcapkit.const.reg.ethertype import EtherType
 from pcapkit.protocols.link.arp import ARP
-from pcapkit.utilities.exceptions import ProtocolError
+from pcapkit.utilities.exceptions import FieldValueError, ProtocolError
 
 #: A protocol type that is neither IPv4 nor IPv6.
 NON_IP = EtherType.get(0x88B5)
@@ -52,6 +52,84 @@ def test_from_data_round_trips(self) -> None:
                 self.assertEqual(ARP.from_data(parsed.info).data, data)
 
 
+class TestARPNonIPAddressType(unittest.TestCase):
+    """A non-IP ``ptype`` checks the address type too. See #2166.
+
+    Only the IPv4 and IPv6 branches went through ``parse_ip_address``, so any
+    other ``ptype`` returned a :obj:`bool` or :class:`int` unchanged, and
+    :meth:`~pcapkit.protocols.link.arp.ARP.make` then failed with ``TypeError:
+    object of type 'bool' has no len()``.
+    """
+
+    def test_bool_raises_field_value_error(self) -> None:
+        cases = {
+            'non-IP': dict(ptype=NON_IP),
+            'IPv4 with plen 6': dict(ptype=EtherType.Internet_Protocol_version_4, plen=6),
+        }
+        for name, kwargs in cases.items():
+            for value in (True, False):
+                with self.subTest(case=name, value=value):
+                    with self.assertRaises(FieldValueError) as context:
+                        ARP(spa=value, tpa=value, **kwargs)
+                    self.assertIn('must not be a bool', str(context.exception))
+
+    def test_non_bytes_raises_protocol_error(self) -> None:
+        for value in (5, 1.5, None, ['a1', 'b2', 'c3']):
+            with self.subTest(value=value):
+                with self.assertRaises(ProtocolError):
+                    ARP(ptype=NON_IP, spa=value, tpa=value)
+
+    def test_bytes_and_bytearray_pack_as_given(self) -> None:
+        proto = ARP(ptype=NON_IP, spa=b'\xa1\xb2\xc3', tpa=bytearray(b'\xd4\xe5\xf6'),
+                    sha='01:23:45:67:89:ab', tha='fe:dc:ba:98:76:54')
+        self.assertEqual(proto.data, ARP_NON_IP)
+
+
+class TestARPHardwareAddressType(unittest.TestCase):
+    """The hardware address resolver checks the address type too. See #2166.
+
+    A :obj:`bool` or :class:`int` ``sha``/``tha`` reached the MAC pattern
+    (``TypeError: expected string or bytes-like object``) for Ethernet, and fell
+    through unchanged (``TypeError: object of type 'bool' has no len()``) for
+    any other ``htype``.
+    """
+
+    HTYPES = {'Ethernet': Hardware.Ethernet, 'non-Ethernet': Hardware.Fibre_Channel}
+
+    def test_bool_raises_field_value_error(self) -> None:
+        for name, htype in self.HTYPES.items():
+            for value in (True, False):
+                with self.subTest(htype=name, value=value):
+                    with self.assertRaises(FieldValueError) as context:
+                        ARP(htype=htype, sha=value, tha=value)
+                    self.assertIn('must not be a bool', str(context.exception))
+
+    def test_unsupported_type_raises_protocol_error(self) -> None:
+        for name, htype in self.HTYPES.items():
+            for value in (5, None, [1, 2, 3, 4, 5, 6], memoryview(b'\x01' * 6)):
+                with self.subTest(htype=name, value=value):
+                    with self.assertRaises(ProtocolError):
+                        ARP(htype=htype, sha=value, tha=value)
+
+    def test_supported_forms_still_pack(self) -> None:
+        raw = bytes.fromhex('0123456789ab')
+        cases = (
+            (Hardware.Ethernet, '01:23:45:67:89:ab'),
+            (Hardware.Ethernet, '01-23-45-67-89-ab'),
+            (Hardware.Ethernet, b'01:23:45:67:89:ab'),
+            (Hardware.Ethernet, bytearray(b'01:23:45:67:89:ab')),
+            (Hardware.Fibre_Channel, '0123456789ab'),
+            (Hardware.Fibre_Channel, '01:23:45:67:89:ab'),
+            (Hardware.Fibre_Channel, raw),
+            (Hardware.Fibre_Channel, bytearray(raw)),
+        )
+        for htype, value in cases:
+            with self.subTest(htype=htype.name, value=value):
+                proto = ARP(htype=htype, sha=value, tha=value)
+                self.assertEqual(proto.data[8:14], raw)
+                self.assertEqual(proto.data[18:24], raw)
+
+
 class TestARPLengthFields(unittest.TestCase):
     """``hlen``/``plen`` are derived when omitted and checked when given."""