From ec1b90269f78cd4c2320a9fbe38dd6d9cff98b97 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 29 Sep 2026 20:40:49 -0400 Subject: [PATCH] docs(protocols): cite the extension-header registry in ESP/AH, not protocol-numbers-1.csv (#931) `ESP` and `AH` justified their double inheritance by citing `protocol-numbers-1.csv`'s *IPv6 Extension Header* column -- the derived signal #926 repointed the const generator away from, precisely because it disagreed with the authoritative registry (it flagged BIT-EMU/147 as an extension header; the real registry omits it). - esp.py:978-993 / ah.py:54-67 -- extension-header *membership* now rests on IANA's *IPv6 Extension Header Types* registry and this package's own `ExtensionHeader`, per conventions.rst's "being in that registry is what makes something an extension header" ruling. RFC 4303/4302 section 3.1.1 is quoted too, but only for its actual IPv6-context placement sentence, not as membership evidence -- an earlier revision rephrased that sentence as "places it among the IPv6 extension headers", overstating the RFC and contradicting this docstring's own `Note:` on RFC 8200 declining ESP's extension-header status; caught in review and reworded to a plain placement description. - Added the standalone-protocol half #931 asks for: the section's separate IPv4-context sentence is the primary-source evidence that ESP/AH also travel directly as an IPv4 payload, which *qualifies* an already-standalone header for a second base -- it does not *make* the header standalone (conventions.rst's MH case settles that), and the RFC has no say in which base is named. An earlier revision said the RFC "makes it a standalone protocol" and "names IPsec as the second base"; the latter was positionally false (`ESP.__bases__` puts IPsec first, not second) and unsupported by the cited section. Caught in review and reworded to match hip.py's own phrasing. - Checked hip.py for the same stale citation; already correct. Regression tests assert the registry citation and the corrected prose, ban the specific wrong phrasings from all three earlier fix attempts, and pin the registry-membership clause structurally (via a regex on the parenthetical immediately following "registry lists ... at ") so that a future rewording of the IPv4 evidence, not just this exact phrase, would still be caught if attached to the wrong claim. All fail against 382375811 and against the fix's three prior revisions, before passing here. Build: mypy 321/38 (unchanged), pylint 8.67/10 exit 30 (unchanged), isort clean, targeted docstring tests pass (full esp/ah suite not re-run: test_esp_unit.py's module import alone is ~11 minutes). --- pcapkit/protocols/internet/ah.py | 14 +++-- pcapkit/protocols/internet/esp.py | 17 ++++-- tests/protocols/internet/test_ah_unit.py | 70 ++++++++++++++++++++++ tests/protocols/internet/test_esp_unit.py | 73 +++++++++++++++++++++++ 4 files changed, 165 insertions(+), 9 deletions(-) diff --git a/pcapkit/protocols/internet/ah.py b/pcapkit/protocols/internet/ah.py index 5546c22c5c..11f99570b9 100644 --- a/pcapkit/protocols/internet/ah.py +++ b/pcapkit/protocols/internet/ah.py @@ -52,11 +52,17 @@ class AH(IPsec[Data_AH, Schema_AH], IPv6_Ext[Data_AH, Schema_AH], """This class implements Authentication Header. Double-inherited (GitHub issue #917): ``AH`` is both a member of the - IPsec family and an IPv6 extension header -- IANA's - ``protocol-numbers-1.csv`` marks it ``Y`` in the *IPv6 Extension Header* - column (:rfc:`4302`), and this package's own + IPsec family and an IPv6 extension header -- IANA's *IPv6 Extension + Header Types* registry lists it at 51 (:rfc:`4302#section-3.1.1` has + it appear after the hop-by-hop, routing and fragmentation extension + headers in the IPv6 header chain), and this package's own :class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader` registry - agrees. :class:`~pcapkit.protocols.internet.ipsec.IPsec` is first in the + agrees. The same section separately states that, in the context of + IPv4, AH is placed after the IP header and before the next-layer + protocol -- the primary-source evidence that it also travels + directly as an IPv4 payload, which is what qualifies it for a base + besides :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`. + :class:`~pcapkit.protocols.internet.ipsec.IPsec` is first in the bases so that its :meth:`~pcapkit.protocols.internet.ipsec.IPsec.id` keeps precedence. diff --git a/pcapkit/protocols/internet/esp.py b/pcapkit/protocols/internet/esp.py index 88cfcd591c..7a1f505854 100644 --- a/pcapkit/protocols/internet/esp.py +++ b/pcapkit/protocols/internet/esp.py @@ -976,12 +976,19 @@ class ESP(IPsec[Data_ESP, Schema_ESP], IPv6_Ext[Data_ESP, Schema_ESP], """This class implements Encapsulating Security Payload. Double-inherited (GitHub issue #917), mirroring - :class:`~pcapkit.protocols.internet.ah.AH`: IANA's - ``protocol-numbers-1.csv`` marks ``ESP`` ``Y`` in the *IPv6 Extension - Header* column (:rfc:`4303`), and this package's own + :class:`~pcapkit.protocols.internet.ah.AH`: IANA's *IPv6 Extension + Header Types* registry lists ``ESP`` at 50 (:rfc:`4303#section-3.1.1` + has it appear after the hop-by-hop, routing and fragmentation + extension headers in the IPv6 header chain), and this package's own :class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader` registry - agrees (``ESP = 50``), so it must honour the same extension-mode contract - as its siblings. :attr:`payload` and :attr:`protochain` come from + agrees (``ESP = 50``), so it must honour the same extension-mode + contract as its siblings. The same section separately states that, + in the context of IPv4, ESP is placed after the IP header and + before the next-layer protocol -- the primary-source evidence that + it also travels directly as an IPv4 payload, which is what + qualifies it for a base besides + :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`. + :attr:`payload` and :attr:`protochain` come from :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`; :attr:`protocol` is spelled out below for the reason given there. diff --git a/tests/protocols/internet/test_ah_unit.py b/tests/protocols/internet/test_ah_unit.py index 56330c007a..619690b187 100644 --- a/tests/protocols/internet/test_ah_unit.py +++ b/tests/protocols/internet/test_ah_unit.py @@ -114,6 +114,76 @@ def test_ah_read_properties_and_ipsec_id(self) -> None: self.assertTrue(post_proto._extf) post_init.assert_called_once() + def test_docstring_cites_the_extension_header_registry(self) -> None: + """GitHub issue #931. + + The class docstring's extension-header *membership* claim must + rest on IANA's authoritative *IPv6 Extension Header Types* + registry (and this package's own registry, generated from it), + not the ``protocol-numbers-1.csv`` *IPv6 Extension Header* column + that #926 repointed the const generator away from -- that column + is a derived signal, not the registry :rfc:`8200#section-4` names + as authoritative, and disagreed with it on header 147 + (``BIT_EMU``). + + :rfc:`4302#section-3.1.1` is cited too, but only for what it + actually says: an IPv6-context placement recommendation ("should + appear after hop-by-hop, routing, and fragmentation extension + headers"), not a membership assertion -- membership is the + registry's claim, per + ``docs/source/contributing/conventions.rst``'s "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. + + The section's separate IPv4-context sentence ("placed after the + IP header ... before the next layer protocol") is legitimate + prose here too -- it is the primary-source evidence that AH + also travels directly as an IPv4 payload, exactly as + :mod:`~pcapkit.protocols.internet.hip` cites its own IPv4 + appendix for the same reason. Per + ``docs/source/contributing/conventions.rst``'s "own-protocolhood + on its own is not sufficient" ruling (MH is the settling case), + 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 + would forbid the legitimate use kept above. + """ + import re + + from pcapkit.protocols.internet.ah import AH + + # Normalise the docstring's line-wrapping before matching, since the + # citation may be split across lines by hand-wrapping. + doc = ' '.join((AH.__doc__ or '').split()) + self.assertNotIn('protocol-numbers-1.csv', doc) + self.assertNotIn('among the IPv6 extension headers', doc) + self.assertNotIn('second base', doc) + self.assertNotIn('directly after an IPv4 header', doc) + + # Pin the claim, not just the phrase: whatever the RFC-placement + # parenthetical attached directly to the registry-membership + # sentence says, it must not smuggle in IPv4 evidence for that + # claim, regardless of how a future revision words it. + membership_clause = re.search(r'registry lists it at 51 \((.*?)\)', doc) + self.assertIsNotNone(membership_clause, 'could not locate the membership clause') + self.assertNotIn('IPv4', membership_clause.group(1)) + + self.assertIn('IPv6 Extension Header Types', doc) + self.assertIn(':rfc:`4302#section-3.1.1`', doc) + self.assertIn('hop-by-hop, routing and fragmentation', doc) + self.assertIn('IPv6 header chain', doc) + self.assertIn('travels directly as an IPv4 payload', doc) + self.assertIn('qualifies it for a base besides', doc) + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/internet/test_esp_unit.py b/tests/protocols/internet/test_esp_unit.py index 4f4ad5970c..995420c321 100644 --- a/tests/protocols/internet/test_esp_unit.py +++ b/tests/protocols/internet/test_esp_unit.py @@ -1038,6 +1038,79 @@ def test_ipv6_extension_header_position(self) -> None: self.assertIn(str(ESP.__index__()), [str(key) for key in ipv6.extension_headers]) self.assertIn('ESP', str(ipv6.protochain)) + def test_docstring_cites_the_extension_header_registry(self) -> None: + """GitHub issue #931. + + The class docstring's extension-header *membership* claim must + rest on IANA's authoritative *IPv6 Extension Header Types* + registry (and this package's own registry, generated from it), + not the ``protocol-numbers-1.csv`` *IPv6 Extension Header* column + that #926 repointed the const generator away from -- that column + is a derived signal, not the registry :rfc:`8200#section-4` names + as authoritative, and disagreed with it on header 147 + (``BIT_EMU``). + + :rfc:`4303#section-3.1.1` is cited too, but only for what it + actually says: an IPv6-context placement recommendation ("should + appear after hop-by-hop, routing, and fragmentation extension + headers"), not a membership assertion -- membership is the + registry's claim, per + ``docs/source/contributing/conventions.rst``'s "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 + docstring's ``Note:`` (which says RFC 8200 declines ESP + extension-header status); that was caught in review on the + second version of this fix. + + The section's separate IPv4-context sentence ("placed after the + IP header ... before the next layer protocol") is legitimate + prose here too -- it is the primary-source evidence that ESP + also travels directly as an IPv4 payload, exactly as + :mod:`~pcapkit.protocols.internet.hip` cites its own IPv4 + appendix for the same reason. Per + ``docs/source/contributing/conventions.rst``'s "own-protocolhood + on its own is not sufficient" ruling (MH is the settling case), + 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 (``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. + """ + import re + + from pcapkit.protocols.internet.esp import ESP + + # Normalise the docstring's line-wrapping before matching, since the + # citation may be split across lines by hand-wrapping. + doc = ' '.join((ESP.__doc__ or '').split()) + self.assertNotIn('protocol-numbers-1.csv', doc) + self.assertNotIn('among the IPv6 extension headers', doc) + self.assertNotIn('second base', doc) + self.assertNotIn('directly after an IPv4 header', doc) + + # Pin the claim, not just the phrase: whatever the RFC-placement + # parenthetical attached directly to the registry-membership + # sentence says, it must not smuggle in IPv4 evidence for that + # claim, regardless of how a future revision words it. + membership_clause = re.search(r'registry lists ``ESP`` at 50 \((.*?)\)', doc) + self.assertIsNotNone(membership_clause, 'could not locate the membership clause') + self.assertNotIn('IPv4', membership_clause.group(1)) + + self.assertIn('IPv6 Extension Header Types', doc) + self.assertIn(':rfc:`4303#section-3.1.1`', doc) + self.assertIn('hop-by-hop, routing and fragmentation', doc) + self.assertIn('IPv6 header chain', doc) + self.assertIn('travels directly as an IPv4 payload', doc) + self.assertIn('qualifies it for a base besides', doc) + if __name__ == '__main__': unittest.main()