diff --git a/pcapkit/dumpkit/common.py b/pcapkit/dumpkit/common.py index cd83f1a53..c930c4bb4 100644 --- a/pcapkit/dumpkit/common.py +++ b/pcapkit/dumpkit/common.py @@ -247,6 +247,47 @@ def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]': # concrete writer this applies to today. escape_strings = issubclass(output, dictdumper.plist.PLIST) + def escape_key(key: 'Any') -> 'Any': + """Escape a mapping key on its way to the writer. + + Args: + key: Mapping key, as the writer would interpolate it. + + Returns: + The key unchanged where ``output`` needs no escaping, otherwise the + escaped text of the writer's own rendering of it. + + Note: + :meth:`~dictdumper.plist.PLIST._append_dict` writes a key straight + into ``'{item}'`` and calls + :meth:`~dictdumper.dumper.Dumper._encode_value` on the *value* two + lines later, never on the key -- so :meth:`DictDumper.object_hook` + is handed every value the writer will interpolate but no key at all, + and cannot escape one on the way out the way it does a value. Each + branch below that builds a mapping therefore escapes its own keys + through here. + + A non-:class:`str` key is rendered with :func:`format`, which is the + very conversion ``'{item}'.format(item=key)`` already applies to it, + so the ```` text is what it always was apart from the escaping. + Rendering such a key rather than passing it over is deliberate: + :file:`examples/captures/test.pcapng` keys the TLS key log entries + of its decryption secrets block by a raw :class:`bytes` client + random (:meth:`TLSKeyLog.post_process + `), and + the ``bytes`` repr of that one carries ``&``, ``<`` *and* ``>``. That + is what made the fixture's ``plist`` report unparseable: + :func:`xml.etree.ElementTree.parse` stopped at the key's ``&`` on + line 1517 of 1958. The same key also breaks the fixture's ``json`` + report, but on the quotes in that repr rather than on these three + characters, so that half is :mod:`dictdumper`'s to fix and is left + exactly as it is. + + """ + if not escape_strings: + return key + return xml.sax.saxutils.escape(format(key, '')) + class DictDumper(output): """Customised :class:`~dictdumper.dumper.Dumper` object.""" @@ -266,11 +307,13 @@ def object_hook(self, o: 'Any') -> 'Any': deeply nested, so escaping the :class:`str` result here on the way out reaches every string the writer will ever interpolate raw, not merely the ones built directly in this method. The - one exception is a :class:`~pcapkit.corekit.multidict.MultiDict` - key: :meth:`~dictdumper.dumper.Dumper._append_dict` writes a - dict's keys straight from the mapping without ever calling - this method on them, so a key built from :func:`render_enum` - is escaped inline, right where it is built, instead. + one exception is a mapping *key*: + :meth:`~dictdumper.dumper.Dumper._append_dict` writes those + straight from the mapping without ever calling this method on + them, so both branches that hand the writer a mapping -- a + :class:`~pcapkit.corekit.multidict.MultiDict` and a plain + :class:`dict` -- escape their own keys through + :func:`escape_key` instead. """ if isinstance(o, decimal.Decimal): @@ -286,12 +329,16 @@ def object_hook(self, o: 'Any') -> 'Any': for key, val in o.items(multi=True): if isinstance(key, (enum.Enum, aenum.Enum)): key = render_enum(key) - if escape_strings: - key = xml.sax.saxutils.escape(key) - temp[key].append(val) + temp[escape_key(key)].append(val) result = temp elif isinstance(o, dict): - result = o + # NOTE: rebuilt only where the keys need escaping, so every other + # output is still handed the caller's own mapping rather than a + # copy of it. + if escape_strings: + result = {escape_key(key): val for key, val in o.items()} + else: + result = o elif isinstance(o, (enum.Enum, aenum.Enum)): addon = {key: val for key, val in o.__dict__.items() if not key.startswith('_')} if addon: diff --git a/tests/dumpkit/test_common_unit.py b/tests/dumpkit/test_common_unit.py index 0af1f9578..1d5ebdfcd 100644 --- a/tests/dumpkit/test_common_unit.py +++ b/tests/dumpkit/test_common_unit.py @@ -42,6 +42,23 @@ def encode(encoded_value, file): return encode +class IdentityDumper: + """A stub whose ``object_hook`` is the identity for anything it is handed. + + Unlike :class:`BaseDumper` -- used to prove the *fallback* is reached at all + -- this is what :meth:`dictdumper.dumper.Dumper.object_hook` itself does, so + it is what a non-PLIST format such as ``json``, ``tree`` or ``text`` + actually sees. + + """ + + def object_hook(self, value: 'object') -> 'object': + return value + + def _encode_func(self, value: 'object') -> 'object': + raise NotImplementedError + + class SlotObject: __slots__ = ('name',) @@ -49,6 +66,36 @@ def __init__(self) -> None: self.name = 'slot' +#: The raw :class:`bytes` client random :file:`examples/captures/test.pcapng` +#: keys a TLS key log entry by, and the text the PLIST writer interpolates for +#: it. Every character from ``0x20`` to ``0x3F``, so the ``bytes`` repr carries +#: ``&``, ``<`` and ``>`` -- and a quote of each kind, which nothing escapes +#: because neither is special inside an XML text node. +BYTES_KEY = bytes(range(0x20, 0x40)) +RAW_BYTES_KEY = """b' !"#$%&\\'()*+,-./0123456789:;<=>?'""" +ESCAPED_BYTES_KEY = """b' !"#$%&\\'()*+,-./0123456789:;<=>?'""" + + +def plist_like_dumper() -> 'object': + """A dumper whose ``output`` shares PLIST's class identity, not its file I/O. + + :func:`~pcapkit.dumpkit.common.make_dumper` decides whether to escape from + ``issubclass(output, dictdumper.plist.PLIST)``, so the stub has to inherit + the real writer -- but nothing here writes a report, and PLIST's + ``__init__`` opens a file. + + """ + import dictdumper.plist + + from pcapkit.dumpkit.common import make_dumper + + class PlistLikeDumper(dictdumper.plist.PLIST): + def __init__(self, *args: 'object', **kwargs: 'object') -> None: # pylint: disable=super-init-not-called + pass + + return make_dumper(PlistLikeDumper)('unused') + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class DumpkitCommonTests(unittest.TestCase): def setUp(self) -> None: @@ -190,18 +237,10 @@ def test_make_dumper_escapes_strings_only_for_plist_like_output(self) -> None: passed back through this hook the way a value is. """ - import dictdumper.plist - from pcapkit.corekit.multidict import MultiDict from pcapkit.dumpkit.common import make_dumper - class PlistLikeDumper(dictdumper.plist.PLIST): - """Shares PLIST's class identity without its file I/O.""" - - def __init__(self, *args: 'object', **kwargs: 'object') -> None: # pylint: disable=super-init-not-called - pass - - plist_dumper = make_dumper(PlistLikeDumper)('unused') + plist_dumper = plist_like_dumper() self.assertEqual(plist_dumper.object_hook('a & b '), 'a & b <c>') unknown = enum.IntEnum('', {'': 1}) @@ -214,22 +253,91 @@ def __init__(self, *args: 'object', **kwargs: 'object') -> None: # pylint: disa converted = plist_dumper.object_hook(multidict) self.assertEqual(converted['<unknown>::<unassigned> [1]'], ['value']) - # NOTE: unlike BaseDumper above (used elsewhere in this class to prove - # the *fallback* is reached at all), this stub's object_hook is the - # identity for anything it does not itself convert -- exactly what - # dictdumper.dumper.Dumper.object_hook does -- so it is what a - # non-PLIST format such as json/tree/text actually sees. - class IdentityDumper: - def object_hook(self, value: 'object') -> 'object': - return value - - def _encode_func(self, value: 'object') -> 'object': - raise NotImplementedError - plain_dumper = make_dumper(IdentityDumper)() self.assertEqual(plain_dumper.object_hook('a & b '), 'a & b ') self.assertEqual(plain_dumper.object_hook(member), ':: [1]') + def test_make_dumper_escapes_mapping_keys_for_plist_like_output(self) -> None: + """GitHub issue #772, the half the hook above cannot reach. + + ``dictdumper/plist.py:202`` writes a key as + ``'{item}'.format(item=item)`` and calls ``_encode_value`` on + the *value* two lines down, never on the key -- so + :meth:`object_hook` is handed every value the writer interpolates and no + key at all, and the escaping proven above cannot reach one. Both + branches that build a mapping therefore escape their own keys: + :class:`~pcapkit.corekit.multidict.MultiDict`, where only an + enum-derived key was escaped before, and a plain :class:`dict`, where + nothing was. + + The :class:`bytes` key is :file:`examples/captures/test.pcapng`'s own, + and it is the reason that fixture's ``plist`` report did not parse: + ``&``, ``<`` and ``>`` all inside one key. It is rendered with + :func:`format`, i.e. the conversion the writer's interpolation already + applies to it, so the ```` text is unchanged apart from the + escaping -- which is what the :data:`RAW_BYTES_KEY` assertion pins, + since an expected escaping is only as good as the rendering it is + derived from. + + """ + from pcapkit.corekit.multidict import MultiDict, OrderedMultiDict + + plist_dumper = plist_like_dumper() + + # A plain dict: str keys, and the bytes key of the fixture. + self.assertEqual(format(BYTES_KEY, ''), RAW_BYTES_KEY) + converted = plist_dumper.object_hook({'a & b ': 'value', BYTES_KEY: 'secret'}) + self.assertEqual(list(converted), ['a & b <c>', ESCAPED_BYTES_KEY]) + self.assertEqual(converted['a & b <c>'], 'value') + self.assertEqual(converted[ESCAPED_BYTES_KEY], 'secret') + + # A MultiDict: a plain-str key was interpolated raw until now, and the + # bytes key of the fixture arrives through here rather than through the + # dict branch -- its TLS key log entries are an OrderedMultiDict. + multidict = OrderedMultiDict() + multidict.add('a & b ', 'value') + multidict.add(BYTES_KEY, 'secret') + converted = plist_dumper.object_hook(multidict) + self.assertEqual(list(converted), ['a & b <c>', ESCAPED_BYTES_KEY]) + self.assertEqual(converted[ESCAPED_BYTES_KEY], ['secret']) + + # An enum-derived key is still escaped exactly once: the escaping moved + # out of this branch and into one shared helper, so escaping it twice is + # the shape of mistake that move could have made. + unknown = enum.IntEnum('', {'': 1}) + member = getattr(unknown, '') + multidict = MultiDict() + multidict.add(member, 'value') + converted = plist_dumper.object_hook(multidict) + self.assertEqual(list(converted), ['<unknown>::<unassigned> [1]']) + self.assertNotIn('&', ''.join(converted)) + + def test_make_dumper_hands_other_output_the_mapping_it_was_given(self) -> None: + """``json``, ``tree`` and ``text`` take all three characters literally. + + So no key is escaped for them, and -- the stronger statement, and the + one that keeps this branch free of any risk of double-escaping -- a + plain :class:`dict` is not even rebuilt: the writer is handed the + caller's own mapping, with the caller's own key objects in it, exactly + as it was before #772. A :class:`bytes` key stays :class:`bytes` there, + which is what ``dictdumper``'s ``json`` writer then breaks on, for + reasons of its own. + + """ + from pcapkit.corekit.multidict import OrderedMultiDict + from pcapkit.dumpkit.common import make_dumper + + plain_dumper = make_dumper(IdentityDumper)() + + mapping = {'a & b ': 'value', BYTES_KEY: 'secret'} + self.assertIs(plain_dumper.object_hook(mapping), mapping) + + multidict = OrderedMultiDict() + multidict.add('a & b ', 'value') + multidict.add(BYTES_KEY, 'secret') + converted = plain_dumper.object_hook(multidict) + self.assertEqual(list(converted), ['a & b ', BYTES_KEY]) + def test_an_unassigned_port_dumps_its_addon_keys_in_the_declared_order(self) -> None: """GitHub issue #575's fallback must not reorder what it renders. diff --git a/tests/dumpkit/test_plist_escaping_regression.py b/tests/dumpkit/test_plist_escaping_regression.py new file mode 100644 index 000000000..772c0b429 --- /dev/null +++ b/tests/dumpkit/test_plist_escaping_regression.py @@ -0,0 +1,147 @@ +# -*- coding: utf-8 -*- +"""Reports of a capture whose keys carry XML special characters. + +:file:`tests/dumpkit/test_common_unit.py` pins what +:meth:`~pcapkit.dumpkit.common.make_dumper.DictDumper.object_hook` returns. This +module pins the only thing that actually matters about it -- that the file on +disk parses -- against the fixture that used to prove it did not. + +:file:`examples/captures/test.pcapng` carries a decryption secrets block whose +TLS key log entries are keyed by a raw :class:`bytes` client random +(:meth:`~pcapkit.protocols.schema.misc.pcapng.TLSKeyLog.post_process`), so the +report's key is a ``bytes`` repr holding ``&``, ``<`` and ``>``. GitHub issue +#772 and JarryShaw/DictDumper#125 are the two halves of that: ``dictdumper`` +interpolates a mapping key into its markup without escaping it, and until the +in-repo half landed the ``plist`` and ``xml`` reports of this fixture stopped +parsing at line 1517 of 1958 with ``not well-formed (invalid token)``. + +This module is fixture-tier by its ``_regression.py`` name -- ``test.pcapng`` is +generated rather than committed, so a unit-tier module may not read it (see +:mod:`tests._tiers`). + +""" +from __future__ import annotations + +import importlib.util +import pathlib +import tempfile +import unittest +import xml.etree.ElementTree as ET + +from tests._support import close_extractor, purge_modules, sample_path + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: The fixture's client random as the writer spells it, i.e. the ``bytes`` repr +#: of every character from ``0x20`` to ``0x3F``. +RAW_BYTES_KEY = """b' !"#$%&\\'()*+,-./0123456789:;<=>?'""" +#: The same key with the three XML entities escaped, and nothing else touched -- +#: both quotes stay as they are, being legal in an XML text node. +ESCAPED_BYTES_KEY = """b' !"#$%&\\'()*+,-./0123456789:;<=>?'""" +#: The first of the three undeclared PCAP-NG option types this fixture carries, +#: as :func:`~pcapkit.dumpkit.common.render_enum` spells it and the writer has to +#: escape it. It arrives as a mapping *key*, which is what makes it the +#: double-escape witness: ``&lt;`` here would mean the escaping ran twice. +ESCAPED_PSEUDO_MEMBER_KEY = 'OptionType::<unassigned> [opt_unknown [2]]' + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PlistKeyEscapingTests(unittest.TestCase): + """The reports of :file:`test.pcapng`, one per output format.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-772-') + self.addCleanup(tmpdir.cleanup) + self.tmp_path = pathlib.Path(tmpdir.name) + + def report(self, capture: 'str', fmt: 'str') -> 'pathlib.Path': + """Extract ``capture`` into a report of ``fmt`` and return its path.""" + from pcapkit.interface import extract + + output = self.tmp_path / f'{capture}.{fmt}' + extractor = extract(fin=sample_path(capture), fout=str(output), format=fmt, + store=False, extension=False) + self.addCleanup(close_extractor, extractor) + + self.assertEqual(extractor.length, 5) + return output + + def test_the_plist_report_of_a_bytes_keyed_mapping_parses(self) -> None: + """The regression itself: the document has to be well-formed XML. + + :func:`plistlib.load` cannot stand in for the parser here -- + ``dictdumper`` writes a ```` with fractional seconds, which it + rejects for reasons that have nothing to do with #772 (see + ``PlistRoundTripTests`` in + :file:`tests/integration/test_output_formats.py`) -- so this asserts + well-formedness with :mod:`xml.etree.ElementTree` and then reads the key + back out of the tree rather than out of the text, which is the part a + string comparison would not have caught. + + """ + report = self.report('test.pcapng', 'plist') + + keys = [element.text for element in ET.parse(report).iter('key')] + self.assertIn(ESCAPED_BYTES_KEY, report.read_text(encoding='utf-8')) + # ElementTree resolves the entities, so the key comes back as the octets + # the fixture holds: escaping is a transport detail, not a rename. + self.assertIn(RAW_BYTES_KEY, keys) + + def test_the_xml_report_is_the_same_writer_and_parses_too(self) -> None: + """``'xml'`` and ``'plist'`` both map to ``dictdumper.PLIST``. + + ``Extractor.__output__`` gives the two formats the same writer, so + neither can be fixed without the other -- but that is a mapping a future + change could redirect, and the format a user asked for by name is the one + whose report has to parse. + + """ + report = self.report('test.pcapng', 'xml') + + ET.parse(report) + self.assertIn(ESCAPED_BYTES_KEY, report.read_text(encoding='utf-8')) + + def test_the_json_and_tree_reports_keep_the_key_verbatim(self) -> None: + """Nothing is escaped for the formats that take the characters literally. + + ``json``, ``tree`` and ``text`` accept ``&``, ``<`` and ``>`` as + themselves, so escaping them there would corrupt every report in those + formats. The ``json`` report of this fixture is separately unparseable -- + ``dictdumper`` writes a key as ``'"{item}": '`` and this one's repr holds + a quote -- and that is deliberately still true here: it is + JarryShaw/DictDumper#125's half, not this one's. + + """ + for fmt in ('json', 'tree', 'text'): + with self.subTest(format=fmt): + text = self.report('test.pcapng', fmt).read_text(encoding='utf-8') + + self.assertIn(RAW_BYTES_KEY, text) + self.assertNotIn('&', text) + self.assertNotIn('<', text) + + def test_the_plist_report_escapes_a_pseudo_member_key_exactly_once(self) -> None: + """An already-escaped rendering must not be escaped a second time. + + This fixture carries three undeclared PCAP-NG option types, each of which + reaches the writer as a mapping key rendered + ``OptionType:: [opt_unknown [N]]`` -- #771's case, and now the + same escaping as every other key rather than a special case beside it. + ``&lt;`` anywhere in the report is what a second pass over an + already-escaped key would leave behind, so it is what this looks for. + + """ + text = self.report('test.pcapng', 'plist').read_text(encoding='utf-8') + + self.assertIn(ESCAPED_PSEUDO_MEMBER_KEY, text) + self.assertNotIn('&lt;', text) + self.assertNotIn('&gt;', text) + self.assertNotIn('&amp;', text) + # Nothing was left raw either, which is the other way to pass the line above. + self.assertNotIn('', text) + + +if __name__ == '__main__': + unittest.main()