diff --git a/tests/protocols/internet/test_ah_unit.py b/tests/protocols/internet/test_ah_unit.py index 655eee61e7..ebfda58776 100644 --- a/tests/protocols/internet/test_ah_unit.py +++ b/tests/protocols/internet/test_ah_unit.py @@ -137,8 +137,7 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: "being *in* that registry is what makes something an extension header" ruling. Rephrasing the placement sentence as "places it among the IPv6 - extension headers" overstated the RFC; that was caught in review - on the second version of this fix. + extension headers" overstates the RFC. The section's separate IPv4-context sentence ("placed after the IP header ... before the next layer protocol") is legitimate @@ -152,13 +151,10 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: that fact *qualifies* an already-standalone header for a base besides :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`; it does not *make* the header standalone, and the RFC has no - say in which base is named. An earlier revision of this - docstring's ``ESP`` counterpart claimed the RFC "makes it a - standalone protocol" and "names ``IPsec`` as the second base", - the latter positionally false and unsupported by the cited - section; both were caught in review on the third version of - this fix. So this test targets the exact wrong phrasing from - the earlier rounds rather than banning "IPv4" outright, which + say in which base is named, so a claim that the RFC "makes it a + standalone protocol" or "names ``IPsec`` as the second base" is + unsupported by the cited section. So this test targets the wrong + phrasings themselves rather than banning "IPv4" outright, which would forbid the legitimate use kept above. """ import re diff --git a/tests/protocols/internet/test_esp_unit.py b/tests/protocols/internet/test_esp_unit.py index 08fc53e417..d76cac2605 100644 --- a/tests/protocols/internet/test_esp_unit.py +++ b/tests/protocols/internet/test_esp_unit.py @@ -302,9 +302,9 @@ def test_security_association_rejects_a_bool_destination(self) -> None: """``destination`` calls :func:`ipaddress.ip_address` directly, with no version check at all -- unlike the ``Field`` classes in :mod:`pcapkit.corekit.fields.ipaddress`, ESP does not route through - that abstraction. Before this fix, + that abstraction. This guards against ``SecurityAssociation(spi=1, destination=True).destination`` silently - became ``IPv4Address('0.0.0.1')``, with no exception and no warning. + becoming ``IPv4Address('0.0.0.1')``, with no exception and no warning. See #491. """ from pcapkit.protocols.internet.esp import SecurityAssociation @@ -1059,10 +1059,9 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: "being *in* that registry is what makes something an extension header" ruling. Rephrasing the placement sentence as "places it among the IPv6 - extension headers" overstated the RFC and contradicted this same + extension headers" overstates the RFC and contradicts this same docstring's ``Note:`` (which says RFC 8200 declines ESP - extension-header status); that was caught in review on the - second version of this fix. + extension-header status). The section's separate IPv4-context sentence ("placed after the IP header ... before the next layer protocol") is legitimate @@ -1077,14 +1076,13 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: besides :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`; it does not *make* the header standalone, and the RFC has no say in which base is named (``IPsec`` versus ``Internet``) -- - an earlier revision claimed the RFC "makes it a standalone - protocol" and "names ``IPsec`` as the second base", the latter - positionally false (``ESP.__bases__`` puts ``IPsec`` first, not - second) and unsupported by the cited section; both were caught - in review on the third version of this fix. So this test - targets the exact wrong phrasing from the earlier rounds rather - than banning "IPv4" outright, which would forbid the legitimate - use kept above. + so a claim that the RFC "makes it a standalone protocol" or + "names ``IPsec`` as the second base" is unsupported by the cited + section, and the latter is positionally false too + (``ESP.__bases__`` puts ``IPsec`` first, not second). So this + test targets the wrong phrasings themselves rather than banning + "IPv4" outright, which would forbid the legitimate use kept + above. """ import re diff --git a/tests/protocols/internet/test_ext_chain_limit_rebuild_1619_unit.py b/tests/protocols/internet/test_ext_chain_limit_rebuild_1619_unit.py index 8636353850..fe1b34f2ca 100644 --- a/tests/protocols/internet/test_ext_chain_limit_rebuild_1619_unit.py +++ b/tests/protocols/internet/test_ext_chain_limit_rebuild_1619_unit.py @@ -10,7 +10,7 @@ one that reached the limit warned of it, twice for IPv6 and up to 33 times for IPv4, and went up to 32 headers past where the parse had stopped. -Now a rebuilt payload takes its place in its chain from the layer it is the +A rebuilt payload takes its place in its chain from the layer it is the payload of, as #1610 has it take its place in the frame, and only the layer being rebuilt warns, once its own parse gets to the limit. @@ -182,7 +182,8 @@ def test_where_both_limits_fall_on_one_layer_rebuilding_warns_once(self) -> None def test_where_the_frame_limit_falls_first_rebuilding_warns_once(self) -> None: # Guard: 40 tunnels put the 65th layer at the chain's 24th header, short - # of the chain limit; the rebuild warned once before #1619 too. + # of the chain limit; the rebuild warns once with or without the #1619 + # fix. from pcapkit.protocols.link.ethernet import Ethernet for carrier in ETHERTYPE: diff --git a/tests/protocols/internet/test_hip_host_id_di_width_unit.py b/tests/protocols/internet/test_hip_host_id_di_width_unit.py index ec37c4f05e..04c94d437a 100644 --- a/tests/protocols/internet/test_hip_host_id_di_width_unit.py +++ b/tests/protocols/internet/test_hip_host_id_di_width_unit.py @@ -71,7 +71,7 @@ def test_made_host_id_packs_the_rfc_layout(self) -> None: hi_pub_key=PUB_KEY, di=DI, di_type=DITypes.FQDN) self.assertEqual(schema.len, 14) - # 26 octets before #1118: two zero octets after the DI word. + # Without the #1118 fix, 26 octets: two zero octets after the DI word. self.assertEqual(schema.pack().hex(), rfc_host_id().hex()) def test_rfc_host_id_parses_algorithm_and_di(self) -> None: @@ -83,7 +83,7 @@ def test_rfc_host_id_parses_algorithm_and_di(self) -> None: raw = rfc_host_id() schema = HostIDParameter.unpack(raw, len(raw)) # type: ignore[arg-type] - # Read as 1 -- the curve -- before #1118. + # Without the #1118 fix, read as 1 -- the curve. self.assertEqual(schema.algorithm, HIAlgorithm.ECDSA) self.assertEqual(int(schema.algorithm), 7) self.assertEqual(schema.di_data['type'], DITypes.FQDN) diff --git a/tests/protocols/internet/test_hip_locator_flags_unit.py b/tests/protocols/internet/test_hip_locator_flags_unit.py index fdca4b425f..c2692d0ae2 100644 --- a/tests/protocols/internet/test_hip_locator_flags_unit.py +++ b/tests/protocols/internet/test_hip_locator_flags_unit.py @@ -52,7 +52,7 @@ def _parse(self, frame: bytes): # type: ignore[no-untyped-def] with warnings.catch_warnings(): # Nested ``Locator`` schemas warn about their placeholder length; - # that is pre-existing noise unrelated to the flags octet. + # that is noise unrelated to the flags octet. warnings.simplefilter('ignore') return HIP(frame, len(frame), extension=True) diff --git a/tests/protocols/internet/test_hip_locator_set_length_unit.py b/tests/protocols/internet/test_hip_locator_set_length_unit.py index 816aa96597..0fe5d562eb 100644 --- a/tests/protocols/internet/test_hip_locator_set_length_unit.py +++ b/tests/protocols/internet/test_hip_locator_set_length_unit.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- -"""HIP ``LOCATOR_SET``, whose ``Length`` unit and padding source were both wrong. +"""HIP ``LOCATOR_SET`` counts ``Length`` in bytes and pads from its own length. -GitHub issue #679. Two pre-existing defects, neither fixable alone: +GitHub issue #679. Two defects, neither fixable alone: 1. ``LocatorSetParameter.padding`` read the **nested** ``Locator.len`` rather than the parameter's. A :class:`~pcapkit.corekit.fields.collections.ListField` packs @@ -9,7 +9,7 @@ :meth:`Schema.pack ` opens with ``packet.update(self.__dict__)`` -- and :class:`Locator` declares a ``len`` of its own, so by the time ``padding`` was evaluated ``pkt['len']`` was the last - locator's. That is 4 for any IPv6 locator whatever the count, so the old + locator's. That is 4 for any IPv6 locator whatever the count, so that expression appended exactly four octets to every ``LOCATOR_SET``. 2. The parameter's ``len`` was ``sum(Locator.len)``, in the 4-octet units :rfc:`8046#section-4` gives ``Locator Length``, where :rfc:`7401` @@ -30,7 +30,7 @@ **The cancellation was never general, and that is the whole reason this module sweeps shapes rather than counts.** It needs every locator to be 24 octets and there to be at least one of them. Measured on ``f0999858e``, the shapes where it -does not hold were non-conformant before this fix: +does not hold were non-conformant without the fix: =================== ============== ====== ========================= shape declared ``len`` packed RFC total for the contents @@ -46,7 +46,7 @@ =================== ============== ====== ========================= Four of the seven were not even multiples of eight. A sweep over n = 1..5 of -plain locators passed by cancellation before the fix and would pass a half-fix +plain locators passes by cancellation without the fix and would pass a half-fix too, so it discriminates nothing; the empty set, the SPI-bearing variant and a mixed set in both orders are the shapes that do. @@ -97,13 +97,12 @@ #: :func:`~pcapkit.protocols.schema.internet.hip.locator_value_selector` accepts #: only ``type == 0, len == 4`` or ``type == 1, len == 5`` -- so ``spi=`` without #: an explicit ``type=1`` raises ``FieldValueError: invalid locator type or -#: length``. That is a separate rough edge in the maker, noted rather than fixed -#: here, and it is why an earlier attempt to measure these shapes could not reach -#: the SPI path at all. +#: length``. That is a separate rough edge in the maker, and without ``type=1`` +#: these shapes could not reach the SPI path at all. SPI = {'ip': '2001:db8::2', 'spi': 0xdeadbeef, 'type': 1} #: Every shape, with the octets each locator record occupies. Ordered so that the -#: three the old cancellation covered come first and the four it did not follow. +#: shapes the cancellation covers come first and the ones it does not follow. SHAPES = ( ('empty', [], 0), ('plain n=1', [PLAIN], 24), @@ -293,8 +292,7 @@ def test_every_shape_parses_back_to_the_locators_it_was_built_from(self) -> None of them, one per field, because ``SchemaField`` seeds a nested schema's ``__length__`` at the ``-1`` placeholder :meth:`Schema.pack ` - documents; that is pre-existing, unrelated to ``Length`` units, and - unchanged by this fix -- what changed is that it now fires once per + documents; that is unrelated to ``Length`` units, and it fires once per locator actually parsed rather than once per truncated first locator. """ from pcapkit.const.hip.parameter import Parameter @@ -349,8 +347,8 @@ def test_a_lone_locator_set_parameter_survives_a_hip_packet(self) -> None: with warnings.catch_warnings(): # The per-locator ``__length__`` warnings described above are - # pre-existing noise on a now-correct parse; the assertions - # below are what this case is about. + # noise unrelated to ``Length`` units; the assertions below + # are what this case is about. warnings.simplefilter('ignore') parsed = HIP(octets, len(octets), extension=True) diff --git a/tests/protocols/internet/test_hip_parameterless_length_unit.py b/tests/protocols/internet/test_hip_parameterless_length_unit.py index f921663d0b..382a35f9b1 100644 --- a/tests/protocols/internet/test_hip_parameterless_length_unit.py +++ b/tests/protocols/internet/test_hip_parameterless_length_unit.py @@ -6,7 +6,7 @@ field in 8-octet units excluding the first 8, so the two 16-octet HITs alone make 4 the minimum. The header's own re-parse then computed a parameter length of ``-32`` and raised ``ProtocolError``. :meth:`HIP.read -` now rejects a wire Header Length +` rejects a wire Header Length below 4 up front. Every case builds its own octets in memory and reads no capture. diff --git a/tests/protocols/internet/test_hip_r1_counter_width_unit.py b/tests/protocols/internet/test_hip_r1_counter_width_unit.py index 7e1a632ba9..5142cce1dc 100644 --- a/tests/protocols/internet/test_hip_r1_counter_width_unit.py +++ b/tests/protocols/internet/test_hip_r1_counter_width_unit.py @@ -20,8 +20,8 @@ this one's tail, and a real peer's 16-octet ``R1_COUNTER`` leaves its last four octets to be read as the following parameter's ``Type``. -Why the existing suite could not see it ---------------------------------------- +Why the suite could not see it +------------------------------ Three things hid it at once, and the third is the reason this module asserts against the RFC's arithmetic rather than against a round trip. @@ -30,10 +30,10 @@ each parameter in a frame, and two four-octet shortfalls sum to eight, so the record area stays 8-aligned and ``HIP.make``'s ``total_length // 8 + 4`` comes out exact. -2. Before #651 the padding aligned the parameter *contents* rather than the - record, which at ``Length = 12`` appended exactly the four surplus octets this - parameter was missing. Correcting the padding did not break ``R1_COUNTER``; it - stopped compensating for it. +2. A padding rule that aligns the parameter *contents* rather than the record + (the #651 defect) appends, at ``Length = 12``, exactly the four surplus octets + this parameter was missing. So correcting the padding does not break + ``R1_COUNTER``; it stops compensating for it. 3. The round trip cannot see it at all, because pcapkit's reader consumes exactly the twelve octets its writer wrote. Only a comparison against the RFC's own stride -- or a real peer -- can tell the difference, which is what @@ -106,7 +106,7 @@ def test_r1_counter_record_is_the_rfc_total_at_both_codes(self) -> None: The exact octets are pinned, not just the length, because the length alone would pass for a record padded to 16 with a four-octet counter as readily as for one carrying the eight-octet counter the RFC specifies -- - and the padding is what the pre-#651 rule appended. So the assertion + that padding is what a contents-aligning rule (#651) appends. So the assertion names where the eight octets sit: four of ``Reserved`` after the type-and-length header, then the counter, then no padding at all, since ``rfc_total(12) - 4 - 12`` is zero. @@ -128,7 +128,7 @@ def test_r1_counter_record_is_the_rfc_total_at_both_codes(self) -> None: self.assertEqual(len(raw), rfc_total(12)) self.assertEqual(len(raw), 16) self.assertEqual(len(raw) % 8, 0) - # Twelve octets before #672, four of them the counter: + # Without the #672 fix, twelve octets, four of them the counter: # ``00 8x 00 0c 00 00 00 00 00 00 00 01``. self.assertEqual( raw.hex(), prefix + '000c' + '00000000' + '0000000000000001') @@ -175,12 +175,12 @@ def test_a_lone_r1_counter_parameter_survives_a_hip_packet(self) -> None: both codes; the two-copy pairing in :mod:`examples.generators.options` is what concealed that. - Code 128 is deliberately not exercised here. #690 registered - ``R1CounterParameter`` for 128 as well as 129, so it now reaches this - same class rather than parsing as an ``UnassignedParameter`` -- - that coverage lives with the fix, in + Code 128 is deliberately not exercised here. ``R1CounterParameter`` is + registered for 128 as well as 129 (#690), so 128 reaches this same + class rather than parsing as an ``UnassignedParameter`` -- that + coverage lives in :meth:`HIPUnitTests.test_hip_r1_counter_code_128_resolves_to_its_own_schema`, - not here; this module stays scoped to the width #672 fixed. + not here; this module stays scoped to the width (#672). """ from pcapkit.const.hip.parameter import Parameter from pcapkit.protocols.internet.hip import HIP @@ -213,7 +213,7 @@ def test_the_generator_gives_r1_counter_a_non_zero_counter(self) -> None: walk from 1 violation to 2, the new one being ``frame 101: type 0 padding not zeroed: aabbccdd``. - The width is fixed now, so the walk finds nothing here either way. This + With the width right, the walk finds nothing here either way. This pins the override anyway, because a fixture that cannot discriminate a wrong width from a right one is what let this defect survive as long as it did -- the same trap #608 fell into with the ``SOLUTION`` field. diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index 8662d077c8..7619ab6d23 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -1297,12 +1297,12 @@ def test_hip_parameter_constructors_cover_keyword_paths(self) -> None: ).hmac, b'relh') def test_hip_make_param_encrypted_preserves_iv_on_pack(self) -> None: - # #556: ``_make_param_encrypted`` used to pass ``cipher=`` to + # #556: guards against ``_make_param_encrypted`` passing ``cipher=`` to # ``Schema_EncryptedParameter``, a keyword the schema does not # accept (the packing-time cipher lookup keys off ``__cipher__``, - # a packet-context key -- not a constructible field), so the value - # was dropped with an ``UnknownFieldWarning`` and the schema's own - # ``pre_unpack`` fallback then always treated the parameter as + # a packet-context key -- not a constructible field), so that the value + # is dropped with an ``UnknownFieldWarning`` and the schema's own + # ``pre_unpack`` fallback then always treats the parameter as # cipher-less, silently packing an AES-cipher ``ENCRYPTED`` # parameter without its IV. Building one through ``make`` and # packing it must retain the IV. @@ -1875,8 +1875,8 @@ def test_hip_reg_info_parameter_rejects_underflowing_length(self) -> None: :func:`struct.calcsize` for a negative length -- its own ``while length > 0`` loop just returns an empty list instead -- so this parsed to an empty ``reg_type`` with no exception and no diagnostic: - ``proto.info.parameters[930].reg_type == ()``, confirmed against the - pre-fix tree at ``da2422728``, rather than rejecting the malformed + ``proto.info.parameters[930].reg_type == ()``, confirmed without the + fix at ``da2422728``, rather than rejecting the malformed ``Length``. """ @@ -1937,25 +1937,22 @@ def test_hip_transport_format_list_parameter_accepts_an_empty_list_at_length_zer """#463/#466: ``TRANSPORT_FORMAT_LIST`` has no two-octet prefix, so ``Length = 0`` is a legitimate empty list, not a malformed one. - A first pass at #463 applied the same ``pkt['len'] - 2`` guard used - by :class:`NATTraversalModeParameter`, :class:`ESPTransformParameter` - and :class:`HIPTransportModeParameter` to this parameter's ``formats`` - field too, on the assumption that the expression was byte-identical - across all four sites for the same reason. It is not: :rfc:`7401` - Section 5.2.11 defines ``Length`` as literally "2x number of TF - types", with nothing between ``Length`` and the list to account for. - So ``Length = 0`` with an empty ``formats`` list -- which packs - correctly on ``main`` as ``08010000`` -- was turned into a raise by - that first pass, a regression rather than a declined fix. This - parses it back to confirm the corrected :func:`~pcapkit.protocols. - schema.internet.hip.transport_format_list_len` accepts it again. - - Two copies, and since #651 not because one is unrepresentable -- one - now is -- but because two consecutive parameters are what prove the - reader's *stride*. A parser that pads by the wrong amount still reads - a lone parameter correctly and only lands in the wrong place at the - start of the next one, so a single-parameter fixture cannot tell a - correct padding rule from any other. + The ``pkt['len'] - 2`` guard used by + :class:`NATTraversalModeParameter`, :class:`ESPTransformParameter` and + :class:`HIPTransportModeParameter` does not apply to this parameter's + ``formats`` field: :rfc:`7401` Section 5.2.11 defines ``Length`` as + literally "2x number of TF types", with nothing between ``Length`` and + the list to account for. Applying it anyway turns ``Length = 0`` with an + empty ``formats`` list -- which packs as ``08010000`` -- into a raise. + This parses it back to confirm :func:`~pcapkit.protocols. + schema.internet.hip.transport_format_list_len` accepts it. + + Two copies -- a lone one is representable (#651) -- because two + consecutive parameters are what prove the reader's *stride*. A parser + that pads by the wrong amount still reads a lone parameter correctly + and only lands in the wrong place at the start of the next one, so a + single-parameter fixture cannot tell a correct padding rule from any + other. """ from pcapkit.const.hip.parameter import Parameter @@ -1982,19 +1979,18 @@ def test_hip_transport_format_list_parameter_accepts_an_empty_list_at_length_zer self.assertEqual(copies[1].tf_type, ()) def test_hip_transport_format_list_parameter_parses_the_full_declared_length(self) -> None: - """#463/#466: a since-corrected revision's ``- 2`` under-read every - *non-empty* ``TRANSPORT_FORMAT_LIST``, silently dropping trailing - entries; this proves the fully declared list now survives. - - Pre-#463, on ``main``, with the then-one-octet ``item_type`` still in - place: ``Length = 4`` and four one-octet transport format entries - underflowed to ``pkt['len'] - 2 == 2``, sizing the list at only two - entries and silently dropping the last two octets. That bug is - independent of the item-width defect fixed alongside it below -- - confirmed directly against a ``main``-shaped schema object: - ``TransportFormatListParameter(type=..., len=4, formats=[10, 20, 30, - 40])`` packs to twelve octets and reads back as ``formats == - [Unassigned_10, Unassigned_20]``. + """#463/#466: guards against a ``- 2`` under-read of every + *non-empty* ``TRANSPORT_FORMAT_LIST``, which silently drops trailing + entries; this proves the fully declared list survives. + + Without the #463 fix, and with one-octet ``item_type`` entries: + ``Length = 4`` and four one-octet transport format entries underflowed + to ``pkt['len'] - 2 == 2``, sizing the list at only two entries and + silently dropping the last two octets. That defect is independent of + the item-width one below -- confirmed directly against a schema object + of that shape: ``TransportFormatListParameter(type=..., len=4, + formats=[10, 20, 30, 40])`` packed to twelve octets and read back as + ``formats == [Unassigned_10, Unassigned_20]``. Each ``TF type`` entry is two octets, not one -- :rfc:`7401` Section 5.2.11 fixes it at "2x number of TF types" and the diagram shows two @@ -2002,10 +1998,10 @@ def test_hip_transport_format_list_parameter_parses_the_full_declared_length(sel :class:`HIPTransportModeParameter`'s ``mode`` field and the items at :func:`two_octet_prefix_list_len`'s other two call sites, all three two-octet. So this test's four entries are four *two*-octet values - (``Length = 8``), and with both defects fixed -- + (``Length = 8``), and with :func:`~pcapkit.protocols.schema.internet.hip. transport_format_list_len` returning ``Length`` unchanged, and - ``item_type=EnumField(length=2, ...)`` -- all four survive the round + ``item_type=EnumField(length=2, ...)``, all four survive the round trip rather than being dropped or double-counted. """ @@ -2021,10 +2017,10 @@ def test_hip_transport_format_list_parameter_parses_the_full_declared_length(sel # Two copies of: type(2)=2049 len(2)=8, four two-octet format entries # (10, 20, 30, 40), then four octets of padding. Eight octets of # contents are 8-aligned on their own, which is exactly the residue at - # which the pre-#651 rule (align the contents, ignore the four-octet - # type-and-length header) appended nothing and left the record four - # octets short; RFC 7401 5.2.1 asks for 11 + 8 - (8 + 3) % 8 == 16 -- - # so 16 octets each and 32 together. + # which a contents-aligning rule (align the contents, ignore the + # four-octet type-and-length header; #651) appends nothing and leaves + # the record four octets short; RFC 7401 5.2.1 asks for + # 11 + 8 - (8 + 3) % 8 == 16 -- so 16 octets each and 32 together. one = (2049).to_bytes(2, 'big') + (8).to_bytes(2, 'big') + b''.join( n.to_bytes(2, 'big') for n in (10, 20, 30, 40)) + bytes(4) self.assertEqual(len(one), 16) @@ -2167,15 +2163,15 @@ def test_hip_nat_traversal_mode_parameter_round_trips_through_the_maker(self) -> ``NATTraversalModeParameter(type=..., len=..., modes=...)`` picks a self-consistent ``len`` by hand and so cannot expose a maker/schema disagreement. Building through the maker instead pins the two to - agree: with the one-octet ``item_type`` this module carried before - this fix, one entry's maker-computed ``len=4`` reversed through + agree: with a one-octet ``item_type``, one entry's maker-computed + ``len=4`` reverses through :func:`~pcapkit.protocols.schema.internet.hip. two_octet_prefix_list_len` back to ``(len - 2) // 1 == 2`` items -- - ``[1, 0]``, one spurious trailing entry -- while with the corrected + ``[1, 0]``, one spurious trailing entry -- while with the two-octet ``item_type`` it reverses to ``(len - 2) // 2 == 1``, matching what went in. - Measured directly against this defect before this fix, byte for + Measured directly against this defect without the fix, byte for byte: ``modes=[1]`` packed to ``0260000400000100000000`` (11 octets, declared ``len=4``) and read back as ``modes == [1, 0]``. @@ -2209,15 +2205,15 @@ def test_hip_esp_transform_parameter_round_trips_through_the_maker(self) -> None ``mode``. A hand-built ``ESPTransformParameter(type=..., len=..., suites=...)`` picks a self-consistent ``len`` by hand and so cannot expose a maker/schema disagreement. Building through the maker - instead pins the two to agree: with the one-octet ``item_type`` this - module carried before this fix, one entry's maker-computed ``len=4`` - reversed through :func:`~pcapkit.protocols.schema.internet.hip. + instead pins the two to agree: with a one-octet ``item_type``, one + entry's maker-computed ``len=4`` reverses through + :func:`~pcapkit.protocols.schema.internet.hip. two_octet_prefix_list_len` back to ``(len - 2) // 1 == 2`` items -- - ``[1, 0]``, one spurious trailing entry -- while with the corrected + ``[1, 0]``, one spurious trailing entry -- while with the two-octet ``item_type`` it reverses to ``(len - 2) // 2 == 1``, matching what went in. - Measured directly against this defect before this fix, byte for + Measured directly against this defect without the fix, byte for byte: ``suites=[1]`` packed to ``0fff000400000100000000`` (11 octets, declared ``len=4``) and read back as ``suites == [1, 0]``. @@ -2245,24 +2241,23 @@ def test_hip_nat_traversal_mode_and_esp_transform_survive_the_full_parser(self) is necessary but not sufficient, since the full parser is what a real caller actually uses. - Before this fix, feeding a maker-built ``modes=[1]`` parameter - through the full parser did not reproduce the *same* phantom-entry + Without the fix, feeding a maker-built ``modes=[1]`` parameter + through the full parser does not reproduce the *same* phantom-entry symptom the direct schema round trip shows -- the single 11-octet - parameter this module's ``len`` arithmetic produced was not a - multiple of eight, so :meth:`HIP.make` raised ``ProtocolError: - HIPv2: invalid format`` before a packet even existed to parse, and a - hand-built two-copy packet instead corrupted the second copy and - emitted ``SchemaWarning: packet length < 0``. Either way the full - parser did not silently return a wrong value; it failed outright, - which is what this test pins now that the fix makes it succeed - instead. - - Since #651 both parameters are 8-aligned on their own -- ``len = 4`` + parameter the module's ``len`` arithmetic produces is not a + multiple of eight, so :meth:`HIP.make` raises ``ProtocolError: + HIPv2: invalid format`` before a packet even exists to parse, and a + hand-built two-copy packet instead corrupts the second copy and + emits ``SchemaWarning: packet length < 0``. Either way the full + parser fails outright rather than silently returning a wrong value; + this test pins that it succeeds. + + Both parameters are 8-aligned on their own (#651) -- ``len = 4`` (two ``reserved`` octets and one two-octet entry) is exactly the residue at which :rfc:`7401` Section 5.2.1 wants no padding at all, - ``11 + 4 - (4 + 3) % 8 == 8``, and at which the old contents-aligning - rule appended four octets that must not have been there. The two - copies stay, because two consecutive parameters are what prove the + ``11 + 4 - (4 + 3) % 8 == 8``, and at which a contents-aligning rule + appends four octets that must not be there. Two copies are used, + because two consecutive parameters are what prove the reader's stride rather than merely its handling of one record. """ @@ -2310,40 +2305,38 @@ def test_hip_r1_counter_code_128_resolves_to_its_own_schema(self) -> None: """#690: HIPv1's ``R1_Counter`` (code 128) must not fall through to ``UnassignedParameter`` on the parse path. - ``R1CounterParameter`` used to register itself with ``code= - Enum_Parameter.R1_COUNTER`` alone -- 129, the HIPv2 spelling. - :attr:`~pcapkit.protocols.internet.hip.HIP.__parameter__` already - carries two hand-written entries -- not a name-normalisation rule -- - mapping both 128 and 129 to ``_read_param_r1_counter``, and - ``_make_param_r1_counter`` already built a real ``R1CounterParameter`` - for 128 -- so construction worked and only parsing was broken. The + This guards against ``R1CounterParameter`` registering itself with + ``code=Enum_Parameter.R1_COUNTER`` alone -- 129, the HIPv2 spelling. + :attr:`~pcapkit.protocols.internet.hip.HIP.__parameter__` carries two + hand-written entries -- not a name-normalisation rule -- mapping both + 128 and 129 to ``_read_param_r1_counter``, and + ``_make_param_r1_counter`` builds a real ``R1CounterParameter`` for 128 + -- so construction works either way and only parsing can break. The *schema* used to unpack a parameter off the wire is chosen by a different mapping, :attr:`~pcapkit.protocols.schema.schema.EnumSchema. registry`, wired into :class:`HIP`'s ``param`` field as ``OptionField(..., registry=Parameter.registry)``, and that one is - keyed solely by the ``code=`` a schema class declares. So code 128 - fell to ``Parameter.__default__`` -- ``UnassignedParameter``, whose - schema has ``value: bytes`` and no ``counter`` -- and - ``_read_param_r1_counter`` failed reading ``schema.counter`` off it - with ``AttributeError: 'UnassignedParameter' object has no attribute - 'counter'``. Measured verbatim on the tree before this fix, with the - exact bytes this test builds. + keyed solely by the ``code=`` a schema class declares. So without the + fix code 128 falls to ``Parameter.__default__`` -- + ``UnassignedParameter``, whose schema has ``value: bytes`` and no + ``counter`` -- and ``_read_param_r1_counter`` fails reading + ``schema.counter`` off it with ``AttributeError: 'UnassignedParameter' + object has no attribute 'counter'``. Measured verbatim without the fix, + with the exact bytes this test builds. The expected wire bytes below are derived from ``R1CounterParameter.__fields__`` rather than a hex literal, on - purpose: a literal is exactly what made the first version of this - test fail the moment #696 widened ``counter`` from four octets to - eight (12-octet records became 16). Deriving from the field widths - keeps this test about the registry dispatch it names, rather than - about a wire width #696's own ``test_hip_r1_counter_width_unit.py`` - already pins. + purpose: a literal would tie this test to the width of ``counter``, + which ``test_hip_r1_counter_width_unit.py`` pins (#696). Deriving from + the field widths keeps this test about the registry dispatch it + names. Two copies, matching ``test_hip_nat_traversal_mode_and_esp_transform_survive_the_full_parser``'s own reason: proving the reader's stride across repeated occurrences - of the same code, not working around any alignment defect. With #696 - landed a lone ``R1_COUNTER`` record already packs to a self-aligned - 16 octets and parses cleanly on its own -- measured directly, one + of the same code, not working around any alignment defect. A lone + ``R1_COUNTER`` record packs to a self-aligned 16 octets (#696) and + parses cleanly on its own -- measured directly, one copy raises nothing and reads back the same counter -- so two copies here are a stride check, not a workaround. @@ -2353,7 +2346,7 @@ def test_hip_r1_counter_code_128_resolves_to_its_own_schema(self) -> None: from pcapkit.protocols.internet.hip import HIP from pcapkit.protocols.schema.internet import hip as hip_schema - # The schema registry itself: both codes now resolve to the same class. + # The schema registry itself: both codes resolve to the same class. self.assertIs(hip_schema.Parameter.registry[Parameter.R1_Counter], hip_schema.R1CounterParameter) self.assertIs(hip_schema.Parameter.registry[Parameter.R1_COUNTER], @@ -2508,12 +2501,13 @@ def test_hip_solution_parameter_length_is_two_whole_octet_fields(self) -> None: #J`` of ``RHASH_len / 8`` octets *each*. The ``/ 4`` is shorthand for twice ``/ 8`` and holds only because ``RHASH_len`` -- the natural output length of a hash function, in bits -- is a whole number of octets. - :meth:`~pcapkit.protocols.internet.hip.HIP._make_param_solution` used to - apply that shorthand to an arbitrary :meth:`int.bit_length`, where the - identity fails, and so emitted an odd contents width that + This guards against + :meth:`~pcapkit.protocols.internet.hip.HIP._make_param_solution` + applying that shorthand to an arbitrary :meth:`int.bit_length`, where + the identity fails, and so emitting an odd contents width that :meth:`~pcapkit.protocols.internet.hip.HIP._read_param_solution`'s ``(len - 4) % 2`` guard rejects -- the library refusing to parse what it - had just built. + has just built. Two independent symptoms, both asserted below. The odd ``len`` is rejected outright; and because @@ -2644,10 +2638,7 @@ def test_hip_puzzle_and_solution_keep_the_on_wire_field_width(self) -> None: ``EXPECTED_FAILURES`` entry is that the cycle *closes*: nothing raises, the integers survive, and the round trip silently yields a parameter describing a different puzzle -- a conformant peer reads ``RHASH_len = 8`` bits where - the sender said 64. It only became reachable end to end once #608 was fixed - (#629); before that the undersized rebuild tripped - :meth:`~pcapkit.protocols.internet.hip.HIP._read_param_solution`'s parity - guard first and failed loudly. + the sender said 64. The octets below are written by hand, not by this library's own builder, which is the only way to present it with a value narrower than its field. @@ -2668,7 +2659,7 @@ def test_hip_puzzle_and_solution_keep_the_on_wire_field_width(self) -> None: # then Random #I and Puzzle solution #J as 8 octets each carrying 1. # # Reserved is 0x20 rather than the conformant 0x00 *only* so that this case - # isolates #653 on the unfixed tree: a 0x00 there trips #654's + # isolates #653 without the #654 fix: a 0x00 there trips #654's # ``math.log2(0)`` first and the width loss is never reached. #654's own # test below uses the conformant 0x00. wire = bytes.fromhex('0141' '0014' '01' '20' '6f70' @@ -2688,7 +2679,7 @@ def test_hip_puzzle_and_solution_keep_the_on_wire_field_width(self) -> None: rebuilt = proto._make_param_solution(Parameter.SOLUTION, parsed, version=2) self.assertEqual(rebuilt.len, 20) - self.assertNotEqual(rebuilt.len, 6) # what the unfixed builder produced + self.assertNotEqual(rebuilt.len, 6) # the width-losing value (#653) # Compared without the trailing padding, and then against the re-packed # source schema rather than against the literal. Both are deliberate: the # padding rule is pinned separately (#651/#664), and this test is about the @@ -2714,7 +2705,7 @@ def test_hip_puzzle_and_solution_keep_the_on_wire_field_width(self) -> None: rebuilt = proto._make_param_puzzle(Parameter.PUZZLE, parsed, version=2) self.assertEqual(rebuilt.len, 12) - self.assertNotEqual(rebuilt.len, 5) # what the unfixed builder produced + self.assertNotEqual(rebuilt.len, 5) # the width-losing value (#653) self.assertEqual(bytes(rebuilt)[:4 + rebuilt.len], wire[:4 + unpacked.len]) self.assertEqual(bytes(rebuilt), bytes(unpacked)) @@ -2726,15 +2717,16 @@ def test_hip_solution_second_octet_is_reserved_not_a_lifetime(self) -> None: when received" -- and :rfc:`5201#section-5.2.5` says the same, so there is no HIP version under which it is a duration. Only ``PUZZLE`` (:rfc:`7401#section-5.2.4`) has a ``Lifetime`` at that offset, and only - §5.2.4 defines the ``2^(value - 32)`` seconds encoding that pcapkit was - applying to both. Measured on ``origin/main`` at ``0c7f2b7c9``, the octet + §5.2.4 defines the ``2^(value - 32)`` seconds encoding, which this + guards against applying to both. Measured at ``0c7f2b7c9``, where it was + applied to both, the octet came out as ``0x20``, ``0x21``, ``0x25`` or ``0x2b`` depending on the lifetime asked for, and the one value the RFC actually permits -- zero -- could not be written at all. Three things are asserted, in the order they matter. The octet is zero for a from-scratch build, whatever else is passed. A parameter that arrives with - the conformant ``0x00`` survives the round trip, where it used to be + the conformant ``0x00`` survives the round trip rather than being unbuildable. And a parameter that arrives with a non-zero ``Reserved`` re-emits that same octet rather than a re-derived one, because round-trip fidelity is what #653 is about and "ignored when received" is honoured by @@ -2751,7 +2743,7 @@ def test_hip_solution_second_octet_is_reserved_not_a_lifetime(self) -> None: proto = object.__new__(HIP) options = OrderedMultiDict() - # The data model no longer claims SOLUTION has a lifetime. + # The data model does not claim SOLUTION has a lifetime. self.assertIn('reserved', hip_data.SolutionParameter.__annotations__) self.assertNotIn('lifetime', hip_data.SolutionParameter.__annotations__) self.assertIn('reserved', hip_schema.SolutionParameter.__annotations__) @@ -2775,9 +2767,9 @@ def test_hip_solution_second_octet_is_reserved_not_a_lifetime(self) -> None: for stale in (0x20, 0x21, 0x25, 0x2b): self.assertNotEqual(bytes(schema)[5], stale) - # A conformant SOLUTION -- Reserved = 0x00 -- parsed and re-serialised. On - # the unfixed tree this raised a bare ValueError from ``math.log2(0.0)``, - # because 0x00 was read as ``2 ** (0 - 32)`` seconds, which is below + # A conformant SOLUTION -- Reserved = 0x00 -- parsed and re-serialised. + # Without the fix this raises a bare ValueError from ``math.log2(0.0)``, + # because 0x00 is read as ``2 ** (0 - 32)`` seconds, which is below # timedelta's microsecond resolution and rounds to timedelta(0). # Compared against the re-packed source schema rather than against the # literal, and with the trailing padding excluded, for the reason given in @@ -2831,16 +2823,16 @@ def test_hip_puzzle_lifetime_guard_raises_an_in_library_error(self) -> None: positive input``, names neither HIP nor the parameter nor the field. :class:`~pcapkit.utilities.exceptions.ProtocolError` is the right member of - that family rather than the nearest-named one. It is already what both - readers raise for a malformed ``PUZZLE`` or ``SOLUTION``, and it is declared + that family rather than the nearest-named one. It is what both readers + raise for a malformed ``PUZZLE`` or ``SOLUTION``, and it is declared ``ProtocolError(BaseError, ValueError)`` -- so it joins the family *and* - stays catchable by any caller already written around the - :exc:`ValueError` that escapes. Both halves are asserted below, - because the second is what makes this a non-breaking change. + stays catchable by any caller written around a bare :exc:`ValueError`. + Both halves are asserted below, because the second is what keeps such + a caller working. :class:`~pcapkit.utilities.exceptions.EnumError` would not do: it is ``EnumError(BaseError, TypeError)``, so it would silently stop being caught. - A parsed PUZZLE no longer reaches the guard: its ``Lifetime`` octet is + A parsed PUZZLE does not reach the guard: its ``Lifetime`` octet is kept as ``lifetime_exponent`` and written back as is (#1323), so an octet of ``0x00`` -- ``2^-32`` seconds, which :class:`~datetime.timedelta` rounds to zero -- re-serialises unchanged. @@ -2894,7 +2886,7 @@ def test_hip_puzzle_lifetime_guard_raises_an_in_library_error(self) -> None: self.assertEqual(parsed.lifetime, datetime.timedelta(0)) self.assertEqual(proto._make_param_puzzle(Parameter.PUZZLE, parsed, version=2).lifetime, 0) - # A positive lifetime still encodes exactly as it did before. + # A positive lifetime encodes as the exponent ``log2(seconds) + 32``. self.assertEqual(proto._make_param_puzzle( Parameter.PUZZLE, version=2, index=1, lifetime=1, opaque=b'op', random=1 << 63, @@ -2953,12 +2945,11 @@ def test_hip_puzzle_and_solution_size_from_version_under_hipv1(self) -> None: self.assertEqual(value.bit_length(), bits) # HIPv1: the RFC's constants, at every width, narrow values - # included. PUZZLE goes first deliberately -- its keyword arguments - # are the same before and after this change, so on the unfixed tree + # included. PUZZLE goes first deliberately -- without the #655 fix # this assertion is reached and reports #655 directly (measured: # ``AssertionError: 12 != 5`` at 1 bit). SOLUTION's cannot be, since - # there the unfixed builder crashes on its own ``lifetime`` default - # before any length is computed -- which is #654. + # without the #654 fix its builder crashes on its own ``lifetime`` + # default before any length is computed. schema = proto._make_param_puzzle( Parameter.PUZZLE, version=1, index=1, lifetime=1, opaque=b'op', random=value, @@ -2991,8 +2982,8 @@ def test_hip_puzzle_and_solution_size_from_version_under_hipv1(self) -> None: ).len, solution_v2) # The two versions agree only where the derived width happens to be - # the RFC's constant, which is the whole of the defect: they used to - # agree everywhere. + # the RFC's constant. Agreeing everywhere is the whole of the + # defect (#655). self.assertEqual(solution_v2 == 20, bits in (57, 64)) self.assertEqual(puzzle_v2 == 12, bits in (57, 64)) @@ -3064,8 +3055,8 @@ def test_hip_puzzle_and_solution_accept_an_explicit_field_width(self) -> None: random=1, rhash_len=rhash_len, ) - # Nor is one too narrow for the value it has to hold: silently truncating - # is what an undersized `len` used to do. + # Nor is one too narrow for the value it has to hold: an undersized `len` + # would silently truncate it. with self.assertRaises(ProtocolError): proto._make_param_solution( Parameter.SOLUTION, version=2, index=1, reserved=0, opaque=b'op', @@ -3120,8 +3111,8 @@ def test_hip_parameter_total_length_matches_the_rfc_7401_formula(self) -> None: -- and that matters more here than usual, because **pcapkit round-trips its own output whatever this formula says**: the writer and the reader - shared one wrong expression, so their disagreement with a real peer was - invisible to every construct-parse-construct test in the suite. Only an + share one expression, so a wrong one disagrees with a real peer + invisibly to every construct-parse-construct test in the suite. Only an independent statement of the RFC's arithmetic can see it, which is what ``rfc_total`` below is. It is written out longhand from the RFC text and deliberately does *not* call @@ -3134,23 +3125,25 @@ def test_hip_parameter_total_length_matches_the_rfc_7401_formula(self) -> None: * ``Length = 4``, a whole ``SEQ``, and ``Length = 20``, a whole ``SOLUTION``: contents plus the four-octet header are *already* - 8-aligned, so the RFC wants **no padding at all**. The old rule - appended four octets that must not be there, and a rule that dropped + 8-aligned, so the RFC wants **no padding at all**. A contents-aligning + rule appends four octets that must not be there, and a rule that dropped the outer ``% 8`` -- ``8 - (Length + 4) % 8`` rather than ``(8 - (Length + 4) % 8) % 8`` -- would append eight. Only the residue ``Length % 8 == 4`` separates the correct answer from both. * ``Length = 0``, ``8``, ``16``: contents are 8-aligned on their own, so - the old rule appended nothing and left the record four octets short. + a contents-aligning rule appends nothing and leaves the record four + octets short. This is the residue at which the defect *under*-pads, and it is the one a test of "is the result at least as long as the contents" cannot see. * ``Length`` not a multiple of four -- 1, 2, 3, 5, 6, 7 -- where the pad is 3, 2, 1, 7, 6, 5. A formula that only ever moved in steps of four, - which both the old and the fixed one look like at a glance, is caught - here and nowhere else. + which both a contents-aligning rule and the RFC's look like at a + glance, is caught here and nowhere else. - The old and the correct formula never agree, at any ``Length``: there is - no residue at which this test would have passed before the fix. + A contents-aligning and the correct formula never agree, at any + ``Length``: there is no residue at which this test passes with the + former. """ from pcapkit.protocols.schema.internet import hip as hip_schema @@ -3160,7 +3153,7 @@ def rfc_total(length: int) -> int: return 11 + length - (length + 3) % 8 def pre_651_total(length: int) -> int: - """What every padding site computed before #651.""" + """Contents-aligning padding, the rule #651 guards against.""" return 4 + length + (8 - (length % 8)) % 8 for length in range(64): @@ -3175,16 +3168,17 @@ def pre_651_total(length: int) -> int: self.assertEqual(4 + length + padding, total) self.assertGreaterEqual(padding, 0) self.assertLessEqual(padding, 7) - # and the defect is gone at every single residue, not on average + # and differs from the defective rule at every single residue, + # not on average self.assertNotEqual(total, pre_651_total(length)) # Which parameters this governs, stated rather than implied: every padding # site in ``schema/internet/hip.py`` and every reported record length in # ``internet/hip.py`` routes through these two helpers -- 45 of the 46 # parameter schemas and 48 of the 49 reported lengths. The forty-sixth is - # ``LOCATOR_SET``, which since #679 uses + # ``LOCATOR_SET``, which uses # :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len` - # instead: the same arithmetic, read off a snapshot of the parameter's + # instead (#679): the same arithmetic, read off a snapshot of the parameter's # own ``Length`` rather than off ``pkt['len']``, which its nested # locators shadow while they pack. The counts and the identity of that # one callback are asserted directly in @@ -3228,17 +3222,18 @@ def pre_651_total(length: int) -> int: def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> None: """#651/#664/#679: two padding callbacks cover all 46, and no third exists. - #651 routed every HIP padding site through - :func:`~pcapkit.protocols.schema.internet.hip.parameter_padding_len`, with - one deliberate exception. ``LOCATOR_SET`` carried two defects that - cancelled at the shape its tests sampled -- a ``Length`` in 4-octet units - where :rfc:`7401` Section 5.2.1 counts bytes, and a padding callback - reading the nested ``Locator.len`` instead of the parameter's -- so - correcting either alone made the wire output worse, and #664 documented - the exclusion rather than leaving it to look accidental. - - **#679 fixed the pair, so the exclusion is no longer a hold-back.** - ``LocatorSetParameter.padding`` now uses + Every HIP padding site goes through + :func:`~pcapkit.protocols.schema.internet.hip.parameter_padding_len` + (#651), with one deliberate exception. ``LOCATOR_SET`` carried two + defects that cancelled at the shape its tests sampled -- a ``Length`` + in 4-octet units where :rfc:`7401` Section 5.2.1 counts bytes, and a + padding callback reading the nested ``Locator.len`` instead of the + parameter's -- so correcting either alone made the wire output worse, + and #664 documented the exclusion rather than leaving it to look + accidental. + + **With the pair fixed (#679), the exclusion is not a hold-back.** + ``LocatorSetParameter.padding`` uses :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len`, which defers to ``parameter_padding_len`` for the arithmetic and differs from it only in *where it reads the length from*: the @@ -3246,15 +3241,15 @@ def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> that :func:`~pcapkit.protocols.schema.internet.hip.locator_set_len_callback` takes before the nested locators overwrite ``pkt['len']``. - So the shape this guards has changed, and the reason to keep guarding it - has not. Three failure modes, none of which anything else catches: + Three failure modes make the shape worth guarding, none of which + anything else catches: * **A third expression appearing.** A later change that gives some parameter an inline lambda -- to make a pinned literal pass, say -- would be indistinguishable from the two legitimate callbacks by inspection. ``bespoke`` below must stay empty. * **The ``LOCATOR_SET`` callback regressing to a raw ``pkt['len']`` - read.** That is the pre-#664 state, and it silently re-introduces + read.** That silently re-introduces defect 1: the value read would be the last locator's 4, so the padding would be four octets at every locator count again. * **``LOCATOR_SET`` being "tidied" onto ``parameter_padding_len`` @@ -3345,8 +3340,8 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None ``SEQ`` is the one to read first. It carries a single four-octet Update ID, so it is complete in eight octets and needs no padding whatsoever; - pre-#651 pcapkit emitted twelve, appending four octets a conformant - receiver would read as the start of the next parameter. :rfc:`7401` + without the #651 fix pcapkit emits twelve, appending four octets a + conformant receiver would read as the start of the next parameter. :rfc:`7401` Section 5.3.5 puts a ``SEQ`` or an ``ACK`` on every ``UPDATE``, so this is not a corner of the parameter space. @@ -3358,9 +3353,9 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None Nothing here imports :func:`~pcapkit.protocols.schema.internet.hip.parameter_total_len`: the - RFC's formula is written out inline, so on a pre-#651 tree this fails - with a real octet-count mismatch rather than with an - :exc:`AttributeError` for a helper that does not exist there yet. + RFC's formula is written out inline, so without #651 this fails with a + real octet-count mismatch rather than with an :exc:`AttributeError` for + a helper that does not exist there. """ from pcapkit.const.hip.parameter import Parameter @@ -3368,17 +3363,15 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None proto = object.__new__(HIP) - # (maker, kwargs, declared Length, total octets, pre-#651 octets) + # (maker, kwargs, declared Length, total octets, octets without #651) cases = [ ('_make_param_seq', {'update_id': 0x01020304}, 4, 8, 12), ('_make_param_esp_info', {}, 12, 16, 20), # #608 sizes SOLUTION's ``len`` from the operands' own widths, so # reaching the RFC's ``Length = 20`` needs eight octets of each: - # 4 + 2 * ceil(57 / 8) == 20. No ``lifetime=`` here: an earlier - # revision of this case passed one to dodge #654's ``math.log2(0)``, - # and #665 removed that keyword, after which it survived only by - # being swallowed by ``**kwargs``. Dead keywords in a table like this - # read as requirements, so it is gone. + # 4 + 2 * ceil(57 / 8) == 20. No ``lifetime=`` here: SOLUTION takes + # none (#665), and a dead keyword swallowed by ``**kwargs`` in a + # table like this reads as a requirement. ('_make_param_solution', {'index': 1, 'opaque': b'op', 'random': 1 << 56, 'solution': 1 << 56}, 20, 24, 28), @@ -3403,7 +3396,7 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None self.assertEqual(len(packed), total) self.assertEqual(len(packed) % 8, 0) self.assertEqual(len(packed), 11 + declared - (declared + 3) % 8) - # the octet count that would have been emitted before the fix, + # the octet count without the #651 fix, # so the case is stated as a difference rather than a value self.assertNotEqual(len(packed), pre_651) # any padding present is zeroed, as 5.2.1 requires of the sender @@ -3426,7 +3419,7 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None # contents-aligned one -- this is the protocol module's sites self.assertEqual(copy.length, 8) - # and the header's own ``len`` is now exact rather than exact-in-pairs: + # and the header's own ``len`` is exact rather than exact-in-pairs: # 4 + 16 // 8 == 6, with nothing lost to the floor division. rebuilt = bytes(HIP(parameters=[(Parameter.SEQ, {'update_id': 0x01020304})] * 2, extension=True, next=6, packet=1, version=2, @@ -3436,7 +3429,7 @@ def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None self.assertEqual(rebuilt[40:], seq * 2) def test_hip_encrypted_data_length_excludes_reserved_and_iv(self) -> None: - """#651: ``ENCRYPTED``'s ``data`` field had to be fixed with the padding. + """#651: ``ENCRYPTED``'s ``data`` length excludes ``Reserved`` and the IV. :rfc:`7401` Section 5.2.18 puts ``Reserved``, ``IV`` and the encrypted data all inside ``Length``, and @@ -3444,16 +3437,16 @@ def test_hip_encrypted_data_length_excludes_reserved_and_iv(self) -> None: match -- but the ``data`` field's length callback subtracted only the ``iv``, so it claimed four octets more than the parameter holds. - The two defects cancelled at some residues of ``Length`` and not others. - Measured across all eight, the old total agreed with the RFC at - ``Length % 8`` in ``{0, 5, 6, 7}`` and was eight octets over at - ``{1, 2, 3, 4}`` -- so ``Length = 8`` is one of the four where the + The two defects cancel at some residues of ``Length`` and not others. + Measured across all eight, the total without the #651 fix agreed with + the RFC at ``Length % 8`` in ``{0, 5, 6, 7}`` and was eight octets over + at ``{1, 2, 3, 4}`` -- so ``Length = 8`` is one of the four where the module emitted RFC-conformant ``ENCRYPTED`` octets while getting both halves wrong. Fixing the padding alone would have taken ``ENCRYPTED`` from right at four of the eight residues to four octets too long at all - eight, so the pair is asserted here together: ``Length = 8`` (where they - used to cancel) and ``Length = 4`` (where they did not, and the record - used to be eight octets over). + eight, so the pair is asserted here together: ``Length = 8`` (where the + defects cancel) and ``Length = 4`` (where they do not, and the record + is eight octets over). """ from pcapkit.const.hip.cipher import Cipher @@ -3465,8 +3458,8 @@ def test_hip_encrypted_data_length_excludes_reserved_and_iv(self) -> None: proto = object.__new__(HIP) # Length = 8: four ``reserved`` octets and four of data. The record is - # 16 octets, which is also what the pre-#651 tree emitted -- by the two - # errors cancelling rather than by either being right. + # 16 octets, which is also what the code emits without the #651 fix -- + # by the two errors cancelling rather than by either being right. schema = proto._make_param_encrypted( Parameter.ENCRYPTED, version=2, cipher=Cipher.NULL_ENCRYPT, data=b'DATA') @@ -3475,20 +3468,20 @@ def test_hip_encrypted_data_length_excludes_reserved_and_iv(self) -> None: bytes.fromhex('028100080000000044415441') + bytes(4)) self.assertEqual(len(bytes(schema)), 16) - # Length = 4: no data at all, and the record is 8 octets. Pre-#651 this - # packed 16 -- the ``data`` field zero-extending b'' out to four octets - # and the padding rule adding four more. + # Length = 4: no data at all, and the record is 8 octets. Without the + # #651 fix this packs 16 -- the ``data`` field zero-extending b'' out + # to four octets and the padding rule adding four more. empty = proto._make_param_encrypted( Parameter.ENCRYPTED, version=2, cipher=Cipher.NULL_ENCRYPT, data=b'') self.assertEqual(empty.len, 4) self.assertEqual(bytes(empty), bytes.fromhex('0281000400000000')) # The residue claim in the docstring, asserted rather than asserted in - # prose. The old record total was ``8 + Length + (-Length % 8)``: four - # octets of ``reserved``, a ``data`` field four octets too wide, and - # contents-aligned padding. It agreed with the RFC at exactly four of - # the eight residues -- which is why a suite that only ever built - # ``Length = 8`` could not see either defect. + # prose. Without the #651 fix the record total is + # ``8 + Length + (-Length % 8)``: four octets of ``reserved``, a + # ``data`` field four octets too wide, and contents-aligned padding. It agrees with the RFC + # at exactly four of the eight residues -- which is why a suite that + # only ever builds ``Length = 8`` cannot see either defect. def old_total(length: int) -> int: return 8 + length + (-length % 8) diff --git a/tests/protocols/internet/test_hip_v2_only_parameters_unit.py b/tests/protocols/internet/test_hip_v2_only_parameters_unit.py index d2d9f4c883..ae087e95a1 100644 --- a/tests/protocols/internet/test_hip_v2_only_parameters_unit.py +++ b/tests/protocols/internet/test_hip_v2_only_parameters_unit.py @@ -3,9 +3,10 @@ GitHub issue #1133. :rfc:`7401` introduces ``DH_GROUP_LIST`` (511), ``HIP_CIPHER`` (579), ``HIT_SUITE_LIST`` (715) and ``TRANSPORT_FORMAT_LIST`` -(2049); :rfc:`5201` has none of them. All four built and parsed back at -version 1. They now raise :class:`~pcapkit.utilities.exceptions.ProtocolError` -on both sides, as ``R1_Counter`` does for the opposite mismatch. +(2049); :rfc:`5201` has none of them. Guards against all four building and +parsing back at version 1: they raise +:class:`~pcapkit.utilities.exceptions.ProtocolError` on both sides, as +``R1_Counter`` does for the opposite mismatch. """ from __future__ import annotations diff --git a/tests/protocols/internet/test_internet_unit.py b/tests/protocols/internet/test_internet_unit.py index c1a67c6361..18d82c0eaa 100644 --- a/tests/protocols/internet/test_internet_unit.py +++ b/tests/protocols/internet/test_internet_unit.py @@ -73,11 +73,10 @@ def test_layer_register_and_protocol_byte_reader(self) -> None: class Other(Raw): pass - # GitHub issue #718 added an identity guard, so re-registering the - # exact same class at the same code -- what this used to do -- is now - # a silent no-op; see + # The identity guard from GitHub issue #718 makes re-registering the + # exact same class at the same code a silent no-op; see # ``test_register_stays_quiet_on_same_object_reregistration`` for that - # case. A genuinely different class is needed to still warn here. + # case. A genuinely different class is needed to warn here. with mock.patch('pcapkit.protocols.internet.internet.warn') as warn: DummyInternet.register(TransType.get(250), Other) warn.assert_called_once() @@ -85,12 +84,11 @@ class Other(Raw): def test_register_stays_quiet_on_same_object_reregistration(self) -> None: """GitHub issue #718: replaying a call with the identical class is a no-op. - Before the fix the guard fired on presence alone, so the *second* of - two calls registering the exact same class under the exact same code - warned about an overwrite that never happened. Matches the identity - guard :func:`register_protocol - ` already - applies. + This guards against the guard firing on presence alone, so that the + *second* of two calls registering the exact same class under the exact + same code warns about an overwrite that never happens. Matches the + identity guard :func:`register_protocol + ` applies. """ from pcapkit.const.reg.transtype import TransType @@ -226,7 +224,7 @@ def test_dispatching_an_unregistered_transtype_leaves_the_registry_alone(self) - ip = IPv4(packet) # The payload is still reachable as Raw, labelled with the protocol - # number it arrived with -- only the registry write is gone. + # number it arrived with -- only the registry write is skipped. self.assertIsInstance(ip.payload, Raw) self.assertEqual(bytes(ip.payload), b'abcd') self.assertEqual(ip.payload.info.protocol, TransType.ICMP) diff --git a/tests/protocols/internet/test_ipv4_header_truncated_payload_runtime.py b/tests/protocols/internet/test_ipv4_header_truncated_payload_runtime.py index 828611ee71..2f67615ea1 100644 --- a/tests/protocols/internet/test_ipv4_header_truncated_payload_runtime.py +++ b/tests/protocols/internet/test_ipv4_header_truncated_payload_runtime.py @@ -5,7 +5,7 @@ handed the next layer the length the header declared even when the capture held none of its octets, so its UDP layer was parsed from an empty stream into an all-zero header, and :meth:`~pcapkit.protocols.protocol.ProtocolBase.from_data` -rebuilt those 8 octets as if they had been on the wire. Such a payload is now a +rebuilt those 8 octets as if they had been on the wire. Such a payload is a :class:`~pcapkit.protocols.misc.null.NoPayload`. ``test.pcapng`` frame 3, cut inside the IPv4 header itself (18 of 20 octets), is kept as raw octets (:issue:`1170`). diff --git a/tests/protocols/internet/test_ipv4_later_fragment_raw_unit.py b/tests/protocols/internet/test_ipv4_later_fragment_raw_unit.py index 9f3d7699a9..827a031505 100644 --- a/tests/protocols/internet/test_ipv4_later_fragment_raw_unit.py +++ b/tests/protocols/internet/test_ipv4_later_fragment_raw_unit.py @@ -9,10 +9,10 @@ :rfc:`791#section-3.2` puts the upper-layer header in the first fragment only. A later one carries a slice of the datagram's data from its offset on, and that -slice starts with no header, so it is now a +slice starts with no header, so it is a :class:`~pcapkit.protocols.misc.raw.Raw` leaf, as Wireshark shows it (``data``, under ``-o ip.defragment:FALSE``). A first fragment (offset zero, MF set) and -an unfragmented datagram are still dissected as before. +an unfragmented datagram are dissected on their Protocol field. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. @@ -113,7 +113,7 @@ def test_first_and_unfragmented_datagrams_are_dissected(self) -> None: def test_issue_repro_later_fragment_in_a_frame(self) -> None: # the issue's reproduction, carried in an Ethernet frame as a capture - # would carry it: ``'UDP' in frame`` no longer holds + # would carry it: ``'UDP' in frame`` does not hold from pcapkit.protocols.internet.ipv4 import IPv4 from pcapkit.protocols.link.ethernet import Ethernet from pcapkit.protocols.misc.raw import Raw @@ -135,7 +135,7 @@ def test_issue_repro_later_fragment_in_a_frame(self) -> None: def test_made_later_fragment_parses_as_raw(self) -> None: # make -> parse: a later fragment built with a UDP object as its payload - # writes the UDP octets, reads back as raw (as IPv6 does since #1545), + # writes the UDP octets, reads back as raw (as IPv6 does, #1545), # and rebuilds to the octets made from pcapkit.protocols.internet.ipv4 import IPv4 from pcapkit.protocols.misc.raw import Raw diff --git a/tests/protocols/internet/test_ipv4_roundtrip_unit.py b/tests/protocols/internet/test_ipv4_roundtrip_unit.py index 923aa1ac21..d8f1a952d7 100644 --- a/tests/protocols/internet/test_ipv4_roundtrip_unit.py +++ b/tests/protocols/internet/test_ipv4_roundtrip_unit.py @@ -7,7 +7,7 @@ * #1316 -- ``make()`` padded with an EOOL after every unaligned option, so the reader stopped there and the later options were lost; * #1317 -- ``from_data`` dropped the parsed NOP/EOOL layout and the octets after - the EOOL, and re-padded per option. The octets after the EOOL are now the + the EOOL, and re-padded per option. The octets after the EOOL are the ``padding`` data field, written back verbatim; the header length follows from the options and those octets, so edited options recompute it; * #1318 -- an option with an unassigned code could be neither rebuilt nor built; @@ -201,7 +201,7 @@ def test_edited_options_recompute_the_header_length(self) -> None: self.assertEqual(data[0], 0x47) self.assertEqual(data[20:].hex(), '88040001' '94040000') - # one option removed shrinks it, rather than zero-filling the old length + # one option removed shrinks it, rather than zero-filling to the length it had edited = self.parse(header('88040001' '94040000')).info.to_dict() edited['options'].pop(OptionNumber.RTRALT) data = IPv4.from_data(edited).data diff --git a/tests/protocols/internet/test_ipv4_ts_sec_roundtrip_unit.py b/tests/protocols/internet/test_ipv4_ts_sec_roundtrip_unit.py index f3048b9bde..bb0b66305d 100644 --- a/tests/protocols/internet/test_ipv4_ts_sec_roundtrip_unit.py +++ b/tests/protocols/internet/test_ipv4_ts_sec_roundtrip_unit.py @@ -6,10 +6,10 @@ inexact, so a stamp of 1001 ms rebuilt as 1000. - GitHub issue #1392: the 1-3 octets a ``TS`` length leaves after its last whole slot were a zero-packing padding field, so they rebuilt as zeros. They - are now the ``remainder`` data field, kept as read. + are the ``remainder`` data field, kept as read. - GitHub issue #1393: ``_make_opt_sec`` set every field termination indicator from the octet's position, so a non-conforming one was rewritten. The wire - bits are now the ``termination`` data field, written back on rebuild. + bits are the ``termination`` data field, written back on rebuild. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/protocols/internet/test_ipv4_ts_slots_unit.py b/tests/protocols/internet/test_ipv4_ts_slots_unit.py index b7cae3c760..2567f5c2a8 100644 --- a/tests/protocols/internet/test_ipv4_ts_slots_unit.py +++ b/tests/protocols/internet/test_ipv4_ts_slots_unit.py @@ -5,7 +5,7 @@ ``pointer - 5`` octets before the pointer, and the rest of the option was a :class:`~pcapkit.corekit.fields.strings.PaddingField` that packed zeros, so ``from_data`` over a parsed header zeroed every slot not yet filled in. Those -slots are now the ``remaining`` data field and a ``make()`` keyword, the same +slots are the ``remaining`` data field and a ``make()`` keyword, the same shape #1319 gave LSR, SSR and RR. Every case builds its own octets in memory and reads no capture. Classes are diff --git a/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py b/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py index 935e8af81b..219cfdd8ec 100644 --- a/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py +++ b/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py @@ -6,7 +6,7 @@ NIC fills it in later. :meth:`IPv4.read ` handed the next layer ``0 - hdr_len`` octets, so TCP fell back to :class:`~pcapkit.protocols.misc.raw.Raw` and the flow was never traced. A Total -Length of 0 now means the rest of the captured frame, with no trailer, as dpkt +Length of 0 means the rest of the captured frame, with no trailer, as dpkt (``ip.py``) and Wireshark ("presumed TSO") read it, and the rebuild writes the 0 back. Following the owner's ruling to match Wireshark, any other Total Length shorter than the header is a "Bogus IP length": no payload layer, the octets diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index e18ec2c82d..7df46eab99 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -120,8 +120,8 @@ def test_ipv4_from_data_rebuilds_a_parsed_datagram(self) -> None: could not rebuild a datagram that had been *parsed*, only one that had been built by hand. The test above covers :meth:`IPv4._make_data ` in isolation, which is - why #494 went in without this surfacing: ``_make_data`` is one input to - ``from_data``, and both remaining faults were downstream of it. + why it cannot surface this (#494): ``_make_data`` is one input to + ``from_data``, and both faults are downstream of it. The wire literals below are parsed rather than constructed, so the info they yield carries what parsing actually produces -- a @@ -129,8 +129,8 @@ def test_ipv4_from_data_rebuilds_a_parsed_datagram(self) -> None: :class:`~pcapkit.protocols.misc.null.NoPayload` instance for the payload, and an option *container* for the options -- rather than the constructor-shaped values :meth:`IPv4.make - ` is normally handed. Each one - raised before the fix: the first two with ``ProtocolUnbound: unsupported + ` is normally handed. Without + the fix, each one raises: the first two with ``ProtocolUnbound: unsupported type ``, the third with the same naming ``NoPayload``. @@ -138,12 +138,9 @@ def test_ipv4_from_data_rebuilds_a_parsed_datagram(self) -> None: generated by :file:`examples/generators/make_samples.py`, inlined so the test does not depend on the fixture captures being present. - It is named by its option rather than by its index because this fix - renumbers that capture: the option-padding half below lets the generator - emit the ``RR``, ``LSR``, ``SID`` and ``SSR`` frames it previously could - not write at all, which grows ``options-ipv4.pcap`` from 8 frames (456 - octets) to 12 (784) and moves this frame from 8th to 12th. An index here - would have been correct when written and wrong once the fix landed. + It is named by its option rather than by its index, because an index into + the generated capture shifts whenever the generator gains or drops a + frame. """ from pcapkit.const.ipv4.option_number import OptionNumber @@ -223,14 +220,13 @@ def test_ipv4_make_accepts_a_protocol_instance_as_payload(self) -> None: raw_payload = Raw(packet=payload) self.assertIsInstance(raw_payload, ProtocolBase) # ... and it is *not* a ``Protocol``, which is the whole of the - # defect. Asserting the negative matters because the branch was not - # uncovered before the fix -- it was covered by the only class in the - # tree that satisfied it, ``DummyProtocol`` in - # tests/protocols/schema/test_schema_unit.py, which subclasses - # ``Protocol`` and so packed happily while every protocol in the - # library raised. Without this line the same hole could be reopened - # by making a protocol subclass ``Protocol`` rather than by fixing - # the check. + # defect. Asserting the negative matters because a ``Protocol`` + # payload alone covers the branch: ``DummyProtocol`` in + # :mod:`tests.protocols.schema.test_schema_unit` subclasses + # ``Protocol`` and so packs under either check, while without the + # fix every protocol in the library raises. Without this line the + # same hole could be reopened by making a protocol subclass + # ``Protocol`` rather than by fixing the check. self.assertNotIsInstance(raw_payload, Protocol) self.assertEqual(proto.make(protocol=6, payload=raw_payload).pack(), expected) @@ -257,19 +253,20 @@ def test_schema_pack_packs_a_protocol_base_payload_on_its_own(self) -> None: branch of ``Schema.pack`` tested ``isinstance(data, Protocol)`` where it meant :class:`~pcapkit.protocols.protocol.ProtocolBase`. - The three assertions are the three things the widened check has to get - right at once, and they pull in different directions: + The three assertions are the three things the ``PayloadField`` branch has + to get right at once, and they pull in different directions: * a :class:`~pcapkit.protocols.protocol.ProtocolBase` payload packs to its own octets -- the defect; - * :class:`~pcapkit.protocols.protocol.Protocol` is still a subclass of - ``ProtocolBase``, so an externally defined engine that *did* satisfy the - old check still satisfies the new one. That is the claim the ``NOTE`` on - the fix makes about not regressing anything, and it is the reason - widening the check is safe rather than merely correct; - * anything that is neither protocol, schema nor :obj:`bytes` still raises - :exc:`~pcapkit.utilities.exceptions.ProtocolUnbound`, so the branch was - widened and not simply removed. + * :class:`~pcapkit.protocols.protocol.Protocol` is a subclass of + ``ProtocolBase``, so an externally defined engine that satisfies an + ``isinstance(data, Protocol)`` check satisfies the ``ProtocolBase`` one + too. That is the claim the ``NOTE`` in ``Schema.pack``'s + ``PayloadField`` branch makes, and why testing for ``ProtocolBase`` is + safe rather than merely correct; + * anything that is neither protocol, schema nor :obj:`bytes` raises + :exc:`~pcapkit.utilities.exceptions.ProtocolUnbound`, so the branch + guards its input rather than accepting everything. """ from pcapkit.protocols.misc.raw import Raw @@ -299,8 +296,8 @@ def header(value: 'object') -> 'Schema_IPv4': self.assertNotIsInstance(raw_payload, Protocol) self.assertEqual(header(raw_payload).pack(), expected) - # an external ``Protocol`` engine is a ``ProtocolBase`` too, so widening - # the check cannot have cost anything that used to work + # an external ``Protocol`` engine is a ``ProtocolBase`` too, so testing + # for ``ProtocolBase`` turns away no ``Protocol`` payload self.assertTrue(issubclass(Protocol, ProtocolBase)) with warnings.catch_warnings(): @@ -578,7 +575,7 @@ def test_ipv4_make_opt_sec_sizes_the_bitmap_from_the_bit_count(self) -> None: proto = object.__new__(IPv4) # One octet: GENSER at bit 0 from the most significant, and a clear - # terminator. Formerly an IndexError. + # terminator. Guards against an IndexError. schema = proto._make_opt_sec(OptionNumber.SEC, authorities=[ProtectionAuthority.GENSER]) self.assertEqual(schema.data, b'\x80') @@ -662,10 +659,9 @@ def test_ipv4_make_opt_sec_rejects_a_termination_bit_as_an_authority(self) -> No proto._make_opt_sec(OptionNumber.SEC, authorities=[ProtectionAuthority(value)]) - # A negative index would have written to the terminator through Python's + # A negative index would write to the terminator through Python's # negative indexing rather than raising -- silent corruption, and the one - # way the IndexError could still have been reached after the arithmetic - # was fixed. + # way to reach the IndexError with the arithmetic right. with self.assertRaises(ProtocolError): proto._make_opt_sec(OptionNumber.SEC, authorities=[-1]) @@ -718,7 +714,7 @@ def test_ipv4_sid_option_is_four_octets_wide_on_the_wire(self) -> None: warnings.simplefilter('always') parsed = IPv4(header, len(header)) - # The over-read was the library naming its own defect. It is gone. + # The over-read warning would be the library naming its own defect. self.assertNotIn('packet length < 0: -2', [str(entry.message) for entry in caught]) @@ -1286,7 +1282,7 @@ def test_ipv4_unregistered_option_code_does_not_mutate_the_registry(self) -> Non self.assertEqual(set(registry), before) # The OrderedMultiDict branch too, which builds the option out - # of ``option.data`` since #1318 gave ``data`` a default. + # of ``option.data``, which has a default (#1318). proto._make_ipv4_options(OrderedMultiDict([ (code, types.SimpleNamespace(data=b'xx')), ])) @@ -1375,9 +1371,9 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: (OptionNumber.NOP, {}), ]) # Four octets of raw option and the four of SID, with the NOP dropped as - # padding. This read 12 before #534, when SID packed to six octets and so - # dragged a NOP and an EOOL in behind it; four is already 32-bit aligned, - # so the alignment branch is not reached here at all any more. + # padding. SID packs to four octets (#534) -- not six, which would drag a + # NOP and an EOOL in behind it and read 12; four is already 32-bit + # aligned, so the alignment branch is not reached here at all. self.assertEqual(total_length, 8) self.assertEqual([type(item).__name__ for item in options], ['bytes', 'SIDOption']) @@ -1433,8 +1429,8 @@ def opt_type(code): type=opt_type(OptionNumber.SEC), length=4, level=ClassificationLevel.Unclassified, - # ``NSA`` where this was ``Field_Termination_Indicator``, which #537 - # now rejects. It is also the more faithful data model: the + # ``NSA`` rather than ``Field_Termination_Indicator``, which is + # rejected here (#537). It is also the more faithful data model: the # terminator is structure, and ``_read_opt_sec`` never puts it in # ``flags`` in the first place. flags=(ProtectionAuthority.GENSER, ProtectionAuthority.NSA), @@ -1613,9 +1609,9 @@ def opt_type(code): ]) # Both NOPs are dropped as padding, on the bytes branch and the schema # branch respectively, which is what this case is here to cover. Only the - # SID option survives, and since #534 it is four octets and needs no - # padding of its own -- this was 8 across 3 entries while the option - # packed to six and pulled a NOP and an EOOL in after it. + # SID option survives, and it is four octets (#534) and needs no padding + # of its own -- a six-octet SID would pull a NOP and an EOOL in after it, + # for 8 across 3 entries. self.assertEqual(schema_total, 4) self.assertEqual(len(schema_options), 1) self.assertEqual(schema_options[0].sid, 9) @@ -2036,9 +2032,10 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: The header below sets ``ihl`` to 7 -- 8 octets of options, all present, with 8 octets of payload behind them -- for an unassigned option (code 31) declaring ``length=12``, with only 6 of its 10 data octets inside - the option area. Those 4 missing octets used to be read as zeros and - rebuilt as if captured. The header itself is complete, so the - header-length check (#1404) passes, and :meth:`OptionField.unpack + the option area. This guards against reading those 4 missing octets as + zeros and rebuilding them as if captured. The header itself is + complete, so the header-length check (#1404) passes, and + :meth:`OptionField.unpack ` refuses the option with :exc:`~pcapkit.utilities.exceptions.ProtocolError` (#1325). Inside a frame the datagram is kept as the octets captured, a diff --git a/tests/protocols/internet/test_ipv6_calipso_make_validation_unit.py b/tests/protocols/internet/test_ipv6_calipso_make_validation_unit.py index 162dbde506..d0a9891a0c 100644 --- a/tests/protocols/internet/test_ipv6_calipso_make_validation_unit.py +++ b/tests/protocols/internet/test_ipv6_calipso_make_validation_unit.py @@ -1,14 +1,13 @@ # -*- coding: utf-8 -*- """Constructing a CALIPSO option validates its data length and its pad. -GitHub issue #1408: ``Opt Data Len`` is one octet, but ``make()`` wrote it -unchecked, so a bitmap plus pad over 255 octets produced a corrupt length -octet. A pad that was not :obj:`bytes`, or a bitmap that was neither -bytes-like nor an iterable of octets, surfaced as a raw :exc:`struct.error`, :exc:`TypeError` or -:exc:`ValueError`. All of these now raise -:exc:`~pcapkit.utilities.exceptions.ProtocolError`. ``make()`` also built an -odd ``Cmpt Length``, which RFC 5570 permits, but the reader rejected it; the -reader now accepts it. +GitHub issue #1408: ``Opt Data Len`` is one octet, so ``make()`` raises +:exc:`~pcapkit.utilities.exceptions.ProtocolError` for a bitmap plus pad over +255 octets rather than write a corrupt length octet. It raises the same for a +pad that is not :obj:`bytes`, or a bitmap that is neither bytes-like nor an +iterable of octets, rather than a raw :exc:`struct.error`, :exc:`TypeError` or +:exc:`ValueError`. The reader accepts the odd ``Cmpt Length`` that ``make()`` +builds and RFC 5570 permits. The classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`, so that they belong to the diff --git a/tests/protocols/internet/test_ipv6_calipso_pad_unit.py b/tests/protocols/internet/test_ipv6_calipso_pad_unit.py index 3fcdcb5b58..274ba998e8 100644 --- a/tests/protocols/internet/test_ipv6_calipso_pad_unit.py +++ b/tests/protocols/internet/test_ipv6_calipso_pad_unit.py @@ -4,7 +4,7 @@ GitHub issue #1403: those octets were a zero-packing padding field missing from the data model, so :meth:`~pcapkit.protocols.protocol.ProtocolBase.from_data` dropped them, shrank ``Opt Data Len`` and padded the header with a ``PadN`` -instead. They are now the ``pad`` data field, written back on rebuild. +instead. They are the ``pad`` data field, written back on rebuild. Every case builds its own octets in memory and reads no capture. The classes are imported inside each test, after diff --git a/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py b/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py index 6d3ccdde38..5dd91a68e7 100644 --- a/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py @@ -11,9 +11,9 @@ dissected, though it was without the Fragment header. :rfc:`8200#section-4.5` puts every header through the upper-layer one in the -first fragment, at offset zero, so the walk now goes on past its Fragment -header. A later fragment starts with no header at all, and still ends the walk. -The packet split at the Fragment header, which reassembly takes, is unchanged. +first fragment, at offset zero, so the walk goes on past its Fragment header. +A later fragment starts with no header at all, and ends the walk. The packet +split that reassembly takes stays at the Fragment header. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/protocols/internet/test_ipv6_ext_chain_limit_1604_unit.py b/tests/protocols/internet/test_ipv6_ext_chain_limit_1604_unit.py index 7afb473cfa..c778e05689 100644 --- a/tests/protocols/internet/test_ipv6_ext_chain_limit_1604_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_chain_limit_1604_unit.py @@ -9,7 +9,7 @@ and 90 on 3.14, from a script at the default limit -- and IPv6, which walks its chain in a loop, went on to any depth. -Now both stop at :data:`~pcapkit.utilities.config.EXTENSION_HEADER_LIMIT` +Both stop at :data:`~pcapkit.utilities.config.EXTENSION_HEADER_LIMIT` headers in a row: what follows is kept as :class:`~pcapkit.protocols.misc.raw.Raw`, with one :exc:`~pcapkit.utilities.warnings.ProtocolWarning`, however long the chain and diff --git a/tests/protocols/internet/test_ipv6_ext_header_rebuild_unit.py b/tests/protocols/internet/test_ipv6_ext_header_rebuild_unit.py index 2bb417311d..bf3105c076 100644 --- a/tests/protocols/internet/test_ipv6_ext_header_rebuild_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_header_rebuild_unit.py @@ -7,7 +7,7 @@ octet after it, so ``type(ex).from_data(ex.info).data`` came back short: 8 of the 28 octets of an 8-octet header followed by a UDP datagram. -``data`` now holds the header alone, which is also what the parent's +``data`` holds the header alone, which is also what the parent's ``from_data`` concatenates, so the parent rebuild is pinned alongside. Every case builds its own octets in memory and reads no capture. Classes are diff --git a/tests/protocols/internet/test_ipv6_ext_unit.py b/tests/protocols/internet/test_ipv6_ext_unit.py index 13dc954250..4395d6646e 100644 --- a/tests/protocols/internet/test_ipv6_ext_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_unit.py @@ -250,9 +250,9 @@ def test_overrun_inside_a_real_chain_stops_the_walk_honestly(self) -> None: def test_shim6_has_no_dedicated_parser_and_dispatches_directly(self) -> None: """``Shim6`` (140) is a real IANA extension header this package has - never implemented, so it used to default to plain ``Raw`` via - ``Internet.__proto__``'s defaultdict -- registered here directly - instead, so no exception is even involved. + no dedicated parser for. It is registered to ``IPv6_Ext`` directly + rather than left to default to plain ``Raw`` via + ``Internet.__proto__``'s defaultdict, so no exception is even involved. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader from pcapkit.const.reg.transtype import TransType @@ -318,7 +318,7 @@ def test_shim6_registration_does_not_leak_into_ipv4_parsing(self) -> None: IPv4 packet whose protocol byte happens to be 140 walk an IPv6-style extension-header chain. The version gate on ``__post_init__`` sends construction back through the caller's own ``@beholder``, which - restores exactly the pre-existing ``Raw`` fallback -- verified + falls back to plain ``Raw`` -- verified against a real :class:`~pcapkit.protocols.internet.ipv4.IPv4` packet, not just the class in isolation. """ @@ -331,7 +331,7 @@ def test_shim6_registration_does_not_leak_into_ipv4_parsing(self) -> None: self.assertIsInstance(ipv4.payload, Raw) # the enum member's own name, not a chain walked out of an IPv4 - # payload -- this is the exact pre-#891-registration rendering. + # payload. self.assertEqual(str(ipv4.protochain), 'IPv4:Shim6') # -- item 1 (cross-review): an unimplemented terminal code must not ------ @@ -351,15 +351,14 @@ def test_unimplemented_terminal_code_stops_the_walk_not_the_packet(self) -> None parser, also resolves to ``Raw``); this file covers one to keep the test proportionate, per the review's own framing. - GitHub issue #925: this used to pick ``BIT-EMU`` (147) for the - example. Unlike 253/254, 147 turned out not to be in IANA's - authoritative *IPv6 Extension Header Types* registry at all -- it - leaked in from a stale cross-reference in the *Protocol Numbers* - registry -- and :class:`~pcapkit.const.ipv6.extension_header. - ExtensionHeader` no longer carries it. 253 exercises the identical - code path (an extension-header code this package has not - implemented a dedicated parser for) while remaining a code IANA - actually recognises as one, which 147 no longer is. + GitHub issue #925: the example is 253, not ``BIT-EMU`` (147). Unlike + 253/254, 147 is not in IANA's authoritative *IPv6 Extension Header + Types* registry at all -- it appears only through a stale + cross-reference in the *Protocol Numbers* registry -- and + :class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader` does not + carry it. 253 exercises the identical code path (an extension-header + code this package has not implemented a dedicated parser for) while + being a code IANA actually recognises as one. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader from pcapkit.const.reg.transtype import TransType @@ -371,7 +370,7 @@ def test_unimplemented_terminal_code_stops_the_walk_not_the_packet(self) -> None ipv6 = IPv6(io.BytesIO(raw), len(raw)) # the header this class exists to protect -- lost entirely under the - # pre-#891 defect, since the whole IPv6 layer became a bare ``Raw``. + # #891 defect, where the whole IPv6 layer becomes a bare ``Raw``. self.assertEqual(str(ipv6.src), '::1') self.assertEqual(str(ipv6.dst), '::1') @@ -667,18 +666,18 @@ def test_esp_extension_mode_is_no_longer_dead(self) -> None: self.assertIs(ESP.__index__(), TransType.ESP) def test_base_role_protocol_delegates_past_the_fallback_answer(self) -> None: - """The trap this change had to avoid, pinned directly. + """The trap the shared base has to avoid, pinned directly. Every subclass implements ``protocol`` as ``return super().protocol``, - and ``IPv6_Ext`` now sits between it and + and ``IPv6_Ext`` sits between it and :class:`~pcapkit.protocols.protocol.ProtocolBase` in the MRO. Without the ``__data__`` discriminator in :attr:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext.protocol`, that ``super()`` lands on the *fallback-role* body, which reads ``self._info.protocol`` -- a field none of the eight data models has. - Measured before the discriminator was added: ``AttributeError`` on all - eight. Both parenting shapes are exercised, since ``AH``/``ESP`` reach - the base through ``IPsec``. + Measured without the discriminator: ``AttributeError`` on every one. + Both parenting shapes are exercised, since ``AH``/``ESP`` reach the + base through ``IPsec``. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader from pcapkit.const.reg.transtype import TransType @@ -767,10 +766,10 @@ def test_length_hint_is_the_eight_octet_rfc_6564_minimum(self) -> None: self.assertEqual(object.__new__(IPv6_Ext).__length_hint__(), 8) def test_next_is_now_shared_by_every_subclass(self) -> None: - """None of the eight declared a ``next`` property before #917, so - reading one raised :exc:`AttributeError`. They inherit the base's now, - which is sound because :rfc:`8200#section-4.1` puts a Next Header octet - first in every extension header and all eight record it under that name. + """Every subclass inherits the base's ``next`` property (#917) -- + without it, reading one raises :exc:`AttributeError` -- which is sound + because :rfc:`8200#section-4.1` puts a Next Header octet first in every + extension header and every subclass records it under that name. """ from pcapkit.const.reg.transtype import TransType from pcapkit.protocols.internet.hopopt import HOPOPT diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 5f3606261f..c19263951a 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -384,9 +384,9 @@ 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 + ahead of dispatching to the per-type constructor; a bare + ``ipaddress.ip_address`` there converts ``dst=True`` 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. """ @@ -412,9 +412,9 @@ 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: + *derived* from the converted addresses, an unguarded bool corrupts the + compression metadata alongside the address list. Measured without the + fix: .. code-block:: text @@ -468,7 +468,7 @@ def test_ipv6_route_read_data_type_errors_report_real_routing_type(self) -> None these methods bind a ``type`` parameter -- so every message read ``[TypeNo ]`` instead of the routing type number that ``header.type`` already carries. Asserting only ``ProtocolError`` was - raised (as the pre-existing coverage does) would not have caught + raised (as the other coverage does) would not have caught this, so this checks the rendered message text directly. """ from pcapkit.const.ipv6.routing import Routing @@ -550,7 +550,7 @@ def test_ipv6_route_read_make_registry_and_property_edges(self) -> None: # Hdr Ext Len in 8-octet units (:rfc:`8200#section-4.4`): 4 octets of # raw data is exactly the 4-octet fixed part every routing type's # data starts with, so no additional 8-octet units are needed. See - # #487 -- this used to assert ``1``, the pre-fix octet-count value. + # #487 for the defect, which gave the octet-count value ``1`` here. self.assertEqual(made_bytes.length, 0) # Raw bytes are unpacked into the routing type's own schema, which # the per-type reader needs (#1236). @@ -572,7 +572,7 @@ def test_ipv6_route_read_make_registry_and_property_edges(self) -> None: ) # 4 reserved + 16-octet address = 20 octets of type-specific data -> # Hdr Ext Len = (20 - 4) / 8 = 2 (:rfc:`8200#section-4.4`). See #487 - # -- this used to assert ``3``, the pre-fix (wrong-sign) value. + # for the defect, which gave the wrong-sign value ``3`` here. self.assertEqual(made_schema.length, 2) with self.assertRaises(ProtocolError): proto.make(data=object()) @@ -682,8 +682,8 @@ def _assert_option_header_read_make_and_registry_edges(self, protocol_cls: type) # NOTE: "No options" is not the same as "no options area". The header is at # least (0 + 1) * 8 = 8 octets and 2 of those are the fixed part, so the # remaining 6 have to be padding options -- an empty options list is not a - # thing that can go on the wire, and asserting for one here used to pass - # only because ``make`` emitted a 2-octet header while declaring 8. + # thing that can go on the wire; asserting for one here passes only if + # ``make`` emits a 2-octet header while declaring 8. made_empty = proto.make(next=TransType.TCP) self.assertEqual(made_empty.next, TransType.TCP) self.assertEqual(made_empty.len, 0) @@ -1047,9 +1047,9 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) - # NOTE: The lengths below are the length of the *options area*, and the two # octets of the fixed header precede it, so a well-formed options area is 6 # octets short of a multiple of 8 -- never a multiple of 8 itself. Each - # assertion is followed by the octet count that justifies it, since these - # numbers used to be 6 too large (or to account padding that was emitted two - # octets longer than it was counted as). + # assertion is followed by the octet count that justifies it, since a + # number 6 too large (or one accounting padding emitted two octets longer + # than it is counted as) is the easy mistake here. options, total_length = make_options([ b'\x00', @@ -1070,8 +1070,8 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) - # NOTE: A shortfall of exactly 2 octets, i.e. ``2 + opt_len`` 6 past a # multiple of 8, is one ``PadN`` of ``Opt Data Len`` 0 rather than two - # ``Pad1`` (#1793). That is a 4-octet option, not the 6-octet one this - # case used to pass -- 2 + 6 is already aligned and needs no padding. + # ``Pad1`` (#1793). That is a 4-octet option, not a 6-octet one -- + # 2 + 6 is already aligned and needs no padding. pad2_options, pad2_length = make_options([ bytes([Option.CALIPSO]) + b'123', ]) @@ -1082,9 +1082,8 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) - self.assertEqual(pad2_options[1].type, Option.PadN) self.assertEqual(pad2_options[1].pack(), b'\x01\x00') - # NOTE: Likewise, the option that needs no padding is the 6-octet one; the - # 8-octet option this case used to pass leaves 2 + 8 = 10 and now draws 6 - # octets of padding. + # NOTE: Likewise, the option that needs no padding is the 6-octet one; an + # 8-octet option leaves 2 + 8 = 10 and draws 6 octets of padding. no_pad_options, no_pad_length = make_options([ bytes([Option.CALIPSO]) + b'12345', ]) @@ -1324,8 +1323,8 @@ def assert_bad(reader, option_schema) -> None: bad_smf = schema.SMFHashBasedDPDOption(type=Option.SMF_DPD, len=3, hav=b'\x81\x02\x03') object.__setattr__(bad_smf, 'mode', 2) assert_bad(proto._read_opt_smf_dpd, bad_smf) - # #775 tier 1: an unresolvable TaggerID name now raises KeyError out - # of Enum_TaggerID.get() instead of minting a member -- this reader + # #775 tier 1: an unresolvable TaggerID name raises KeyError out + # of Enum_TaggerID.get() rather than minting a member -- this reader # must turn that into its own ProtocolError, not let the KeyError # escape. Mirrors the SeedID 'Bogus_Seed_For_Test' case below. bad_tagger_id = schema.SMFIdentificationBasedDPDOption( @@ -1572,9 +1571,10 @@ def test_ipv6_route_rpl_packs_a_multi_address_list(self) -> None: ``IPv6_Route._make_data_type_rpl``) still holds ``self.addresses`` as the ``list[bytes]`` the caller passed in -- one already- compressed address per item -- rather than the concatenated - ``bytes`` a parse produces. ``post_process`` used to assume the - latter unconditionally, so packing sliced and re-joined the *list* - as though it were that concatenated buffer and raised. Merely + ``bytes`` a parse produces. This guards against ``post_process`` + assuming the latter unconditionally, so that packing slices and + re-joins the *list* as though it were that concatenated buffer and + raises. Merely constructing the schema does not exercise this: the defect is only reachable through an actual pack. """ @@ -1616,7 +1616,7 @@ def test_ipv6_route_rpl_fixed_area_is_four_octets(self) -> None: **4** octets. The schema declared it as two ``UInt8Field`` plus a 3-octet ``BitField``, which is **5**, and so built a header one octet narrower than the ``Hdr Ext Len`` computed from that inflated data - area declared. Measured before the fix, for the two addresses the + area declared. Measured without the fix, for the two addresses the round-trip table uses:: len(bytes(made)) == 41 # 4 + (5 + 32) @@ -1628,7 +1628,7 @@ def test_ipv6_route_rpl_fixed_area_is_four_octets(self) -> None: The assertions below are deliberately split. The ``make`` ones go through the public entry point and are blind to how the schema spells - its fields, so they fail on the pre-fix tree with those exact numbers + its fields, so without the fix they fail with those exact numbers rather than with a constructor error; the schema-level one pins the nibble placement, which is the thing a reader of the RFC diagram would check. @@ -1710,7 +1710,7 @@ def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> second = ip_address('2001:db8::2') # Uncompressed: Hdr Ext Len 4, CmprI/CmprE/Pad all 0, two full - # addresses. 8 + 8*4 == 40 octets. The old '% 16' bound rejected this. + # addresses. 8 + 8*4 == 40 octets. A '% 16' bound would reject this. uncompressed = (bytes([0x11, 0x04, 0x03, 0x02]) + b'\x00\x00\x00\x00' + first.packed + second.packed) self.assertEqual(len(uncompressed), 40) @@ -1734,11 +1734,11 @@ def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> info = IPv6_Route(io.BytesIO(compressed), len(compressed), extension=True).info self.assertEqual(info.length, 48) self.assertEqual((info.cmpr_i, info.cmpr_e, info.pad), (4, 4, 4)) - # All THREE elements, not two. ``post_process`` used to subtract - # ``pad_len`` a second time from a buffer whose own length callback had - # already taken it off, losing one address per ``16 - CmprI`` octets of - # padding -- invisible for as long as the guard rejected every padded - # header before this point could be reached. + # All THREE elements, not two. This guards against ``post_process`` + # subtracting ``pad_len`` a second time from a buffer whose own length + # callback has already taken it off, which loses one address per + # ``16 - CmprI`` octets of padding -- invisible while a guard rejects + # every padded header before this point can be reached. self.assertEqual(len(info.ip), 3) self.assertEqual(list(info.ip), [first.packed[4:], second.packed[4:], ip_address('2001:db8::3').packed[4:]]) @@ -1755,9 +1755,9 @@ def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> f'IPv6-Route: [TypeNo {Routing.RPL_Source_Route_Header}] invalid format') # CmprI is 4 bits, so the divisor ``16 - CmprI`` bottoms out at 1 and - # can never be zero. Before #564 ``cmpr_i`` was a whole octet, so a - # hostile packet could set it to 16 and make ``post_process`` divide by - # zero; the field width now rules that out structurally. + # can never be zero. With a whole-octet ``cmpr_i`` (#564) a hostile + # packet could set it to 16 and make ``post_process`` divide by zero; + # the field width rules that out structurally. max_cmpr = (bytes([0x11, 0x01, 0x03, 0x02]) + bytes([0xff, 0x00, 0x00, 0x00]) + bytes(8)) self.assertEqual(len(max_cmpr), 16) @@ -1792,9 +1792,9 @@ def test_ipv6_route_rpl_pad_is_zero_when_nothing_is_elided(self) -> None: dst=ip_address('fe80::1'), ip=[ip_address('2001:db8::1'), ip_address('2001:db8::2')], ) - # ``pad`` is spelled the same before and after #564, so this assertion - # is the measurement rather than a consequence of the reshaped schema: - # it reads 8 on the pre-fix tree. + # ``pad`` is spelled the same with and without the #564 fix, so this + # assertion is the measurement rather than a consequence of the + # schema's shape: without the fix it reads 8. self.assertEqual(schema.pad['pad_len'], 0) self.assertEqual(schema.cmpr['cmpr_i'], 0) self.assertEqual(schema.cmpr['cmpr_e'], 0) @@ -1921,7 +1921,7 @@ def _assert_smf_dpd_options_parse_from_the_wire(self, protocol_cls: type) -> Non octet, and the area handed to the mode schema has to be two octets wider than it, since that schema parses the option header itself. - The first case is #431's reproducer verbatim. On the pristine tree it does + The first case is #431's reproducer verbatim. Without the fix it does not fail, it never returns -- 15 seconds of CPU with no exception and no diagnostic -- so every case runs under a deadline: a regression here has to fail the suite rather than wedge it. @@ -1989,19 +1989,21 @@ def _assert_identification_based_dpd_options_parse_from_the_wire(self, protocol_ route: its length comes from ``Opt Data Len`` too, but the octets it covers are a TaggerID whose own width comes from the ``TidLen`` nibble [:rfc:`6621#section-8.1`], so a wrong ``Opt Data Len`` mis-sizes the - identifier rather than the whole option. Both cases below hang the - pristine tree exactly as the hash-based ones do. + identifier rather than the whole option. Without the fix, both cases + below hang exactly as the hash-based ones do. Only the option itself is asserted here, not the padding after it -- that parity is what :meth:`_assert_identification_based_dpd_null_tid_option_area_matches` - below pins instead. Until #441, HOPOPT and IPv6-Opts disagreed on how much - of the option area an ``SMFIdentificationBasedDPDOption`` left over, - because the IPv6-Opts schema carried an extra forward-matched ``test`` - field its HOPOPT twin did not: the field consumed no bytes but still - occupied a :attr:`Schema.__buffer__ ` - slot, so ``len(schema)`` over-reported the option by one octet under - IPv6-Opts. That was its own ``len(data)``-is-not-consumed defect, distinct - from the one #431 is about. + below pins instead. It guards against the #441 defect, where HOPOPT and + IPv6-Opts disagree on how much of the option area an + ``SMFIdentificationBasedDPDOption`` leaves over, because the IPv6-Opts + schema carries an extra forward-matched ``test`` field its HOPOPT twin + does not: the field consumes no bytes but still occupies a + :attr:`Schema.__buffer__ + ` slot, so + ``len(schema)`` over-reports the option by one octet under IPv6-Opts. + That is its own ``len(data)``-is-not-consumed defect, distinct from the + one #431 is about. """ from pcapkit.const.ipv6.option import Option @@ -2048,12 +2050,12 @@ def _assert_identification_based_dpd_null_tid_option_area_matches(self, protocol """#441: a null-TaggerID I-DPD option must consume exactly what HOPOPT does. This is the issue's own reproduction: a null-TaggerID I-DPD option with a - zero-octet identifier, followed by one octet of ``PadN``. ``ipv6_opts.py``'s - ``SMFIdentificationBasedDPDOption`` carried a stray ``test`` - :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` that its HOPOPT - twin did not -- see + zero-octet identifier, followed by one octet of ``PadN``. It guards + against ``ipv6_opts.py``'s ``SMFIdentificationBasedDPDOption`` carrying a + stray ``test`` :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` + that its HOPOPT twin does not -- see :meth:`_assert_identification_based_dpd_options_parse_from_the_wire` above - for the mechanism. On the pristine tree this option parses under HOPOPT + for the mechanism. Without the fix this option parses under HOPOPT but raises ``ProtocolError: IPv6-Opts: invalid format`` under IPv6-Opts, for the identical octets. @@ -2372,12 +2374,13 @@ def _assert_ilnp_nonce_option_is_sized_by_the_ceiling(self, protocol_cls: type) under-declared ``len`` does not merely mis-state a length -- it silently truncates the nonce on the wire, with no exception raised. - The builder used to size with ``math.ceil(nonce.bit_length() // 8)``. - ``//`` floors, and :func:`math.ceil` of an :class:`int` is a no-op, so - the ceiling was never actually taken: every nonce whose bit length was - not an exact multiple of eight got sized short, and any nonce below 256 - was declared as *zero* octets and dropped from the wire altogether - (``nonce=9`` packed to ``b'\\x8b\\x00'`` and parsed back as ``0``). + This guards against the builder sizing with + ``math.ceil(nonce.bit_length() // 8)``. ``//`` floors, and + :func:`math.ceil` of an :class:`int` is a no-op, so the ceiling is never + actually taken: every nonce whose bit length is not an exact multiple of + eight gets sized short, and any nonce below 256 is declared as *zero* + octets and dropped from the wire altogether (``nonce=9`` packed to + ``b'\\x8b\\x00'`` and parsed back as ``0``). ``0xFFFFFF`` -- bit length 24, the value the option-round-trip generator happens to use -- is one of the few values floor division @@ -2550,20 +2553,17 @@ def test_ipv6_route_source_route_make_accepts_tuple_and_list_addresses(self) -> Boundaries: zero addresses, one, and two, so a fix that special-cases "empty" or stops one item short of the general case is still caught. - #487 update: this test previously round-tripped through - ``Schema_SourceRoute.unpack`` directly rather than a full - ``IPv6_Route`` read, because ``_read_data_type_src``'s own - ``(header.length - 8) % 16`` guard rejected every ``length`` - ``IPv6_Route.make`` computed for this routing type, for any address - count -- and separately, the ``length`` ``make`` computed was itself - wrong (raw octets, not the 8-octet units :rfc:`8200#section-4.4` - specifies for ``Hdr Ext Len``). Both halves are fixed now (see #487), - so this goes through the full ``IPv6_Route`` construct-then-parse - path -- which is a strictly stronger check than unpacking the nested - schema alone -- and the expected on-wire ``Hdr Ext Len`` byte below - changed from the old (buggy) octet counts ``0x04``/``0x14``/``0x24`` - to the RFC-correct 8-octet-unit values ``0x00``/``0x02``/``0x04``. - The tuple-versus-list assertion this test exists for is unchanged. + #487: this goes through the full ``IPv6_Route`` construct-then-parse + path -- a strictly stronger check than unpacking the nested schema + alone. Without the #487 fix, ``_read_data_type_src``'s own + ``(header.length - 8) % 16`` guard rejects every ``length`` + ``IPv6_Route.make`` computes for this routing type, for any address + count, and that ``length`` is itself wrong (raw octets, not the + 8-octet units :rfc:`8200#section-4.4` specifies for ``Hdr Ext Len``). + The expected on-wire ``Hdr Ext Len`` byte below is the RFC-correct + 8-octet-unit value ``0x00``/``0x02``/``0x04``, not the octet count + ``0x04``/``0x14``/``0x24``. The tuple-versus-list assertion is what + this test exists for. """ from ipaddress import ip_address diff --git a/tests/protocols/internet/test_ipv6_jumbogram_unit.py b/tests/protocols/internet/test_ipv6_jumbogram_unit.py index 31d512be8f..24b273196a 100644 --- a/tests/protocols/internet/test_ipv6_jumbogram_unit.py +++ b/tests/protocols/internet/test_ipv6_jumbogram_unit.py @@ -6,15 +6,15 @@ Options header. :class:`~pcapkit.protocols.internet.ipv6.IPv6` read only the Payload Length, so its next layer was :class:`~pcapkit.protocols.misc.null.NoPayload` and the whole jumbo payload -ended up in ``info.trailer``. The Jumbo Payload Length now gives the payload -extent, and only the octets past it are ``trailer``. Every packet still -rebuilds byte for byte. +ended up in ``info.trailer``. The Jumbo Payload Length gives the payload +extent, and only the octets past it are ``trailer``. Every packet rebuilds +byte for byte. GitHub issue #1687: :rfc:`2675#section-3` lists four errors a receiver must detect -- a Payload Length of 0 with a Hop-by-Hop Options header and no Jumbo Payload option, a non-zero Payload Length with the option, a Jumbo Payload -Length below 65,536, and the option with a Fragment header. The parser read -each as a jumbogram, or warned and went on; it now rejects each, as +Length below 65,536, and the option with a Fragment header. Guards against the +parser reading each as a jumbogram, or warning and going on: it rejects each, as :meth:`IPv4.read ` rejects a version other than 4, so behind Ethernet the frame stops at the IPv6 layer, kept raw. And ``IPv6.make`` with no ``payload_length`` writes a Payload Length of 0 for a diff --git a/tests/protocols/internet/test_ipv6_later_fragment_raw_unit.py b/tests/protocols/internet/test_ipv6_later_fragment_raw_unit.py index 0ca39f6d8d..1f674c5960 100644 --- a/tests/protocols/internet/test_ipv6_later_fragment_raw_unit.py +++ b/tests/protocols/internet/test_ipv6_later_fragment_raw_unit.py @@ -9,9 +9,9 @@ :rfc:`8200#section-4.5` puts the upper-layer header in the first fragment only. A later one carries a slice of the Fragmentable Part from its offset on, and -that slice starts with no header, so it is now a +that slice starts with no header, so it is a :class:`~pcapkit.protocols.misc.raw.Raw` leaf. A first fragment, and an atomic -one (offset zero, M clear), are still dissected as before. +one (offset zero, M clear), are dissected on the Fragment header's Next Header. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/protocols/internet/test_ipv6_option_helpers_1519_unit.py b/tests/protocols/internet/test_ipv6_option_helpers_1519_unit.py index d6f0ac10cf..8f7e3110b3 100644 --- a/tests/protocols/internet/test_ipv6_option_helpers_1519_unit.py +++ b/tests/protocols/internet/test_ipv6_option_helpers_1519_unit.py @@ -2,10 +2,10 @@ """The IPv6 option helpers HOPOPT and IPv6-Opts share. GitHub issue #1519. The field helpers of the HOPOPT and IPv6-Opts option -schemas were two copies differing only in the error prefix and in which -module's option schemas the selectors return. They now live once, in -:mod:`pcapkit.protocols.schema.internet.ipv6_option`, and each header module -binds its own prefix and schemas. This module pins the shared helpers under +schemas live once, in :mod:`pcapkit.protocols.schema.internet.ipv6_option`, +rather than as two copies differing only in the error prefix and in which +module's option schemas the selectors return; each header module binds its own +prefix and schemas. This module pins the shared helpers under both prefixes, that each header delegates to them with its own, and that every error message keeps its exact text. diff --git a/tests/protocols/internet/test_ipv6_options_roundtrip_unit.py b/tests/protocols/internet/test_ipv6_options_roundtrip_unit.py index d0864ac1b0..d3213a3306 100644 --- a/tests/protocols/internet/test_ipv6_options_roundtrip_unit.py +++ b/tests/protocols/internet/test_ipv6_options_roundtrip_unit.py @@ -16,8 +16,8 @@ zeroed. * #1237: an unassigned Quick-Start function could not be read. -#1200 is the other direction: a zero-length ``SMF_DPD`` option parsed as H-DPD -by reading the next option's type octet, and is now rejected. +#1200 is the other direction: a zero-length ``SMF_DPD`` option is rejected, +rather than parsed as H-DPD by reading the next option's type octet. Every case builds its own octets in memory and reads no capture. The classes are imported inside each test, after diff --git a/tests/protocols/internet/test_ipv6_version_1680_unit.py b/tests/protocols/internet/test_ipv6_version_1680_unit.py index 41cf2305fc..f9acacd458 100644 --- a/tests/protocols/internet/test_ipv6_version_1680_unit.py +++ b/tests/protocols/internet/test_ipv6_version_1680_unit.py @@ -4,10 +4,11 @@ GitHub issue #1680: :meth:`IPv6.read ` copied the Version nibble without checking it, so a header of any Version was read as IPv6, and :meth:`~pcapkit.protocols.internet.ipv6.IPv6.from_data`, which -always writes 6, rebuilt it with another first octet. It now rejects any Version -but 6, as :meth:`IPv4.read ` rejects -any but 4: each concrete class reads only its own protocol. Behind Ethernet or -inside a tunnel the frame stops there, kept raw and rebuilt as captured. +always writes 6, rebuilt it with another first octet. The reader rejects any +Version but 6, as :meth:`IPv4.read ` +rejects any but 4: each concrete class reads only its own protocol. Behind +Ethernet or inside a tunnel the frame stops there, kept raw and rebuilt as +captured. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`, diff --git a/tests/protocols/internet/test_mh_flags_reserved_unit.py b/tests/protocols/internet/test_mh_flags_reserved_unit.py index bafe1f982d..228a682171 100644 --- a/tests/protocols/internet/test_mh_flags_reserved_unit.py +++ b/tests/protocols/internet/test_mh_flags_reserved_unit.py @@ -4,7 +4,7 @@ GitHub issue #1418: 24 MH ``flags`` bit fields named only some of their bits, and :class:`~pcapkit.corekit.fields.strings.BitField` packs an unnamed bit as zero, so a message, option or sub-structure whose reserved bits were set came -back with them cleared. Each site now names a ``reserved`` sub-field, read into +back with them cleared. Each site names a ``reserved`` sub-field, read into the data model (``flags_reserved`` on a message, since :attr:`MH.reserved` is the fixed-header octet, and ``reserved`` elsewhere) and written back by ``make()``, where it defaults to ``0``. diff --git a/tests/protocols/internet/test_mh_padn_contents_unit.py b/tests/protocols/internet/test_mh_padn_contents_unit.py index 78b9344000..bd12adf355 100644 --- a/tests/protocols/internet/test_mh_padn_contents_unit.py +++ b/tests/protocols/internet/test_mh_padn_contents_unit.py @@ -4,8 +4,8 @@ GitHub issue #1329, its ``PadN`` remainder: the contents of a ``PadN`` mobility option, and of a Flow Identification ``PadN`` sub-option, were held by a :class:`~pcapkit.corekit.fields.strings.PaddingField` that packed zeros, so -``0106aabbccddeeff`` came back as ``0106000000000000``. The contents are now -kept on the data model (``data``) and written back; a fresh build still writes +``0106aabbccddeeff`` came back as ``0106000000000000``. The contents are +kept on the data model (``data``) and written back; a fresh build writes zeros. The root cause is #1223. Every case builds its own octets in memory and reads no capture. :class:`MH` is diff --git a/tests/protocols/internet/test_mh_reserved_padding_unit.py b/tests/protocols/internet/test_mh_reserved_padding_unit.py index 89f63f8076..edfe68db22 100644 --- a/tests/protocols/internet/test_mh_reserved_padding_unit.py +++ b/tests/protocols/internet/test_mh_reserved_padding_unit.py @@ -5,10 +5,10 @@ :class:`~pcapkit.corekit.fields.strings.PaddingField` holds -- in a message body, an option, a Flow Identification sub-option, an ANI sub-option or a QoS attribute -- was dropped by its reader, so a rebuild wrote zeros: Home Test Init -reserved ``aabb`` came back as ``0000``. #1436 made the field pack the octets it -is given; the readers now keep them on the data model (``msg_reserved`` on a +reserved ``aabb`` came back as ``0000``. The field packs the octets it is given +(#1436), and the readers keep them on the data model (``msg_reserved`` on a message, ``reserved`` elsewhere) and the makers write them back. A fresh build -still writes zeros. +writes zeros. Each case is one Mobility Header carrying the site under test, with its reserved octets set to ``aa``, ``aabb`` or ``aabbcc``, next to the same header diff --git a/tests/protocols/internet/test_mh_timestamp_info_1886_unit.py b/tests/protocols/internet/test_mh_timestamp_info_1886_unit.py index 087244f36e..cbcfc9a7bf 100644 --- a/tests/protocols/internet/test_mh_timestamp_info_1886_unit.py +++ b/tests/protocols/internet/test_mh_timestamp_info_1886_unit.py @@ -1,14 +1,13 @@ # -*- coding: utf-8 -*- """``NTPTimestamp`` and ``PMIPv6Timestamp`` are ``Info`` data models (GitHub issue #1886). -Both were named tuples in :mod:`pcapkit.protocols.internet.mh`. They are now -:class:`~pcapkit.protocols.data.data.Data` models in +Both are :class:`~pcapkit.protocols.data.data.Data` models in :mod:`pcapkit.protocols.data.internet.mh`, beside the options that hold them, so :meth:`~pcapkit.corekit.infoclass.Info.from_dict` can resolve the annotations of -those options and rebuild them. These checks pin what that changes: +those options and rebuild them. These checks pin that: -* each is an :class:`~pcapkit.corekit.infoclass.Info`, read by attribute and no - longer a tuple; +* each is an :class:`~pcapkit.corekit.infoclass.Info`, read by attribute and + not a tuple; * a Mobility Header carrying the option that holds it rebuilds byte for byte, from its info and from ``info.to_dict()``; * the dumpers write it keyed by field; diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 3625f2ed78..bc963e04ed 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -128,9 +128,9 @@ def parse(hexstr: str) -> None: lambda: parse('1101' 'c8' '00' '1234' '00000000000000000000')), # a Binding Refresh Request carrying one 8-octet option of type 0xC8. # The code has to be outside the registry itself, not merely one this - # module happens not to handle: every one of the 71 registered option - # types now has a dispatch entry, so a registered code would already be - # in the table and prove nothing about insertion on a miss. + # module happens not to handle: every registered option type has a + # dispatch entry, so a registered code would already be in the table + # and prove nothing about insertion on a miss. ('option', MH.register_option, Option(0xC8), MH.__dict__['__option__'], lambda: parse('1101' '00' '00' '1234' '0000' 'c806' '000000000000')), ('extension', MH.register_extension, CGAExtension(0xFF), @@ -1061,9 +1061,9 @@ def test_mh_fbu_fback_lifetime_is_plain_seconds_unlike_bu_ba(self) -> None: (c.f. #493). The BU/BA cases here are the control: they show the ``* 4`` / ``/ 4`` - scaling this module uses elsewhere is deliberately preserved, so the - FBU/FBack assertions below are evidence of the fix rather than of a - module-wide convention change. + scaling this module uses elsewhere applies there, so the FBU/FBack + assertions below pin those two messages' own units rather than a + module-wide convention. """ from pcapkit.const.mh.packet import Packet from pcapkit.const.mh.status_code import StatusCode @@ -1367,11 +1367,11 @@ def test_mh_fmipv6_round_trip_is_byte_identical(self) -> None: exp_type = Option.Experimental_Mobility_Option # NOTE: Every case below is 8-octet aligned and so draws no padding option, - # which keeps these cases about the FMIPv6 message and option types. It used - # to be a workaround as well: ``_make_opt_pad`` round-tripped a ``PadN`` data - # model two octets too long, because it copied the data model's whole-option - # ``length`` into the schema's ``Option Length`` field. That is fixed, and - # ``test_mh_padding_option_data_model_round_trips`` covers it. + # which keeps these cases about the FMIPv6 message and option types. + # ``_make_opt_pad`` round-tripping a ``PadN`` data model two octets too + # long, by copying the data model's whole-option ``length`` into the + # schema's ``Option Length`` field, is covered by + # ``test_mh_padding_option_data_model_round_trips``. cases = [ ('FBU', Packet.Fast_Binding_Update, { 'seq': 0x1234, 'ack': True, 'home': True, 'lla_compat': False, @@ -1436,14 +1436,13 @@ def test_mh_fmipv6_round_trip_is_byte_identical(self) -> None: self.assertEqual(rebuilt, raw) with self.subTest('FBack, unassigned status now raises on construction'): - # This used to be in ``cases`` above, round-tripping a bare int for - # a status value RFC 5568 leaves unassigned. Constructing from - # ``data=`` builds the wire bytes and then reads them straight - # back to populate ``.info`` -- that read-back is what used to - # mint ``Unassigned_77`` harmlessly. Per the owner's ruling on - # GitHub issue #877 (see + # A status value RFC 5568 leaves unassigned is not one of the + # ``cases`` above. Constructing from ``data=`` builds the wire bytes + # and then reads them straight back to populate ``.info`` -- that + # read-back is where a minting enum would produce ``Unassigned_77``. + # Per the owner's ruling on GitHub issue #877 (see # ``test_mh_rfc5568_local_enums_cover_unregistered_value_sets``), - # the closed :class:`FastBindingAcknowledgmentStatus` now raises + # the closed :class:`FastBindingAcknowledgmentStatus` raises # instead, and it does so on construction itself, before there is # any wire-format bytes object to round-trip at all. from pcapkit.utilities.exceptions import EnumValueError @@ -1460,18 +1459,17 @@ def test_mh_rfc5568_local_enums_cover_unregistered_value_sets(self) -> None: Both are therefore enumerated in :mod:`pcapkit.protocols.internet.mh` itself rather than in :mod:`pcapkit.const.mh`. Per the owner's ruling - on GitHub issue #877, both stay **closed**: GitHub issue #880 found - that a value the RFC leaves unassigned used to be minted a - ``Unassigned_N`` placeholder member -- through a path that bypassed + on GitHub issue #877, both stay **closed**: a value the RFC leaves + unassigned raises rather than being minted an ``Unassigned_N`` + placeholder member (GitHub issue #880) -- a minting path bypasses ``_missing_``'s own ``0 <= value <= 255`` guard and, for an unknown - *name*, aliased a second unknown key onto the first at a shared ``-1`` - sentinel. Both enums now raise instead of minting, so an unassigned - code no longer grows the class. Over IPv4 ``@beholder`` replaces the - MH layer and everything above it with - :class:`~pcapkit.protocols.misc.raw.Raw`; over IPv6 only the MH - header is replaced, by ``IPv6_Ext``, and the layers above it still - parse -- see ``test_mh_local_enums_raise_and_do_not_alias`` for that - distinction and the other two RFC-inline enums. + *name*, aliases a second unknown key onto the first at a shared ``-1`` + sentinel. So an unassigned code does not grow the class. Over IPv4 + ``@beholder`` replaces the MH layer and everything above it with + :class:`~pcapkit.protocols.misc.raw.Raw`; over IPv6 only the MH header + is replaced, by ``IPv6_Ext``, and the layers above it still parse -- + see ``test_mh_local_enums_raise_and_do_not_alias`` for that distinction + and the other two RFC-inline enums. """ from pcapkit.const.mh.option import Option from pcapkit.const.mh.packet import Packet @@ -1487,7 +1485,7 @@ def test_mh_rfc5568_local_enums_cover_unregistered_value_sets(self) -> None: mhlla_type = Option.Mobility_Header_Link_Layer_Address_option # The membership assertions come first, before any lookup that raises - # gets a chance to run: ``_missing_`` is closed now, but pinning the + # gets a chance to run: ``_missing_`` is closed, but pinning the # declared set up front keeps this test honest about exactly what is # -- and is not -- a member, independent of the raise behaviour below. with self.subTest('FBack status members, RFC 5568 section 6.2.3'): @@ -1555,7 +1553,7 @@ def handover_initiate(code: int) -> object: with self.subTest('unassigned values raise instead of minting'): # 2..127 is the unassigned half of the "accepted" range, 132..255 of - # the "rejected" one; both must now raise rather than parse. + # the "rejected" one; both must raise rather than parse. for wire in (2, 77, 127, 132, 255): with self.assertRaises(EnumValueError): fback(wire) @@ -1572,12 +1570,11 @@ def handover_initiate(code: int) -> object: enum_cls(value) with self.subTest('get() backports string and integer lookups, and no longer mints'): - # An unknown *name* is ``EnumKeyError`` since GitHub issue #923 -- - # both ``get`` overrides used to raise ``EnumValueError`` for it, so - # that a name miss and a value miss reported identically, and that - # is the conversion #923's ruling rejects: stdlib ``E['nosuch']`` - # raises ``KeyError``, so the name half is ``KeyError``-derived and - # only ``_missing_``'s value half stays ``ValueError``-derived. + # An unknown *name* is ``EnumKeyError`` (GitHub issue #923), not + # ``EnumValueError``, so a name miss and a value miss do not report + # identically: stdlib ``E['nosuch']`` raises ``KeyError``, so the + # name half is ``KeyError``-derived and only ``_missing_``'s value + # half is ``ValueError``-derived. from pcapkit.utilities.exceptions import EnumKeyError self.assertIs(FastBindingAcknowledgmentStatus.get(130), @@ -1635,27 +1632,21 @@ def handover_initiate(code: int) -> object: chksum=parsed.chksum, data=parsed)), hi_raw) def test_mh_get_default_works_uniformly_through_the_inherited_method(self) -> None: - """GitHub issue #935: the two kept ``get`` overrides are deleted, not widened. - - GitHub issue #930 re-parented :class:`FastBindingAcknowledgmentStatus` - and :class:`IPv6AddressPrefixCode` onto - :class:`~pcapkit.corekit.enum.EnumLookup`, which made both - **advertise** the base's two-argument ``get(key, default)`` through - inheritance while their own kept ``@staticmethod`` overrides still - only accepted one -- calling either with a ``default`` raised - ``TypeError: get() takes 1 positional argument but 2 were given``. - GitHub issue #935's first ruling, the owner's lean toward option 1 of - that thread, widened both signatures to accept ``default`` rather than - delete them. The owner then asked, on GitHub pull request #940, why the - two overrides were needed at all when they could fall back to the base - class's, and his final ruling there went further: he preferred the - second option, deleting both overrides outright. Both classes now - inherit ``get`` from the base exactly as :class:`LMAAddressCode` and - :class:`LocalizedRoutingStatus` -- the two pure re-parents already in - this module -- always have, so ``default`` now works the same way on - all four, uniformly, because there is only one implementation left to - call. This test fails with the ``TypeError`` above against the tree at - 382375811, before either ruling landed. + """GitHub issue #935: ``get(key, default)`` is inherited, not overridden. + + :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode` are parented onto + :class:`~pcapkit.corekit.enum.EnumLookup` (GitHub issue #930), which + makes both **advertise** the base's two-argument ``get(key, default)``. + This guards against either keeping a one-argument ``@staticmethod`` + override, so that calling it with a ``default`` raises + ``TypeError: get() takes 1 positional argument but 2 were given``. Per + the owner's final ruling on GitHub pull request #940, the overrides are + deleted rather than widened: both classes inherit ``get`` from the base, + as :class:`LMAAddressCode` and :class:`LocalizedRoutingStatus` -- the + module's pure re-parents -- do, so ``default`` works the same way on + every class here, because there is only one implementation to call. + This test fails with the ``TypeError`` above at 382375811. """ from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode, @@ -1666,9 +1657,9 @@ def test_mh_get_default_works_uniformly_through_the_inherited_method(self) -> No LMAAddressCode, LocalizedRoutingStatus) with self.subTest('a default argument no longer raises TypeError'): - # This is exactly the call GitHub issue #935 reports as failing - # on the tree before either ruling landed: a second positional - # argument to a ``@staticmethod`` that only declared one. + # This is exactly the call GitHub issue #935 reports as failing: + # a second positional argument to a ``@staticmethod`` that declares + # only one. for cls in classes: member = next(iter(cls)) try: @@ -1692,8 +1683,8 @@ def test_mh_get_default_works_uniformly_through_the_inherited_method(self) -> No with self.subTest('a default naming no registered member still raises, not TypeError'): # The issue's own repro: a default that does not itself resolve # does not silently swallow the miss -- it falls through to the - # same EnumKeyError ``key`` alone would have raised, uniformly - # on all four now. + # same EnumKeyError ``key`` alone raises, uniformly on every class + # here. for cls in classes: with self.subTest(cls=cls.__name__): with self.assertRaises(EnumKeyError) as caught: @@ -1741,17 +1732,13 @@ def test_mh_local_enums_raise_and_do_not_alias(self) -> None: from pcapkit.utilities.exceptions import EnumValueError with self.subTest('LocalizedRoutingStatus and LMAAddressCode raise and now have get()'): - # Both had zero callers repo-wide (tests included), so GitHub issue - # #880 deleted the hand-rolled ``get()`` outright rather than - # rebuilding it on the immutable contract -- true on the tree #880 - # left behind, where neither class carried ``get`` at all. GitHub - # issue #930 re-parented both onto - # :class:`~pcapkit.corekit.enum.EnumLookup`, which is what gives - # every non-registry enumeration ``get``/``get_all`` for free, so - # ``hasattr`` now reads the other way -- the immutable contract - # itself is what this subtest still pins: the base's own ``get`` - # still cannot mint, so an unassigned value raises there exactly - # as it does through the bare constructor. + # Neither carries a hand-rolled ``get()`` (GitHub issue #880); both + # inherit ``get``/``get_all`` from + # :class:`~pcapkit.corekit.enum.EnumLookup` (GitHub issue #930), as + # every non-registry enumeration does. This subtest pins the + # immutable contract: the base's own ``get`` cannot mint, so an + # unassigned value raises there exactly as it does through the bare + # constructor. from pcapkit.utilities.exceptions import EnumKeyError for enum_cls, unassigned in ((LocalizedRoutingStatus, 50), (LMAAddressCode, 50)): @@ -1764,14 +1751,14 @@ def test_mh_local_enums_raise_and_do_not_alias(self) -> None: enum_cls.get('NOT_A_REAL_MEMBER') with self.subTest('the shared -1 aliasing defect is gone'): - # Before the fix, ``get('bogus_one')`` minted a member at the - # shared sentinel -1, and ``get('bogus_two')`` then resolved to + # This guards against ``get('bogus_one')`` minting a member at the + # shared sentinel -1, and ``get('bogus_two')`` then resolving to # that same member -- a name that lies about itself. Both calls - # must now raise independently, and neither may touch the + # must raise independently, and neither may touch the # class's own lookup tables while doing so. # - # ``EnumKeyError`` rather than ``EnumValueError`` since GitHub - # issue #923: these are *name* misses, and #923's ruling keeps a + # ``EnumKeyError`` rather than ``EnumValueError`` (GitHub issue + # #923): these are *name* misses, and #923's ruling keeps a # name miss ``KeyError``-shaped after stdlib ``E['nosuch']``. from pcapkit.utilities.exceptions import EnumKeyError @@ -1786,10 +1773,10 @@ def test_mh_local_enums_raise_and_do_not_alias(self) -> None: self.assertEqual(enum_cls._value2member_map_, before_values) # pylint: disable=no-member with self.subTest("_missing_'s range invariant holds for all four"): - # ``extend_enum`` used to install a member at an explicit value - # without consulting ``_missing_`` at all, which is how a value - # the class's own guard rejects (e.g. -1) ended up a member of it. - # With no minting path left, no lookup -- in range or out of it -- + # ``extend_enum`` installs a member at an explicit value without + # consulting ``_missing_`` at all, which is how a value the class's + # own guard rejects (e.g. -1) can end up a member of it. With no + # minting path, no lookup -- in range or out of it -- # may change the declared member set. for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LocalizedRoutingStatus, LMAAddressCode): @@ -1865,7 +1852,7 @@ def wrap_in_ipv4(mh_payload: bytes) -> IPv4: # generically -- next header ``UDP`` and a length that exactly # matches this ``fback_raw`` payload -- so ``IPv6.read`` returns # normally: IPv6's own header fields (source, destination, hop - # limit) survive, and the chain now ends honestly at the bad + # limit) survive, and the chain ends honestly at the bad # header's replacement rather than swallowing the packet. def wrap_in_ethernet_ipv6(mh_payload: bytes) -> Ethernet: ipv6_header = struct.pack( @@ -1890,10 +1877,10 @@ def wrap_in_ethernet_ipv6(mh_payload: bytes) -> Ethernet: def test_mh_message_flags_pack_each_bit_independently(self) -> None: """Regression test: a cleared flag must not be emitted as a set bit. - :class:`~pcapkit.corekit.fields.strings.BitField` used to seed its per-bit - buffer with NUL bytes and then truth-test it, so the ASCII ``b'0'`` written - for a cleared bit read back as set and every named bit came out as ``1`` - regardless of the value handed in. + This guards against :class:`~pcapkit.corekit.fields.strings.BitField` + seeding its per-bit buffer with NUL bytes and then truth-testing it, so + that the ASCII ``b'0'`` written for a cleared bit reads back as set and + every named bit comes out as ``1`` regardless of the value handed in. """ from pcapkit.const.mh.packet import Packet from pcapkit.const.reg.transtype import TransType @@ -2085,7 +2072,7 @@ def test_mh_opaque_message_body_cannot_be_padded(self) -> None: proto = object.__new__(MH) - # each of these previously emitted 6 + n octets while declaring only 8 + # without the fix, each of these emits 6 + n octets while declaring only 8 for n in (4, 6, 8): with self.subTest(body=n): with mock.patch('pcapkit.protocols.internet.mh.warn') as warned: @@ -2093,7 +2080,7 @@ def test_mh_opaque_message_body_cannot_be_padded(self) -> None: self.assertEqual(warned.call_count, 1) self.assertIs(warned.call_args.args[1], ProtocolWarning) - # the emitted header is now exactly what it declares + # the emitted header is exactly what it declares total = 6 + len(padded) self.assertEqual(total % 8, 0) self.assertEqual(((len(padded) + 6) // 8) * 8, total) @@ -2261,10 +2248,11 @@ def test_mh_experimental_cga_extensions_round_trip(self) -> None: at all, so an opaque payload is the whole of the correct parse rather than a placeholder for a better one. All three share a handler. - The ``data.length`` assertion below read ``len(payload) + 2`` until #512. - That contradicted the ``len(schema.pack())`` assertion four lines above it - -- the same test proved the wire was ``len(payload) + 4`` and then asserted - the parsed length was two octets less. The ``+ 4`` is the wire's own count: + The ``data.length`` assertion below is ``len(payload) + 4``, not + ``len(payload) + 2`` (#512): the latter contradicts the + ``len(schema.pack())`` assertion four lines above it -- the same test + would prove the wire is ``len(payload) + 4`` and then assert the parsed + length two octets less. The ``+ 4`` is the wire's own count: a CGA extension's Extension Type and Extension Data Length are two octets each [:rfc:`4581#section-2`], not one each as an :rfc:`6275#section-6.2` mobility option's are. @@ -2457,11 +2445,10 @@ def test_mh_pmipv6_options_round_trip_byte_for_byte(self) -> None: Note: :attr:`~pcapkit.const.mh.option.Option.CGA_Parameters` is absent, - deliberately: it now parses (see - :meth:`test_mh_cga_parameters_option_now_parses`), but nobody has - verified this test's stricter round-trip identity for it yet -- - that is a separate, deliberate scope decision left for whoever - takes it on next, not implied by parsing alone. + deliberately: it parses (see + :meth:`test_mh_cga_parameters_option_now_parses`), but this test's + stricter round-trip identity is not verified for it -- a separate, + deliberate scope decision, not implied by parsing alone. """ import ipaddress @@ -2874,7 +2861,7 @@ def test_mh_length_derived_addresses_reject_a_bool(self) -> None: ``True`` into ``0.0.0.1`` and ``False`` into ``0.0.0.0``, sized the option as IPv4, and packed it with no exception and no warning -- so #500's guard in :meth:`~pcapkit.corekit.fields.ipaddress._IPAddressField.pre_process` - never saw a :obj:`bool` at all. Measured before the fix: + never saw a :obj:`bool` at all. Measured without the fix: .. code-block:: text @@ -2929,7 +2916,7 @@ def test_mh_length_derived_addresses_reject_a_bool(self) -> None: self.assertEqual(schema.length, 6) self.assertEqual(schema.address, ipaddress.ip_address('0.0.0.1')) - # and a malformed address is now an in-library error rather than + # and a malformed address is an in-library error rather than # ipaddress's own bare ValueError, which ``except BaseError`` cannot catch with self.assertRaises(FieldValueError) as context: proto._make_opt_bid(Option.Binding_Identifier, # type: ignore[arg-type] @@ -2982,16 +2969,16 @@ def test_mh_bool_is_still_a_valid_value_for_an_integer_field(self) -> None: def test_mh_dmnp_prefix_length_range_check_survives_the_bool_guard(self) -> None: """The prefix-length range check needs a test that does not depend on #508. - Before #508's fix, the only thing reaching this check with an IPv4 address - was ``prefix=True`` at the default ``prefix_length=64``: the bool was - laundered into ``0.0.0.1``, and ``64 > 32`` then raised -- which is exactly - why #508's own sweep read this site as already guarded. It was not; the - ``ProtocolError`` was about the *prefix length*, not the prefix. + Without #508's fix, the only thing reaching this check with an IPv4 + address is ``prefix=True`` at the default ``prefix_length=64``: the bool + is laundered into ``0.0.0.1``, and ``64 > 32`` then raises -- which is + exactly why #508's own sweep read this site as already guarded. It was + not; the ``ProtocolError`` was about the *prefix length*, not the prefix. - Now that the bool is refused earlier, that accident no longer exercises the - range check at all, so this test reaches it with a real address instead. - Without it, the fix silently drops coverage of a branch that was only ever - covered by the defect it removes. + With the bool refused earlier, that accident does not exercise the range + check at all, so this test reaches it with a real address instead. + Without this test, the branch would be covered only by the defect #508 + removes. """ from pcapkit.const.mh.option import Option from pcapkit.protocols.internet.mh import MH @@ -3141,15 +3128,16 @@ def test_mh_fid_suboption_takes_an_unassigned_suboption_from_its_data_model(self def test_mh_mn_id_option_length_matches_packed_octets(self) -> None: """The MN-ID option's declared length must count what actually gets packed. - ``_make_opt_mn_id`` used to size the identifier from the *Python type* of - the ``identifier`` argument rather than from ``subtype_val``, which is what - actually selects the wire format (see ``mn_id_selector``). For the - ``IPv6_Address`` subtype -- the method's own default -- the schema always - packs a fixed 16-octet address + This guards against ``_make_opt_mn_id`` sizing the identifier from the + *Python type* of the ``identifier`` argument rather than from + ``subtype_val``, which is what actually selects the wire format (see + ``mn_id_selector``). For the ``IPv6_Address`` subtype -- the method's own + default -- the schema always packs a fixed 16-octet address (:class:`~pcapkit.corekit.fields.ipaddress.IPv6AddressField` ignores any - declared length entirely), so a ``str`` or ``int`` identifier, including - the no-argument default, declared a ``length`` that disagreed with what was - actually packed: ``len(packed) != length + 2``. See #448. + declared length entirely), so sized by type, a ``str`` or ``int`` + identifier, including the no-argument default, declares a ``length`` that + disagrees with what is actually packed: ``len(packed) != length + 2``. + See #448. """ import ipaddress @@ -3158,9 +3146,9 @@ def test_mh_mn_id_option_length_matches_packed_octets(self) -> None: proto = object.__new__(MH) - # every documented ``identifier`` type, against the subtype that used to - # mis-size three of the four -- including the method's own default, which - # is itself one of the broken types (``str``). + # every documented ``identifier`` type, against the subtype that sizing + # by type gets wrong for most of them -- including the method's own + # default, ``str``. cases = ( ('default (no identifier)', {}), ("str '::'", {'identifier': '::'}), @@ -3180,20 +3168,17 @@ def test_mh_mn_id_option_length_matches_packed_octets(self) -> None: def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: """An ``int`` identifier converts to each subtype's own wire form. - ``_make_opt_mn_id`` used to size an ``int`` identifier from the - integer's own :meth:`int.bit_length` regardless of ``subtype``, but - never actually turned it into the octets that width described -- - ``BytesField`` received the ``int`` itself, and ``struct.pack()`` - cannot do anything with that (#467). An earlier revision of this fix - (9b26fa387) rejected ``int`` outright for every subtype but - ``IPv6_Address``, on the reasoning that there was no non-arbitrary - width to convert it to. That reasoning held for ``NAI`` (its field is - a ``StringField``, and NAI is text, not a number) but was wrong for - the other six: ``id_len = math.ceil(identifier.bit_length() / 8)`` - *was* the right, non-arbitrary width all along, self-consistent with - the declared ``length`` by construction -- what was missing was - actually converting ``identifier`` to those octets via - :meth:`int.to_bytes` before handing it to the schema. See #467, #468. + For every ``BytesField`` subtype, + ``id_len = math.ceil(identifier.bit_length() / 8)`` is the right, + non-arbitrary width, self-consistent with the declared ``length`` by + construction, and ``_make_opt_mn_id`` converts ``identifier`` to those + octets via :meth:`int.to_bytes` before handing it to the schema. + ``NAI`` has no such width: its field is a ``StringField``, and NAI is + text, not a number. This guards against sizing an ``int`` identifier + from that width without converting it -- ``BytesField`` then receives + the ``int`` itself, and ``struct.pack()`` cannot do anything with that + (#467) -- and against rejecting ``int`` outright for every subtype but + ``IPv6_Address``, as 9b26fa387 did. See #467, #468. """ import io @@ -3252,13 +3237,12 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: # ``OverflowError`` and ``ipaddress.IPv6Address`` an # ``AddressValueError`` -- itself a bare ``ValueError``. So the guard # sits before the subtype dispatch rather than inside the ``int`` - # branch, and ``IPv6_Address`` is covered here too: an earlier revision - # of this fix guarded only inside that branch, which the - # ``IPv6_Address`` dispatch never reaches, leaving + # branch, and ``IPv6_Address`` is covered here too: a guard only inside + # that branch, which the ``IPv6_Address`` dispatch never reaches, leaves # ``identifier=-5, subtype=IPv6_Address`` leaking # ``AddressValueError: -5 (< 0) is not permitted as an IPv6 address`` # out of the very handler that exists to stop bare stdlib exceptions - # escaping. Confirmed to fail against that revision. + # escaping. Confirmed to fail against such a guard. for subtype in ('NAI', 'IPv6_Address', 'IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'): with self.subTest(subtype=subtype, identifier=-5): @@ -3268,10 +3252,9 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: identifier=-5) self.assertIsInstance(ctx.exception, BaseError) - # the ``IPv6_Address`` subtype is unaffected -- an ``int`` identifier - # still converts to its fixed 16-octet wire form via - # :class:`ipaddress.IPv6Address`, which both converts and validates, - # as #448 fixed and neither revision of this fix has touched. + # for the ``IPv6_Address`` subtype, an ``int`` identifier converts to + # its fixed 16-octet wire form via :class:`ipaddress.IPv6Address`, + # which both converts and validates (#448). schema = proto._make_opt_mn_id( # type: ignore[arg-type] Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, identifier=0x1234) self.assertEqual(schema.length, 17) @@ -3279,8 +3262,8 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: # ``IPv6_Address`` is the one subtype with an *upper* bound as well, and it # is checked on both sides of the boundary rather than at some large value, - # which is how this gap survived the first pass: the earlier probe stopped - # at ``2**128 - 1``, exactly one below where the answer changes. Above the + # since a probe that stops at ``2**128 - 1`` sits exactly one below where + # the answer changes. Above the # bound ``ipaddress.IPv6Address`` raises ``AddressValueError`` -- a bare # ``ValueError`` -- so it must be rejected in-library instead. The bound is # subtype-dependent: the ``BytesField`` subtypes have no ceiling and simply @@ -3336,8 +3319,9 @@ def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None closed the ``int`` half of that union. But each subtype's field (c.f. ``mn_id_selector``) accepts exactly *one* of ``bytes``/``str`` -- a ``StringField`` for ``NAI``, a ``BytesField`` for the other six - -- so handing the wrong one, or a value of neither type, used to - leak a bare stdlib exception instead of an in-library one: + -- so this guards against handing the wrong one, or a value of + neither type, leaking a bare stdlib exception instead of an in-library + one: * ``bytes``/list/dict for ``NAI`` -- ``AttributeError`` (``StringField`` calls ``identifier.encode(...)`` to pack) @@ -3402,9 +3386,9 @@ def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None self.assertIsInstance(ctx.exception, ProtocolError) self.assertIn('IPv6_Address', str(ctx.exception)) - # the matching type, including the empty-value boundary, still works - # for every subtype -- this fix must reject the wrong type, not - # tighten what already worked. + # the matching type, including the empty-value boundary, works for + # every subtype -- the guard must reject the wrong type, not tighten + # what is valid. schema = proto._make_opt_mn_id( # type: ignore[arg-type] Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier='') self.assertEqual(schema.length, 1) @@ -3442,13 +3426,13 @@ def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None def test_mh_mn_id_option_rejects_a_bool_identifier_for_every_subtype(self) -> None: """A ``bool`` identifier is a caller mistake, not a one-octet integer. - ``bool`` is an :class:`int` subclass, so before #469's review flagged it - ``True``/``False`` fell through to the #468 int-conversion path and - silently produced a plausible-looking wire form -- ``IPv6Address(1)``, - that is ``::1``, for ``IPv6_Address``, and a one-octet identifier for the - six ``BytesField`` subtypes. Refused for every subtype now, before the - subtype dispatch, so neither numeric path can reach it. A caller who - genuinely wants the integer passes ``int(flag)``, which the message says. + ``bool`` is an :class:`int` subclass, so unguarded, ``True``/``False`` + fall through to the #468 int-conversion path and silently produce a + plausible-looking wire form -- ``IPv6Address(1)``, that is ``::1``, for + ``IPv6_Address``, and a one-octet identifier for the ``BytesField`` + subtypes (#469). Refused for every subtype, before the subtype + dispatch, so neither numeric path can reach it. A caller who genuinely + wants the integer passes ``int(flag)``, which the message says. """ from pcapkit.const.mh.mn_id_subtype import MNIDSubtype from pcapkit.const.mh.option import Option @@ -3514,8 +3498,8 @@ def test_mh_nested_suboptions_build_from_raw_kwargs(self) -> None: ``data=`` bound to the model parameter and ``kwargs.get('data')`` always saw nothing. The payload was dropped with no exception and the length written as though it were empty -- ``vendor`` and ``subtype`` arrived intact, which - made it look like a partial success. The parameter is now ``option``, which - no sub-option field is called. + made it look like a partial success. The parameter is ``option``, which no + sub-option field is called. """ from pcapkit.const.mh.ani_suboption import ANISuboption from pcapkit.const.mh.flow_id_suboption import FlowIDSuboption @@ -3568,22 +3552,18 @@ def test_mh_nested_suboptions_build_from_raw_kwargs(self) -> None: self.assertIn(payload, option.pack()) def test_mh_cga_parameters_option_now_parses(self) -> None: - """The CGA Parameters option parses -- this used to be pinned as broken. + """The CGA Parameters option parses end to end. - This test used to be - ``test_mh_cga_parameters_option_is_unparsable_upstream``, pinning a - :exc:`KeyError` on the exact reproduction below: sizing + This guards against a :exc:`KeyError` on the exact reproduction below: + sizing :attr:`~pcapkit.protocols.schema.internet.mh.CGAParameter.extensions` - read ``pkt['length']``, a name only the enclosing + reads ``pkt['length']``, a name only the enclosing :class:`~pcapkit.protocols.schema.internet.mh.CGAParametersOption` - declared, and :class:`~pcapkit.corekit.fields.misc.SchemaField` handed - a nested schema a fresh packet dict rather than the enclosing - option's. Fixed by #445 (the nested lookup now falls through to the - enclosing schema). The second half of the fault this test's own - docstring named -- a :class:`~pcapkit.corekit.fields.misc. - ForwardMatchField` miscounted into the nested schema's length -- was - fixed separately, by #446/#456. Both landed, so the option this test - exists for now parses end to end; see also + declares, so :class:`~pcapkit.corekit.fields.misc.SchemaField` has to + let the nested lookup fall through to the enclosing schema rather than + hand it a fresh packet dict (#445). It also guards against a + :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` miscounted into + the nested schema's length (#446/#456). See also :meth:`tests.corekit.test_fields_misc_packet_context. CGAParametersRegressionTests.test_cga_parameters_option_now_parses_end_to_end` for the same reproduction asserting the parsed fields. diff --git a/tests/protocols/internet/test_padn_two_octets_1793_unit.py b/tests/protocols/internet/test_padn_two_octets_1793_unit.py index abfe35e996..5664cb60bc 100644 --- a/tests/protocols/internet/test_padn_two_octets_1793_unit.py +++ b/tests/protocols/internet/test_padn_two_octets_1793_unit.py @@ -4,9 +4,9 @@ GitHub issue #1793: HOPOPT, IPv6-Opts and MH each filled a two-octet shortfall with two ``Pad1`` options. :rfc:`8200#section-4.2`, and :rfc:`6275#section-6.2.2` for mobility options, say a ``PadN`` should be used rather than multiple -``Pad1``, so two octets are now one ``PadN`` of length ``0``: the octets -``01 00``. One octet is still a ``Pad1``, and parsed padding is still rebuilt as -it was read -- only freshly built padding changed. +``Pad1``, so two octets are one ``PadN`` of length ``0``: the octets +``01 00``. One octet is a ``Pad1``, and parsed padding is rebuilt as it was +read -- the rule covers only freshly built padding. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/protocols/internet/test_quick_start_func_unit.py b/tests/protocols/internet/test_quick_start_func_unit.py index b74530b11d..c2afbdb00f 100644 --- a/tests/protocols/internet/test_quick_start_func_unit.py +++ b/tests/protocols/internet/test_quick_start_func_unit.py @@ -5,7 +5,7 @@ declares only under ``TYPE_CHECKING`` and which only ``_QSOption.post_process`` sets on the parse path. A schema built by ``_make_opt_qs`` never passes through that hook, so constructing any Quick-Start option raised ``AttributeError: -'QuickStartRequestOption' object has no attribute 'func'``. The readers now +'QuickStartRequestOption' object has no attribute 'func'``. The readers take the function from ``flags['func']``, the bit-field they already read ``rate`` from. diff --git a/tests/protocols/link/test_ethernet_mac_roundtrip_unit.py b/tests/protocols/link/test_ethernet_mac_roundtrip_unit.py index 82811ba6d8..2fceca49cf 100644 --- a/tests/protocols/link/test_ethernet_mac_roundtrip_unit.py +++ b/tests/protocols/link/test_ethernet_mac_roundtrip_unit.py @@ -15,7 +15,7 @@ :func:`~tests._support.reimport_once_per_class`, not at module load: a class bound at load belongs to whatever :mod:`pcapkit` import was live then, and once an earlier module has purged it, the payload :class:`Raw` resolves to the new -import, which the old :class:`ProtocolBase` rejects, so ``from_data`` drops it. +import, which the stale :class:`ProtocolBase` rejects, so ``from_data`` drops it. """ diff --git a/tests/protocols/link/test_l2tp_version_unit.py b/tests/protocols/link/test_l2tp_version_unit.py index b2c96e5e10..c859a5d8fc 100644 --- a/tests/protocols/link/test_l2tp_version_unit.py +++ b/tests/protocols/link/test_l2tp_version_unit.py @@ -197,12 +197,10 @@ def test_l2tpv3_over_ip_degrades_to_raw_rather_than_a_fabricated_header(self) -> """Binding ``L2TPv2`` at 115 must not invent an L2TPv2 header. This is the acceptance test for issue #548's actual resolution. It - performs the registration the issue asked for -- and that - :func:`~pcapkit.foundation.registry.protocols.register_protocol_code` - once documented as its worked example -- then feeds it a genuine + performs the registration the issue asked for, then feeds it a genuine :rfc:`3931` §4.1.1 v3-over-IP data message. - Measured before the version guard landed, that combination reported + Measured without the version guard, that combination reported ``version=4``, ``tunnelid=0x5678`` and ``sessionid=0xff03``: a confident header assembled out of the top half of a Session ID and the first two octets of a PPP frame. Degrading to @@ -264,11 +262,10 @@ def udp(src: 'int', dst: 'int', payload: bytes) -> bytes: self.assertEqual(udp_info['raw']['packet'], payload) def test_udp_port_1701_is_unaffected_by_the_guard(self) -> None: - """The encapsulation that *is* L2TPv2's still dissects end to end. + """The encapsulation that *is* L2TPv2's dissects end to end. :rfc:`2661` puts L2TPv2 on UDP port 1701, which is the binding - ``UDP.__proto__`` already carries and the one this change must leave - alone. + ``UDP.__proto__`` carries and the one the guard must leave alone. """ def udp(src: 'int', dst: 'int', payload: bytes) -> bytes: diff --git a/tests/protocols/link/test_l2tpv2_ospf_trailer_kept_unit.py b/tests/protocols/link/test_l2tpv2_ospf_trailer_kept_unit.py index 6d242faf75..881bac8839 100644 --- a/tests/protocols/link/test_l2tpv2_ospf_trailer_kept_unit.py +++ b/tests/protocols/link/test_l2tpv2_ospf_trailer_kept_unit.py @@ -8,7 +8,7 @@ L2TPv2 header of Length 8 followed by two octets rebuilt as 8 octets of its 10, and an OSPF header of Packet Length 24 followed by two as 24 of its 26. -Each layer now owns them as ``trailer``, as IPv4, IPv6 (#1209) and IPX (#1433) +Each layer owns them as ``trailer``, as IPv4, IPv6 (#1209) and IPX (#1433) do, and :meth:`make` writes a ``trailer`` outside the length it declares. Every case builds its own octets in memory and reads no capture. Classes are diff --git a/tests/protocols/link/test_l2tpv2_roundtrip_unit.py b/tests/protocols/link/test_l2tpv2_roundtrip_unit.py index 0e43057576..41440c36db 100644 --- a/tests/protocols/link/test_l2tpv2_roundtrip_unit.py +++ b/tests/protocols/link/test_l2tpv2_roundtrip_unit.py @@ -9,7 +9,7 @@ GitHub issue #1208: :meth:`~pcapkit.protocols.link.l2tpv2.L2TPv2.make` set ``S`` only when both ``ns`` and ``nr`` were given, so ``L2TPv2(ns=7)`` silently wrote neither. :rfc:`2661` §3.1 has ``S`` mark the presence of both fields together, -so a lone value is now refused with +so a lone value is refused with :class:`~pcapkit.utilities.exceptions.ProtocolError`. Every case builds its own octets in memory and reads no capture. diff --git a/tests/protocols/link/test_link_unit.py b/tests/protocols/link/test_link_unit.py index 61ef109f32..5038e2c6e0 100644 --- a/tests/protocols/link/test_link_unit.py +++ b/tests/protocols/link/test_link_unit.py @@ -255,9 +255,9 @@ def test_arp_make_builds_schema_with_resolved_addresses(self) -> None: 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: + ``bool`` is an :class:`int` subclass, so unguarded, + ``ipaddress.IPv4Address(True)``/``ipaddress.IPv6Address(True)`` convert + without complaint. Measured without the fix: .. code-block:: text @@ -451,10 +451,10 @@ class Other(Raw): # ``register`` resolves a ``ModuleDescriptor`` before storing it, # so the entry above is already the plain ``Raw`` class, not the - # descriptor -- passing ``Raw`` again would now be the same-object - # no-op GitHub issue #718 added an identity guard for (see - # ``test_link_register_stays_quiet_on_same_object_reregistration``). - # A genuinely different class is needed to still warn here. + # descriptor -- passing ``Raw`` again would be the same-object + # no-op that the identity guard from GitHub issue #718 makes silent + # (see ``test_link_register_stays_quiet_on_same_object_reregistration``). + # A genuinely different class is needed to warn here. with mock.patch('pcapkit.protocols.link.link.warn') as warn: Link.register(custom_code, Other) warn.assert_called_once() @@ -469,12 +469,11 @@ class Other(Raw): def test_link_register_stays_quiet_on_same_object_reregistration(self) -> None: """GitHub issue #718: replaying a call with the identical class is a no-op. - Before the fix the guard fired on presence alone, so the *second* of - two calls registering the exact same class under the exact same code - warned about an overwrite that never happened. Matches the identity - guard :func:`register_protocol - ` already - applies. + This guards against the guard firing on presence alone, so that the + *second* of two calls registering the exact same class under the exact + same code warns about an overwrite that never happens. Matches the + identity guard :func:`register_protocol + ` applies. """ from pcapkit.const.reg.ethertype import EtherType @@ -753,10 +752,11 @@ def test_vlan_properties_read_and_make_variants(self) -> None: self.assertEqual(explicit_schema.type, EtherType.Address_Resolution_Protocol) def test_vlan_dei_is_read_from_the_dei_bit_not_the_pcp(self) -> None: - """``read`` used to report ``bool(tci['pcp'])`` as the DEI flag. + """``read`` reports the DEI bit as the DEI flag, not ``bool(tci['pcp'])``. - That is wrong in both directions, and invisible whenever the two happen - to agree -- which is why the case below pins them in *dis*agreement. + Reading it from the PCP is wrong in both directions, and invisible + whenever the two happen to agree -- which is why the case below pins + them in *dis*agreement. """ from pcapkit.const.reg.ethertype import EtherType @@ -766,9 +766,9 @@ def test_vlan_dei_is_read_from_the_dei_bit_not_the_pcp(self) -> None: from pcapkit.protocols.schema.link.vlan import VLAN as SchemaVLAN cases = ( - # priority set, drop-eligible clear -- the old code said True here + # priority set, drop-eligible clear -- reading the PCP says True here (PriorityLevel.CA, False, False), - # priority clear, drop-eligible set -- the old code said False here + # priority clear, drop-eligible set -- reading the PCP says False here (PriorityLevel.BE, True, True), (PriorityLevel.BE, False, False), (PriorityLevel.CA, True, True),