From 1750eae2d16b3eb8f594534a98ba5cd881545ea9 Mon Sep 17 00:00:00 2001 From: Thomas Zerr Date: Thu, 10 Aug 2023 14:20:07 +0200 Subject: [PATCH] Fix marshalling invalid XML characters If someone tries to persist data with invalid XML characters, they now get filtered out. Committed-by: Rene Pfeuffer --- ...essions.yml => endless_user_sessions.yaml} | 0 gradle/changelog/marshalling-invalid-xml.yaml | 2 + .../xml/FilterInvalidCharXMLStreamWriter.java | 226 +++++++++ .../FilterInvalidCharXMLStreamWriterTest.java | 449 ++++++++++++++++++ .../java/sonia/scm/store/JAXBDataStore.java | 3 +- .../sonia/scm/store/TypedStoreContext.java | 4 +- .../main/java/sonia/scm/xml/XmlStreams.java | 3 +- 7 files changed, 684 insertions(+), 3 deletions(-) rename gradle/changelog/{endless_user_sessions.yml => endless_user_sessions.yaml} (100%) create mode 100644 gradle/changelog/marshalling-invalid-xml.yaml create mode 100644 scm-core/src/main/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriter.java create mode 100644 scm-core/src/test/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriterTest.java diff --git a/gradle/changelog/endless_user_sessions.yml b/gradle/changelog/endless_user_sessions.yaml similarity index 100% rename from gradle/changelog/endless_user_sessions.yml rename to gradle/changelog/endless_user_sessions.yaml diff --git a/gradle/changelog/marshalling-invalid-xml.yaml b/gradle/changelog/marshalling-invalid-xml.yaml new file mode 100644 index 0000000000..459c4853e1 --- /dev/null +++ b/gradle/changelog/marshalling-invalid-xml.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Marshalling of invalid xml characters diff --git a/scm-core/src/main/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriter.java b/scm-core/src/main/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriter.java new file mode 100644 index 0000000000..5d9ee0503a --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriter.java @@ -0,0 +1,226 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.xml; + +import javax.xml.namespace.NamespaceContext; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamWriter; + +public class FilterInvalidCharXMLStreamWriter implements XMLStreamWriter, AutoCloseable { + + private final XMLStreamWriter writer; + + private final static String invalidXmlCharsPattern = "[" + "\u0000-\u0008" + "\u000B-\u000C" + "\u000E-\u001F" + "\uD800-\uDFFF" + "\uFFFE-\uFFFF" + "]"; + + public FilterInvalidCharXMLStreamWriter(XMLStreamWriter writer) { + this.writer = writer; + } + + @Override + public void writeStartElement(String localName) throws XMLStreamException { + writer.writeStartElement(filterInvalidXmlChars(localName)); + } + + @Override + public void writeStartElement(String namespaceURI, String localName) throws XMLStreamException { + writer.writeStartElement(filterInvalidXmlChars(namespaceURI), filterInvalidXmlChars(localName)); + } + + @Override + public void writeStartElement(String prefix, String localName, String namespaceURI) throws XMLStreamException { + writer.writeStartElement( + filterInvalidXmlChars(prefix), + filterInvalidXmlChars(localName), + filterInvalidXmlChars(namespaceURI) + ); + } + + @Override + public void writeEmptyElement(String localName) throws XMLStreamException { + writer.writeEmptyElement(filterInvalidXmlChars(localName)); + } + + @Override + public void writeEmptyElement(String namespaceURI, String localName) throws XMLStreamException { + writer.writeEmptyElement(filterInvalidXmlChars(namespaceURI), filterInvalidXmlChars(localName)); + } + + @Override + public void writeEmptyElement(String prefix, String localName, String namespaceURI) throws XMLStreamException { + writer.writeEmptyElement( + filterInvalidXmlChars(prefix), + filterInvalidXmlChars(localName), + filterInvalidXmlChars(namespaceURI) + ); + } + + @Override + public void writeEndElement() throws XMLStreamException { + writer.writeEndElement(); + } + + @Override + public void writeEndDocument() throws XMLStreamException { + writer.writeEndDocument(); + } + + @Override + public void writeAttribute(String localName, String value) throws XMLStreamException { + writer.writeAttribute(filterInvalidXmlChars(localName), filterInvalidXmlChars(value)); + } + + @Override + public void writeAttribute(String namespaceURI, String localName, String value) throws XMLStreamException { + writer.writeAttribute( + filterInvalidXmlChars(namespaceURI), + filterInvalidXmlChars(localName), + filterInvalidXmlChars(value) + ); + } + + @Override + public void writeAttribute(String prefix, String namespaceURI, String localName, String value) throws XMLStreamException { + writer.writeAttribute( + filterInvalidXmlChars(prefix), + filterInvalidXmlChars(namespaceURI), + filterInvalidXmlChars(localName), + filterInvalidXmlChars(value) + ); + } + + @Override + public void writeNamespace(String prefix, String namespaceURI) throws XMLStreamException { + writer.writeNamespace(filterInvalidXmlChars(prefix), filterInvalidXmlChars(namespaceURI)); + } + + @Override + public void writeDefaultNamespace(String namespaceURI) throws XMLStreamException { + writer.writeDefaultNamespace(filterInvalidXmlChars(namespaceURI)); + } + + @Override + public void writeComment(String data) throws XMLStreamException { + writer.writeComment(filterInvalidXmlChars(data)); + } + + @Override + public void writeProcessingInstruction(String target) throws XMLStreamException { + writer.writeProcessingInstruction(filterInvalidXmlChars(target)); + } + + @Override + public void writeProcessingInstruction(String target, String data) throws XMLStreamException { + writer.writeProcessingInstruction(filterInvalidXmlChars(target), filterInvalidXmlChars(data)); + } + + @Override + public void writeCData(String data) throws XMLStreamException { + writer.writeCData(filterInvalidXmlChars(data)); + } + + @Override + public void writeDTD(String dtd) throws XMLStreamException { + writer.writeDTD(filterInvalidXmlChars(dtd)); + } + + @Override + public void writeEntityRef(String name) throws XMLStreamException { + writer.writeEntityRef(filterInvalidXmlChars(name)); + } + + @Override + public void writeStartDocument() throws XMLStreamException { + writer.writeStartDocument(); + } + + @Override + public void writeStartDocument(String version) throws XMLStreamException { + writer.writeStartDocument(filterInvalidXmlChars(version)); + } + + @Override + public void writeStartDocument(String encoding, String version) throws XMLStreamException { + writer.writeStartDocument(filterInvalidXmlChars(encoding), filterInvalidXmlChars(version)); + } + + @Override + public void writeCharacters(String text) throws XMLStreamException { + writer.writeCharacters(filterInvalidXmlChars(text)); + } + + @Override + public void writeCharacters(char[] text, int start, int len) throws XMLStreamException { + StringBuilder sb = new StringBuilder(); + for(int i = start; i < start + len; ++i) { + sb.append(text[i]); + } + + writer.writeCharacters(filterInvalidXmlChars(sb.toString())); + } + + @Override + public String getPrefix(String uri) throws XMLStreamException { + return writer.getPrefix(filterInvalidXmlChars(uri)); + } + + @Override + public void setPrefix(String prefix, String uri) throws XMLStreamException { + writer.setPrefix(filterInvalidXmlChars(prefix), filterInvalidXmlChars(uri)); + } + + @Override + public void setDefaultNamespace(String uri) throws XMLStreamException { + writer.setDefaultNamespace(filterInvalidXmlChars(uri)); + } + + @Override + public void setNamespaceContext(NamespaceContext context) throws XMLStreamException { + writer.setNamespaceContext(context); + } + + @Override + public NamespaceContext getNamespaceContext() { + return writer.getNamespaceContext(); + } + + @Override + public Object getProperty(String name) throws IllegalArgumentException { + return writer.getProperty(filterInvalidXmlChars(name)); + } + + @Override + public void close() throws XMLStreamException { + writer.close(); + } + + @Override + public void flush() throws XMLStreamException { + writer.flush(); + } + + private String filterInvalidXmlChars(String input) { + return input.replaceAll(invalidXmlCharsPattern, ""); + } +} diff --git a/scm-core/src/test/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriterTest.java b/scm-core/src/test/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriterTest.java new file mode 100644 index 0000000000..ab244c7041 --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/xml/FilterInvalidCharXMLStreamWriterTest.java @@ -0,0 +1,449 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.xml; + + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import javax.xml.stream.XMLOutputFactory; +import javax.xml.stream.XMLStreamException; +import java.io.ByteArrayOutputStream; +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class FilterInvalidCharXMLStreamWriterTest { + + private static List invalidXmlChars; + + @BeforeAll + static void setupInvalidChars() { + invalidXmlChars = new ArrayList<>(); + + for(int i = 0; i < 0x110000; ++i) { + if(i == 0x9 || i == 0xA || i == 0xD || (i >= 0x20 && i <= 0xD7FF) || (i >= 0xE000 && i <= 0xFFFD) || (i >= 0x10000 && i <= 0x10FFFF)) { + continue; + } + + invalidXmlChars.add(i); + } + } + + @Test + void shouldWriteDefaultStartOfXml() throws XMLStreamException { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartDocument(); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + + @Test + void shouldWriteStartOfXmlWithVersion() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartDocument(addInvalidChars("1.1", invalidChar)); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteStartOfXmlWithVersionAndEncoding() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartDocument(addInvalidChars("UTF-8", invalidChar), addInvalidChars("1.1", invalidChar)); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteStartElement() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartElement(addInvalidChars("Root", invalidChar)); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + + @Test + void shouldWriteEndOfDocument() throws XMLStreamException { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartElement("Root"); + writer.writeStartElement("Child"); + writer.writeEndDocument(); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + + @Test + void shouldWriteAttribute() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeStartElement("Root"); + writer.writeAttribute(addInvalidChars("attribute", invalidChar), addInvalidChars("value", invalidChar)); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteProcessingInstruction() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeProcessingInstruction(addInvalidChars("Target", invalidChar)); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteProcessingInstructionWithData() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeProcessingInstruction( + addInvalidChars("Target", invalidChar), + addInvalidChars("InstructionData", invalidChar) + ); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteCData() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeCData( + addInvalidChars("Data", invalidChar) + ); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteDtd() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeDTD( + addInvalidChars("", invalidChar) + ); + } + + assertThat(outputStream.toString()).isEqualTo(""); + } + } + + @Test + void shouldWriteEntityRef() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeEntityRef( + addInvalidChars("Name", invalidChar) + ); + } + + assertThat(outputStream.toString()).isEqualTo("&Name;"); + } + } + + @Test + void shouldWriteCharactersFromString() throws XMLStreamException { + for (int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeCharacters( + addInvalidChars("&&", invalidChar) + ); + } + + assertThat(outputStream.toString()).isEqualTo("&<Some Random String>&"); + } + } + + @ParameterizedTest + @CsvSource({"test,0,4,test", "test,1,2,es"}) + void shouldWriteCharactersFromBuffer(String text, String start, String len, String expectedResult) throws XMLStreamException { + char[] chars = text.toCharArray(); + int startIndex = Integer.parseInt(start); + int length = Integer.parseInt(len); + + + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + writer.writeCharacters(chars, startIndex, length); + } + + assertThat(outputStream.toString()).isEqualTo(expectedResult); + } + + @Test + void shouldWriteCharactersFromBuffer() throws XMLStreamException { + for(int invalidChar : invalidXmlChars) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + + try (FilterInvalidCharXMLStreamWriter writer = new FilterInvalidCharXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(outputStream))) { + char[] unfilteredChars = addInvalidChars("Test", invalidChar).toCharArray(); + writer.writeCharacters(unfilteredChars, 0, unfilteredChars.length); + } + + assertThat(outputStream.toString()).isEqualTo("Test"); + } + } + + private String addInvalidChars(String input, int invalidChar) { + StringBuilder sb = new StringBuilder(); + sb.appendCodePoint(invalidChar); + sb.append(input); + sb.appendCodePoint(invalidChar); + + return sb.toString(); + } +} diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java index 5a60e26b16..b0df8fd5a9 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBDataStore.java @@ -29,6 +29,7 @@ import com.google.common.collect.ImmutableMap.Builder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.security.KeyGenerator; +import sonia.scm.xml.XmlStreams; import javax.xml.bind.JAXBException; import javax.xml.bind.Marshaller; @@ -75,7 +76,7 @@ public class JAXBDataStore extends FileBasedStore implements DataStore marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, Boolean.TRUE); CopyOnWrite.withTemporaryFile( - temp -> marshaller.marshal(item, temp.toFile()), + temp -> marshaller.marshal(item, XmlStreams.createWriter(temp.toFile())), file.toPath(), () -> cache.put(file, item) ); diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java b/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java index 2d5aaae858..fe4306ff5d 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/TypedStoreContext.java @@ -24,6 +24,8 @@ package sonia.scm.store; +import sonia.scm.xml.XmlStreams; + import javax.xml.bind.JAXBContext; import javax.xml.bind.JAXBException; import javax.xml.bind.Marshaller; @@ -73,7 +75,7 @@ final class TypedStoreContext { } void marshal(Object object, File file) { - withMarshaller(marshaller -> marshaller.marshal(object, file)); + withMarshaller(marshaller -> marshaller.marshal(object, XmlStreams.createWriter(file))); } void withMarshaller(ThrowingConsumer consumer) { diff --git a/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java b/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java index 4b5b649d29..0a8d712534 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java +++ b/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java @@ -95,7 +95,8 @@ public final class XmlStreams { private static AutoCloseableXMLWriter createWriter(Writer writer) throws XMLStreamException { IndentXMLStreamWriter indentXMLStreamWriter = new IndentXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(writer)); - return new AutoCloseableXMLWriter(indentXMLStreamWriter, writer); + FilterInvalidCharXMLStreamWriter filterInvalidCharXMLStreamWriter = new FilterInvalidCharXMLStreamWriter(indentXMLStreamWriter); + return new AutoCloseableXMLWriter(filterInvalidCharXMLStreamWriter, writer); } public static final class AutoCloseableXMLReader extends StreamReaderDelegate implements AutoCloseable {