From b5f33f85b84fb510dbae1d6360f0b76ffff74f30 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 01:43:09 -0400 Subject: [PATCH 1/2] corekit: raise FieldValueError, not a bare ValueError, for a malformed IP field value _IPAddressField.pre_process let a bare ValueError from ipaddress.ip_address() escape when a value was malformed, one line above where it already raises the library's own FieldValueError for a value that is merely the wrong IP version -- so `except BaseError` could not reliably catch a bad field value; whether the exception was in-library depended on how the value was wrong. The same unguarded-conversion pattern was in every pre_process/post_process of this module. Fixes #465. - pcapkit/corekit/fields/ipaddress.py: adds a _reraise_as_field_value_error context manager and wraps every ipaddress.* conversion in it, preserving the original message via `from error`. Fixes two independently-confirmed defects: the malformed-value escape in all four public classes' pre_process (as reported), and a second one found while checking reachability of the post_process sites -- IPv4InterfaceField.post_process raised the same bare ValueError when the wire's trailing four "netmask" octets are not a contiguous netmask (e.g. 0.255.0.255), reachable through unpack() alone. The remaining post_process sites are not reachable -- wire bytes there are always exactly the field's fixed length, and any such octet string converts cleanly, or the value is already range-checked before use -- but are wrapped too for consistency. Raises: sections added/extended to match. - tests/corekit/test_fields_ipaddress.py: new cases pin FieldValueError (and BaseError) for a malformed value on all four public field classes, a case for the newly found post_process netmask defect, and a case pinning that the pre-existing wrong-version FieldValueError message is not relabelled as a malformed-value message. Full suite: 1009 passed, 17 skipped, 1557 subtests, at PYTHONSAFEPATH=1, interpreter 3.14.7. Baseline at fa128959e (origin/main): 1006 passed, 17 skipped, 1553 subtests, before the 3 new tests existed. mypy (Makefile's invocation) reports the same 125 errors in 40 files before and after -- the only ipaddress.py lines it flags are two pre-existing return-value errors, unrelated to this change and merely shifted by the new lines. --- pcapkit/corekit/fields/ipaddress.py | 108 ++++++++++++++++++++++--- tests/corekit/test_fields_ipaddress.py | 77 ++++++++++++++++++ 2 files changed, 174 insertions(+), 11 deletions(-) diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index 35b5a6c678..d1911b7189 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -3,6 +3,7 @@ import abc import ipaddress +from contextlib import contextmanager from typing import TYPE_CHECKING, Generic, TypeVar, cast from pcapkit.corekit.fields.field import Field, NoValue @@ -15,7 +16,7 @@ if TYPE_CHECKING: from ipaddress import IPv4Address, IPv4Interface, IPv6Address, IPv6Interface - from typing import Any, Callable + from typing import Any, Callable, Iterator from typing_extensions import Literal, Self @@ -28,6 +29,45 @@ _IT = TypeVar('_IT', 'IPv4Interface', 'IPv6Interface') +@contextmanager +def _reraise_as_field_value_error(description: str) -> 'Iterator[None]': + """Translate a bare :exc:`ValueError` from :mod:`ipaddress` into :exc:`FieldValueError`. + + Every conversion in this module ultimately calls into the stdlib + :mod:`ipaddress` module, which raises a bare :exc:`ValueError` (or a + subclass of it, e.g. :exc:`~ipaddress.AddressValueError` or + :exc:`~ipaddress.NetmaskValueError`) for a malformed value. Left alone, + that exception is not an instance of + :exc:`~pcapkit.utilities.exceptions.BaseError`, unlike every other + exception this module raises -- so a caller cannot rely on + ``except BaseError`` to catch a bad field value. Wrapping the conversion + in this context manager re-raises it as :exc:`FieldValueError` instead, + preserving the original message. + + Args: + description: Human-readable description of the value being + converted, used to build the :exc:`FieldValueError` message. + + Raises: + FieldValueError: If the code inside the ``with`` block raises + :exc:`ValueError`. + + """ + try: + yield + except FieldValueError: + # NOTE: ``FieldValueError`` is itself a ``ValueError``, so without this + # clause first, a ``FieldValueError`` raised inside the ``with`` block + # (e.g. a version-mismatch check) would be caught below and re-wrapped, + # losing its original message. Callers are expected to keep such + # raises outside the ``with`` block, but this is the same ordering + # trap ``ProtocolError`` carries at ``exceptions.py``, so it is guarded + # here too rather than relied upon by convention alone. + raise + except ValueError as error: + raise FieldValueError(f'{description}: {error}') from error + + class _IPField(Field[_T], Generic[_T]): """Internal IP related value for protocol fields. @@ -64,11 +104,16 @@ def pre_process(self, value: '_AT | bytes | int | str', packet: 'dict[str, Any]' Returns: Processed field value. + Raises: + FieldValueError: If ``value`` is not a valid IP address, or if it + is the wrong IP version for this field. + """ if isinstance(value, (ipaddress.IPv4Address, ipaddress.IPv6Address)): ip = value # type: IPv4Address | IPv6Address else: - ip = ipaddress.ip_address(value) + with _reraise_as_field_value_error('invalid IP address'): + ip = ipaddress.ip_address(value) if ip.version != self.version: raise FieldValueError(f'IP version mismatch: {ip.version} != {self.version}') @@ -84,8 +129,17 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> '_AT': Returns: Processed field value. + Raises: + FieldValueError: If ``value`` is the wrong IP version for this + field. ``value`` cannot actually fail the underlying + :func:`ipaddress.ip_address` conversion here -- it is always + exactly 4 or 16 octets, fixed by this field's length, and any + such octet string is a valid address -- but the conversion is + still wrapped for consistency with the rest of this module. + """ - val = ipaddress.ip_address(value) + with _reraise_as_field_value_error('invalid IP address'): + val = ipaddress.ip_address(value) if val.version != self.version: raise FieldValueError(f'IP version mismatch: {val.version} != {self.version}') return val # type: ignore[return-value] @@ -179,11 +233,16 @@ def pre_process(self, value: 'IPv4Interface | bytes | int | str', packet: 'dict[ Returns: Processed field value. + Raises: + FieldValueError: If ``value`` is not a valid IP interface, or if + it is the wrong IP version for this field. + """ if isinstance(value, ipaddress.IPv4Interface): val = value else: - val = ipaddress.ip_interface(value) # type: ignore[assignment] + with _reraise_as_field_value_error('invalid IP interface'): + val = ipaddress.ip_interface(value) # type: ignore[assignment] if val.version != self.version: raise FieldValueError(f'IP version mismatch: {val.version} != {self.version}') @@ -201,16 +260,27 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'IPv4Interfa Returns: Processed field value. + Raises: + FieldValueError: If the trailing four octets are not a valid + dotted netmask, or if the resulting interface is the wrong IP + version for this field. The leading four octets cannot + actually fail here -- they are always exactly 4 octets, fixed + by this field's length, and any such octet string is a valid + address -- but the conversion is still wrapped for + consistency with the rest of this module. + Notes: The trailing four octets are a dotted netmask, as written by :meth:`pre_process` -- not a prefix length as in :meth:`IPv6InterfaceField.post_process`. """ - ip = ipaddress.IPv4Address(value[:4]) - mask = ipaddress.IPv4Address(value[4:]) + with _reraise_as_field_value_error('invalid IPv4 address'): + ip = ipaddress.IPv4Address(value[:4]) + mask = ipaddress.IPv4Address(value[4:]) - val = ipaddress.ip_interface(f'{ip}/{mask}') + with _reraise_as_field_value_error('invalid IPv4 interface'): + val = ipaddress.ip_interface(f'{ip}/{mask}') if val.version != self.version: raise FieldValueError(f'IP version mismatch: {val.version} != {self.version}') return val @@ -247,11 +317,16 @@ def pre_process(self, value: 'IPv6Interface | bytes | int | str', packet: 'dict[ Returns: Processed field value. + Raises: + FieldValueError: If ``value`` is not a valid IP interface, or if + it is the wrong IP version for this field. + """ if isinstance(value, ipaddress.IPv6Interface): val = value else: - val = ipaddress.ip_interface(value) # type: ignore[assignment] + with _reraise_as_field_value_error('invalid IP interface'): + val = ipaddress.ip_interface(value) # type: ignore[assignment] if val.version != self.version: raise FieldValueError(f'IP version mismatch: {val.version} != {self.version}') @@ -271,7 +346,16 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'IPv6Interfa Raises: FieldValueError: If the trailing octet is not a valid IPv6 prefix - length, i.e. greater than 128. + length, i.e. greater than 128, or if the resulting interface + is the wrong IP version for this field. Neither the leading + sixteen octets nor the final :func:`ipaddress.ip_interface` + call can actually fail here -- the former is always exactly + 16 octets, fixed by this field's length, and any such octet + string is a valid address; the latter is only ever reached + once the prefix length has already been checked above, and + any prefix length in ``0..128`` is valid. Both conversions + are still wrapped for consistency with the rest of this + module. Notes: The trailing octet is the prefix length as a binary integer, as @@ -279,13 +363,15 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'IPv6Interfa :meth:`IPv4InterfaceField.post_process`. """ - ip = ipaddress.IPv6Address(value[:16]) + with _reraise_as_field_value_error('invalid IPv6 address'): + ip = ipaddress.IPv6Address(value[:16]) prefixlen = value[16] if prefixlen > 128: raise FieldValueError(f'invalid IPv6 prefix length: {prefixlen}') - val = ipaddress.ip_interface(f'{ip}/{prefixlen}') + with _reraise_as_field_value_error('invalid IPv6 interface'): + val = ipaddress.ip_interface(f'{ip}/{prefixlen}') if val.version != self.version: raise FieldValueError(f'IP version mismatch: {val.version} != {self.version}') return val diff --git a/tests/corekit/test_fields_ipaddress.py b/tests/corekit/test_fields_ipaddress.py index cab607e131..7ff6a901b4 100644 --- a/tests/corekit/test_fields_ipaddress.py +++ b/tests/corekit/test_fields_ipaddress.py @@ -151,6 +151,83 @@ def test_ipv4_interface_rejects_the_other_version(self) -> None: with self.assertRaises(FieldValueError): IPv4InterfaceField().pre_process('2001:db8::1/64', {}) + def test_pre_process_malformed_value_raises_in_library_error(self) -> None: + """A malformed address/interface string used to let a bare + :exc:`ValueError` from :mod:`ipaddress` escape -- unlike the very next + statement in the same method, which already raised the library's own + :exc:`FieldValueError` for a value that is merely the wrong IP + version. So ``except BaseError`` could not reliably catch a bad field + value: whether the exception was in-library depended on *how* the + value was wrong. All four public field classes must now raise + :exc:`FieldValueError` here too, with the original :mod:`ipaddress` + message preserved. + """ + from pcapkit.corekit.fields.ipaddress import ( + IPv4AddressField, IPv6AddressField, IPv4InterfaceField, IPv6InterfaceField, + ) + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + cases = [ + (IPv4AddressField(), 'not-an-address'), + (IPv6AddressField(), 'not-an-address'), + (IPv4InterfaceField(), 'not-an-interface'), + (IPv6InterfaceField(), 'not-an-interface'), + ] + for field, bad_value in cases: + with self.subTest(field=type(field).__name__): + with self.assertRaises(FieldValueError) as context: + field.pre_process(bad_value, {}) + # ``FieldValueError`` subclasses ``BaseError``, but assert the + # in-library type directly (above) rather than only this. + self.assertIsInstance(context.exception, BaseError) + # the original stdlib ``ipaddress`` message must survive the + # translation, not just some generic replacement text + self.assertIn(repr(bad_value), str(context.exception)) + + def test_wrong_version_message_is_not_relabelled_as_a_malformed_value(self) -> None: + """Wrapping the conversion must not broaden into swallowing the + pre-existing wrong-version ``FieldValueError`` -- its message stays + the version-mismatch message, not the "invalid IP ..." message used + for a genuinely malformed value. + """ + from pcapkit.corekit.fields.ipaddress import IPv6AddressField, IPv6InterfaceField + from pcapkit.utilities.exceptions import FieldValueError + + with self.assertRaises(FieldValueError) as address_context: + IPv6AddressField().pre_process(ipaddress.IPv4Address('1.2.3.4'), {}) + self.assertIn('IP version mismatch', str(address_context.exception)) + self.assertNotIn('invalid IP', str(address_context.exception)) + + with self.assertRaises(FieldValueError) as interface_context: + IPv6InterfaceField().pre_process(ipaddress.IPv4Interface('1.2.3.4/24'), {}) + self.assertIn('IP version mismatch', str(interface_context.exception)) + self.assertNotIn('invalid IP', str(interface_context.exception)) + + 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 + trailing four octets are meant to be a dotted netmask. Unlike the + leading four octets (always exactly 4 octets, so always a valid + address), those trailing octets are not guaranteed to form a + *contiguous* netmask -- e.g. a capture with a malformed or corrupted + IPv4 interface option can carry ``0.255.0.255``, which + :func:`ipaddress.ip_interface` rejects with a bare + :exc:`~ipaddress.NetmaskValueError`. This is reachable through + :meth:`~pcapkit.corekit.fields.field.FieldBase.unpack` alone, with no + malformed-length input required, and must raise :exc:`FieldValueError` + instead. + """ + from pcapkit.corekit.fields.ipaddress import IPv4InterfaceField + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + field = IPv4InterfaceField() + raw = ipaddress.IPv4Address('1.2.3.4').packed + bytes([0, 255, 0, 255]) + + with self.assertRaises(FieldValueError) as context: + field.unpack(raw, {}) + self.assertIsInstance(context.exception, BaseError) + self.assertIn('0.255.0.255', str(context.exception)) + if __name__ == '__main__': unittest.main() From 8273f35757d663fca1bfae1854977a727a12861d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 08:51:38 -0400 Subject: [PATCH 2/2] corekit: import contextlib rather than its contextmanager name (#465) --- pcapkit/corekit/fields/ipaddress.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index d1911b7189..b2d144695e 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -2,8 +2,8 @@ """IP address field class""" import abc +import contextlib import ipaddress -from contextlib import contextmanager from typing import TYPE_CHECKING, Generic, TypeVar, cast from pcapkit.corekit.fields.field import Field, NoValue @@ -29,7 +29,7 @@ _IT = TypeVar('_IT', 'IPv4Interface', 'IPv6Interface') -@contextmanager +@contextlib.contextmanager def _reraise_as_field_value_error(description: str) -> 'Iterator[None]': """Translate a bare :exc:`ValueError` from :mod:`ipaddress` into :exc:`FieldValueError`.