fix(dumpkit): JSON output for test.pcapng does not parse

pcapkit/dumpkit/common.py

@@ -15,11 +15,13 @@
 import decimal
 import enum
 import ipaddress
+import json
 import xml.sax.saxutils
 from typing import TYPE_CHECKING
 
 import aenum
 import dictdumper.dumper
+import dictdumper.json
 import dictdumper.plist
 
 from pcapkit.corekit.infoclass import Info
@@ -244,6 +246,12 @@ def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]':
     # use it directly -- so :class:`~dictdumper.plist.PLIST` is the only
     # concrete writer this applies to.
     escape_strings = issubclass(output, dictdumper.plist.PLIST)
+    # NOTE: :class:`~dictdumper.json.JSON` escapes every string *value* it
+    # writes, but interpolates a mapping key into ``'"{item}": '`` raw, so a key
+    # carrying a ``"`` or a ``\`` closes the JSON string early (GitHub issue
+    # #1152). The defect is :mod:`dictdumper`'s, so it is worked around here.
+    escape_json_keys = issubclass(output, dictdumper.json.JSON)
+    escape_keys = escape_strings or escape_json_keys
 
     def escape_key(key: 'Any') -> 'Any':
         """Escape a mapping key on its way to the writer.
@@ -257,33 +265,39 @@ def escape_key(key: 'Any') -> 'Any':
 
         Note:
             :meth:`~dictdumper.plist.PLIST._append_dict` writes a key straight
-            into ``'<key>{item}</key>'`` 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.
+            into ``'<key>{item}</key>'``, and
+            :meth:`~dictdumper.json.JSON._append_object` into ``'"{item}": '``;
+            both call :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 ``<key>`` text is unchanged apart from the escaping.
+            so the key text is unchanged 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
             <pcapkit.protocols.schema.misc.pcapng.TLSKeyLog.post_process>`), and
-            the ``bytes`` repr of that one carries ``&``, ``<`` *and* ``>``.
-            Unescaped, that makes the fixture's ``plist`` report unparseable:
-            :func:`xml.etree.ElementTree.parse` stops at the key's ``&``. 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 alone.
+            the ``bytes`` repr of that one carries ``&``, ``<``, ``>``, ``"``
+            *and* ``\\``. Unescaped, that makes the fixture's ``plist`` report
+            unparseable at the key's ``&``, and its ``json`` report at the
+            key's ``"``.
+
+            For ``json`` the key is escaped by :func:`json.dumps` with the
+            surrounding quotes stripped, since the writer supplies its own. Like
+            :meth:`~dictdumper.json.JSON._append_string`, that spells every
+            non-ASCII character as a ``\\uXXXX`` escape.
 
         """
-        if not escape_strings:
-            return key
-        return xml.sax.saxutils.escape(format(key, ''))
+        if escape_strings:
+            return xml.sax.saxutils.escape(format(key, ''))
+        if escape_json_keys:
+            return json.dumps(format(key, ''))[1:-1]
+        return key
 
     class DictDumper(output):
         """Customised :class:`~dictdumper.dumper.Dumper` object."""
@@ -332,7 +346,7 @@ def object_hook(self, o: 'Any') -> 'Any':
                 # 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:
+                if escape_keys:
                     result = {escape_key(key): val for key, val in o.items()}
                 else:
                     result = o

tests/dumpkit/test_json_key_escaping_regression.py

@@ -0,0 +1,61 @@
+# -*- coding: utf-8 -*-
+"""The JSON report of :file:`test.pcapng` parses (GitHub issue #1152).
+
+The fixture's decryption secrets block keys its TLS key log entries by a raw
+:class:`bytes` client random, whose repr holds both a ``"`` and a ``\\``.
+:class:`dictdumper.json.JSON` wrote that key unescaped, and :func:`json.load`
+stopped at line 1019 with ``Expecting ':' delimiter``.
+
+This module is fixture-tier by its ``_regression.py`` name -- ``test.pcapng`` is
+generated rather than committed (see :mod:`tests._tiers`).
+
+"""
+from __future__ import annotations
+
+import importlib.util
+import json
+import pathlib
+import tempfile
+import unittest
+
+from tests._support import close_extractor, reimport_once_per_class, 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 renders it, i.e. the ``bytes`` repr
+#: of every character from ``0x20`` to ``0x3F``.
+RAW_BYTES_KEY = """b' !"#$%&\\'()*+,-./0123456789:;<=>?'"""
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class TestPcapngJSONReportTests(unittest.TestCase):
+    """The ``json`` report of :file:`test.pcapng`."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+        tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-1152-')
+        self.addCleanup(tmpdir.cleanup)
+        self.tmp_path = pathlib.Path(tmpdir.name)
+
+    def test_the_json_report_parses_and_keeps_the_bytes_key(self) -> None:
+        """The report loads, and the client random reads back unchanged."""
+        from pcapkit.interface import extract
+
+        output = self.tmp_path / 'test.pcapng.json'
+        extractor = extract(fin=sample_path('test.pcapng'), fout=str(output), format='json',
+                            store=False, extension=False)
+        self.addCleanup(close_extractor, extractor)
+
+        with output.open(encoding='utf-8') as file:
+            report = json.load(file)
+
+        entries = [frame['secrets_data']['entries'] for frame in report.values()
+                   if isinstance(frame, dict) and 'secrets_data' in frame]
+        self.assertEqual(len(entries), 1)
+        self.assertIn(RAW_BYTES_KEY, entries[0]['CLIENT_RANDOM'])
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/dumpkit/test_json_key_escaping_unit.py

@@ -0,0 +1,84 @@
+# -*- coding: utf-8 -*-
+"""JSON reports of a mapping whose keys need escaping.
+
+:class:`dictdumper.json.JSON` escapes every string *value* it writes, but
+interpolates a mapping key into ``'"{item}": '`` raw, so a key holding a ``"``
+or a ``\\`` closes the JSON string early and the report stops parsing (GitHub
+issue #1152). :func:`~pcapkit.dumpkit.common.make_dumper` escapes the keys on
+the writer's behalf; this module pins that the file on disk parses and that the
+key reads back as the writer's own rendering of it.
+
+"""
+from __future__ import annotations
+
+import importlib.util
+import json
+import pathlib
+import tempfile
+import unittest
+
+HAS_RUNTIME = importlib.util.find_spec('dictdumper') is not None
+
+#: Keys that each break an unescaped ``'"{item}": '``, or would if escaped wrongly.
+AWKWARD_KEYS = (
+    b' !"#$%&\'()*+,-./0123456789:;<=>?',  # test.pcapng's client random
+    'quote " and backslash \\',
+    'tab\tnewline\ncontrol\x01',
+    'non-ascii é中',
+)
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class JSONKeyEscapingTests(unittest.TestCase):
+    """Dump one mapping per container type and read the report back."""
+
+    def setUp(self) -> None:
+        tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-1152-')
+        self.addCleanup(tmpdir.cleanup)
+        self.tmp_path = pathlib.Path(tmpdir.name)
+
+    def dump(self, fmt: 'str', value: 'object') -> 'pathlib.Path':
+        """Write ``{'entries': value}`` through the customised ``fmt`` writer."""
+        import dictdumper
+
+        from pcapkit.dumpkit.common import make_dumper
+
+        output = self.tmp_path / f'report.{fmt}'
+        writer = make_dumper({'json': dictdumper.JSON, 'tree': dictdumper.Tree}[fmt])
+        writer(str(output))({'entries': value}, name='Frame 1')
+        return output
+
+    def test_a_multidict_with_awkward_keys_parses(self) -> None:
+        """The #1152 path: ``TLSKeyLog`` keys an ``OrderedMultiDict`` by bytes."""
+        from pcapkit.corekit.multidict import OrderedMultiDict
+
+        entries = OrderedMultiDict()  # type: OrderedMultiDict[object, int]
+        for index, key in enumerate(AWKWARD_KEYS):
+            entries.add(key, index)
+
+        report = json.loads(self.dump('json', entries).read_text(encoding='utf-8'))
+
+        self.assertEqual(report['Frame 1']['entries'],
+                         {format(key, ''): [index] for index, key in enumerate(AWKWARD_KEYS)})
+
+    def test_a_plain_dict_with_awkward_keys_parses(self) -> None:
+        """A plain :class:`dict` goes through a separate branch of the hook."""
+        entries = {key: index for index, key in enumerate(AWKWARD_KEYS)}
+
+        report = json.loads(self.dump('json', {'nested': entries}).read_text(encoding='utf-8'))
+
+        self.assertEqual(report['Frame 1']['entries']['nested'],
+                         {format(key, ''): index for index, key in enumerate(AWKWARD_KEYS)})
+
+    def test_the_tree_report_keeps_the_key_verbatim(self) -> None:
+        """``tree`` takes the characters literally, so nothing is escaped there."""
+        key = 'quote " and backslash \\'
+
+        text = self.dump('tree', {key: 0}).read_text(encoding='utf-8')
+
+        self.assertIn(key, text)
+        self.assertNotIn('\\"', text)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/dumpkit/test_plist_escaping_regression.py

@@ -23,6 +23,7 @@
 from __future__ import annotations
 
 import importlib.util
+import json
 import pathlib
 import tempfile
 import unittest
@@ -109,17 +110,17 @@ def test_the_json_and_tree_reports_keep_the_key_verbatim(self) -> None:
 
         ``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.
+        formats. ``json`` does escape the key's ``"`` and ``\\`` as a JSON
+        string must (GitHub issue #1152), so it is checked against the key's JSON
+        spelling rather than its raw text.
 
         """
         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)
+                expected = json.dumps(RAW_BYTES_KEY) if fmt == 'json' else RAW_BYTES_KEY
+                self.assertIn(expected, text)
                 self.assertNotIn('&amp;', text)
                 self.assertNotIn('&lt;', text)