Skip to content

fix(dumpkit): escape mapping keys for the plist writer (#772) - #784

Merged
JarryShaw merged 1 commit into
mainfrom
fix-772-plist-dict-keys
Sep 25, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix-772-plist-dict-keys

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

The in-repo half of #772. #771 escaped every dumped value for the plist writer, but
_append_dict interpolates a key into '<key>{item}</key>' and only calls _encode_value on
the value, so object_hook never sees a key. Both branches that build a mapping now escape their
own keys through one escape_key helper — a MultiDict, where #771 escaped an enum-derived key
inline and nothing else, and a plain dict, where nothing was escaped.

Non-str keys are rendered with format, not skipped, and not str-ed arbitrarily. format(key, '')
is the conversion '{item}'.format(item=key) already applies, so the <key> text is unchanged
apart from the escaping. It has to be handled: test.pcapng's decryption secrets block keys its TLS
key log entries by a raw bytes client random whose repr carries &, < and > — and that key
reaches the writer through an OrderedMultiDict, so fixing only the plain-dict branch would not
have fixed the fixture.

test.pcapng before: ET.parse fails, not well-formed (invalid token): line 1517, column 19.
After: parses. Measured over 6 captures × 5 formats on fe80b8525: 2 of the 30 reports changed
(that capture's plist and xml, which are one writer under two names), by exactly one line;
the other 28 — every json, tree and text report included — are byte-identical. Nothing
double-escapes: no &amp;lt; anywhere.

pcapkit/dumpkit/common.py stays at 100% coverage over tests/dumpkit/ (84 → 88 statements,
38 → 40 branches, no new misses). The upstream half is JarryShaw/DictDumper#125, which would make
this workaround redundant; this does not wait on it.

Closes #772 — the pcapkit-side half. The upstream _encode_value-on-keys change stays
tracked at JarryShaw/DictDumper#125, in that repo, and is not a hold on this board.

- common.py: `object_hook` is never handed a mapping key -- dictdumper's
  `_append_dict` interpolates it into `'<key>{item}</key>'` and calls
  `_encode_value` on the value only -- so both branches that build a
  mapping now escape their own keys through one `escape_key` helper: a
  MultiDict, where #771 escaped an enum-derived key inline and nothing
  else, and a plain dict, where nothing was escaped at all.
- A non-str key is rendered with `format`, i.e. the writer's own
  interpolation, so the `<key>` text is unchanged apart from the
  escaping. examples/captures/test.pcapng is why that case is handled at
  all: its decryption secrets block keys the TLS key log entries by a raw
  bytes client random whose repr carries `&`, `<` and `>`.
- json, tree and text are untouched -- `escape_key` is a no-op for them
  and a plain dict is not even rebuilt, so the writer still gets the
  caller's own mapping with the caller's own key objects in it.
- tests/dumpkit/test_plist_escaping_regression.py is new, fixture-tier
  because test.pcapng is generated rather than committed. It pins that
  the plist and xml reports parse, that json/tree/text keep the key
  verbatim, and that nothing is escaped twice.

Fixes #772. The upstream half is JarryShaw/DictDumper#125, where
`_append_dict` should call `_encode_value` on a key; this does not wait
on it.

Measured on fe80b85 across 6 captures x 5 formats: `ET.parse` of
test.pcapng's plist failed at line 1517 of 1958 before and parses after;
exactly 2 of the 30 reports changed -- that capture's plist and xml,
which are one writer under two names -- and by exactly one line, leaving
the other 28 byte-identical. pcapkit/dumpkit/common.py stays at 100%
coverage over tests/dumpkit/, 84 -> 88 statements and 38 -> 40 branches,
no new misses. make pylint unchanged (6 pre-existing messages), make mypy
clean.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review at 9bec70e22 (sonnet, a different model from the author): GOOD TO GO, no required changes. Every number in the body reproduced independently, and the merge was checked rather than assumed.

The corrected diagnosis is confirmed at source. dictdumper/plist.py writes '{tabs}<key>{item}</key>\n'.format(tabs=tabs, item=item) then file.write(keys), calling _encode_value only on the value — never the key. And the OrderedMultiDict key really is bytes.fromhex(random) from TLSKeyLog.post_process (pcapkit/protocols/schema/misc/pcapng.py:1975-1985), so it is raw bytes reaching the MultiDict branch, not a plain-dict key. That is the thing my original #772 comment got wrong and you corrected.

The format() over str() choice is right, not over-careful — and the reviewer proved it rather than agreeing with it. Probed a key whose __format__ differs from __str__: format(k, '') equals the writer's own '{item}'.format(item=k) while str(k) diverges. Also checked an int key and a str-subclassing Enum key, both matching. So the equivalence the design rests on holds across the key types that can actually occur.

2 of 30, re-derived from its own fixtures. It generated captures itself and dumped 6 pcapng captures × 5 formats on fe80b8525 versus the head: exactly 2 changed (test.pcapng.plist and .xml), each by exactly one line (1517c1517), the other 28 byte-identical, and zero &amp;amp;lt; / &amp;amp;gt; / &amp;amp;amp; anywhere. ET.parse on the base fails at not well-formed (invalid token): line 1517, column 19, in a report of 1958 lines. Its capture set was chosen independently and still reproduces your counts exactly.

Double-escaping ruled out structurally, not just by test: the old inline escape(key) at the enum branch is gone from the diff, replaced by the single escape_key() that gates on escape_strings itself. Identity preservation confirmed — assertIs holds and nothing rebuilds on the non-escape path. Coverage matched at 84→88 statements and 38→40 branches, 100% with zero misses, cross-checked under plain unittest at 14 + 4.

Merge into current main (0419c1c97) done by the reviewer itself: clean, and on the merge result tests/dumpkit/ gives 24 passed / 71 subtests with the same 88/40 coverage, plus tests/test_tier_guard.py and tests/project/test_isort_clean.py green to rule out interaction with #782's tier-guard changes.

Two incidental findings, neither blocking. The stale skip reason at tests/integration/test_pcapng_end_to_end.py:185 is confirmed — and it is internally inconsistent even before this PR, citing line 1376 where the measured failure is 1517. Its plist half dies with this change; its json half survives. Correctly left untouched, and I am filing it separately. Second: a pre-existing isort ordering quirk in tests/dumpkit/test_common_unit.py's import block, identical on the base. That is not a gap in the isort test — the Makefile isort: target covers pcapkit, pcapkit/const, pcapkit/vendor, util/*.py and examples/generators/*.py, and tests/ is deliberately outside its scope. Nothing to chase.

CI fully green: 27 success, 3 expected skips.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw
JarryShaw merged commit c6eede3 into main Sep 25, 2026
31 checks passed
@JarryShaw
JarryShaw deleted the fix-772-plist-dict-keys branch September 25, 2026 14:23
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 25, 2026
JarryShaw added a commit that referenced this pull request Sep 25, 2026
#784 taught pcapkit/dumpkit/common.py's escape_key to XML-escape mapping
keys, so the plist half of this class's docstring is no longer true: the
plist report of test.pcapng now parses as well-formed XML. Only the json
writer's quoting defect (dictdumper/json.py:224, unescaped quotes in a
bytes-repr key) still stands, still open upstream as
JarryShaw/DictDumper#125.

- Class docstring: drop the claim that both json and plist reports come
  out unparseable; note that plist parses since #784.
- Method docstring: replace the stale "line 915"/"line 1376" numbers
  with what this run actually measured on current main -- json fails at
  line 1019 column 12, and plist no longer fails at all -- and flag the
  json number as fixture-dependent rather than a fixed fact, since a
  drifted instance of exactly this number is what #785 was filed over.

Measured on ef859f7: json.load raises "Expecting ':' delimiter: line
1019 column 12" on the regenerated test.pcapng; xml.etree.ElementTree.parse
now succeeds on the same fixture's plist report. Verified 1517/1958 was the
correct pre-#784 plist failure line (0419c1c, matching #785's own
measurement), so 1376 had already drifted before #784 landed. The skip
reason itself named no false claim and needed no change. Build/tests: 6
passed, 1 skipped (unchanged) in tests/integration/test_pcapng_end_to_end.py.
JarryShaw added a commit that referenced this pull request Sep 25, 2026
#784 taught pcapkit/dumpkit/common.py's escape_key to XML-escape mapping
keys, so the plist half of this class's docstring is no longer true: the
plist report of test.pcapng now parses as well-formed XML. Only the json
writer's quoting defect (dictdumper/json.py:224, unescaped quotes in a
bytes-repr key) still stands, still open upstream as
JarryShaw/DictDumper#121.

- Class docstring: drop the claim that both json and plist reports come
  out unparseable; note that plist parses since #784.
- Method docstring: replace the stale "line 915"/"line 1376" numbers
  with what this run actually measured on current main -- json fails at
  line 1019 column 12, and plist no longer fails at all -- and flag the
  json number as fixture-dependent rather than a fixed fact, since a
  drifted instance of exactly this number is what #785 was filed over.

Measured on ef859f7: json.load raises "Expecting ':' delimiter: line
1019 column 12" on the regenerated test.pcapng; xml.etree.ElementTree.parse
now succeeds on the same fixture's plist report. Verified 1517/1958 was the
correct pre-#784 plist failure line (0419c1c, matching #785's own
measurement), so 1376 had already drifted before #784 landed. The skip
reason itself named no false claim and needed no change. Build/tests: 6
passed, 1 skipped (unchanged) in tests/integration/test_pcapng_end_to_end.py.
JarryShaw added a commit that referenced this pull request Sep 25, 2026
#784 added an escape_key helper to pcapkit/dumpkit/common.py that XML-escapes
mapping keys, so the plist half of this class's docstring is no longer true: the
plist report of test.pcapng now parses as well-formed XML. Only the json
writer's quoting defect (dictdumper/json.py:224, unescaped quotes in a
bytes-repr key) still stands, still open upstream as
JarryShaw/DictDumper#121.

- Class docstring: drop the claim that both json and plist reports come
  out unparseable; note that plist parses since #784.
- Method docstring: replace the stale "line 915"/"line 1376" numbers
  with what this run actually measured on current main -- json fails at
  line 1019 column 12, and plist no longer fails at all -- and flag the
  json number as fixture-dependent rather than a fixed fact, since a
  drifted instance of exactly this number is what #785 was filed over.

Measured on ef859f7: json.load raises "Expecting ':' delimiter: line
1019 column 12" on the regenerated test.pcapng; xml.etree.ElementTree.parse
now succeeds on the same fixture's plist report. Verified 1517/1958 was the
correct pre-#784 plist failure line (0419c1c, matching #785's own
measurement), so 1376 had already drifted before #784 landed. The skip
reason itself named no false claim and needed no change. Build/tests: 6
passed, 1 skipped (unchanged) in tests/integration/test_pcapng_end_to_end.py.
JarryShaw added a commit that referenced this pull request Sep 25, 2026
#784 added an escape_key helper to pcapkit/dumpkit/common.py that XML-escapes
mapping keys, so the plist half of this class's docstring is no longer true: the
plist report of test.pcapng now parses as well-formed XML. Only the json
writer's quoting defect (dictdumper/json.py:224, unescaped quotes in a
bytes-repr key) still stands, still open upstream as
JarryShaw/DictDumper#121.

- Class docstring: drop the claim that both json and plist reports come
  out unparseable; note that plist parses since #784.
- Method docstring: replace the stale "line 915"/"line 1376" numbers
  with what this run actually measured on current main -- json fails at
  line 1019 column 12, and plist no longer fails at all -- and flag the
  json number as fixture-dependent rather than a fixed fact, since a
  drifted instance of exactly this number is what #785 was filed over.

Measured on ef859f7: json.load raises "Expecting ':' delimiter: line
1019 column 12" on the regenerated test.pcapng; xml.etree.ElementTree.parse
now succeeds on the same fixture's plist report. Verified 1517/1958 was the
correct pre-#784 plist failure line (0419c1c, matching #785's own
measurement), so 1376 had already drifted before #784 landed. The skip
reason itself named no false claim and needed no change. Build/tests: 6
passed, 1 skipped (unchanged) in tests/integration/test_pcapng_end_to_end.py.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

dictdumper PLIST writer does not escape angle brackets, so a pseudo-member makes the dump unparseable

1 participant