diff --git a/pipeline/fiftyone_pipeline_engines_fiftyone/src/fiftyone_pipeline_engines_fiftyone/share_usage.py b/pipeline/fiftyone_pipeline_engines_fiftyone/src/fiftyone_pipeline_engines_fiftyone/share_usage.py index 210dfb0..3650c27 100644 --- a/pipeline/fiftyone_pipeline_engines_fiftyone/src/fiftyone_pipeline_engines_fiftyone/share_usage.py +++ b/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 = { + '&': '&', + '"': '"', + "'": ''', + '<': '<', + '>': '>', +} -# 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('&', '&')\ - .replace('"', '"')\ - .replace('\'', ''')\ - .replace('<', '<')\ - .replace('>', '>') - - 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: """! diff --git a/pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_share_usage_record.py b/pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_share_usage_record.py new file mode 100644 index 0000000..30b2b0b --- /dev/null +++ b/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 = "test" + 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('
Mozilla/5.0
', xml) + + def test_invalid_character_is_written_as_hex_and_marked_escaped(self): + xml = self.record("a" + chr(0x1f) + "b") + self.assertIn( + '
a' + BS + 'x001fb
', + 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&bd\"e'") + self.assertIn(">a&b<c>d"e'<", 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 + "", 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 + "", xml) + xml = self.record("a" * 9998 + emoji + emoji + "z") + self.assertIn( + ' truncated="true">' + "a" * 9998 + emoji + emoji + "", + 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() diff --git a/pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_usage.py b/pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_usage.py index 4f1bea4..5afaaee 100644 --- a/pipeline/fiftyone_pipeline_engines_fiftyone/tests/test_usage.py +++ b/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"
1{replacementChar}2
", app.received[0]) + self.assertNotIn(chr(0x1f), app.received[0]) + self.assertIn('
1\\x001f2
', 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'
{"X" * 512}
', app.received[0]) + self.assertIn(f'
{"X" * 10000}
', app.received[0]) def test_send_more_than_once(self): """!