From 38f55c043841886982e01710b48bd880a43caf24 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 15 Oct 2026 20:29:38 -0400 Subject: [PATCH] fix(const): reject an out-of-range port in AppType.register (#1792) AppType.register neither called nor overrode _validate_value, so TCP.register(70000, 'x') minted a member that AppType.get(70000, proto='tcp') then returned, although get raises ValueError for a port outside 0-65535. - AppType overrides _validate_value with the check _missing_ makes and raises EnumValueError, as the registry-protocol convention asks of an override, with the message get raises, so register and get refuse the same ports in the same words. register calls it after its duplicate check, as EnumRegistry.register does. Template and generated module changed identically, including the EnumValueError import. - EnumLookup._validate_value's docstring no longer says AppType skips it. - Re-point the line citations the change shifts. - tests/const/test_const_apptype_register_range_1792_unit.py covers TCP, UDP, SCTP and DCCP; it fails on main. Closes #1792 --- .../conventions/mint-criterion.rst | 2 +- pcapkit/const/reg/apptype/apptype.py | 31 +++++ pcapkit/corekit/enum.py | 4 +- pcapkit/vendor/reg/apptype/apptype.py | 31 +++++ ..._const_apptype_register_range_1792_unit.py | 131 ++++++++++++++++++ tests/project/test_rfc_anchor_fragments.py | 2 +- 6 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 tests/const/test_const_apptype_register_range_1792_unit.py diff --git a/docs/source/contributing/conventions/mint-criterion.rst b/docs/source/contributing/conventions/mint-criterion.rst index 225dae6bc..962ebe81a 100644 --- a/docs/source/contributing/conventions/mint-criterion.rst +++ b/docs/source/contributing/conventions/mint-criterion.rst @@ -10,7 +10,7 @@ a ``_missing_`` override, which decides what happens when a value has no member. MRO, so they carry no declared-but-unassigned range to resolve. 121 define one themselves; the other four are the transport subclasses of :class:`~pcapkit.const.reg.apptype.apptype.AppType`, which inherit -``AppType._missing_`` (:file:`pcapkit/const/reg/apptype/apptype.py:2958`). Three +``AppType._missing_`` (:file:`pcapkit/const/reg/apptype/apptype.py:2994`). Three behaviours follow from it, and which one a given range gets is a **design decision, not a style preference**: diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index c45069caf..d77c96f76 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -17,6 +17,7 @@ from pcapkit.corekit.enum import EnumLookup, EnumRegistry from pcapkit.corekit.sentinels import NO_DEFAULT +from pcapkit.utilities.exceptions import EnumValueError __all__ = ['TransportProtocol', 'AppType'] @@ -2773,6 +2774,27 @@ def _sanitize_identifier(cls, name: 'str') -> 'str': identifier = f'{identifier}_' return identifier + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Reject ``value`` unless it is a port number in ``0``-``65535``. + + Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` + with the check :meth:`_missing_` makes, and the message it raises, so + :meth:`register` refuses exactly the ports :meth:`get` refuses, in the + same words (:issue:`1792`). + + Args: + value: Port number about to be minted. + + Raises: + EnumValueError: If ``value`` is not an :class:`int` in + ``0``-``65535``. A :exc:`ValueError`, so an ``except + ValueError`` caller is unaffected. + + """ + if not (isinstance(value, int) and 0 <= value <= 65535): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + @classmethod def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arguments-renamed """Mint the first, canonical member at ``port``, in this registry alone. @@ -2793,6 +2815,10 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg the operation where guessing the right transport for the caller is the wrong default. + Routes ``port`` through :meth:`_validate_value` after the duplicate + check, as :meth:`~pcapkit.corekit.enum.EnumRegistry.register` does, so + a port :meth:`get` refuses cannot be minted here either. + Args: port: Port number to register under. Must not already carry a member of this registry -- see :meth:`register_alias` for @@ -2811,6 +2837,10 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg is not a valid Python identifier at all; or if the identifier it does sanitise to already names an unrelated member of this registry. + EnumValueError: If ``port`` is not an :class:`int` in + ``0``-``65535``, from :meth:`_validate_value`. A + :exc:`ValueError`, carrying the message :meth:`get` raises for + the same port. """ if cls.__registry__ is None: @@ -2823,6 +2853,7 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg f'({", ".join(member.svc for member in existing)}); use ' f'{cls.__name__}.register_alias() to add a further service name ' 'for it') + cls._validate_value(port) identifier = cls._sanitize_identifier(name) if not identifier.isidentifier(): diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 990eb42a0..8c9b4ea37 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -218,9 +218,9 @@ def _validate_value(cls, value: 'Any') -> 'None': Of the registries that override :meth:`EnumRegistry.register` to mint through a path of their own, :meth:`OptionType.register ` - calls it at the same point and + and :meth:`AppType.register ` - does not. + both call it at the same point. :meth:`EnumRegistry.register_alias` does not call it, and does not need to: it refuses any ``value`` that is not already registered, so the value diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index a0e48c51d..638eb8cdc 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -110,6 +110,7 @@ from pcapkit.corekit.enum import EnumLookup, EnumRegistry from pcapkit.corekit.sentinels import NO_DEFAULT +from pcapkit.utilities.exceptions import EnumValueError __all__ = ['TransportProtocol', '{NAME}'] @@ -728,6 +729,27 @@ def _sanitize_identifier(cls, name: 'str') -> 'str': identifier = f'{{identifier}}_' return identifier + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Reject ``value`` unless it is a port number in ``0``-``65535``. + + Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` + with the check :meth:`_missing_` makes, and the message it raises, so + :meth:`register` refuses exactly the ports :meth:`get` refuses, in the + same words (:issue:`1792`). + + Args: + value: Port number about to be minted. + + Raises: + EnumValueError: If ``value`` is not an :class:`int` in + ``0``-``65535``. A :exc:`ValueError`, so an ``except + ValueError`` caller is unaffected. + + """ + if not ({FLAG}): + raise EnumValueError(f'{{value!r}} is not a valid {{cls.__name__}}') + @classmethod def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=arguments-renamed """Mint the first, canonical member at ``port``, in this registry alone. @@ -748,6 +770,10 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu the operation where guessing the right transport for the caller is the wrong default. + Routes ``port`` through :meth:`_validate_value` after the duplicate + check, as :meth:`~pcapkit.corekit.enum.EnumRegistry.register` does, so + a port :meth:`get` refuses cannot be minted here either. + Args: port: Port number to register under. Must not already carry a member of this registry -- see :meth:`register_alias` for @@ -766,6 +792,10 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu is not a valid Python identifier at all; or if the identifier it does sanitise to already names an unrelated member of this registry. + EnumValueError: If ``port`` is not an :class:`int` in + ``0``-``65535``, from :meth:`_validate_value`. A + :exc:`ValueError`, carrying the message :meth:`get` raises for + the same port. """ if cls.__registry__ is None: @@ -778,6 +808,7 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu f'({{", ".join(member.svc for member in existing)}}); use ' f'{{cls.__name__}}.register_alias() to add a further service name ' 'for it') + cls._validate_value(port) identifier = cls._sanitize_identifier(name) if not identifier.isidentifier(): diff --git a/tests/const/test_const_apptype_register_range_1792_unit.py b/tests/const/test_const_apptype_register_range_1792_unit.py new file mode 100644 index 000000000..d75a3ab8d --- /dev/null +++ b/tests/const/test_const_apptype_register_range_1792_unit.py @@ -0,0 +1,131 @@ +# -*- coding: utf-8 -*- +"""``AppType.register`` refuses a port that ``get`` refuses. + +GitHub issue #1792. :meth:`~pcapkit.const.reg.apptype.apptype.AppType.register` +neither called nor overrode +:meth:`~pcapkit.corekit.enum.EnumLookup._validate_value`, so ``TCP.register(70000, +'x')`` minted a member, and ``AppType.get(70000, proto='tcp')`` then returned it +although ``get`` raises :exc:`ValueError` for a port outside ``0``-``65535`` +(#758). ``AppType`` now overrides the hook with :meth:`_missing_`'s own check and +``register`` calls it after its duplicate check, as the base ``register`` does. +The override raises :exc:`~pcapkit.utilities.exceptions.EnumValueError`, as the +registry-protocol convention asks of one, with the message ``get`` raises. + +Each test imports the registries after +:func:`~tests._support.reimport_once_per_class`, whose ``restore`` puts the +class's import back afterwards, so a member minted here does not leak. + +""" +from __future__ import annotations + +import itertools +import unittest + +from tests._support import reimport_once_per_class + +#: Serial for probe names, so that no refusal is a name collision instead. +_SERIAL = itertools.count() + +#: Ports outside ``0``-``65535``, on both sides of the range. +OUT_OF_RANGE = (-1, 65536, 70000) + +#: Ports that are not an :class:`int` at all, and hash like no registered port. +NOT_AN_INT = ('8080', 1.5) + + +def _registries() -> 'tuple[type, ...]': + from pcapkit.const.reg.apptype import DCCP, SCTP, TCP, UDP + + return TCP, UDP, SCTP, DCCP + + +class AppTypeRegisterRange1792Tests(unittest.TestCase): + """``register`` and ``get`` agree on what a port number is.""" + + def setUp(self) -> None: + reimport_once_per_class(self, restore=True) + + def assertRefusedWithoutMinting(self, registry: 'type', port: 'object') -> 'str': + """Assert ``registry.register(port, ...)`` raises and mints nothing. + + The refusal is an :exc:`~pcapkit.utilities.exceptions.EnumValueError`, + and so still a :exc:`ValueError` to an ``except ValueError`` caller. + + Returns: + The message the refusal carried. + + """ + from pcapkit.utilities.exceptions import EnumValueError + + members = dict(registry.__members__) # type: ignore[attr-defined] + with self.assertRaises(ValueError) as caught: + registry.register(port, f'pypcapkit-1792-probe-{next(_SERIAL)}') # type: ignore[attr-defined] + self.assertIsInstance(caught.exception, ValueError) + self.assertIs(type(caught.exception), EnumValueError) + self.assertEqual(dict(registry.__members__), members) # type: ignore[attr-defined] + self.assertFalse(registry.__registry__.getlist(port)) # type: ignore[attr-defined] + return str(caught.exception) + + def test_register_refuses_an_out_of_range_port_as_get_does(self) -> None: + for registry in _registries(): + for port in OUT_OF_RANGE: + with self.subTest(registry=registry.__name__, port=port): + message = self.assertRefusedWithoutMinting(registry, port) + with self.assertRaises(ValueError) as lookup: + registry.get(port) # type: ignore[attr-defined] + self.assertEqual(message, str(lookup.exception)) + self.assertEqual(message, f'{port!r} is not a valid {registry.__name__}') + + def test_register_refuses_a_non_int_port_as_the_constructor_does(self) -> None: + for registry in _registries(): + for port in NOT_AN_INT: + with self.subTest(registry=registry.__name__, port=port): + message = self.assertRefusedWithoutMinting(registry, port) + with self.assertRaises(ValueError) as built: + registry(port) + self.assertEqual(message, str(built.exception)) + + def test_get_still_raises_after_a_refused_register(self) -> None: + from pcapkit.const.reg.apptype import AppType + + for registry in _registries(): + for port in OUT_OF_RANGE: + with self.subTest(registry=registry.__name__, port=port): + with self.assertRaises(ValueError): + registry.register(port, f'pypcapkit-1792-probe-{next(_SERIAL)}') # type: ignore[attr-defined] + with self.assertRaises(ValueError): + registry.get(port) # type: ignore[attr-defined] + with self.assertRaises(ValueError): + AppType.get(port, proto=registry.__transport__) # type: ignore[attr-defined] + + def test_register_still_mints_the_top_port_of_the_range(self) -> None: + from pcapkit.const.reg.apptype import AppType + + port = 65535 + for registry in _registries(): + with self.subTest(registry=registry.__name__): + self.assertFalse(registry.__registry__.getlist(port)) # type: ignore[attr-defined] + member = registry.register(port, 'pypcapkit-1792-top') # type: ignore[attr-defined] + self.assertEqual(member.port, port) + self.assertIs(member.proto, registry.__transport__) # type: ignore[attr-defined] + self.assertIs(registry.get(port), member) # type: ignore[attr-defined] + self.assertIs(AppType.get(port, proto=registry.__transport__), member) # type: ignore[attr-defined] + + def test_validate_value_agrees_with_missing_on_every_probe(self) -> None: + from pcapkit.utilities.exceptions import EnumValueError + + for registry in _registries(): + for port in (0, 65535, *OUT_OF_RANGE, *NOT_AN_INT): + with self.subTest(registry=registry.__name__, port=port): + try: + registry._missing_(port) # type: ignore[attr-defined] + except ValueError as error: + with self.assertRaises(EnumValueError) as caught: + registry._validate_value(port) # type: ignore[attr-defined] + self.assertEqual(str(caught.exception), str(error)) + else: + self.assertIsNone(registry._validate_value(port)) # type: ignore[attr-defined] + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/project/test_rfc_anchor_fragments.py b/tests/project/test_rfc_anchor_fragments.py index 4e56785ae..670a0715a 100644 --- a/tests/project/test_rfc_anchor_fragments.py +++ b/tests/project/test_rfc_anchor_fragments.py @@ -41,7 +41,7 @@ test exists to catch, the same mistake as the allowlist above one layer down. The generators that produce them, :file:`pcapkit/vendor/tcp/mp_tcp_option.py:59` and -:file:`pcapkit/vendor/reg/apptype/apptype.py:1169` (both parsing IANA's +:file:`pcapkit/vendor/reg/apptype/apptype.py:1200` (both parsing IANA's ``RFC8684, Section 3.1`` with the same ``re.fullmatch(r'RFC(?P\\d+)(, Section (?P.*?))?', ...)``, which discards the literal word ``Section`` and keeps only the number), are fixed to emit ``#section-{sec}``, and the ten