diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/Constants.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/Constants.java index f53a7bf2..b098ee13 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/Constants.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/Constants.java @@ -28,10 +28,10 @@ public class Constants { /** - * The maximum length of a piece of evidence's value which can be added to - * the usage data being sent. + * The maximum length, in characters, of a piece of evidence's value which + * can be added to the usage data being sent. */ - public static final int SHARE_USAGE_MAX_EVIDENCE_LENGTH = 512; + public static final int SHARE_USAGE_MAX_EVIDENCE_LENGTH = 10000; /** * Session id evidence suffix. This is prefixed by the query constant. diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElement.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElement.java index c56e032f..6fb71eae 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElement.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElement.java @@ -40,9 +40,6 @@ import java.net.URI; import java.util.*; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; -import java.util.stream.IntStream; -import java.util.stream.Stream; import java.util.zip.GZIPOutputStream; import static fiftyone.pipeline.engines.fiftyone.flowelements.Constants.SHARE_USAGE_DEFAULT_HTTP_POST_TIMEOUT; @@ -422,7 +419,13 @@ private void streamXml(List allData, OutputStream os) throws XML */ protected void writeXmlData(XMLStreamWriter writer, ShareUsageData data) throws XMLStreamException { writer.writeStartElement("Device"); - // --- write invariant data + // The elements follow the order of the specification's example. + // The SessionID used to track a series of requests + writeXmlElement(writer, "SessionId", data.sessionId); + // The sequence number of the request in a series of requests. + writeXmlElement(writer, "Sequence", String.valueOf(data.sequence)); + // The UTC date/time this entry was written + writeXmlElement(writer, "DateSent", DATE_FMT.format(new Date())); // The version number of the Pipeline API writeXmlElement(writer, "Version", coreVersion); // Write Pipeline information @@ -435,21 +438,13 @@ protected void writeXmlData(XMLStreamWriter writer, ShareUsageData data) throws writeXmlElement(writer, "Language", "java"); // The software language version writeXmlElement(writer, "LanguageVersion", languageVersion); + // The client IP of the request + writeXmlElement(writer, "ClientIP", data.clientIP); // The IP of this server writeXmlElement(writer, "ServerIP", getHostAddress()); // The OS name and version writeXmlElement(writer, "Platform", osVersion); - // --- write variable data - // The SessionID used to track a series of requests - writeXmlElement(writer, "SessionId", data.sessionId); - // The sequence number of the request in a series of requests. - writeXmlElement(writer, "Sequence", String.valueOf(data.sequence)); - // The UTC date/time this entry was written - writeXmlElement(writer, "DateSent", DATE_FMT.format(new Date())); - // The client IP of the request - writeXmlElement(writer, "ClientIP", data.clientIP); - // Write all other evidence data that has been included. for (Map.Entry> category : data.evidenceData.entrySet()) { for (Map.Entry entry : category.getValue().entrySet()) { @@ -461,7 +456,7 @@ protected void writeXmlData(XMLStreamWriter writer, ShareUsageData data) throws writer.writeStartElement(entry.getKey()); } if (replacedString.isReplaced()) { - writer.writeAttribute("replaced", "true"); + writer.writeAttribute("escaped", "true"); } if (replacedString.isTruncated()) { writer.writeAttribute("truncated", "true"); @@ -488,45 +483,55 @@ protected void writeXmlElement(XMLStreamWriter writer, * replace characters that cause problems in XML with the "Replacement character" */ public static class ReplacedString { - // a set of valid XML character values (ignoring valid controls x09, x0a, x0d, x85) - static final Set VALID_XML_CHARS = - Stream.of( IntStream.range(0x20,0x7F), - IntStream.range(0xA0, 0x100)) - .flatMapToInt(i -> i) - .boxed() - .collect(Collectors.toCollection(HashSet::new)); - // an array describing whether a character value is valid - static final Boolean[] IS_VALID_XML_CHAR = IntStream - .range(0, 0x100) - .mapToObj(VALID_XML_CHARS::contains) - .toArray(Boolean[]::new); - - // maximum string length + // maximum string length, in characters static final int MAX_LENGTH = SHARE_USAGE_MAX_EVIDENCE_LENGTH; private boolean replaced; private boolean truncated; private final StringBuilder builder; + /** + * Whether the character is allowed in an XML 1.0 document, as the + * Char production in section 2.2 of the XML 1.0 specification says. + * @param codePoint the character to check + * @return true if the character is allowed in XML 1.0 + */ + static boolean isXmlChar(int codePoint) { + return codePoint == 0x9 || codePoint == 0xA || codePoint == 0xD || + (codePoint >= 0x20 && codePoint <= 0xD7FF) || + (codePoint >= 0xE000 && codePoint <= 0xFFFD) || + (codePoint >= 0x10000 && codePoint <= 0x10FFFF); + } + + /** + * Keep the first {@link #MAX_LENGTH} characters of the text, and write + * each character XML 1.0 does not allow as a backslash, an x and the + * four lower-case hexadecimal digits of its code. The limit counts + * characters, so a pair of surrogates is never split. + * @param text the text to prepare + */ public ReplacedString(String text) { if (Objects.isNull(text) || text.length() == 0) { builder = new StringBuilder(0); - // Using 'return' statement in a constructor throws compilation - // error in Jdk 8. - // return; } else { - builder = new StringBuilder(Math.min(text.length(), MAX_LENGTH)); - truncated = text.length() > MAX_LENGTH; - text.chars() - .limit(MAX_LENGTH) - .forEach(c -> { - if (c < IS_VALID_XML_CHAR.length && IS_VALID_XML_CHAR[c]) { - builder.append((char) c); - } else { - builder.append((char) 0xFFFD); - replaced = true; - } - }); + int end = text.length(); + if (text.codePointCount(0, end) > MAX_LENGTH) { + end = text.offsetByCodePoints(0, MAX_LENGTH); + truncated = true; + } + builder = new StringBuilder(end); + int index = 0; + while (index < end) { + int codePoint = text.codePointAt(index); + if (isXmlChar(codePoint)) { + builder.appendCodePoint(codePoint); + } else { + builder.append("\\x").append( + String.format("%04x", codePoint)); + replaced = true; + } + index += Character.charCount(codePoint); + } } } diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ReplacedStringTest.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ReplacedStringTest.java index c98691cc..df4d534b 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ReplacedStringTest.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ReplacedStringTest.java @@ -39,7 +39,7 @@ */ class ReplacedStringTest { - // is transparent to legal characters + // is transparent to the three control characters XML 1.0 allows @Test public void testNormal(){ String aString = IntStream.of(0x9, 0xA, 0xD) @@ -47,8 +47,8 @@ public void testNormal(){ .collect(joining()); ReplacedString replacedString = new ReplacedString(aString); assertFalse(replacedString.isTruncated()); - assertTrue(replacedString.isReplaced()); - assertEquals(3, replacedString.toString().length()); + assertFalse(replacedString.isReplaced()); + assertEquals(aString, replacedString.toString()); } // is transparent to legal characters except \ @Test @@ -83,14 +83,74 @@ public void testNull(){ assertEquals(0, escapedString.toString().length()); assertEquals("", escapedString.toString()); } - // truncates longer than 512 + // truncates longer than 10,000 characters @Test public void testTruncate(){ - String aString = IntStream.range(0,513).mapToObj(i -> "a").collect(joining()); + String aString = repeat("a", 10001); + ReplacedString replacedString = new ReplacedString(aString); + assertTrue(replacedString.isTruncated()); + assertFalse(replacedString.isReplaced()); + assertEquals(10000, replacedString.toString().length()); + } + // keeps a value of exactly 10,000 characters whole + @Test + public void testNotTruncatedAtTheLimit(){ + String aString = repeat("a", 10000); + ReplacedString replacedString = new ReplacedString(aString); + assertFalse(replacedString.isTruncated()); + assertEquals(aString, replacedString.toString()); + } + // the limit counts characters, so a character made of a pair of + // surrogates at the limit is kept whole and the next one is cut + @Test + public void testTruncateNeverSplitsASurrogatePair(){ + String emoji = "\uD83D\uDE00"; + String aString = repeat("a", 9999) + emoji + "b"; ReplacedString replacedString = new ReplacedString(aString); assertTrue(replacedString.isTruncated()); assertFalse(replacedString.isReplaced()); - assertEquals(512, replacedString.toString().length()); + assertEquals(repeat("a", 9999) + emoji, replacedString.toString()); + } + // a pair that would start after the limit is cut with the rest + @Test + public void testTruncateBeforeASurrogatePair(){ + String emoji = "\uD83D\uDE00"; + String aString = repeat("a", 10000) + emoji; + ReplacedString replacedString = new ReplacedString(aString); + assertTrue(replacedString.isTruncated()); + assertEquals(repeat("a", 10000), replacedString.toString()); + } + // counts characters and not UTF-16 code units + @Test + public void testLimitCountsCharactersNotCodeUnits(){ + String emoji = "\uD83D\uDE00"; + ReplacedString atLimit = new ReplacedString(repeat(emoji, 10000)); + assertFalse(atLimit.isTruncated()); + assertEquals(20000, atLimit.toString().length()); + ReplacedString over = new ReplacedString(repeat(emoji, 10001)); + assertTrue(over.isTruncated()); + assertEquals(repeat(emoji, 10000), over.toString()); + } + // the limit counts the value as supplied, so the escapes written for + // invalid characters do not count towards it + @Test + public void testEscapesDoNotCountTowardsTheLimit(){ + String aString = "a" + (char) 0x1 + repeat("b", 9998); + ReplacedString replacedString = new ReplacedString(aString); + assertFalse(replacedString.isTruncated()); + assertTrue(replacedString.isReplaced()); + assertEquals(10000 - 1 + 6, replacedString.toString().length()); + // One character more is cut, whatever the escapes add. + ReplacedString over = new ReplacedString(aString + "c"); + assertTrue(over.isTruncated()); + assertEquals(10000 - 1 + 6, over.toString().length()); + } + private static String repeat(String text, int times) { + StringBuilder builder = new StringBuilder(); + for (int i = 0; i < times; i++) { + builder.append(text); + } + return builder.toString(); } // escapes single digit Hex @Test @@ -99,8 +159,7 @@ public void testEscapedSmall(){ ReplacedString replacedString = new ReplacedString(aString); assertFalse(replacedString.isTruncated()); assertTrue(replacedString.isReplaced()); - System.out.println(replacedString); - assertEquals(5, replacedString.toString().length()); + assertEquals(repeat("\\x0001", 5), replacedString.toString()); } // escapes 2 digit hex @Test @@ -109,8 +168,7 @@ public void testEscapedBigger(){ ReplacedString replacedString = new ReplacedString(aString); assertFalse(replacedString.isTruncated()); assertTrue(replacedString.isReplaced()); - System.out.println(replacedString); - assertEquals(5, replacedString.toString().length()); + assertEquals(repeat("\\x001f", 5), replacedString.toString()); } // does not escape valid chars @Test @@ -123,15 +181,50 @@ public void testEscapedBiggerStill(){ assertEquals(5, replacedString.toString().length()); } - @Test () - public void testEscapedEvenBigger(){ - String aString = IntStream.range(0,5).mapToObj(i -> "\u0100").collect(joining()); + // does not replace characters above U+00FF, which XML 1.0 allows + @Test + public void testNonLatinKept(){ + String aString = "\u0100 \u0416 \u6F22\u5B57 \uD7FF \uE000 \uFFFD"; ReplacedString replacedString = new ReplacedString(aString); assertFalse(replacedString.isTruncated()); - assertTrue(replacedString.isReplaced()); - System.out.println(replacedString); - assertEquals(5, replacedString.toString().length()); - + assertFalse(replacedString.isReplaced()); + assertEquals(aString, replacedString.toString()); + } + // does not replace the controls from U+007F to U+009F, which XML 1.0 allows + @Test + public void testC1ControlsKept(){ + String aString = IntStream.range(0x7F, 0xA0) + .mapToObj(i -> Character.toString((char)i)) + .collect(joining()); + ReplacedString replacedString = new ReplacedString(aString); + assertFalse(replacedString.isReplaced()); + assertEquals(aString, replacedString.toString()); + } + // keeps a pair of surrogates, which is one character above U+FFFF + @Test + public void testSurrogatePairKept(){ + String aString = "a\uD83D\uDE00b"; + ReplacedString replacedString = new ReplacedString(aString); + assertFalse(replacedString.isReplaced()); + assertEquals(aString, replacedString.toString()); + } + // writes a surrogate on its own, U+FFFE and U+FFFF as \x and four + // lower-case hexadecimal digits + @Test + public void testForbiddenCharactersEscaped(){ + String[][] cases = { + {"\uD83D", "\\xd83d"}, {"\uDE00", "\\xde00"}, + {"\uFFFE", "\\xfffe"}, {"\uFFFF", "\\xffff"}, + {"\u0000", "\\x0000"}, {"\u000B", "\\x000b"}}; + for (String[] c : cases) { + ReplacedString replacedString = new ReplacedString("a" + c[0] + "b"); + assertTrue(replacedString.isReplaced()); + assertEquals("a" + c[1] + "b", replacedString.toString()); + } + // A low surrogate before a high one is two surrogates on their own. + ReplacedString reversed = new ReplacedString("\uDE00\uD83D"); + assertTrue(reversed.isReplaced()); + assertEquals("\\xde00\\xd83d", reversed.toString()); } private final XMLOutputFactory xmlOutputFactory = XMLOutputFactory.newInstance(); @@ -140,12 +233,12 @@ public void testEscapedEvenBigger(){ @Test public void testXmlSerialise() throws XMLStreamException { xmlOutputFactory.setProperty("escapeCharacters", true); - String text = ReplacedString.VALID_XML_CHARS.stream() - .map(i->Character.toString((char)i.intValue())) - .collect(joining()); + String text = "\t\n a\u00E9\u0100\u0416\u6F22\u5B57\uD83D\uDE00" + + "\u007F\u0080\u009F\uD7FF\uE000\uFFFD <&>"; + assertEquals(text, new ReplacedString(text).toString()); ByteArrayOutputStream os = new ByteArrayOutputStream(); XMLStreamWriter writer = xmlOutputFactory.createXMLStreamWriter(os, "UTF-8"); - writer.writeStartDocument("UTF-8", "1.1"); + writer.writeStartDocument("UTF-8", "1.0"); writer.writeStartElement("test"); writer.writeCharacters(text); writer.writeEndElement(); @@ -155,8 +248,6 @@ public void testXmlSerialise() throws XMLStreamException { XMLStreamReader reader = xmlInputFactory.createXMLStreamReader(is); reader.next(); String readText = reader.getElementText(); - System.out.println(text); - System.out.println(readText); reader.close(); assertEquals(readText, text); diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElementTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElementTests.java index 5a7a6256..a7ecfecb 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElementTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElementTests.java @@ -40,6 +40,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.w3c.dom.Document; +import org.w3c.dom.Node; import org.w3c.dom.NodeList; import javax.xml.parsers.DocumentBuilder; @@ -797,13 +798,109 @@ public void ShareUsageElement_BadSchema() throws Exception { // Check that the expected values are populated. NodeList f = (NodeList) xpath.compile("//header[@Name='user-agent']").evaluate(doc, XPathConstants.NODESET); assertEquals(1, f.getLength(), "1 user-agent header expected"); - assertEquals("", f.item(0).getTextContent()); - assertEquals("true", f.item(0).getAttributes().getNamedItem("replaced").getTextContent()); + assertEquals("", f.item(0).getTextContent()); + assertEquals("true", f.item(0).getAttributes().getNamedItem("escaped").getTextContent()); NodeList b = (NodeList) xpath.compile("//BadSchema").evaluate(doc, XPathConstants.NODESET); assertEquals(0, b.getLength()); } } + /** + * Check that the invalid characters in a value are marked with the + * attribute the specification names, and that a value that is not too + * long is not marked as truncated. + */ + @Test + public void ShareUsageElement_InvalidCharacter_EscapedAttributeOnly() + throws Exception { + Document doc = shareUserAgent(""); + + Node userAgent = doc.getElementsByTagName("header").item(0); + assertEquals("true", + userAgent.getAttributes().getNamedItem("escaped").getTextContent()); + assertNull(userAgent.getAttributes().getNamedItem("replaced")); + assertNull(userAgent.getAttributes().getNamedItem("truncated")); + assertEquals("", userAgent.getTextContent()); + } + + /** + * Check that a value that is too long is cut and marked as truncated, + * without the escaped attribute. + */ + @Test + public void ShareUsageElement_LongValue_TruncatedAttributeOnly() + throws Exception { + StringBuilder longValue = new StringBuilder(); + for (int i = 0; i < 10001; i++) { + longValue.append('a'); + } + Document doc = shareUserAgent(longValue.toString()); + + Node userAgent = doc.getElementsByTagName("header").item(0); + assertEquals("true", + userAgent.getAttributes().getNamedItem("truncated").getTextContent()); + assertNull(userAgent.getAttributes().getNamedItem("escaped")); + assertEquals(10000, userAgent.getTextContent().length()); + } + + /** + * Check that the values are written in the order the specification's + * example gives, and that no CDATA section or BadSchema element is used. + */ + @Test + public void ShareUsageElement_RecordFollowsTheSpecificationExample() + throws Exception { + Document doc = shareUserAgent(""); + + Node device = doc.getElementsByTagName("Device").item(0); + List names = new ArrayList<>(); + for (int i = 0; i < device.getChildNodes().getLength(); i++) { + Node child = device.getChildNodes().item(i); + assertNotEquals(Node.CDATA_SECTION_NODE, child.getNodeType()); + if (child.getNodeType() == Node.ELEMENT_NODE) { + names.add(child.getNodeName()); + } + } + String[] expected = {"SessionId", "Sequence", "DateSent", "Version", + "Product", "FlowElement", "Language", "LanguageVersion", + "ClientIP", "ServerIP", "Platform"}; + List required = Arrays.asList("SessionId", "Sequence", + "DateSent", "Product", "Language", "LanguageVersion"); + int last = -1; + for (String name : expected) { + int index = names.indexOf(name); + if (!required.contains(name)) { + // Only present when the Pipeline, the request and the + // platform give one. + if (index < 0) { + continue; + } + } + assertTrue(index >= 0, name + " missing from " + names); + assertTrue(index > last, name + " out of order in " + names); + last = index; + } + assertEquals(0, doc.getElementsByTagName("BadSchema").getLength()); + assertEquals(1, doc.getElementsByTagName("header").getLength()); + } + + private Document shareUserAgent(String userAgent) throws Exception { + createShareUsage(1, 1, 1, new ArrayList<>(), + new ArrayList<>(), new ArrayList<>()); + Map evidenceData = new HashMap<>(); + evidenceData.put(EVIDENCE_HEADER_USERAGENT_KEY, userAgent); + evidenceData.put("query.session-id", "session"); + try (FlowData data = pipeline.createFlowData()) { + data.addEvidence(evidenceData); + shareUsageElement.process(data); + assertNotNull(shareUsageElement.getSendDataFuture()); + shareUsageElement.getSendDataFuture().get(); + byte[] output = connector.getBaos().toByteArray(); + return docBuilderFactory.newDocumentBuilder() + .parse(new ByteArrayInputStream(output)); + } + } + /** * Test that the ShareUsageElement generates a session id if one is not * contained in the evidence and adds it to the results.