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

@@ -37,7 +37,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'
 
@@ -284,8 +284,8 @@ def add_to_share_usage(self, data):
                 else:
                     xml += f"<{entry_key}"
                 # Write any attributes
-                if replaced_string.replaced:
-                    xml += ' replaced="true"'
+                if replaced_string.escaped:
+                    xml += ' escaped="true"'
                 if replaced_string.truncated:
                     xml += ' truncated="true"'
                 # End the start element
@@ -361,49 +361,51 @@ 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))
 
-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
+# The characters that are written as entities in element text.
+XML_ENTITIES = {
+    '&': '&amp;',
+    '"': '&quot;',
+    "'": '&apos;',
+    '<': '&lt;',
+    '>': '&gt;',
+}
 
-# an array describing whether a character value is valid
-IS_VALID_XML_CHAR = get_is_valid_char_map()
+
+def is_valid_xml_char(code):
+    return any(low <= code <= high for low, high in VALID_XML_RANGES)
 
 
 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,106 @@
+
+"""
+How the usage sharing element writes an Evidence value that was changed, as
+the Processing section of the usage sharing element specification says, with
+the invalid character and length rules of specifications pull request 122.
+"""
+
+import unittest
+
+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"):
+        element = ShareUsage(requested_package_size=1000)
+        self.addCleanup(element.close)
+        element.constant_xml = "<Version>test</Version>"
+        data = ShareUsageData()
+        data.session_id = "session"
+        data.sequence = 1
+        data.try_add(key, value)
+        element.add_to_share_usage(data)
+        return element.share_data[0]
+
+    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()

pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_usage.py

@@ -304,20 +304,17 @@ def test_invalid_char(self):
             requested_package_size=1,
             endpoint=testEndpoint)
 
-        replacementChar = chr(int("0xFFFD", 16))
-
         pipeline = (PipelineBuilder())\
             .add(usage)\
             .build()
         flowdata = pipeline.create_flowdata()
-        flowdata.evidence.add("header.user-agent", "1ƌ2")
+        flowdata.evidence.add("header.user-agent", "1" + chr(0x1f) + "2")
         flowdata.process()
 
         self.waitForUsageThreads()
         self.assertEqual(len(app.received), 1)
-        self.assertNotIn("ƌ", app.received[0])
-        self.assertIn(replacementChar, app.received[0])
-        self.assertIn(f"<header Name=\"user-agent\" replaced=\"true\">1{replacementChar}2</header>", app.received[0])
+        self.assertNotIn(chr(0x1f), app.received[0])
+        self.assertIn('<header Name="user-agent" escaped="true">1\\x001f2</header>', app.received[0])
 
     def test_truncated_value(self):
         """!
@@ -328,7 +325,7 @@ def test_truncated_value(self):
             requested_package_size=1,
             endpoint=testEndpoint)
 
-        ua = "X" * 1000
+        ua = "X" * 10001
 
         pipeline = (PipelineBuilder())\
             .add(usage)\
@@ -341,7 +338,7 @@ def test_truncated_value(self):
         self.assertEqual(len(app.received), 1)
         self.assertIn("truncated=\"true\"", app.received[0])
         self.assertNotIn(ua, app.received[0])
-        self.assertIn(f'<header Name="user-agent" truncated="true">{"X" * 512}</header>', app.received[0])
+        self.assertIn(f'<header Name="user-agent" truncated="true">{"X" * 10000}</header>', app.received[0])
 
     def test_send_more_than_once(self):
         """!