Repository navigation
Conversation
… a Source message body XsltSaxonAggregationStrategy created its Saxon TransformerFactory with the Saxon defaults, so the same stylesheet ran with different settings depending on whether it was used from the strategy or from the xslt-saxon endpoint. The strategy now configures its factory the same way the endpoint configures the factory it creates (FEATURE_SECURE_PROCESSING enabled, ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_STYLESHEET set to an empty string), through a shared XsltSaxonHelper method the endpoint now calls too. A message body that is already a javax.xml.transform.Source still to be parsed (for example a StreamSource, or the body after convertBodyTo(Source.class)) reaches Saxon without the StAX/SAX conversion the other body types go through. The strategy and the xslt-saxon endpoint now parse such a body 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 unchanged, and a declaration internal to the document is still applied. On the endpoint this follows the existing secureProcessing option. Adds tests for the strategy and the shared helper, updates XsltSaxonDomSourceTest for the new Source handling, and documents the change in the 4.24 upgrade guide. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔁 3 tests passed only after a retry on JDK 25 (3 retried attempts) Recovered flaky tests on JDK 25 (3)
🔬 Scalpel shadow comparison — Scalpel: 10 of 707 tested, 26 compile-only — current: 10 all testedMaveniverse Scalpel detected 10 affected modules (current approach: 10). Skip-tests mode would test 10 modules (2 direct + 9 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (10)
Modules with tests skipped (26)
All tested modules (37 modules, 6m 14s total)Total reactor time: 6m 14s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the parity with the endpoint factory is welcome, and sharing configureExternalAccess(...) is a good way to stop the two from drifting apart. A few points on how the restriction is switched off and on what happens when it fails:
secureProcessingnow also controls the input-parsing restriction, but the option is not marked as a security knob. OnXsltSaxonEndpoint(@UriParam) andXsltSaxonComponent(@Metadata) it should getlabel = "security"(oradvanced,security),security = "insecure:dev"andinsecureValue = "false", socamel.main.profile=prodreportssecureProcessing=false. This is the same pattern asallowExternalEntitieson camel-smooks in the same 4.24 guide. The catalog and DSL then need regenerating.- Linked to 1: the javadoc of
secureProcessingtells Saxon PE/EE users to turn it off to use Java extension functions. With this PR, doing that also brings back unrestricted parsing ofSourcebodies, and nothing tells them so. Could the parsing restriction have its own switch (like smooks'allowExternalEntities, defaultfalse,insecure:dev)? At least the option doc and the upgrade guide should say thatsecureProcessing=falsenow does both things. secureSource(...)fails open (see inline). If the restricted reader cannot be built, it logs a WARN on every exchange and transforms the body without the restriction.XsltSaxonAggregationStrategyhas no opt-out for the newSourceparsing.setTransformerFactory(...)only replaces the factory, whileXsltSaxonBuilderkeepssecureProcessing=true, so the wrapping still happens. The upgrade guide (inline) reads as if a custom factory brings back the old behaviour. Please either reword it or expose a setter on the strategy. It is also worth one sentence thatFEATURE_SECURE_PROCESSINGon a Saxon PE/EE factory disables reflexive Java extension functions in aggregation stylesheets.- Test gap: the strategy now runs with
ACCESS_EXTERNAL_STYLESHEET="", but no test covers an aggregation stylesheet that usesxsl:include/xsl:importfrom the classpath. The endpoint has this (SaxonXsltFeatureRouteTest), and the strategy should have it too, so a classpath include in an aggregation stylesheet stays covered.
Parity with camel-xslt looks fine: the JDK XSLTC factory that XmlConverter creates sets FEATURE_SECURE_PROCESSING, which already sets ACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_STYLESHEET to empty for the input parse, so nothing is needed there.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| 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: {}", |
There was a problem hiding this 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.
| xslt.setEntityResolver(getEntityResolver()); | ||
| xslt.setAllowStAX(allowStAX); | ||
| xslt.setUseJsonBody(useJsonBody); | ||
| xslt.setSecureProcessing(secureProcessing); |
There was a problem hiding this 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.
| `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 |
There was a problem hiding this 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.
|
@oscerd there is a conflict on the upgrade guide |
3 participants
CAMEL-25827: camel-xslt-saxon - restrict external resource access for a Source message body
Problem
XsltSaxonAggregationStrategybuilds its SaxonTransformerFactorywith the Saxon defaults: it doesnot enable
FEATURE_SECURE_PROCESSINGand does not setACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_STYLESHEET. Thexslt-saxonendpoint has configured those on the factory it createssince 4.22.0, so the same stylesheet ran with different settings depending on whether it was used from
an endpoint or from the aggregation strategy.
Separately, a message body that is already a
javax.xml.transform.Sourcestill to be parsed (forexample a
StreamSource, or the body after aconvertBodyTo(Source.class)) reaches Saxon without theStAX/SAX conversion that
String,byte[]andInputStreambodies go through. The transformer factoryhas a
URIResolverinstalled that resolves references the document declares to other resources, so thefactory's
ACCESS_EXTERNAL_*attributes do not cover that parse: the reference is resolved from thebody. This affects the aggregation strategy and the
xslt-saxonendpoint (andcamel-xj, which extendsit).
Change
XsltSaxonHelper.configureExternalAccess(...)(theACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_STYLESHEETblock the endpoint had inline) andXsltSaxonHelper.secureSource(...)(wraps a stream-based
Sourcestill to be parsed with a reader that does not resolve references toexternal resources, leaving a
DOMSource, aStAXSource, or a reader-backedSourceunchanged).XsltSaxonAggregationStrategynow enables secure processing and callsconfigureExternalAccess(...)on its factory, so it is configured like the endpoint's.
SaxonXmlSourceHandlerFactoryImplruns the resolved source throughsecureSource(...)when secureprocessing is on, so a
Sourcebody is parsed with the restricted reader.XsltSaxonBuilderthreadsthe flag through, and
XsltSaxonEndpointpasses its existingsecureProcessingoption, sosecureProcessing=falserestores the previous parsing on the endpoint.configureExternalAccess(...)instead of its inline block, so theendpoint and the strategy cannot drift apart.
A route that needs
XsltSaxonAggregationStrategyto use a differentTransformerFactorycan still setits own with
setTransformerFactory(...).Tests
XsltSaxonAggregationStrategySourceBodyTest- aggregates aSourcebody and checks that a resourcenamed by the body is not read into the result, while a declaration internal to the document is still
applied.
XsltSaxonHelperConfigurationTest- unit tests forconfigureExternalAccess(...)andsecureSource(...)(stream-based source wrapped with a restricted reader; an already-parsed sourceleft unchanged).
XsltSaxonDomSourceTestupdated for the newSourcehandling (a stream-based body is now wrappedwith a restricted reader;
DOMSource/StAXSourcepass through).components/camel-xslt-saxonandcomponents/camel-xjtest suites pass.The change is documented in
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc.Claude Code on behalf of oscerd
🤖 Generated with Claude Code