Skip to content

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

Closed

Description

@JarryShaw

Describe the bug

dictdumper's PLIST writer does not escape XML special characters in string values, so any dumped
value containing < or > produces malformed XML. pcapkit's own nameless pseudo-member renders as
<unknown>::<unassigned> [N] (pcapkit/dumpkit/common.py:168, render_enum), so a plist dump
containing one is unparseable.

Reproduction — three lines, no pcapkit involved:

import dictdumper, tempfile, os, xml.etree.ElementTree as ET
p = os.path.join(tempfile.mkdtemp(), 't.plist')
dictdumper.PLIST(p)({'val': '<unknown>::<unassigned> [54321]'}, name='frame1')
ET.parse(p)
plist contains RAW '<unknown>': True
plist contains escaped '&lt;unknown&gt;': False
parses as XML: False -> mismatched tag: line 8, column 43
   <string><unknown>::<unassigned> [54321]</string>

Expected behavior

< and > escaped as &lt;/&gt; inside <string>, so the document parses.

Additional context

This is live on main today, not new. The pseudo-member has existed since #701 and
EnumField.post_process's docstring already names <unknown>::<unassigned> [28] as its rendered
form, so any capture carrying a genuinely unassigned option code already produces an unparseable
plist. It has gone unnoticed only because no fixture in the corpus hits that path, so
tests/integration/test_output_formats.py::PlistReportTests::test_plist_report_is_well_formed_xml_with_one_key_per_frame
has never seen one.

#771 makes it reachable on essentially every capture, because it routes unassigned ports through
the same pseudo-member and ephemeral ports are ubiquitous. Four CI legs on that PR fail on this single
test — one defect, not four.

The writer is dictdumper's (dictdumper.PLIST), so the escaping is not ours to fix in-tree. Options,
none chosen: fix upstream and pin a new version; escape per-format in make_dumper's hook rather than
in render_enum, since < is legal in the json/text outputs; or rename the pseudo-member to avoid
angle brackets, which changes an established dump spelling. Blocking #771 — see the decision request
there.

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    dependenciesPull requests that update a dependency file
    on Sep 25, 2026
  2. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 25, 2026
  3. JarryShaw commented on Sep 25, 2026

    @JarryShaw
    OwnerAuthor

    Half of this landed with #771 (merged as 9012f57f8), and the remaining half is narrower than the title suggests. Recording both so the issue is not re-derived from scratch — it had no comments until now.

    What landed. make_dumper in pcapkit/dumpkit/common.py now escapes on the way out, conditioned on the writer: escape_strings = issubclass(output, dictdumper.plist.PLIST), with xml.sax.saxutils.escape applied in object_hook. It is conditioned rather than unconditional because json, tree and text accept &/</> literally and would double-escape. dictdumper.xml.XML defines no _append_string of its own, so PLIST is the only concrete writer affected — and 'xml' maps to PLIST in Extractor.__output__, so the two names are one writer. Measured on that PR: 28 of 46 plist/xml outputs were malformed without the escaping, 2 with it.

    What is left, and exactly why. dictdumper's _append_dict writes keys with '{tabs}<key>{item}</key>\n'.format(...) straight out of value.items() and never calls _encode_value on a key — only on the value, two lines later. So object_hook cannot reach keys at all. #771 worked around that for the case it needed by escaping enum-derived MultiDict keys inline, right where render_enum builds them. But a plain dict still returns untouched through elif isinstance(o, dict): result = o, so a plain dict key — and a plain-str MultiDict key — is still interpolated raw.

    That is why test.pcapng is still malformed XML on fe80b8525: its raw-bytes dict key contains ", #, %, < and >. Pre-existing, unchanged by #771, and not covered by its escaping.

    Two fixes, one here and one upstream. In-repo: escape plain dict keys in that branch the same way the MultiDict branch already does — rebuild the mapping with escaped keys when escape_strings. Upstream: JarryShaw/DictDumper#125 is where _append_dict should call _encode_value on keys, which would make the in-repo workaround redundant. The in-repo half does not wait on the upstream one.

    Dispatching the in-repo half.

  4. JarryShaw commented on Sep 25, 2026

    @JarryShaw
    OwnerAuthor

    Correcting my own comment above. I wrote that the residual was plain dict keys, and that test.pcapng stayed malformed because elif isinstance(o, dict): result = o returns untouched. That diagnosis was wrong, and fixing only the plain-dict branch would not have fixed this capture.

    The raw-bytes key is not a plain dict's key. TLSKeyLog.post_process builds collections.defaultdict(OrderedMultiDict) at pcapkit/protocols/schema/misc/pcapng.py:1975, typed dict[TLSKeyLabel, OrderedMultiDict[bytes, bytes]] — so the client random arrives as a key of the OrderedMultiDict, and goes through that branch. #771 escaped only the enum-derived keys in that branch, which is why a bytes key slipped past. The plain dict one level up carries TLSKeyLabel keys rendering as CLIENT_RANDOM, with no special characters and no change in output.

    So the real gap was narrower and in a different place than I described: not-enum keys inside the MultiDict branch, plus plain-dict keys as a separate completeness case. #784 fixes both.

    Verified at pcapng.py:1975-1985 directly. I am recording the correction rather than editing the earlier comment, so the reasoning that led to #784's scope stays legible.

  5. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 25, 2026
  6. added this to the 1.5 milestone on Oct 6, 2026
  7. moved this to Done in PyPCAPKiton Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)dependenciesPull requests that update a dependency file

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions