Skip to content

fix(ipx,ah): IPX from_data raises struct.error; AH reserved field written back as zero - #1338

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1315-1327-ipx-ah-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1315-1327-ipx-ah-roundtrip

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests

  • Followed the coding style — ran isort and pylint on the touched files (no new findings beyond existing too-many-arguments/redefined-builtin); mypy reports no errors in them. Did not run the make targets.

  • make test passes, and a test case covers the change — ran only the modules listed below

  • Added a changelog entry — N/A, added centrally

  • fix — corrects a defect


Closes #1315
Closes #1327

  • IPX: _make_data now packs the parsed Address back into 12 octets (network, node, socket); make gains total_length=None and writes a given value as is, so the parsed len survives.
  • AH: reserved is a UInt16Field, carried as AH.reserved in the data model and make(reserved=0) (RFC 4302 §2.3).
  • Breaking: new required data-class field Data_AH.reserved; new IPX.make(total_length=) keyword.
  • test_ipx_unit.py / test_ah_unit.py: _make_data tests pinned the old pass-through / missing key; updated.

Probe (from_data(parsed.info).data): before, IPX 0000001e+26×00 → struct.error; AH 0604dead… → 06040000…. After: both byte-exact.

Tests: new test_ipx_ah_roundtrip_unit.py 5 passed (7 subtests); without the fix 7 failed; with only the address half, the two length subtests fail. test_ipx_unit 3 passed, test_ah_unit 4 passed, test_option_roundtrip_unit 6 passed, tests/project 389 passed 1 skipped; run_unittest_leg.py protocols/internet 508 tests, 0 failures.

…tten back as zero

- IPX: rebuild the 12 address octets from the parsed network, node and
  socket values; `make` takes `total_length=None` and writes a given
  value as is, so `from_data` keeps the parsed packet length.
- AH: the reserved field is a UInt16Field, carried as `AH.reserved` in
  the data model and as `make(reserved=0)` (RFC 4302 section 2.3).
- Update the IPX and AH `_make_data` unit tests, which pinned the old
  pass-through and the missing reserved key.

Closes #1315
Closes #1327
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on b185ad43a: GOOD TO GO (Sonnet reviewed; the author ran on Opus).

  • Repros: on main, IPX from_data raises struct.error for every packet tried, and AH 0604dead… rebuilds as 06040000…. Here both rebuild byte-exactly across all-zero, len=0, len=0xffff, len above the data and len below 30, and the AH dead/zero/ffff cases.
  • No transposition: an asymmetric header (dst 0a0b0c0d/010203040506/4004, src deadbeef/112233445566/abcd) rebuilds exactly and parses into the right fields.
  • RFC 4302 §2.3, fetched: 16 bits, MUST be zero on send, SHOULD be ignored on receipt, and included in the ICV. So UInt16Field at offset 2 is right.
  • Blast radius: Data_AH( is only built in ah.py; a hand-built Schema_AH without reserved still works (defaults to 0). No caller passes total_length outside the PR. Both edited tests pinned wrong behaviour — the old IPX test fed the string 'dst-node' where 12 octets belong.
  • Tests: the new module gives 7 failures on main. protocols/internet leg 508 tests, 0 failures. No fixture contains IPX or AH, so coverage rests on the unit tests and probes.

Nits, none blocking: the docstring says only "zero on send"; it should also say the field is ICV-covered, which is the real reason to carry it through. An out-of-range reserved is silently masked (-1 → ffff). And the SchemaWarning: packet length < 0 predates this PR but comes from IPX payload decoding in _parse_next_layer — bare AH headers do not emit it, contrary to the PR body.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.91% (unit tier, Python 3.14, b185ad43a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1033 2342 844 90.25%
pcapkit/corekit 1878 91 580 22 94.34%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15782 190 4038 165 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit e1f5adb into main Oct 7, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1315-1327-ipx-ah-roundtrip branch October 7, 2026 20:10
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(ah): the reserved field is written back as zero fix(ipx): from_data always raises struct.error

1 participant