davsclaus commented on code in PR #28160:
URL: https://github.com/apache/camel/pull/28160#discussion_r4324152612
##########
components/camel-xslt-saxon/src/main/java/org/apache/camel/component/xslt/saxon/XsltSaxonEndpoint.java:
##########
@@ -258,6 +249,7 @@ protected XsltSaxonBuilder createXsltBuilder() throws
Exception {
xslt.setEntityResolver(getEntityResolver());
xslt.setAllowStAX(allowStAX);
xslt.setUseJsonBody(useJsonBody);
+ xslt.setSecureProcessing(secureProcessing);
Review Comment:
With this, `secureProcessing` also turns the input-parsing restriction on
and off. Please mark the option on the endpoint and on the component with
`security = "insecure:dev", insecureValue = "false"` (as camel-smooks does for
`allowExternalEntities`). Better still, give the parsing restriction its own
option, so that Saxon PE/EE users who turn `secureProcessing` off for Java
extension functions do not lose it without knowing.
##########
components/camel-xslt-saxon/src/main/java/org/apache/camel/component/xslt/saxon/XsltSaxonHelper.java:
##########
@@ -55,6 +68,64 @@ public static void
configureSecureProcessing(TransformerFactoryImpl factory, boo
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
secureProcessing);
}
+ /**
+ * Restricts the external resource access of the given factory to the
empty set, so that stylesheet compilation and
+ * the {@code document()} function do not reach external resources. This
is the same configuration the
+ * {@code xslt-saxon} endpoint applies to the factory it creates; it is
shared so the endpoint and
+ * {@link XsltSaxonAggregationStrategy} cannot drift apart.
+ */
+ public static void configureExternalAccess(TransformerFactory factory) {
+ try {
+ factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+ } catch (IllegalArgumentException e) {
+ LOG.debug("TransformerFactory does not support {}",
XMLConstants.ACCESS_EXTERNAL_DTD);
+ }
+ try {
+ factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
+ } catch (IllegalArgumentException e) {
+ LOG.debug("TransformerFactory does not support {}",
XMLConstants.ACCESS_EXTERNAL_STYLESHEET);
+ }
+ }
+
+ /**
+ * Returns a source whose parsing does not resolve references to external
resources, for an input that is still to
+ * be parsed. The factory's {@code ACCESS_EXTERNAL_*} attributes do not
cover this parse, because the transformer
+ * factory has a {@link javax.xml.transform.URIResolver} installed that
resolves the reference itself, so the input
+ * is parsed with a dedicated reader instead.
+ * <p>
+ * A {@link SAXSource} that already carries its own {@link XMLReader}, and
any source that is not stream-based (for
+ * example a {@code DOMSource} or a {@code StAXSource}), is returned
unchanged; its content has already been parsed
+ * or is read through a reader the caller controls. Only a stream-based
source still to be parsed
+ * ({@code StreamSource}, including Camel's {@code StringSource}/{@code
BytesSource}, or a reader-less
+ * {@code SAXSource}) is wrapped with the restricted reader.
+ */
+ public static Source secureSource(Source source) {
+ if (source == null) {
+ return null;
+ }
+ if (source instanceof SAXSource saxSource && saxSource.getXMLReader()
!= null) {
+ return source;
+ }
+ InputSource inputSource = SAXSource.sourceToInputSource(source);
+ if (inputSource == null) {
+ return source;
+ }
+ try {
+ SAXParserFactory factory = SAXParserFactory.newInstance();
+ factory.setNamespaceAware(true);
+ factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ factory.setFeature(EXTERNAL_GENERAL_ENTITIES, false);
+ factory.setFeature(EXTERNAL_PARAMETER_ENTITIES, false);
+ factory.setFeature(LOAD_EXTERNAL_DTD, false);
+ XMLReader xmlReader = factory.newSAXParser().getXMLReader();
+ return new SAXSource(xmlReader, inputSource);
+ } catch (ParserConfigurationException | SAXException e) {
+ LOG.warn("Cannot create a restricted XMLReader for the input
source, transforming it as is: {}",
Review Comment:
This fails open: if the `SAXParserFactory` on the classpath does not
recognise one of the features (for example `load-external-dtd` is
Xerces-specific), every exchange logs a WARN and the body is transformed with
no restriction. Since this is the security control, I would rather fail (throw)
than degrade without anyone noticing.
Also, `SAXParserFactory.newInstance()` runs a `FactoryFinder` lookup for
every exchange. Consider creating and configuring the factory once, so a
missing feature fails at startup, and then call only
`newSAXParser().getXMLReader()` for each exchange.
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:
##########
@@ -871,6 +871,26 @@ expanded, and the declaration itself is not passed on to
Smooks.
Set `allowExternalEntities=true` on the endpoint or on the data format for
documents that rely on external entities or
an external DTD.
+=== camel-xslt-saxon - external resource access is restricted for a Source
message body
+
+`XsltSaxonAggregationStrategy` now configures its Saxon `TransformerFactory`
the same way the `xslt-saxon`
+endpoint configures the factory it creates: `FEATURE_SECURE_PROCESSING` is
enabled and `ACCESS_EXTERNAL_DTD`
+and `ACCESS_EXTERNAL_STYLESHEET` are set to an empty string. Previously the
strategy used the Saxon defaults,
+so the same stylesheet ran with different settings depending on whether it was
used from the endpoint or from
+the strategy.
+
+In addition, both the strategy and the `xslt-saxon` endpoint now parse a
message body that is already a
+`javax.xml.transform.Source` still to be parsed, for example a `StreamSource`
or the body after a
+`convertBodyTo(Source.class)`, with a reader that does not resolve references
to external resources declared
+in the document. A `DOMSource`, a `StAXSource`, or a `Source` that already
carries its own reader is left
+unchanged, and a declaration internal to the document is still applied. The
other body types (`String`,
+`byte[]`, `InputStream`) are unaffected, as they already reach the transformer
through Camel's StAX/SAX
+conversion.
+
+On the `xslt-saxon` endpoint, set `secureProcessing=false` to restore the
previous parsing. A route that needs
Review Comment:
For the strategy, `setTransformerFactory(...)` brings back the old factory
settings but not the old `Source` parsing: `XsltSaxonBuilder` still wraps the
body, since its `secureProcessing` stays `true`. Please either say that the
strategy has no switch for the parsing, or add one. A sentence that
`FEATURE_SECURE_PROCESSING` disables reflexive Java extension functions on
Saxon PE/EE would also help users of the aggregation strategy.
--
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]
