Copilot commented on code in PR #21: URL: https://github.com/apache/commons-xml/pull/21#discussion_r3550831128
########## src/test/java/org/apache/commons/xml/XIncludeTest.java: ########## @@ -0,0 +1,419 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.apache.commons.xml.AttackTestSupport.LEAKED_MARKER; +import static org.apache.commons.xml.AttackTestSupport.inputSource; +import static org.apache.commons.xml.AttackTestSupport.resourceUrl; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.StringReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.SAXParserFactory; + +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.DefaultHandler; + +/** + * Tests that XInclude resolution is denied by default on factories from {@link XmlFactories}, and that callers can + * allow-list specific resources via an {@link EntityResolver}. + * + * <p>Each case is exercised in both {@code parse="xml"} and {@code parse="text"} modes, and for both DOM and SAX + * paths. XInclude resolution requires namespace-aware processing; the baseline tests set it explicitly, and the + * hardened factory tests rely on the underlying JAXP implementation being namespace-aware enough to recognise elements + * in the {@code http://www.w3.org/2001/XInclude} namespace.</p> + */ +class XIncludeTest { + + /** Absolute URL of the XML fixture pulled in by {@code parse="xml"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_XML = resourceUrl("referenced.xml").toString(); + + /** Absolute URL of the text fixture pulled in by {@code parse="text"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_TEXT = resourceUrl("referenced.txt").toString(); + + /** Content the allow-list resolver returns for an allowed include; its presence proves the caller's resolver was consulted. */ + private static final String RESOLVED_MARKER = "XINCLUDE-RESOLVED-905bbbce-16ee-4a0c-b165-d1f8c663934c"; + + /** + * Allow-lists the two fixture URLs, returning the appropriate in-memory content for each: {@link #RESOLVED_MARKER} + * wrapped as XML for {@link #REFERENCED_XML}, and as plain text for {@link #REFERENCED_TEXT}. Anything else returns + * {@code null} so the hardening's deny-all floor refuses it. Mirrors a caller allow-listing trusted resources. + */ + private static final class AllowListResolver implements EntityResolver { + + @Override + public InputSource resolveEntity(final String publicId, final String systemId) { + final InputSource source; + if (REFERENCED_XML.equals(systemId)) { + source = new InputSource(new StringReader("<allowed>" + RESOLVED_MARKER + "</allowed>")); + } else if (REFERENCED_TEXT.equals(systemId)) { + source = new InputSource(new StringReader(RESOLVED_MARKER)); + } else { + return null; + } + source.setPublicId(publicId); + source.setSystemId(systemId); + return source; + } + } + + /** Resolver that resolves nothing, so the hardening's deny-all floor must refuse every lookup and never leak. */ + private static final EntityResolver NO_OP_RESOLVER = (publicId, systemId) -> null; + + /** XML wrapper for xi:include in the given {@code parse} mode referencing {@code href}. */ + private static String xiIncludeXml(final String href, final String parseMode) { + return "<?xml version=\"1.0\"?>\n" + + "<root xmlns:xi=\"http://www.w3.org/2001/XInclude\";>\n" + + " <xi:include href=\"" + href + "\" parse=\"" + parseMode + "\"/>\n" + + "</root>"; + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final DocumentBuilderFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final SAXParserFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + //region Baseline: unhardened JAXP resolves the include + + @Test + @Tag("dom") + void baselineDomLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertEquals(LEAKED_MARKER, text.trim(), + "Baseline DOM parse=xml should leak marker; got: " + text); + } + + @Test + @Tag("dom") + void baselineDomLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertTrue(text != null && text.contains(LEAKED_MARKER), + "Baseline DOM parse=text should leak marker; got: " + text); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(LEAKED_MARKER, captured.toString().trim(), + "Baseline SAX parse=xml should leak marker; got: " + captured); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertTrue(captured.toString().contains(LEAKED_MARKER), + "Baseline SAX parse=text should leak marker; got: " + captured); + } + + //endregion + + //region Hardened factory: fails closed (throws) + + @Test + @Tag("dom") + void hardenedDomBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=xml should throw"); + } + + @Test + @Tag("dom") + void hardenedDomBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=text should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=xml should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=text should throw"); + } + + //endregion + + //region Hardened factory + allow-list resolver: allowed href works, non-allowed throws + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> builder.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> reader.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + //endregion + + //region harden(XMLReader): reader with XInclude enabled before hardening is blocked + + @Test + @Tag("sax") + void hardenReaderBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + // Reader from an unhardened factory that already has XInclude enabled + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); + assertThrows(SAXException.class, () -> reader.parse(input), + "harden(reader) should block XInclude parse=xml on reader with XInclude already enabled"); + } + + @Test + @Tag("sax") + void hardenReaderBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); Review Comment: Same issue as above: the hardened reader returned by `XmlFactories.harden(...)` must be used; otherwise the deny-all resolver floor can be overwritten because the code keeps using the original `XMLReader`. ########## src/test/java/org/apache/commons/xml/XIncludeTest.java: ########## @@ -0,0 +1,419 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.apache.commons.xml.AttackTestSupport.LEAKED_MARKER; +import static org.apache.commons.xml.AttackTestSupport.inputSource; +import static org.apache.commons.xml.AttackTestSupport.resourceUrl; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.StringReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.SAXParserFactory; + +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.DefaultHandler; + +/** + * Tests that XInclude resolution is denied by default on factories from {@link XmlFactories}, and that callers can + * allow-list specific resources via an {@link EntityResolver}. + * + * <p>Each case is exercised in both {@code parse="xml"} and {@code parse="text"} modes, and for both DOM and SAX + * paths. XInclude resolution requires namespace-aware processing; the baseline tests set it explicitly, and the + * hardened factory tests rely on the underlying JAXP implementation being namespace-aware enough to recognise elements + * in the {@code http://www.w3.org/2001/XInclude} namespace.</p> + */ +class XIncludeTest { + + /** Absolute URL of the XML fixture pulled in by {@code parse="xml"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_XML = resourceUrl("referenced.xml").toString(); + + /** Absolute URL of the text fixture pulled in by {@code parse="text"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_TEXT = resourceUrl("referenced.txt").toString(); + + /** Content the allow-list resolver returns for an allowed include; its presence proves the caller's resolver was consulted. */ + private static final String RESOLVED_MARKER = "XINCLUDE-RESOLVED-905bbbce-16ee-4a0c-b165-d1f8c663934c"; + + /** + * Allow-lists the two fixture URLs, returning the appropriate in-memory content for each: {@link #RESOLVED_MARKER} + * wrapped as XML for {@link #REFERENCED_XML}, and as plain text for {@link #REFERENCED_TEXT}. Anything else returns + * {@code null} so the hardening's deny-all floor refuses it. Mirrors a caller allow-listing trusted resources. + */ + private static final class AllowListResolver implements EntityResolver { + + @Override + public InputSource resolveEntity(final String publicId, final String systemId) { + final InputSource source; + if (REFERENCED_XML.equals(systemId)) { + source = new InputSource(new StringReader("<allowed>" + RESOLVED_MARKER + "</allowed>")); + } else if (REFERENCED_TEXT.equals(systemId)) { + source = new InputSource(new StringReader(RESOLVED_MARKER)); + } else { + return null; + } + source.setPublicId(publicId); + source.setSystemId(systemId); + return source; + } + } + + /** Resolver that resolves nothing, so the hardening's deny-all floor must refuse every lookup and never leak. */ + private static final EntityResolver NO_OP_RESOLVER = (publicId, systemId) -> null; + + /** XML wrapper for xi:include in the given {@code parse} mode referencing {@code href}. */ + private static String xiIncludeXml(final String href, final String parseMode) { + return "<?xml version=\"1.0\"?>\n" + + "<root xmlns:xi=\"http://www.w3.org/2001/XInclude\";>\n" + + " <xi:include href=\"" + href + "\" parse=\"" + parseMode + "\"/>\n" + + "</root>"; + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final DocumentBuilderFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final SAXParserFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + //region Baseline: unhardened JAXP resolves the include + + @Test + @Tag("dom") + void baselineDomLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertEquals(LEAKED_MARKER, text.trim(), + "Baseline DOM parse=xml should leak marker; got: " + text); + } + + @Test + @Tag("dom") + void baselineDomLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertTrue(text != null && text.contains(LEAKED_MARKER), + "Baseline DOM parse=text should leak marker; got: " + text); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(LEAKED_MARKER, captured.toString().trim(), + "Baseline SAX parse=xml should leak marker; got: " + captured); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertTrue(captured.toString().contains(LEAKED_MARKER), + "Baseline SAX parse=text should leak marker; got: " + captured); + } + + //endregion + + //region Hardened factory: fails closed (throws) + + @Test + @Tag("dom") + void hardenedDomBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=xml should throw"); + } + + @Test + @Tag("dom") + void hardenedDomBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=text should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=xml should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=text should throw"); + } + + //endregion + + //region Hardened factory + allow-list resolver: allowed href works, non-allowed throws + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> builder.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> reader.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + //endregion + + //region harden(XMLReader): reader with XInclude enabled before hardening is blocked + + @Test + @Tag("sax") + void hardenReaderBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + // Reader from an unhardened factory that already has XInclude enabled + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); + assertThrows(SAXException.class, () -> reader.parse(input), + "harden(reader) should block XInclude parse=xml on reader with XInclude already enabled"); + } + + @Test + @Tag("sax") + void hardenReaderBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); + assertThrows(SAXException.class, () -> reader.parse(input), + "harden(reader) should block XInclude parse=text on reader with XInclude already enabled"); + } + + @Test + @Tag("sax") + void hardenReaderAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); Review Comment: This test is intended to verify that an allow-list resolver is chained *under* a non-removable deny-all floor. Because the return value of `XmlFactories.harden(reader)` is ignored, `reader.setEntityResolver(...)` replaces the floor entirely, so unresolved hrefs would fall back to default fetching instead of staying blocked. ########## src/test/java/org/apache/commons/xml/XIncludeTest.java: ########## @@ -0,0 +1,419 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.apache.commons.xml.AttackTestSupport.LEAKED_MARKER; +import static org.apache.commons.xml.AttackTestSupport.inputSource; +import static org.apache.commons.xml.AttackTestSupport.resourceUrl; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.StringReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.SAXParserFactory; + +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.DefaultHandler; + +/** + * Tests that XInclude resolution is denied by default on factories from {@link XmlFactories}, and that callers can + * allow-list specific resources via an {@link EntityResolver}. + * + * <p>Each case is exercised in both {@code parse="xml"} and {@code parse="text"} modes, and for both DOM and SAX + * paths. XInclude resolution requires namespace-aware processing; the baseline tests set it explicitly, and the + * hardened factory tests rely on the underlying JAXP implementation being namespace-aware enough to recognise elements + * in the {@code http://www.w3.org/2001/XInclude} namespace.</p> + */ +class XIncludeTest { + + /** Absolute URL of the XML fixture pulled in by {@code parse="xml"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_XML = resourceUrl("referenced.xml").toString(); + + /** Absolute URL of the text fixture pulled in by {@code parse="text"} includes; carries {@link AttackTestSupport#LEAKED_MARKER}. */ + private static final String REFERENCED_TEXT = resourceUrl("referenced.txt").toString(); + + /** Content the allow-list resolver returns for an allowed include; its presence proves the caller's resolver was consulted. */ + private static final String RESOLVED_MARKER = "XINCLUDE-RESOLVED-905bbbce-16ee-4a0c-b165-d1f8c663934c"; + + /** + * Allow-lists the two fixture URLs, returning the appropriate in-memory content for each: {@link #RESOLVED_MARKER} + * wrapped as XML for {@link #REFERENCED_XML}, and as plain text for {@link #REFERENCED_TEXT}. Anything else returns + * {@code null} so the hardening's deny-all floor refuses it. Mirrors a caller allow-listing trusted resources. + */ + private static final class AllowListResolver implements EntityResolver { + + @Override + public InputSource resolveEntity(final String publicId, final String systemId) { + final InputSource source; + if (REFERENCED_XML.equals(systemId)) { + source = new InputSource(new StringReader("<allowed>" + RESOLVED_MARKER + "</allowed>")); + } else if (REFERENCED_TEXT.equals(systemId)) { + source = new InputSource(new StringReader(RESOLVED_MARKER)); + } else { + return null; + } + source.setPublicId(publicId); + source.setSystemId(systemId); + return source; + } + } + + /** Resolver that resolves nothing, so the hardening's deny-all floor must refuse every lookup and never leak. */ + private static final EntityResolver NO_OP_RESOLVER = (publicId, systemId) -> null; + + /** XML wrapper for xi:include in the given {@code parse} mode referencing {@code href}. */ + private static String xiIncludeXml(final String href, final String parseMode) { + return "<?xml version=\"1.0\"?>\n" + + "<root xmlns:xi=\"http://www.w3.org/2001/XInclude\";>\n" + + " <xi:include href=\"" + href + "\" parse=\"" + parseMode + "\"/>\n" + + "</root>"; + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final DocumentBuilderFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + /** + * Enables XInclude on the factory under test, skipping the test when the platform refuses. + * + * <p>On Android{@code setXIncludeAware(true)} always throws {@link UnsupportedOperationException}.</p> + */ + private static void assumeXIncludeAware(final SAXParserFactory factory) { + try { + factory.setXIncludeAware(true); + } catch (final UnsupportedOperationException e) { + Assumptions.abort("XInclude not supported on this platform"); + } + } + + //region Baseline: unhardened JAXP resolves the include + + @Test + @Tag("dom") + void baselineDomLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertEquals(LEAKED_MARKER, text.trim(), + "Baseline DOM parse=xml should leak marker; got: " + text); + } + + @Test + @Tag("dom") + void baselineDomLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final Document doc = factory.newDocumentBuilder().parse(input); + final String text = doc.getDocumentElement().getTextContent(); + assertTrue(text != null && text.contains(LEAKED_MARKER), + "Baseline DOM parse=text should leak marker; got: " + text); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(LEAKED_MARKER, captured.toString().trim(), + "Baseline SAX parse=xml should leak marker; got: " + captured); + } + + @Test + @Tag("sax") + void baselineSaxLeaksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final StringBuilder captured = new StringBuilder(); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertTrue(captured.toString().contains(LEAKED_MARKER), + "Baseline SAX parse=text should leak marker; got: " + captured); + } + + //endregion + + //region Hardened factory: fails closed (throws) + + @Test + @Tag("dom") + void hardenedDomBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=xml should throw"); + } + + @Test + @Tag("dom") + void hardenedDomBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(input); + }, "Hardened DOM parse=text should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=xml should throw"); + } + + @Test + @Tag("sax") + void hardenedSaxBlocksParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + assertThrows(SAXException.class, () -> { + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.parse(input); + }, "Hardened SAX parse=text should throw"); + } + + //endregion + + //region Hardened factory + allow-list resolver: allowed href works, non-allowed throws + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(new AllowListResolver()); + final Document doc = builder.parse(input); + assertEquals(RESOLVED_MARKER, doc.getDocumentElement().getTextContent().trim(), + "DOM parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("dom") + void hardenedDomNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> builder.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=xml with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxWithAllowListResolvesParseText() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_TEXT, "text")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(new AllowListResolver()); + final StringBuilder captured = new StringBuilder(); + reader.setContentHandler(new DefaultHandler() { + @Override + public void characters(final char[] ch, final int start, final int length) { + captured.append(ch, start, length); + } + }); + reader.parse(input); + assertEquals(RESOLVED_MARKER, captured.toString().trim(), + "SAX parse=text with allow-list should resolve to the resolver's content"); + } + + @Test + @Tag("sax") + void hardenedSaxNullResolverDoesNotLeak() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + final SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + factory.setNamespaceAware(true); + assumeXIncludeAware(factory); + final XMLReader reader = factory.newSAXParser().getXMLReader(); + reader.setEntityResolver(NO_OP_RESOLVER); + assertThrows(SAXException.class, () -> reader.parse(input), + "a resolver that returns null must not leak: the deny-all floor blocks the real href"); + } + + //endregion + + //region harden(XMLReader): reader with XInclude enabled before hardening is blocked + + @Test + @Tag("sax") + void hardenReaderBlocksParseXml() throws Exception { + final InputSource input = inputSource(xiIncludeXml(REFERENCED_XML, "xml")); + + // Reader from an unhardened factory that already has XInclude enabled + final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); + unhardenedFactory.setNamespaceAware(true); + assumeXIncludeAware(unhardenedFactory); + final XMLReader reader = unhardenedFactory.newSAXParser().getXMLReader(); + XmlFactories.harden(reader); Review Comment: `XmlFactories.harden(reader)` returns a hardened wrapper (e.g., `HardeningXMLReader`) that prevents callers from replacing the deny-all resolver floor. Calling it without assigning the return value means subsequent operations still use the original, bypassable `XMLReader` instance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
