From 0e616ac97f8140cc0ba18f519d77b7073dc6ff0d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 14:49:23 -0400 Subject: [PATCH] fix(tcp): drop the doubled separator from four MPTCP error messages (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four `ProtocolError` messages in the MPTCP option handlers were spelled `f'{self.alias}: : [OptNo …]'`, so they rendered with an empty field between the protocol alias and the option number: TCP: : [OptNo 30] 1: invalid flags combination Cosmetic -- the exception type, the option number and the subtype were always right, and nothing downstream parses these strings -- but it is the string a user sees when an MP_JOIN or DSS option is rejected, and an empty field reads as a value that failed to interpolate. * Four sites, not the one it was reported against. `_read_mptcp_join` and `_make_mptcp_join` ("invalid flags combination"), and both guards of `_make_mptcp_dss` ("missing required fields"). The other three were found by grepping for the pattern; `grep -rn "alias}: : " pcapkit/` returns nothing outside this file. * The correct form is the local majority, not a preference: the same file spells the same prefix without the doubling 28 other times, all `f'{self.alias}: [OptNo {schema.kind}] …'`. The count is 32 after this change and the doubled count is 0. * New `test_tcp_mptcp_error_message_unit.py` asserts each message in **full** rather than by substring, which is the point: the two places that already exercise these messages both match a substring straddling the defect and so pass either side of it -- `test_tcp_mptcp_join_flag_ordering_unit.py:404` (`assertIn`, against `_make_mptcp_join`) and `test_tcp_udp_unit.py:1629` (`assertRaisesRegex`, against `_read_mptcp_join`). Two, not one. Neither module is touched. * The two `_make_mptcp_dss` guards carry the identical message, so they are reached separately -- `dsn` without its data-level fields, and a data-level field without a `dsn` -- since asserting the string once would not show both were fixed. Coverage confirms all four `raise` lines execute. * The test also sweeps the source for the pattern and pins the 32, so a fifth site cannot be added unnoticed and "fixing" the four by deleting them would not pass. That exactness makes it a tripwire on the whole module, so the assertion carries a note telling a future editor to update the number rather than loosen it. All four lines arrived together in `3bba8a1748` (2023-04-10) and were untouched until now. `tests/protocols/transport/` unit tier passes 140 tests and 105 subtests, up from 136 and 103. The new module is 4 tests and 2 subtests, and fails on `main` with `'TCP: : [OptNo 30] 1: invalid flags combination' != 'TCP: [OptNo 30] 1: invalid flags combination'` among 5 failures. `pcapkit/protocols/transport/tcp.py` holds 100% statement and branch coverage either side, at an unchanged 594 statements and 198 branches -- the change is to string literals, so it adds no statement to cover. The seven `test_tcp_runtime.py`/`test_udp_runtime.py` failures in the full directory are the fixture-dependent tier raising `FileNotFoundError` in a tree where `make samples` has not been run, and are unaffected by this commit. Fixes #649 --- pcapkit/protocols/transport/tcp.py | 8 +- .../test_tcp_mptcp_error_message_unit.py | 239 ++++++++++++++++++ 2 files changed, 243 insertions(+), 4 deletions(-) create mode 100644 tests/protocols/transport/test_tcp_mptcp_error_message_unit.py diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index 2043b855d..5df9b6613 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -1574,7 +1574,7 @@ def _read_mptcp_join(self, schema: 'Schema_MPTCPJoin', *, options: 'Option') -> return self._read_join_synack(schema, options=options) # type: ignore[arg-type] if Enum_Flags.SYN not in self._flags and Enum_Flags.ACK in self._flags: # MP_JOIN-ACK return self._read_join_ack(schema, options=options) # type: ignore[arg-type] - raise ProtocolError(f'{self.alias}: : [OptNo {schema.kind}] {schema.subtype}: invalid flags combination') + raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] {schema.subtype}: invalid flags combination') def _read_join_syn(self, schema: 'Schema_MPTCPJoinSYN', *, options: 'Option') -> 'Data_MPTCPJoinSYN': # pylint: disable=unused-argument """Read Join Connection option for Initial SYN. @@ -2828,7 +2828,7 @@ def _make_mptcp_join(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTC return self._make_join_synack(subtype, opt, **kwargs) # type: ignore[arg-type] if Enum_Flags.SYN not in self._flags and Enum_Flags.ACK in self._flags: # MP_JOIN-ACK return self._make_join_ack(subtype, opt, **kwargs) # type: ignore[arg-type] - raise ProtocolError(f'{self.alias}: : [OptNo {Enum_Option.Multipath_TCP}] {subtype}: invalid flags combination') + raise ProtocolError(f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] {subtype}: invalid flags combination') def _make_join_syn(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCPJoinSYN]' = None, *, backup: 'bool' = False, @@ -2994,9 +2994,9 @@ def _make_mptcp_dss(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCP flag_m = cast('int', dsn).bit_length() > 32 if flag_M else False if flag_M and (ssn is None or dl_len is None or checksum is None): - raise ProtocolError(f'{self.alias}: : [OptNo {Enum_Option.Multipath_TCP}] {subtype}: missing required fields') + raise ProtocolError(f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] {subtype}: missing required fields') if not flag_M and (ssn is not None or dl_len is not None or checksum is not None): - raise ProtocolError(f'{self.alias}: : [OptNo {Enum_Option.Multipath_TCP}] {subtype}: missing required fields') + raise ProtocolError(f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] {subtype}: missing required fields') return Schema_MPTCPDSS( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), diff --git a/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py b/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py new file mode 100644 index 000000000..e9b764191 --- /dev/null +++ b/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py @@ -0,0 +1,239 @@ +"""#649 -- the four MPTCP error messages that carried a doubled separator. + +Four :exc:`~pcapkit.utilities.exceptions.ProtocolError` messages in the MPTCP +option handlers were spelled ``f'{self.alias}: : [OptNo …]'``, so they rendered +with an empty field between the protocol alias and the option number:: + + TCP: : [OptNo 30] 1: invalid flags combination + +The defect is presentational -- the exception type, the option number and the +subtype were always right, and nothing downstream parses these strings -- but it +is the string a user sees when an MP_JOIN or DSS option is rejected, and an empty +field reads as a value that failed to interpolate. + +The correct form is not a matter of taste. The same file spells the same prefix +without the doubling in 28 other places, all +``f'{self.alias}: [OptNo {schema.kind}] …'``, so the four were outliers against a +house form in their own module; the fix moves them into it and the count becomes +32. Two sibling test modules already quote the single-separator form in their +docstrings, which is further evidence of what was intended. + +Every message here is asserted **in full** rather than by substring. That is the +point of the change: a substring assertion on ``'invalid flags combination'`` +passes both before and after it, so the two places that already exercise these +messages could not have caught this, and needed no edit -- + +* ``test_tcp_mptcp_join_flag_ordering_unit.py:404``, which asserts + ``assertIn('invalid flags combination', str(caught.exception))`` against + ``_make_mptcp_join``, and +* ``test_tcp_udp_unit.py:1629``, which asserts + ``assertRaisesRegex(ProtocolError, 'invalid flags combination')`` against + ``_read_mptcp_join``. + +Two, not one: both match a substring that straddles the defect, and both still +pass unchanged. Neither module is touched by this change. + +All four lines arrived together in ``3bba8a1748`` (2023-04-10) and were untouched +until now. + +""" +from __future__ import annotations + +import pathlib +import unittest +from typing import TYPE_CHECKING + +import importlib.util + +from tests._support import purge_modules + +if TYPE_CHECKING: + from typing import Any + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Header fields for a segment built through the public constructor, spelled with +#: the parameter names :meth:`TCP.make ` +#: actually declares. The same mapping, and the same reason for not reusing +#: :data:`examples.generators.options.TCP_BASE`, as +#: :data:`tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit.TCP_HEADER`. +TCP_HEADER = { + 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, + 'psh': False, 'rst': False, 'fin': False, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent': 0, + 'payload': b'', +} + +#: The rendering of a rejected MP_JOIN, in full. ``30`` is +#: :attr:`Option.Multipath_TCP ` +#: and ``1`` is :attr:`MPTCPOption.MP_JOIN +#: `; both format as their +#: integer value because both registries are :class:`~enum.IntEnum`-flavoured. +JOIN_MESSAGE = 'TCP: [OptNo 30] 1: invalid flags combination' + +#: The rendering of a DSS option missing its required fields, in full. ``2`` is +#: :attr:`MPTCPOption.DSS `. +DSS_MESSAGE = 'TCP: [OptNo 30] 2: missing required fields' + +#: What the four messages read before the fix, kept as a literal so each test can +#: assert it is gone rather than only that the new form is present. +DOUBLED_PREFIX = 'TCP: : ' + + +class DummyData(dict): + """Attribute access over a mapping, for standing in as an option schema.""" + + __getattr__ = dict.__getitem__ + + +def build_mptcp(*, syn: 'bool' = False, ack: 'bool' = False, **option: 'Any') -> 'Any': + """Build a segment carrying one MPTCP option, through the public constructor. + + Args: + syn: Whether to set the ``SYN`` flag, which selects the MP_JOIN layout. + ack: Whether to set the ``ACK`` flag, which selects the MP_JOIN layout. + **option: The option's own arguments, including ``subtype``. + + Returns: + The constructed :class:`~pcapkit.protocols.transport.tcp.TCP` instance. + + Note: + :mod:`pcapkit` is imported inside the function rather than at module + level, for the reason the sibling MPTCP modules document at length: other + modules in this suite purge ``pcapkit``'s submodules from + :data:`sys.modules`, so a name bound at collection time can end up + pointing at a schema class built from a stale ``Schema`` base. + + """ + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + return TCP(syn=syn, ack=ack, + options=[(Enum_Option.Multipath_TCP, option)], # type: ignore[arg-type] + **TCP_HEADER) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class TCPMPTCPErrorMessageUnitTests(unittest.TestCase): + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_read_mptcp_join_message(self) -> None: + """``_read_mptcp_join``'s fall-through, the message #649 was reported against. + + Reached by calling the dispatcher directly on a *parsed* flagless segment. + The public read path cannot get here -- + :func:`~pcapkit.protocols.schema.transport.tcp.mptcp_data_selector` + rejects a flagless MP_JOIN with a ``FieldError`` before the dispatcher + runs -- so the branch is latent, but latent only by virtue of a guard in + a different file, and the string is still what a caller reaching it sees. + The segment is parsed rather than having ``_flags`` written onto a bare + ``object.__new__(TCP)``, because a hand-written attribute proves nothing + about what production puts there. + + """ + import struct + + from pcapkit.const.tcp.flags import Flags as Enum_Flags + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption + from pcapkit.const.tcp.option import Option + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import ProtocolError + + # A bare 20-octet header -- data offset 5, no options -- with no flags set. + raw = struct.pack('!HHIIBBHHH', 1, 2, 0, 0, 5 << 4, 0x00, 0, 0, 0) + proto = TCP(raw, len(raw)) + self.assertEqual(proto._flags, Enum_Flags(0)) + + schema = DummyData(kind=Option.Multipath_TCP, subtype=MPTCPOption.MP_JOIN) + with self.assertRaises(ProtocolError) as caught: + proto._read_mptcp_join(schema, options=DummyData()) # type: ignore[arg-type] + + self.assertEqual(str(caught.exception), JOIN_MESSAGE) + self.assertNotIn(DOUBLED_PREFIX, str(caught.exception)) + + def test_make_mptcp_join_message(self) -> None: + """``_make_mptcp_join``'s fall-through, through the public constructor. + + Unlike the read side this one *is* reachable from a caller: a segment with + neither SYN nor ACK carrying an MP_JOIN matches none of the three + :rfc:`8684` section 3.2 layouts, and #634 is what lets the library's own + error through rather than a bare :exc:`TypeError` from the membership + test above it. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as caught: + build_mptcp(syn=False, ack=False, + subtype=MPTCPOption.MP_JOIN, hmac=bytes(20)) + + self.assertEqual(str(caught.exception), JOIN_MESSAGE) + self.assertNotIn(DOUBLED_PREFIX, str(caught.exception)) + + def test_make_mptcp_dss_messages(self) -> None: + """Both of ``_make_mptcp_dss``'s guards, which are two lines, not one. + + The two carry the identical message, so asserting the string once would + not show that both were fixed. They are reached separately instead: + + * ``dsn`` given and ``ssn``/``dl_len``/``checksum`` absent -- the + data-level fields a DSN requires are missing. + * ``dsn`` absent and one of them given -- a data-level field present with + no DSN to belong to. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption + from pcapkit.utilities.exceptions import ProtocolError + + cases = { + 'dsn without its data-level fields': {'dsn': 1}, + 'data-level field without a dsn': {'ssn': 1}, + } + for label, option in cases.items(): + with self.subTest(case=label): + with self.assertRaises(ProtocolError) as caught: + build_mptcp(subtype=MPTCPOption.DSS, **option) + + self.assertEqual(str(caught.exception), DSS_MESSAGE) + self.assertNotIn(DOUBLED_PREFIX, str(caught.exception)) + + def test_no_doubled_separator_survives_anywhere_in_the_module(self) -> None: + """The source-level sweep, so a fifth site cannot be added unnoticed. + + The four were found by grepping for the pattern rather than by reading the + one that was reported, and three of the four came from that grep. A test + that pins only the four strings would not notice a fifth appearing, so + the absence is asserted over the whole file -- and the single-separator + count is asserted too, since deleting the four lines outright would + satisfy an absence check on its own. + + """ + import pcapkit.protocols.transport.tcp as tcp_module + + source = pathlib.Path(tcp_module.__file__).read_text(encoding='utf-8') + + self.assertNotIn("f'{self.alias}: : ", source) + self.assertEqual(source.count("f'{self.alias}: : "), 0) + + # 28 before the fix, 32 after: the four joined the house form rather than + # being removed. A guard against "fixed" by deletion. + # + # NOTE TO A FUTURE EDITOR: this count is deliberately exact, and it is + # therefore a tripwire on the whole module rather than on the four lines + # #649 was about. Adding or removing *any* `f'{self.alias}: [OptNo …]'` + # message anywhere in this ~3000-line file will fail this assertion for a + # reason that has nothing to do with the doubled separator. That is + # intended -- the alternative, asserting only the absence, passes if the + # four lines are deleted outright -- so when you legitimately change the + # number of such messages, update the number here rather than loosening + # the assertion. The one above it, on the doubled form, is the one that + # must stay at 0 forever. + self.assertEqual(source.count("f'{self.alias}: [OptNo"), 32) + + +if __name__ == '__main__': + unittest.main()