Copilot commented on code in PR #103:
URL:
https://github.com/apache/commons-secure-xml/pull/103#discussion_r4094820855
##########
pom.xml:
##########
@@ -116,6 +116,19 @@ limitations under the License.
<version>${commons.jdependency.version}</version>
<scope>test</scope>
</dependency>
+ <!-- JAXB examples. -->
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ <version>2.3.1</version>
Review Comment:
The new JAXB versions are hard-coded here, unlike the other explicitly
versioned dependencies, which use `commons.*.version` properties in the
properties block. Put both versions in properties and reference those
properties so dependency updates remain centralized.
##########
src/main/javadoc/overview.html:
##########
@@ -526,6 +526,73 @@ <h1>
href="../threat_model.html">Threat Model</a> documents the resulting
contract.
</p>
</section>
+ <section id="jaxb">
+ <h1>
+ <img src="org/apache/commons/xml/secure/doc-files/leaf.svg"
style="height: 1em; padding-right: 0.25em" alt="leaf">Migrating JAXB
+ </h1>
+ This section shows you how to secure XML parsing and JAXB unmarshalling.
+ <p>
+ JAXB does not provide a portable guarantee that external entity
resolution is disabled or that Billion Laughs entity-expansion payloads are
bounded by default.
+ Below are the two recommended approaches for integrating <strong>Apache
Commons Secure XML</strong> to harden your unmarshalling pipeline.
+ </p>
+ <h2>Option 1: SAX-based Secure Unmarshalling (Recommended)</h2>
+ <p>
+ This approach uses
+ <code>SecureSAXParserFactory</code>
+ to construct a hardened, secure
+ <code>XMLReader</code>
+ which is then wrapped inside a
+ <code>SAXSource</code>
+ .
+ </p>
+ <pre>
+public MyJaxbModel unmarshalSecurelyWithSax(InputStream xmlStream) throws
Exception {
+ JAXBContext context = JAXBContext.newInstance(MyJaxbModel.class);
+ Unmarshaller unmarshaller = context.createUnmarshaller();
+
+ // Create a secure SAXParserFactory via Apache Commons Secure XML
+ SAXParserFactory spf = SecureSAXParserFactory.newDefaultNSInstance();
+
+ // Generate a hardened XMLReader and wrap the input source
+ XMLReader xmlReader = spf.newSAXParser().getXMLReader();
+ SAXSource source = new SAXSource(xmlReader, new InputSource(xmlStream));
+
+ // With the default resolver configuration, external DTDs and entities are
prevented from being fetched or resolved, protecting against XXE attacks,
protecting against XXE attacks.
Review Comment:
The phrase `protecting against XXE attacks` is repeated twice in this
user-facing snippet. Remove the duplicate before publishing the overview.
This issue also appears on line 586 of the same file.
##########
android-tests/build.gradle.kts:
##########
@@ -103,9 +103,9 @@ android {
}
}
-// ShadingFootprintTest is a JVM-only build check (it uses jdependency to read
target/classes); exclude it from the Android test compile.
+// These are JVM-only test classes; exclude them from the Android test compile.
tasks.withType<JavaCompile>().configureEach {
- exclude("**/ShadingFootprintTest.java")
+ exclude("**/ShadingFootprintTest.java", "**/JaxbExamples.java",
"**/JaxbTest.java")
Review Comment:
`JaxbExamples.java` does not exist in this repository; the added source is
`JaxbTest.java`. Remove this dead exclusion so the Android source-set
exclusions describe actual files.
##########
src/test/java/org/apache/commons/xml/secure/JaxbTest.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.secure;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+
+import javax.xml.bind.JAXBContext;
+import javax.xml.bind.Unmarshaller;
+import javax.xml.bind.annotation.XmlRootElement;
+import javax.xml.parsers.SAXParserFactory;
+import javax.xml.stream.XMLInputFactory;
+import javax.xml.stream.XMLStreamReader;
+import javax.xml.transform.sax.SAXSource;
+
+import org.junit.jupiter.api.Test;
+import org.xml.sax.InputSource;
+import org.xml.sax.XMLReader;
+
+/**
+ * Tests JAXB integration.
+ * <p>
+ * This class' two public methods serve as examples for the Javadoc {@code
overview.html} file.
+ * </p>
+ */
+public class JaxbTest {
+
+ @XmlRootElement
+ static class MyJaxbModel {
+ // JAXB model fields and methods
+ }
+
+ @Test
+ void testUnmarshalSecurelyWithSax() throws Exception {
+ try (InputStream xmlStream = new
ByteArrayInputStream("<myJaxbModel/>".getBytes(StandardCharsets.UTF_8))) {
+ assertNotNull(new JaxbTest().unmarshalSecurelyWithSax(xmlStream));
+ }
+ }
+
+ @Test
+ void testUnmarshalSecurelyWithStax() throws Exception {
+ try (InputStream xmlStream = new
ByteArrayInputStream("<myJaxbModel/>".getBytes(StandardCharsets.UTF_8))) {
+ assertNotNull(new JaxbTest().unmarshalSecurelyWithStax(xmlStream));
+ }
+ }
+
+ public MyJaxbModel unmarshalSecurelyWithSax(final InputStream xmlStream)
throws Exception {
+ final JAXBContext context = JAXBContext.newInstance(MyJaxbModel.class);
+ final Unmarshaller unmarshaller = context.createUnmarshaller();
+ // Create a secure SAXParserFactory via Apache Commons Secure XML
+ final SAXParserFactory spf =
SecureSAXParserFactory.newDefaultNSInstance();
+ // Generate a hardened XMLReader and wrap the input source
+ final XMLReader xmlReader = spf.newSAXParser().getXMLReader();
+ final SAXSource source = new SAXSource(xmlReader, new
InputSource(xmlStream));
+ // With the default resolver configuration, external DTDs and entities
are prevented from being fetched or resolved, protecting against XXE attacks,
protecting against XXE attacks.
Review Comment:
The phrase `protecting against XXE attacks` is repeated twice in this
comment. Remove the duplicate so the example accurately reads as documentation
rather than a copy-paste artifact.
This issue also appears on line 87 of the same file.
--
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]
