From 4bae4bb61172fa4fde752d1b948818e094d1c4a6 Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Thu, 30 Jan 2020 08:59:56 +0100 Subject: [PATCH] SANTUARIO-523 XMLSecurityStreamReader now correctly parses version, encoding and standalone-ness from XML declaration --- .../stax/impl/XMLSecurityStreamReader.java | 30 ++++-- .../stax/XMLSecurityStreamReaderTest.java | 94 +++++++++++++++++-- .../test/stax/utils/XmlReaderToWriter.java | 7 ++ 3 files changed, 115 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/apache/xml/security/stax/impl/XMLSecurityStreamReader.java b/src/main/java/org/apache/xml/security/stax/impl/XMLSecurityStreamReader.java index 7d6f3d9ba2..24d4a7f684 100644 --- a/src/main/java/org/apache/xml/security/stax/impl/XMLSecurityStreamReader.java +++ b/src/main/java/org/apache/xml/security/stax/impl/XMLSecurityStreamReader.java @@ -33,6 +33,7 @@ import javax.xml.stream.events.EntityReference; import javax.xml.stream.events.Namespace; import javax.xml.stream.events.ProcessingInstruction; +import javax.xml.stream.events.StartDocument; import org.apache.xml.security.exceptions.XMLSecurityException; import org.apache.xml.security.stax.ext.InputProcessorChain; @@ -49,7 +50,11 @@ public class XMLSecurityStreamReader implements XMLStreamReader { private final InputProcessorChain inputProcessorChain; private XMLSecEvent currentXMLSecEvent; - private boolean skipDocumentEvents = false; + private final boolean skipDocumentEvents; + private String version; + private boolean standalone; + private boolean standaloneSet; + private String characterEncodingScheme; private static final String ERR_STATE_NOT_ELEM = "Current state not START_ELEMENT or END_ELEMENT"; private static final String ERR_STATE_NOT_STELEM = "Current state not START_ELEMENT"; @@ -75,9 +80,18 @@ public int next() throws XMLStreamException { inputProcessorChain.reset(); currentXMLSecEvent = inputProcessorChain.processEvent(); eventType = currentXMLSecEvent.getEventType(); - if (eventType == START_DOCUMENT && this.skipDocumentEvents) { - currentXMLSecEvent = inputProcessorChain.processEvent(); - eventType = currentXMLSecEvent.getEventType(); + if (eventType == START_DOCUMENT) { + // Even when skipDocumentEvents is true, we still want to get the information out of the event. + // We only skip the event itself. + StartDocument startDocument = (StartDocument) currentXMLSecEvent; + version = startDocument.getVersion(); + characterEncodingScheme = startDocument.getCharacterEncodingScheme(); + standalone = startDocument.isStandalone(); + standaloneSet = startDocument.standaloneSet(); + if (skipDocumentEvents) { + currentXMLSecEvent = inputProcessorChain.processEvent(); + eventType = currentXMLSecEvent.getEventType(); + } } } catch (XMLSecurityException e) { throw new XMLStreamException(e); @@ -592,22 +606,22 @@ public String getPrefix() { @Override public String getVersion() { - return null; + return version; } @Override public boolean isStandalone() { - return false; + return standalone; } @Override public boolean standaloneSet() { - return false; + return standaloneSet; } @Override public String getCharacterEncodingScheme() { - return null; + return characterEncodingScheme; } @Override diff --git a/src/test/java/org/apache/xml/security/test/stax/XMLSecurityStreamReaderTest.java b/src/test/java/org/apache/xml/security/test/stax/XMLSecurityStreamReaderTest.java index a9dc38483f..81dd37ec60 100644 --- a/src/test/java/org/apache/xml/security/test/stax/XMLSecurityStreamReaderTest.java +++ b/src/test/java/org/apache/xml/security/test/stax/XMLSecurityStreamReaderTest.java @@ -19,6 +19,7 @@ package org.apache.xml.security.test.stax; import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.InputStream; import java.io.InputStreamReader; @@ -51,6 +52,9 @@ import org.junit.jupiter.api.Test; import org.xmlunit.matchers.CompareMatcher; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -105,6 +109,70 @@ public void testIdentityTransformSource() throws Exception { MatcherAssert.assertThat(readTestFile(), CompareMatcher.isSimilarTo(baos.toString(StandardCharsets.UTF_8.name()))); } + @Test + public void testDocumentDeclaration() throws Exception { + String xml = "\n" + + ""; + ByteArrayInputStream xmlInput = new ByteArrayInputStream(xml.getBytes(StandardCharsets.ISO_8859_1)); + XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); + XMLStreamReader stdXmlStreamReader = xmlInputFactory.createXMLStreamReader(xmlInput); + InboundSecurityContextImpl securityContext = new InboundSecurityContextImpl(); + InputProcessorChainImpl inputProcessorChain = new InputProcessorChainImpl(securityContext); + inputProcessorChain.addProcessor(new EventReaderProcessor(stdXmlStreamReader)); + XMLSecurityProperties securityProperties = new XMLSecurityProperties(); + securityProperties.setSkipDocumentEvents(false); + XMLSecurityStreamReader xmlSecurityStreamReader = new XMLSecurityStreamReader(inputProcessorChain, securityProperties); + advanceToFirstEvent(xmlSecurityStreamReader); + assertThat(xmlSecurityStreamReader.getEventType(), is(XMLStreamConstants.START_DOCUMENT)); + assertThat(xmlSecurityStreamReader.getVersion(), is(equalTo("1.1"))); + assertThat(xmlSecurityStreamReader.getCharacterEncodingScheme(), is(equalTo("ISO-8859-1"))); + assertThat(xmlSecurityStreamReader.isStandalone(), is(true)); + assertThat(xmlSecurityStreamReader.standaloneSet(), is(true)); + + assertThat(xmlSecurityStreamReader.hasNext(), is(true)); + assertThat(xmlSecurityStreamReader.next(), is(XMLStreamConstants.START_ELEMENT)); + // Strictly speaking, we should assert that getVersion() etc. throw when not on a START_DOCUMENT event. + // However, we have to be lenient to compensate for XML reader implementations such as Xalan, + // which access getVersion() etc. when they're positioned on START_ELEMENT, _beyond_ START_DOCUMENT. + assertThat(xmlSecurityStreamReader.getVersion(), is(equalTo("1.1"))); + assertThat(xmlSecurityStreamReader.getCharacterEncodingScheme(), is(equalTo("ISO-8859-1"))); + assertThat(xmlSecurityStreamReader.isStandalone(), is(true)); + assertThat(xmlSecurityStreamReader.standaloneSet(), is(true)); + } + + @Test + public void testDocumentDeclarationWhenSkipDocumentEvents() throws Exception { + String xml = "\n" + + ""; + ByteArrayInputStream xmlInput = new ByteArrayInputStream(xml.getBytes(StandardCharsets.ISO_8859_1)); + XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); + XMLStreamReader stdXmlStreamReader = xmlInputFactory.createXMLStreamReader(xmlInput); + InboundSecurityContextImpl securityContext = new InboundSecurityContextImpl(); + InputProcessorChainImpl inputProcessorChain = new InputProcessorChainImpl(securityContext); + inputProcessorChain.addProcessor(new EventReaderProcessor(stdXmlStreamReader)); + XMLSecurityProperties securityProperties = new XMLSecurityProperties(); + securityProperties.setSkipDocumentEvents(true); + XMLSecurityStreamReader xmlSecurityStreamReader = new XMLSecurityStreamReader(inputProcessorChain, securityProperties); + advanceToFirstEvent(xmlSecurityStreamReader); + assertThat(xmlSecurityStreamReader.getEventType(), is(XMLStreamConstants.START_ELEMENT)); + assertThat(xmlSecurityStreamReader.getVersion(), is(equalTo("1.1"))); + assertThat(xmlSecurityStreamReader.getCharacterEncodingScheme(), is(equalTo("ISO-8859-1"))); + assertThat(xmlSecurityStreamReader.isStandalone(), is(true)); + assertThat(xmlSecurityStreamReader.standaloneSet(), is(true)); + } + + /** + * This method advances the reader until it's on the first event, if that's not already the case. + * Depending on the implementation, the {@code xmlStreamReader} may be positioned before or on + * the first event upon creation. + */ + private static void advanceToFirstEvent(XMLStreamReader xmlStreamReader) throws XMLStreamException { + if (xmlStreamReader.getEventType() <= 0) { + assertThat(xmlStreamReader.hasNext(), is(true)); + xmlStreamReader.next(); + } + } + @Test public void testCorrectness() throws Exception { XMLSecurityProperties securityProperties = new XMLSecurityProperties(); @@ -123,6 +191,9 @@ public void testCorrectness() throws Exception { "org/apache/xml/security/c14n/inExcl/plain-soap-1.1.xml")); //hmm why does a streamreader return a DOCUMENT_EVENT before we did call next() ?? + // A: Because some implementations of XMLStreamReader are positioned _on_ the first event rather than before it. + // Woodstox is a typical example of such an implementation. + // We have to compensate for both types of implementation, see the method advanceToFirstEvent(XMLStreamReader). int stdXMLEventType = stdXmlStreamReader.getEventType(); int secXMLEventType = xmlSecurityStreamReader.getEventType(); do { @@ -213,9 +284,9 @@ public void testCorrectness() throws Exception { assertEquals(stdXmlStreamReader.getTextLength(), xmlSecurityStreamReader.getTextLength()); break; case XMLStreamConstants.START_DOCUMENT: - assertEquals(stdXmlStreamReader.getCharacterEncodingScheme(), xmlSecurityStreamReader.getCharacterEncodingScheme()); + assertEquals(StandardCharsets.UTF_8.name(), xmlSecurityStreamReader.getCharacterEncodingScheme()); assertEquals(stdXmlStreamReader.getEncoding(), xmlSecurityStreamReader.getEncoding()); - //assertEquals(stdXmlStreamReader.getVersion(), xmlSecurityStreamReader.getVersion()); + assertEquals(stdXmlStreamReader.getVersion(), xmlSecurityStreamReader.getVersion()); break; case XMLStreamConstants.END_DOCUMENT: break; @@ -269,17 +340,24 @@ private String readTestFile() throws Exception { return stringBuilder.toString(); } + private static XMLStreamReader createXmlStreamReader() throws XMLStreamException { + XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); + xmlInputFactory.setProperty(XMLInputFactory.IS_COALESCING, true); + xmlInputFactory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true); + return xmlInputFactory.createXMLStreamReader(InputProcessor.class.getClassLoader().getResourceAsStream( + "org/apache/xml/security/c14n/inExcl/plain-soap-1.1.xml")); + } + class EventReaderProcessor implements InputProcessor { private XMLStreamReader xmlStreamReader; + EventReaderProcessor(XMLStreamReader xmlStreamReader) { + this.xmlStreamReader = xmlStreamReader; + } + EventReaderProcessor() throws Exception { - XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); - xmlInputFactory.setProperty(XMLInputFactory.IS_COALESCING, true); - xmlInputFactory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true); - xmlStreamReader = - xmlInputFactory.createXMLStreamReader(this.getClass().getClassLoader().getResourceAsStream( - "org/apache/xml/security/c14n/inExcl/plain-soap-1.1.xml")); + this(createXmlStreamReader()); } @Override diff --git a/src/test/java/org/apache/xml/security/test/stax/utils/XmlReaderToWriter.java b/src/test/java/org/apache/xml/security/test/stax/utils/XmlReaderToWriter.java index b98cd4425c..2a3b6e4136 100644 --- a/src/test/java/org/apache/xml/security/test/stax/utils/XmlReaderToWriter.java +++ b/src/test/java/org/apache/xml/security/test/stax/utils/XmlReaderToWriter.java @@ -31,6 +31,13 @@ private XmlReaderToWriter() { public static void writeAll(XMLStreamReader xmlr, XMLStreamWriter writer) throws XMLStreamException { + // Some implementations, Woodstox for example, already position their reader ON the first event, which is. + // typically a START_DOCUMENT event. + // If already positioned on an event, that is indicated by the event type. + // Make sure we don't miss the initial event. + if (xmlr.getEventType() > 0) { + write(xmlr, writer); + } while (xmlr.hasNext()) { xmlr.next(); write(xmlr, writer);