fix(l2tp): refuse a version nibble other than 2, and settle what IP protocol 115 is (#548)

[JarryShaw]

Fixes #548

TL;DR — the issue's premise does not hold, and the PR says why

#548 reports that IP protocol 115 (TransType.L2TP) is registered nowhere and an
L2TP-over-IP capture falls through to Raw, and its follow-up comment proposes
that L2TPv2 declare TransType.L2TP alongside the UDP port it already answers
on — "one class naming two enclosing layers".

That binding is wrong, and I did not make it. RFC 3931 §4.1.1 is what
protocol 115 designates: "L2TPv3 over IP (both versions) utilizes the
IANA-assigned IP protocol ID 115." 115 is a missing class, not a missing
registration, and it stays unbound. What this PR does instead is fix the real,
live defect that sits underneath the confusion, and land the coverage the issue
asked for in its own "Coverage" section.

No L2TPv3 dissector was invented here.

Did #570's code= mechanism reduce this to one declaration?

The mechanism works exactly as advertised — that part of the expectation holds.
register_protocol_code infers Internet from a TransType member and takes
{UDP: 1701} explicitly for a bare port, and [TransType.L2TP, {UDP: 1701}] in
one code= is a legal, working declaration (its own unit test at
tests/protocols/test_protocol_code_registration_unit.py proves the plumbing).

So the mechanism is not the blocker. The declaration is — it would register
the wrong class. #570's docstring shipped that very line as its worked example:

register_protocol_code(L2TPv2, [TransType.L2TP, {UDP: 1701}])

A caller following it got the misparse below. The example now names L2TPv3,
which genuinely is reachable both ways (RFC 3931 §4.1.1 over IP, §4.1.2 over
UDP 1701). Shape unchanged, subject corrected.

Root cause

Two distinct things, at these lines:

1. pcapkit/protocols/link/l2tpv2.py:187 (pre-fix read()) stored the wire
   version nibble unchecked — version=_flag['version'] — while
   pcapkit/protocols/link/l2tpv2.py:119-128 declares version as
   Literal[2], returns a hard-coded 2, and its docstring already promised
   "a datagram carrying any other value is a different protocol reached through
   a different class". Nothing enforced it, so one object gave two answers:

   Ver=2: accepted. protocol.version=2 info.version=2 tunnelid=0x1234 sessionid=0x5678
   Ver=3: accepted. protocol.version=2 info.version=3 tunnelid=0x1234 sessionid=0x5678
   

2. pcapkit/foundation/registry/protocols.py:262 documented the
   L2TPv2-at-115 registration as the canonical example, contradicting four
   other places in the tree that already said 115 waits on L2TPv3
   (pcapkit/protocols/link/l2tp.py:35-41,
   pcapkit/protocols/link/l2tpv2.py:91-95,
   pcapkit/const/reg/transtype.py:388 — whose IANA citation is literally
   [:rfc:`3931`] — and a passing test,
   tests/protocols/test_dispatch_bindings_unit.py:228).

Why binding L2TPv2 at 115 cannot work

RFC 3931 §4.1.1 again: over IP the v3 session header is "free of any
restrictions imposed by coexistence with L2TPv2 and L2F". A v3 data message
over IP opens with the raw 32-bit Session ID and carries no version nibble at
all, so there is nothing a v2 parser could test to recognise the datagram is
not its own. Measured — registering L2TPv2 at 115 and feeding it a v3-over-IP
data message:

protochain : Ethernet:IPv4:L2TPv2:Raw
l2tp.version  : 4   <-- wire has no version field here
l2tp.tunnelid : 0x5678
l2tp.sessionid: 0xff03
SchemaWarning: packet length < 0: -23

tunnelid and sessionid are read out of the bottom half of the Session ID and
the first two octets of the PPP frame behind it. A complete, confident,
fabricated header where Raw was the honest answer.

The fix

L2TPv2.read() raises ProtocolError when the version nibble is not 2 (RFC 2661
§3.1 fixes Ver at 2 and reserves 1 for L2F; RFC 3931 uses 3). That degrades the
payload to Raw through the existing beholder path — no new error handling
— with the reason recorded on the result.

This fixes a live misparse, not a hypothetical one. RFC 3931 §4.1.2 puts
L2TPv3 on port 1701, the port UDP.__proto__ already binds. So real capture
traffic changes:

	before	after
L2TPv3 on UDP 1701	Ethernet:IPv4:UDP:L2TPv2:Raw, invented tunnelid/sessionid	Ethernet:IPv4:UDP:Raw, protocol=1701, octets preserved
L2TPv3 over IP 115 (if bound)	L2TPv2 + fabricated header	Raw, error='L2TPv2: invalid version: 4', octets preserved

RFC 2661 traffic on 1701 is untouched.

Failing-then-passing evidence

Same file, same tree, PYTHONSAFEPATH=1, pcapkit.__file__ asserted inside the
worktree, exit code read from a file (a failing subtest still prints PASSED for
its parent under pytest 9.1.1 with no pytest-subtests).

Before (pcapkit/protocols/link/l2tpv2.py restored from HEAD) — exit code 1:

22 failed, 5 passed, 5 warnings, 1 subtests passed in 5.94s

E   AssertionError: ProtocolError not raised                    (versions 0, 1, 3, 4, 15)
E   AssertionError: 2 != 15                                     (version property vs info.version)
E   AssertionError: 'l2tp' unexpectedly found in {... 'l2tp': {'version': 4,
      'tunnelid': 22136, 'sessionid': 65283, 'hdr_len': 41, ...}}
E   AssertionError: 'Ethernet:IPv4:UDP:L2TPv2:Raw' != 'Ethernet:IPv4:UDP:Raw'

After — exit code 0:

7 passed, 5 warnings, 21 subtests passed in 6.05s

Coverage

coverage run -m pytest (never pytest-cov):

Name                               Stmts   Miss Branch BrPart  Cover
pcapkit/protocols/link/l2tp.py        15      0      0      0   100%
pcapkit/protocols/link/l2tpv2.py      52      0      4      0   100%

Both new guard branches are exercised (4 branches, 0 partial).

Unit tier (exactly what CI runs) — exit code 0:

1186 passed, 8 skipped, 177 warnings, 2698 subtests passed in 1134.43s

The coverage #548 asked for

No test asserts that every TransType member with an implementing class is
reachable through dispatch, which is why this survived.

New tests/protocols/test_dispatch_reachability_unit.py walks the problem from
the class side: every ProtocolBase descendant whose __index__() returns an
enum member must be reachable under that code in the registry its enum type
designates — read from #570's own _CODE_DESTINATIONS, so the audit and the
registration mechanism cannot drift apart.

test_dispatch_registry_unit.py checks the 38 entries that exist can parse;
it cannot see a class nobody registered, which is how OSPF once shipped
reachable from no table. 23 claims verified, no gaps. A companion case
injects a gap and asserts the audit reports it, so this cannot rot into a
permanently green no-op.

It deliberately does not flag 115: nothing claims 115 in the first place, as
both L2TP.__index__ and L2TPv2.__index__ raise.

Found but deliberately not fixed

• InARP and DRARP are reachable through no dispatch of their own. Both
  return the same EtherType as ARP/RARP, and the registry holds the base;
  selection between them is by opcode, which nothing does. The audit treats a
  same-module sibling as reachable and documents why. Pre-existing design, out
  of scope here.
• Six abstract bases return None from __index__ instead of raising
  (Protocol, Internet, IP, IPsec, Link, Transport), while
  pcapkit/protocols/protocol.py:926 annotates it -> 'StdlibEnum | AenumEnum'.
  The concrete bases that raise (VLAN, L2TP, Application) are the intended
  convention. Not touched — it is a contract question spanning protocol.py.
• 21 remaining GH-nnn issue references across pcapkit/ and tests/,
  against the #nnn convention. Fixed only the one on a line this PR already
  edits; the rest would be an unrelated sweep touching files other work owns.
• isort reports pcapkit/foundation/registry/protocols.py unsorted, but it
  does so identically on the unmodified file at HEAD — a venv-vs-project config
  difference, not introduced here. No import in that file was changed.
• L2TPv2.make() still accepts version= other than 2, so construction can
  build octets read() would now refuse. Left alone: no caller or test does it,
  the signature is already Literal[2], and symmetrising it is a construction-path
  change with its own round-trip implications.

Note on the full test tier

pytest tests (all tiers) reports 134 failures in this worktree — all in
*_runtime.py / *_regression.py, all FileNotFoundError /
GeneratedFixtureInUnitTierError from a fresh worktree with no generated
captures (python examples/generators/make_samples.py not run). Zero failures
outside the fixture tier, and none mentions L2TP. The unit tier above is the CI
gate and is green.

[JarryShaw]

✅ GOOD TO GO at head sha 5e5a357db62bc101fb6f3ea4eb423e0daee26ddb: RFC 3931 §4.1.1/§4.1.2 citations confirmed against the primary text; the ProtocolError→beholder→Raw degradation was reproduced live end-to-end with octets preserved; 22 failed, 5 passed→7 passed reproduced exactly by revert-and-rerun; the reachability audit's negative case was confirmed to have real teeth by independently sabotaging audit() and watching it fail; the claimed 134 full-suite failures (fixture-tier, no generated samples) were reproduced exactly with zero L2TP mentions; Fixes #548 present; 21 SUCCESS/2 SKIPPED/1 IN_PROGRESS, 0 FAILURE.

[JarryShaw]

Reviewer: Sonnet; PR authored on Opus 5.

Falsify-not-bless pass on PR #589 (fix/l2tp-version-guard, head 5e5a357db62bc101fb6f3ea4eb423e0daee26ddb), closing #548. This is the PR that refuses the issue's original premise (bind L2TPv2 at IP protocol 115), so each of its five load-bearing claims was checked against a primary source or reproduced directly rather than accepted from the PR's prose.

1. RFC 3931, checked directly

https://www.rfc-editor.org/rfc/rfc3931.txt (or datatracker text) §4.1.1 states L2TPv3 over IP uses IP protocol 115 and that the header there is free of L2TPv2/L2F coexistence restrictions; §4.1.2 puts L2TPv3 over UDP on port 1701, the same port L2TPv2 already owns. Both citations match the PR's claims. RFC 2661 §3.1 fixes Ver at 2 and reserves 1 for L2F — matches the version-guard rationale. Given the coordinator had already verified these two RFC citations independently, I did not re-run the RFC fetch myself for this specific pair — this is the one place in my pass where I carried over rather than re-deriving, and I'm stating that plainly rather than presenting it as independently checked when it wasn't.

2. Is ProtocolError the right failure mode, and does beholder degradation actually work? (independently verified, live)

Read pcapkit/utilities/decorators.py's beholder decorator and pcapkit/protocols/protocol.py's _import_next_layer (which beholder wraps) directly: on any exception from constructing the next-layer protocol object, it catches it, calls self._get_payload() on the outer protocol (the same bytes that were about to be handed to the failing class), and constructs Raw(file_, length, error=str(exc), alias=proto). This is a pre-existing, generic mechanism L2TPv2.read() now triggers via ProtocolError — no new error handling was added.

Then I did not stop at reading the mechanism — I built a synthetic UDP/1701 datagram with an L2TP-shaped header carrying version nibble 3 and ran it through pcapkit.protocols.transport.udp.UDP directly:

protochain: UDP:Raw
type(next_) = Raw
next_.error = "L2TPv2: invalid version: 3"
next_.info.packet == payload  ->  True   (byte-exact)
Raw(protocol=1701, error=L2TPv2: invalid version: 3, packet=b'\x00\x03\x124Vx\xff\xff\xff\xff')

Octets are genuinely preserved, byte-for-byte, and the protochain/error match the PR's claimed table exactly. This is a live reproduction, not a reading of the PR's own claim.

Nothing else relies on the permissive behaviour. Grepped every test file mentioning L2TP (test_dispatch_bindings_unit.py, test_link_unit.py, test_engine_base.py) and the fixture generator (examples/generators/dispatch.py): every constructed/fixture L2TP datagram uses version 2 (0x0002 in the flags word); no test constructs a non-2-version datagram expecting a successful v2 parse. test_l2tp_over_ip_waits_on_an_l2tpv3_class (pre-existing, test_dispatch_bindings_unit.py:228) already asserts TransType.L2TP is unbound and no L2TPv3 class exists, consistent with this PR not registering L2TPv2 at 115.

3. Before/after reproduction

Checked out the head into a throwaway worktree and ran the new tests/protocols/link/test_l2tp_version_unit.py: 7 passed, 5 warnings, 21 subtests passed in 5.93s, exit 0 — matches the PR's claim exactly.

Reverted only pcapkit/protocols/link/l2tpv2.py and pcapkit/protocols/link/l2tp.py to origin/main and reran the same file: 22 failed, 5 passed, 5 warnings, 1 subtests passed in 5.93s, exit 1 — matches the PR's claimed 22 failed, 5 passed, 5 warnings, 1 subtests passed in 5.94s almost to the decisecond.

4. L2TPv2.make() still accepting version≠2 — checked directly, judged non-blocking

Called L2TPv2().make(version=9, tunnel_id=1, session_id=2) directly: it packs version=9 into the wire bytes with zero validation (b'\x80\t\x00\x01\x00\x02', \x09 visible in the flags word). Confirmed the asymmetry is real. Judged not blocking: no caller or test in the tree constructs a non-2 version (confirmed by the same grep as §2), the type annotation is already Literal[2] for static checking, and the PR discloses this explicitly under "Found but deliberately not fixed" rather than hiding it. Symmetrising make() is a separate, disclosed follow-up, not a silent gap.

5. test_dispatch_reachability_unit.py's negative case — verified to have real teeth by independent mutation, not by trusting the PR's claim

Read audit(): it walks ProtocolBase.__subclasses__() transitively (_descendants), so a class defined at test-run time is genuinely discovered, not looked up from a fixed list. Ran the three tests normally: 3 passed. Then sabotaged audit() myself — patched it to return [], 999 unconditionally, simulating exactly the "an import that silently failed, an except that swallowed too much" scenario the test's own docstring worries about — and reran just test_the_audit_detects_a_code_that_nothing_dispatches:

AssertionError: False is not true : audit missed the injected gap; reported []
1 failed in 0.67s

This is stronger evidence than the PR itself offers: I constructed the adversarial mutation independently and watched the guard fail exactly as it should, rather than taking "the guard has teeth" on trust.

6. The 134-failure full-tier claim — reproduced exactly, not merely restated

Confirmed no pre-generated fixtures existed in the fresh worktree. Ran pytest tests (all tiers, no examples/generators/make_samples.py) to completion: 134 failed, 1251 passed, 23 skipped, 2719 subtests passed in 1223.34s — the failure count matches the PR's claim exactly. Breaking down the 134: grep -c FileNotFoundError on the log gives 131; the remaining 3 are AssertionError from tests.integration.test_engine_parity.EngineParityTests.test_every_engine_reports_the_same_frame_count, an unrelated pre-existing integration test whose subTest loop swallows a per-engine FileNotFoundError (missing generated capture) and then fails a bare assertEqual on the resulting incomplete counts dict — same root cause (no generated fixtures), different exception surface, not a FileNotFoundError literally. This is a small imprecision in the PR's "all... FileNotFoundError/GeneratedFixtureInUnitTierError" framing (3 of 134 are not literally that exception type) but the substance — fixture-tier, unrelated to this PR's changes — holds. Zero of the 134 failures mention L2TP, confirmed both by this full run and by a separate, faster pytest tests -k "l2tp or L2TP" across every tier: 13 passed, 1338 deselected, 0 failed.

7. The #570 docstring worked-example change (L2TPv2→L2TPv3) — judgment call

Confirmed the change is real: register_protocol_code(L2TPv2, [TransType.L2TP, {UDP: 1701}]) → register_protocol_code(L2TPv3, [TransType.L2TP, {UDP: 1701}]), with an added Note immediately after stating plainly that L2TPv3 "does not implement yet" and explaining why L2TPv2 at that key is wrong (cross-referencing the measurement in l2tp.py). I judge this the better of the two options the coordinator posed: naming L2TPv3 keeps the example demonstrating the actual motivating two-key-destination case (a class reachable both by inferred type and by explicit port map), which a generic "115 is unbound" placeholder would not; the immediately-following Note makes it impossible to read the example as claiming L2TPv3 exists today. Not blocking.

8. Fixes #548 literal line

Present, verbatim, first line of the PR body: Fixes #548.

9. CI

24 CheckRuns at time of posting: 21 SUCCESS, 2 SKIPPED, 1 IN_PROGRESS, 0 FAILURE. statusCheckRollup.state: PENDING (one job still running), not FAILURE. Changelog drift is among the SUCCESS entries — independently re-verified with python util/changelog_md.py --check in the worktree, exit 0.

Disagreements / open items

• None on the core claims — RFC citations, the degradation mechanism, the before/after counts, and the audit's teeth are all independently confirmed, several by direct reproduction rather than by reading the PR's report.
• One item explicitly not independently re-derived: RFC 3931 §4.1.1/§4.1.2, carried over from the coordinator's own verification rather than re-fetched by me, per §1 above.
• The make() version asymmetry and the docstring worked-example change are both judged non-blocking, disclosed decisions rather than hidden gaps — reasoning given in §4 and §7.

[JarryShaw]

this \ might not work in Sphinx docs. let's just over-length this line. that's fine.

[JarryShaw]

Done — the \ is gone and the role target sits on one unbroken line:

        disagree with
        :attr:`info.version <pcapkit.protocols.data.link.l2tp.L2TP.version>`
        on the same octets. It did before that guard landed, reporting ``2``
        here and ``3`` there.

It turned out not to need the over-length allowance: on its own line the target is 76 characters including indent, so nothing exceeds what this file already carries (its longest pre-existing line is 150). I took the permission as "don't contort it to stay short" rather than "make it long" — no intermediate variable, no shortened target, :attr: kept, and the rendered prose is unchanged since RST collapses the newlines within the paragraph.

I also unbroke the same \ idiom at two more spots I had written, in tests/protocols/link/test_l2tp_version_unit.py (the module docstring's :attr:L2TPv2.version`` and test_ip_protocol_115_has_no_class_to_dispatch_to's `:meth:` back-reference), on the same reasoning.

One I deliberately left alone: pcapkit/foundation/registry/protocols.py:190 has a pre-existing backslash-continued :class:`~pcapkit.const.reg.\ in _iter_code_targets's docstring, which came in with #570 rather than this PR. Happy to fix it here if you'd like it swept up, but I kept it out to avoid widening the diff into code this change isn't otherwise touching.

Rebased onto 087ed3911 in the same amend — the two changelog conflicts are resolved, with CHANGELOG.md regenerated via util/changelog_md.py rather than hand-merged (--check exits 0). New head 98eb6e4dd.

[JarryShaw]

Needs a rebase — and the conflict is confined to the changelog, not to any source file.

#585 merged at 22:06:26Z and #586 at 22:08:21Z, putting main at ec28418c4. Both of them added a bullet to docs/source/changelog/1.5.0.rst and regenerated CHANGELOG.md, which is what this branch now collides with.

Measured with git merge-tree --write-tree ec28418c4 5e5a357db, exit 1:

CONFLICT (content): Merge conflict in CHANGELOG.md
Auto-merging docs/source/changelog/1.5.0.rst
CONFLICT (content): Merge conflict in docs/source/changelog/1.5.0.rst

Those two files are the whole conflict. Nothing under pcapkit/ or tests/ conflicts, so the code this PR changes is untouched by what merged, and the ✅ cross-review at 5e5a357db and the 22-of-22 green CI both still describe the change on its merits.

Resolve the generated file by regenerating it, not by hand

CHANGELOG.md is generated from the newest entry under docs/source/changelog/ by util/changelog_md.py. Hand-merging the conflict markers in it produces a file that no longer matches its source, which the Changelog drift check will then fail. So:

1. Rebase onto ec28418c4.
2. Resolve only docs/source/changelog/1.5.0.rst — keep this PR's bullet alongside the two that landed, rather than choosing between them.
3. Delete the conflicted CHANGELOG.md result and regenerate: python util/changelog_md.py.
4. Gate it locally before pushing: python util/changelog_md.py --check must exit 0. It compares in memory and writes nothing.
5. Keep the single commit and the literal Fixes #548 line intact; force-push with --force-with-lease to this branch only.

Two traps in that regeneration, both of which have already cost a PR in this batch a round: the converter implements only six rules, so an :data:/:func:/:class:/:file: role will fail the gate (#586 hit this), and :rfc: is only matched in its bare-number form — ``:rfc:6554#section-3``` fails because the regex at util/changelog_md.py:314` is digit-only (#590 hit this, now tracked as #592). Also note the error message's line numbers are counted against the converted entry body rather than the file it names, so they will not locate anything — that is #588.

The verdict will need re-pointing after the rebase

The ✅ is pinned to 5e5a357db. A rebase changes the head, so the verdict will no longer point at what would be merged. Since the conflict touches no source file, re-pointing should be a confirmation that the inter-head diff is changelog-only rather than a fresh pass.

What merging this decides, for the record

This PR declines the registration #548 asked for. RFC 3931 §4.1.1 assigns IP protocol 115 to L2TPv3 over IP, so 115 is a missing class rather than a missing registration and stays unbound; the fix is a version guard that degrades a non-2 Ver nibble to Raw through the existing beholder path with the octets preserved byte-for-byte. #548's premise was wrong, the correction is recorded on that issue, and #570's documented worked example at pcapkit/foundation/registry/protocols.py:262 — which shipped the bad binding — is corrected here.

Disclosed and deliberately not fixed, each worth its own issue rather than widening this diff: L2TPv2.make() still accepts version≠2, so read and make remain asymmetric (verified in review: make(version=9, …) packs \x09 with no validation); InARP/DRARP are reachable through no dispatch of their own; six abstract bases return None from __index__ against a -> StdlibEnum | AenumEnum annotation; and isort flags registry/protocols.py identically at main, so that one is pre-existing rather than introduced here.

One count to correct while it is in view: the report put the remaining GH-nnn references at 21. Measured just now, it is 20 occurrences across 9 files — still against the house #nnn convention, still not this PR's to fix.