fix(esp): the extension= keyword is accepted, stored and never read

[JarryShaw]

Describe the bug

ESP accepts an extension= keyword, stores it, and then never reads it. Extension mode is dead code for that one protocol.

pcapkit/protocols/internet/esp.py:1276 assigns self._extf = extension, and no line anywhere in the file reads self._extf. Contrast pcapkit/protocols/internet/ah.py, its sibling under IPsec, which reads it three times (:75, :87, :99) to guard name, protochain and payload, and assigns it at :233.

Counted across the eight implemented IPv6 extension headers:

                _extf   UnsupportedCall   if extension: return
HOPOPT            4            7                   1
IPv6_Route        4            7                   1
IPv6_Opts         4            7                   1
IPv6_Frag         4            7                   1
MH                4            7                   1
HIP               4            7                   1
AH                4            7                   1
ESP               1            0                   0

ESP is the only outlier on all three markers: it has no UnsupportedCall guard properties and no if extension: return short-circuit in read.

Reproduction

Not run against a capture — read from source at origin/main. ESP.read declares extension: 'bool' = False at :996-997 and __post_init__ at :1259-1260, so ESP(..., extension=True) is accepted; :1276 records it; nothing consumes it. The parse therefore behaves identically whether extension is True or False.

Expected behavior

One of two things, and which is a design decision rather than a mechanical fix:

• ESP should support extension mode — then it needs the three UnsupportedCall guard properties and the if extension: return short-circuit its seven siblings have, and _extf becomes load-bearing.
• ESP should not — then extension= should be removed from read and __post_init__ rather than accepted and silently discarded, and :1276 deleted.

Accepting a keyword that changes nothing is the worst of the three, because a caller cannot tell it had no effect.

System information

Source-only; no library version involved. Read at origin/main.

Additional context

Found while measuring the IPv6_Ext shared-base proposal on #891, where it decides whether ESP joins that base. Noted there in #891 so the refactor is not silently made to carry this fix: giving ESP the base would supply the missing guards as a side effect, turning a behaviour-preserving refactor into a behaviour change. This issue exists so that question gets answered on its own merits.

[JarryShaw]

Direction settled by the maintainer's ruling on #891, verbatim:

If per IANA, ESP is an extension header, then it MUST honour the same contracts - that's an implementation defect. And thus we should have ESP double inherit as AH then.

The condition holds. IANA's protocol-numbers-1.csv marks 50,ESP,Encap Security Payload,**Y**,[RFC4303] in the IPv6 Extension Header column, alongside 51,AH,...,Y,[RFC4302] — and this library's own IANA-derived registry already says the same at pcapkit/const/ipv6/extension_header.py:33 (ESP = 50).

So of the two options in the body above, the first is chosen: ESP gains the extension-mode contract. The extension= keyword is not removed.

Concretely that means giving ESP what its seven siblings have — the three UnsupportedCall guard properties reading self._extf (compare ah.py:75,87,99) and the if extension: return short-circuit in read — so that _extf, already assigned at esp.py:1276, becomes load-bearing.

Sequencing: the maintainer also ruled that ESP double-inherits the forthcoming IPv6_Ext base as ESP(IPsec, IPv6_Ext), mirroring AH. Since that base supplies exactly these members, this fix and that re-parent are one change rather than two — implementing the guards by hand now and then hoisting them a PR later would be wasted work. So this issue is deliberately held until the IPv6_Ext base lands, and it is then satisfied by the re-parent.

Marking blocked accordingly. Checkable blocker: the IPv6_Ext base issue, which is itself filed only after #891 merges. Two commands settle whether this is still blocked at any moment:

gh issue list -R JarryShaw/PyPCAPKit --state open --search "IPv6_Ext in:title"
gh issue view 891 -R JarryShaw/PyPCAPKit --json state -q .state

If it turns out the base slips or is abandoned, this unblocks and gets the guards written by hand instead — the defect is real either way and must not be lost to the sequencing.

[JarryShaw]

Scope correction, so this is not closed wholesale when #924 merges: #924 discharges the defect here but not the optimisation.

The substantive half is satisfied by construction. _extf was assigned at esp.py and never read anywhere — a keyword accepted and silently ignored, which is what this issue is about. Under #924 it becomes load-bearing for ESP: payload and protochain read it through the new IPv6_Ext base, and protocol reads it by hand. So extension= now changes behaviour rather than being swallowed.

What #924 does not do: ESP.read still has no if extension: return short-circuit, which its seven siblings all have. That saves wasted decode work rather than changing behaviour, so it is an optimisation this issue never actually asked for — but it is the one difference left between ESP and the other extension headers, and it will look like an oversight to whoever reads the family next.

So when #924 merges, the right move is to close this on the defect and either drop the short-circuit or re-file it as its own small perf item, rather than closing silently and leaving the asymmetry unrecorded. I am not deciding that — flagging it while the context is fresh.

blocked stays until #924 merges; #917 is still open.

[JarryShaw]

Unblocked — #917 closed and #924 merged as 93cf940b3.

The defect is discharged. _extf was assigned at esp.py and never read anywhere, which is exactly what this issue reported: a keyword accepted and silently ignored. Under #924 it is load-bearing — payload and protochain read it through the new IPv6_Ext base, and protocol reads it by hand — so extension= now changes behaviour.

What remains is one asymmetry, and it is not what this issue asked for. ESP.read still has no if extension: return short-circuit, which its seven siblings all have. That saves wasted decode work rather than changing behaviour.

So this can be closed on the defect. I am not closing it myself because the choice is yours: close it now and let the short-circuit go unrecorded, or close it and let me file the short-circuit as a small perf follow-up so the difference between ESP and the other seven is written down somewhere. I would file the follow-up — an unexplained asymmetry in a family of eight is the kind of thing that reads as an oversight to whoever touches it next.

Removing blocked; leaving the issue open pending that choice.

[JarryShaw]

i say the short-circuit must be done in ESP cause it's part of the IPv6 extension headers.

[JarryShaw]

unless RFC tells otherwise.