You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2026/09/24 21:22:41 UTC

[PR] KNOX-3484 - Use Apache Commons Secure XML for all JAXP factories [knox]

ppkarwasz opened a new pull request, #1427:
URL: https://github.com/apache/knox/pull/1427

   [KNOX-3484](https://issues.apache.org/jira/browse/KNOX-3484) - Use Apache Commons Secure XML for all JAXP factories
   
   ## What changes were proposed in this pull request?
   
   Knox configured its XML parsers by hand: each call site set its own subset of features and properties, and some relied on the defaults of whichever implementation (JDK, Woodstox, MOXy, Glassfish JAXB, Commons Digester) served them. This PR replaces those ad hoc settings with one dependency, [Apache Commons Secure XML](https://commons.apache.org/proper/commons-secure-xml/) 1.0.0, whose factories are hardened as delivered: external resources (DTDs, external entities, XInclude, schemas, stylesheets) are ignored, independently of the JAXP implementation.
   
   This is a hardening and maintenance change: the PMC no longer has to configure XML parsers.
   
   - **DOM, TrAX, XPath, Validation:** `XmlUtils`, `UrlRewriteRulesDescriptorAdapter`, `XmlFilterReader`, `ServiceURLPropertyConfig`, `TopologyValidator` and `TopologyToDescriptor` obtain their factories from `SecureDocumentBuilderFactory`, `SecureTransformerFactory`, `SecureXPathFactory` and `SecureSchemaFactory`. The hand-written feature settings are removed.
   - **StAX:** `XmlFilterReader` uses `SecureXMLInputFactory`.
   - **JAXB:** `ServiceDefinitionUnmarshaller`, `TopologyMarshaller` (XML bodies), `ServiceDefinitionsLoader`, `ApplicationDeploymentContributor`, `TopologyToDescriptor`, `RemoteConfigurationRegistriesParser` and `ProviderConfigurationParser` unmarshal through two new helpers, `XmlUtils.unmarshal(Unmarshaller, Class<T>, File)` and `XmlUtils.unmarshal(Unmarshaller, Class<T>, String, InputStream)`, which unmarshal from an `XMLEventReader` created by `SecureXMLInputFactory`. This follows the StAX approach documented for JAXB in apache/commons-secure-xml#103, and makes the result independent of the JAXB provider serving each context.
   - **`TopologyMarshaller`:** selects the XML or JSON reader by exact media type (`application/xml` or `application/json`, parameters ignored) and passes MOXy a fixed media type instead of the request's. Other types, including wildcards, are rejected with HTTP 415.
   - **Commons Digester:** `TopologyUtils`, `XmlGatewayDescriptorImporter` and `XmlUrlRewriteRulesImporter` pass Digester an `XMLReader` created by `SecureSAXParserFactory`. Since Digester 3.2 resolves any absolute system id by itself, each site also installs a resolver that returns `null`, leaving external references to Commons Secure XML.
   - **Enforcement:** a new forbidden-apis execution (`build-tools/forbiddenapis/xml-signatures.txt`, main code only) rejects the plain JAXP factory methods, the `Unmarshaller.unmarshal` overloads that let the JAXB provider create its own parser (`File`, `InputStream`, `Reader`, `URL`, `InputSource`, `Source`) and the Digester constructors that create their own parser. `TopologyMarshaller` is excluded from this check in `gateway-service-admin/pom.xml`, because MOXy parses its JSON request bodies with its own JSON reader. On the current `master` it reports 23 violations in 9 modules, for example:
   
     ```
     [ERROR] Forbidden method invocation: javax.xml.parsers.DocumentBuilderFactory#newInstance() [Use SecureDocumentBuilderFactory from Apache Commons Secure XML]
     [ERROR]   in org.apache.knox.gateway.util.XmlUtils (XmlUtils.java:47)
     [ERROR] Forbidden method invocation: javax.xml.stream.XMLInputFactory#newFactory() [Use SecureXMLInputFactory from Apache Commons Secure XML]
     [ERROR]   in org.apache.knox.gateway.filter.rewrite.impl.xml.XmlFilterReader (XmlFilterReader.java:97)
     [ERROR] Forbidden method invocation: org.apache.commons.digester3.binder.DigesterLoader#newDigester() [Pass an XMLReader created by SecureSAXParserFactory]
     [ERROR]   in org.apache.knox.gateway.util.TopologyUtils (TopologyUtils.java:41)
     ```
   
   **Behavior change:** documents containing a DOCTYPE are now accepted everywhere. External references are ignored and internal entities are expanded within the limits of secure processing. Previously `XmlUtils` rejected any DOCTYPE and the StAX readers disabled DTD support.
   
   Minor fixes in touched code: `XmlUtils.readXml(File)` and `ProviderConfigurationParser.parseXML(File)` no longer leak their input stream, and `ServiceDefinitionsLoader` no longer opens a stream it does not read.
   
   ## How was this patch tested?
   
   - `mvn install` on the affected modules (`gateway-util-common`, `gateway-spi`, `gateway-service-admin`, `gateway-provider-rewrite`, `gateway-provider-rewrite-common`, `gateway-server`, `gateway-discovery-ambari`, `gateway-service-remoteconfig`, `gateway-topology-simple`) and their dependencies: all unit tests, checkstyle, forbidden-apis and dependency analysis pass.
   - `GatewayAdminTopologyFuncTest`: 32/32 pass.
   - No new tests are added, since the guarantees are the library's and are tested there. The existing KNOX-1308 and KNOX-3447 tests (`GatewayAdminTopologyFuncTest#testPutTopologyWithEntityExpansion`, `ServiceDefinitionUnmarshallerTest#testEntityExpansionIsNotResolved`) asserted that internal entities are not expanded; they now assert that the content of an external entity does not leak. Reverting `ServiceDefinitionUnmarshaller` to a plain `XMLInputFactory` makes both unit tests fail.
   
   ## Integration Tests
   
   No integration tests are added or changed: the change does not alter any Knox feature, and the existing suites cover the affected parsing paths.
   
   πŸ€– Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KNOX-3484 - Use Apache Commons Secure XML for all JAXP factories [knox]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1427:
URL: https://github.com/apache/knox/pull/1427#issuecomment-5822611066

   ## Test Results
     4 files    4 suites   41s ⏱️
   127 tests 127 βœ…β€ƒ0 πŸ’€β€ƒ0 ❌
   145 runsβ€Šβ€ƒ145 βœ…β€ƒ0 πŸ’€β€ƒ0 ❌
   
   Results for commit a3177f09.
   
   [test-results]:data:application/gzip;base64,H4sIAEeXtWoC/13MSw7CIBSF4a00jB1chELqZgzP5Ma2GAqjxr1LSUV0+P0nOTvxOLuN3AZ+GciWMTXYHFXCsB6kxWVJx0av8qP7lo35Tw98lgQteIXzT3AxhniWmNf6yccT7bIr38fq7rC6/zNhWTAVEMWolB4mbUECAyuYM8JaAYqLcfJUC8OoNoy83ldcXP4EAQAA
   


-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KNOX-3484 - Use Apache Commons Secure XML for all JAXP factories [knox]

Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1427:
URL: https://github.com/apache/knox/pull/1427#discussion_r4098848188


##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/XmlUtils.java:
##########
@@ -22,41 +22,81 @@
 import java.io.Writer;
 import java.nio.file.Files;
 
-import javax.xml.XMLConstants;
+import jakarta.xml.bind.JAXBException;
+import jakarta.xml.bind.Unmarshaller;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.stream.XMLStreamException;
 import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureTransformerFactory;
+import org.apache.commons.xml.secure.SecureXMLInputFactory;
 import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
+/**
+ * XML parsing and serialization helpers.
+ *
+ * <p>Factories come from <a href="https://commons.apache.org/proper/commons-secure-xml/">Apache Commons Secure XML</a>,
+ * which ignores external resources (DTDs, entities, XInclude, stylesheets).</p>
+ */
 public class XmlUtils {
 
   public static Document readXml( File file ) throws ParserConfigurationException, IOException, SAXException {
-    return readXml(Files.newInputStream(file.toPath()));
+    try (InputStream input = Files.newInputStream(file.toPath())) {
+      return readXml(input);
+    }
   }
 
   public static Document readXml( InputStream input ) throws ParserConfigurationException, IOException, SAXException {
-    DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
-    f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-    f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-    DocumentBuilder b = f.newDocumentBuilder();
-    return b.parse( input );
+    return newDocumentBuilder().parse( input );
   }
 
   public static Document readXml( InputSource source ) throws ParserConfigurationException, IOException, SAXException {
-    DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
-    f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-    f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-    DocumentBuilder b = f.newDocumentBuilder();
-    return b.parse( source );
+    return newDocumentBuilder().parse( source );
+  }
+
+  /**
+   * Unmarshals the XML content of a file.
+   *
+   * @param unmarshaller the JAXB unmarshaller to use
+   * @param type         the expected type of the root object
+   * @param file         the file to read
+   * @param <T>          the expected type of the root object
+   * @return the root object of the content tree
+   * @throws IOException   if the file cannot be read
+   * @throws JAXBException if the content cannot be parsed or unmarshalled
+   */

Review Comment:
   Thanks! Fixed in https://github.com/apache/knox/pull/1427/commits/c9b22d33b99da02fdaa971705cbcbab2617056b9



-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KNOX-3484 - Use Apache Commons Secure XML for all JAXP factories [knox]

Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1427:
URL: https://github.com/apache/knox/pull/1427#discussion_r4098765824


##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/XmlUtils.java:
##########
@@ -22,41 +22,81 @@
 import java.io.Writer;
 import java.nio.file.Files;
 
-import javax.xml.XMLConstants;
+import jakarta.xml.bind.JAXBException;
+import jakarta.xml.bind.Unmarshaller;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.stream.XMLStreamException;
 import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureTransformerFactory;
+import org.apache.commons.xml.secure.SecureXMLInputFactory;
 import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
+/**
+ * XML parsing and serialization helpers.
+ *
+ * <p>Factories come from <a href="https://commons.apache.org/proper/commons-secure-xml/">Apache Commons Secure XML</a>,
+ * which ignores external resources (DTDs, entities, XInclude, stylesheets).</p>
+ */
 public class XmlUtils {
 
   public static Document readXml( File file ) throws ParserConfigurationException, IOException, SAXException {
-    return readXml(Files.newInputStream(file.toPath()));
+    try (InputStream input = Files.newInputStream(file.toPath())) {
+      return readXml(input);
+    }
   }
 
   public static Document readXml( InputStream input ) throws ParserConfigurationException, IOException, SAXException {
-    DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
-    f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-    f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-    DocumentBuilder b = f.newDocumentBuilder();
-    return b.parse( input );
+    return newDocumentBuilder().parse( input );
   }
 
   public static Document readXml( InputSource source ) throws ParserConfigurationException, IOException, SAXException {
-    DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
-    f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-    f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-    DocumentBuilder b = f.newDocumentBuilder();
-    return b.parse( source );
+    return newDocumentBuilder().parse( source );
+  }
+
+  /**
+   * Unmarshals the XML content of a file.
+   *
+   * @param unmarshaller the JAXB unmarshaller to use
+   * @param type         the expected type of the root object
+   * @param file         the file to read
+   * @param <T>          the expected type of the root object
+   * @return the root object of the content tree
+   * @throws IOException   if the file cannot be read
+   * @throws JAXBException if the content cannot be parsed or unmarshalled
+   */

Review Comment:
   @ppkarwasz Use Javadoc `since` tags on new `public` and `protected` elements?



-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org