Mark a usage sharing value with invalid characters as escaped, as the specification says

pipeline/fiftyone_pipeline_engines_fiftyone/src/fiftyone_pipeline_engines_fiftyone/share_usage.py

@@ -21,6 +21,7 @@
 # *********************************************************************
 
 import random
+import re
 import json
 import gzip
 import platform
@@ -44,7 +45,7 @@
 
 # The maximum length of a piece of evidence's value which can be
 # added to the usage data being sent.
-SHARE_USAGE_MAX_EVIDENCE_LENGTH = 512
+SHARE_USAGE_MAX_EVIDENCE_LENGTH = 10000
 
 SHARE_USAGE_VERSION = '1.1'
 
@@ -212,7 +213,7 @@ def get_flow_elements(self):
             # make up the pipeline so a warning is logged
             # but otherwise, the system can continue as normal.
             self._log(
-                "warn",
+                "warning",
                 "Share usage element registered "
                 f"to {'too many' if len(self.pipelines) > 0 else 'no'}"
                 " pipelines. Unable to send share usage information.")
@@ -299,40 +300,30 @@ def add_to_share_usage(self, data):
 
         # --- write variable data
         # The SessionID used to track a series of requests
-        xml += f"<SessionId>{data.session_id}</SessionId>"
+        xml += plain_element("SessionId", data.session_id)
         # The sequence number of the request in a series of requests.
         xml += f"<Sequence>{data.sequence}</Sequence>"
         # The client IP of the request
-        xml += f"<ClientIP>{data.client_ip}</ClientIP>"
+        xml += plain_element("ClientIP", data.client_ip)
 
-        # The UTC date/time this entry was written
-        date = datetime.datetime.now().isoformat() 
+        # The UTC date and time this entry was written, to the second.
+        date = datetime.datetime.now(datetime.timezone.utc).strftime(
+            "%Y-%m-%dT%H:%M:%S")
         xml += f"<DateSent>{date}</DateSent>"
 
-        # Write all other evidence data that has been included.
+        # Write all other evidence data that has been included. The prefix,
+        # or a key with no prefix, is the element name. An entry whose
+        # element name is not an XML name is left out, because it would make
+        # the whole batch unreadable.
         for category_key, category_value in data.evidence_data.items():
             for entry_key, entry_value in category_value.items():
-                replaced_string = ReplacedString(entry_value)
-                # Write start element
                 if len(category_key) > 0:
-                    xml += f"<{category_key} Name=\"{entry_key}\""
-                else:
-                    xml += f"<{entry_key}"
-                # Write any attributes
-                if replaced_string.replaced:
-                    xml += ' replaced="true"'
-                if replaced_string.truncated:
-                    xml += ' truncated="true"'
-                # End the start element
-                xml += ">"
-                # Write the value
-                xml += replaced_string.result
-                # Write end element
-                if len(category_key) > 0:
-                    xml += f"</{category_key}>"
-                else:
-                    xml += f"</{entry_key}>"
-
+                    if XML_NAME.match(category_key):
+                        xml += record_element(
+                            category_key, entry_value, entry_key)
+                elif XML_NAME.match(entry_key):
+                    xml += record_element(entry_key, entry_value)
+
         xml += "</Device>"
 
         self.share_data.append(xml)
@@ -399,49 +390,91 @@ def get_data_from_evidence(self, flow_data):
         return data
 
 
-# a set of valid XML character values (ignoring valid controls x09, x0a, x0d, x85)
-VALID_XML_CHARS = [*range(int("0x20", 16), int("0x7F", 16))]
-VALID_XML_CHARS.extend(range(int("0x40", 16), int("0x100", 16)))
+# The characters that XML 1.0 section 2.2 allows, as ranges of code points.
+VALID_XML_RANGES = (
+    (0x9, 0xA), (0xD, 0xD), (0x20, 0xD7FF), (0xE000, 0xFFFD),
+    (0x10000, 0x10FFFF))
+
+# The characters that are written as entities in element text.
+XML_ENTITIES = {
+    '&': '&amp;',
+    '"': '&quot;',
+    "'": '&apos;',
+    '<': '&lt;',
+    '>': '&gt;',
+}
+
 
-def get_is_valid_char_map():
-  maxChar = int('0x100', 16)
-  isValidChar = {}
-  for c in range(maxChar):
-    isValidChar[c] = c in VALID_XML_CHARS
-  return isValidChar
+def is_valid_xml_char(code):
+    return any(low <= code <= high for low, high in VALID_XML_RANGES)
 
-# an array describing whether a character value is valid
-IS_VALID_XML_CHAR = get_is_valid_char_map()
+
+# The characters XML 1.0 allows in a name, without the colon, which names a
+# namespace.
+XML_NAME_START = (
+    "A-Z_a-z\u00c0-\u00d6\u00d8-\u00f6\u00f8-\u02ff\u0370-\u037d"
+    "\u037f-\u1fff\u200c-\u200d\u2070-\u218f\u2c00-\u2fef\u3001-\ud7ff"
+    "\uf900-\ufdcf\ufdf0-\ufffd\U00010000-\U000effff")
+XML_NAME = re.compile(
+    "[" + XML_NAME_START + "][" + XML_NAME_START +
+    "\\-.0-9\u00b7\u0300-\u036f\u203f-\u2040]*\\Z")
+
+
+def plain_element(tag, value):
+    """!
+    An element of a usage sharing record that is not an Evidence entry, with
+    its value made to fit the XML by ReplacedString and no attributes, as
+    the record gives these elements none.
+    """
+    return "<" + tag + ">" + ReplacedString(value).result + "</" + tag + ">"
+
+
+def record_element(tag, value, name=None):
+    """!
+    One element of a usage sharing record. The value, and the Name attribute
+    when there is one, are made to fit the XML by ReplacedString, and the
+    element is marked escaped or truncated when either changed.
+    """
+    replaced = ReplacedString(value)
+    escaped = replaced.escaped
+    xml = "<" + tag
+    if name is not None:
+        replaced_name = ReplacedString(name)
+        escaped = escaped or replaced_name.escaped
+        xml += ' Name="' + replaced_name.result + '"'
+    if escaped:
+        xml += ' escaped="true"'
+    if replaced.truncated:
+        xml += ' truncated="true"'
+    return xml + ">" + replaced.result + "</" + tag + ">"
 
 
 class ReplacedString:
     """!
-    replace characters that cause problems in XML with the "Replacement character"
+    Makes an Evidence value fit in the usage sharing XML. The value is cut to
+    SHARE_USAGE_MAX_EVIDENCE_LENGTH characters (code points, so a character is
+    never split), a character XML 1.0 does not allow is written as a backslash,
+    "x" and its code in four lower-case hexadecimal digits, and the markup
+    characters are written as entities. `escaped` and `truncated` say what
+    changed.
     """
     def __init__(self, text):
         self.result = ""
-        self.replaced = False
+        self.escaped = False
         self.truncated = False
         if text:
-            escapedText = text\
-                .replace('&', '&amp;')\
-                .replace('"', '&quot;')\
-                .replace('\'', '&apos;')\
-                .replace('<', '&lt;')\
-                .replace('>', '&gt;')
-
-            length = min(len(escapedText), SHARE_USAGE_MAX_EVIDENCE_LENGTH)
-            self.truncated = len(escapedText) > SHARE_USAGE_MAX_EVIDENCE_LENGTH
-            self.result = "".join(map(self.map_char, list(escapedText[0:length])))
-
+            if not isinstance(text, str):
+                text = str(text)
+            self.truncated = len(text) > SHARE_USAGE_MAX_EVIDENCE_LENGTH
+            self.result = "".join(
+                map(self.map_char, text[0:SHARE_USAGE_MAX_EVIDENCE_LENGTH]))
 
     def map_char(self, char):
-        charV = ord(char)
-        if charV < len(IS_VALID_XML_CHAR) and IS_VALID_XML_CHAR[charV]:
-            return char
-        else:
-            self.replaced = True
-            return chr(int("0xFFFD", 16))
+        code = ord(char)
+        if not is_valid_xml_char(code):
+            self.escaped = True
+            return "\\x%04x" % code
+        return XML_ENTITIES.get(char, char)
 
 class ShareUsageData:
     """!

pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_share_usage_record.py

@@ -0,0 +1,172 @@
+
+"""
+How the usage sharing element writes an Evidence value that was changed, as
+the Processing section of the usage sharing element specification says,
+including its invalid character and length rules.
+"""
+
+import re
+import unittest
+import xml.etree.ElementTree as ElementTree
+
+from fiftyone_pipeline_core.logger import Logger
+from fiftyone_pipeline_core.pipelinebuilder import PipelineBuilder
+from fiftyone_pipeline_engines_fiftyone.share_usage import (
+    SHARE_USAGE_MAX_EVIDENCE_LENGTH, ShareUsage, ShareUsageData)
+
+BS = chr(92)
+
+
+class ShareUsageRecordTests(unittest.TestCase):
+
+    def record(self, value, key="header.user-agent", session_id="session",
+               client_ip=""):
+        element = ShareUsage(requested_package_size=1000)
+        self.addCleanup(element.close)
+        element.constant_xml = "<Version>test</Version>"
+        data = ShareUsageData()
+        data.session_id = session_id
+        data.client_ip = client_ip
+        data.sequence = 1
+        data.try_add(key, value)
+        element.add_to_share_usage(data)
+        return element.share_data[0]
+
+    def test_session_id_is_written_so_the_record_parses(self):
+        session_id = 'a<b&c"d' + chr(0x1f)
+        device = ElementTree.fromstring(
+            self.record("Mozilla/5.0", session_id=session_id))
+        session = device.find("SessionId")
+        self.assertEqual('a<b&c"d' + BS + "x001f", session.text)
+        # The escaped and truncated attributes are for Evidence entries,
+        # and the record gives these elements none.
+        self.assertEqual({}, session.attrib)
+
+    def test_client_ip_is_written_so_the_record_parses(self):
+        device = ElementTree.fromstring(
+            self.record("Mozilla/5.0", client_ip="203.0.113.5<&"))
+        self.assertEqual("203.0.113.5<&", device.find("ClientIP").text)
+        self.assertEqual({}, device.find("ClientIP").attrib)
+
+    def test_a_long_session_id_is_cut_without_an_attribute(self):
+        device = ElementTree.fromstring(
+            self.record("Mozilla/5.0", session_id="s" * 10001))
+        self.assertEqual("s" * 10000, device.find("SessionId").text)
+        self.assertEqual({}, device.find("SessionId").attrib)
+
+    def test_name_attribute_is_written_so_the_record_parses(self):
+        device = ElementTree.fromstring(
+            self.record("a value", key='query.a"b<c&d'))
+        self.assertEqual('a"b<c&d', device.find("query").get("Name"))
+        self.assertEqual("a value", device.find("query").text)
+
+    def test_entry_whose_element_name_is_not_an_xml_name_is_left_out(self):
+        for key in ("a<b.field", "a b.field", "1st.field", "<bare>"):
+            xml = self.record("a value", key=key)
+            device = ElementTree.fromstring(xml)
+            self.assertNotIn("a value", xml, key)
+            self.assertEqual("session", device.find("SessionId").text, key)
+
+    def test_date_sent_is_utc_to_the_second(self):
+        device = ElementTree.fromstring(self.record("Mozilla/5.0"))
+        self.assertRegex(
+            device.find("DateSent").text,
+            re.compile(r"\A\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d\Z"))
+
+    def test_registered_to_too_many_pipelines_logs_a_warning(self):
+        logged = []
+
+        class Recorder(Logger):
+            def log_internal(self, level, log):
+                logged.append(level)
+
+        element = ShareUsage(requested_package_size=1000)
+        self.addCleanup(element.close)
+        pipelines = []
+        for _ in range(2):
+            builder = PipelineBuilder({"add_javascript_builder": False,
+                                       "use_setheader_properties": False})
+            builder.add_logger(Recorder("debug"))
+            pipelines.append(builder.build())
+        element.pipelines = pipelines
+        self.assertEqual([], element.get_flow_elements())
+        self.assertEqual(["warning", "warning"], logged)
+
+    def test_limit_is_ten_thousand(self):
+        self.assertEqual(10000, SHARE_USAGE_MAX_EVIDENCE_LENGTH)
+
+    def test_unchanged_value_has_no_attributes(self):
+        xml = self.record("Mozilla/5.0")
+        self.assertIn('<header Name="user-agent">Mozilla/5.0</header>', xml)
+
+    def test_invalid_character_is_written_as_hex_and_marked_escaped(self):
+        xml = self.record("a" + chr(0x1f) + "b")
+        self.assertIn(
+            '<header Name="user-agent" escaped="true">a' + BS + 'x001fb</header>',
+            xml)
+
+    def test_other_invalid_characters(self):
+        for code, text in ((0x0, "0000"), (0x8, "0008"), (0xb, "000b"),
+                           (0xc, "000c"), (0xe, "000e"), (0xd800, "d800"),
+                           (0xdfff, "dfff"), (0xfffe, "fffe"),
+                           (0xffff, "ffff")):
+            xml = self.record("x" + chr(code) + "y")
+            self.assertIn("x" + BS + "x" + text + "y", xml, hex(code))
+            self.assertIn(' escaped="true"', xml, hex(code))
+
+    def test_characters_xml_allows_are_kept(self):
+        value = "t" + chr(9) + chr(10) + chr(13) + chr(0x18c) + chr(0xfffd)
+        xml = self.record(value)
+        self.assertIn(">" + value + "<", xml)
+        self.assertNotIn("escaped", xml)
+
+    def test_markup_characters_are_still_entities(self):
+        xml = self.record("a&b<c>d\"e'")
+        self.assertIn(">a&amp;b&lt;c&gt;d&quot;e&apos;<", xml)
+        self.assertNotIn("escaped", xml)
+
+    def test_value_of_the_limit_is_not_truncated(self):
+        xml = self.record("a" * 10000)
+        self.assertIn(">" + "a" * 10000 + "<", xml)
+        self.assertNotIn("truncated", xml)
+
+    def test_longer_value_is_cut_and_marked_truncated(self):
+        xml = self.record("a" * 10001)
+        self.assertIn(
+            ' truncated="true">' + "a" * 10000 + "</header>", xml)
+
+    def test_length_is_counted_before_markup_characters_are_written(self):
+        xml = self.record("&" * 10000)
+        self.assertNotIn("truncated", xml)
+
+    def test_escapes_do_not_count_towards_the_limit(self):
+        xml = self.record(chr(1) + "a" * 9999)
+        self.assertIn(' escaped="true"', xml)
+        self.assertNotIn("truncated", xml)
+        self.assertIn(chr(92) + "x0001" + "a" * 9999 + "<", xml)
+
+    def test_character_outside_the_basic_plane_is_never_split(self):
+        emoji = chr(0x1f600)
+        xml = self.record("a" * 9999 + emoji + "tail")
+        self.assertIn(
+            ' truncated="true">' + "a" * 9999 + emoji + "</header>", xml)
+        xml = self.record("a" * 9998 + emoji + emoji + "z")
+        self.assertIn(
+            ' truncated="true">' + "a" * 9998 + emoji + emoji + "</header>",
+            xml)
+        self.assertNotIn("escaped", xml)
+
+    def test_invalid_character_in_a_long_value_sets_both(self):
+        xml = self.record(chr(1) + "a" * 10000)
+        self.assertIn(' escaped="true"', xml)
+        self.assertIn(' truncated="true"', xml)
+
+    def test_changed_value_is_written_without_cdata_or_bad_schema(self):
+        xml = self.record("a" + chr(1) + "b")
+        self.assertNotIn("CDATA", xml)
+        self.assertNotIn("BadSchema", xml)
+        self.assertNotIn("replaced", xml)
+
+
+if __name__ == "__main__":
+    unittest.main()