oscerd commented on code in PR #26904:
URL: https://github.com/apache/camel/pull/26904#discussion_r4119960020
##########
components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltUriResolver.java:
##########
@@ -115,4 +172,21 @@ public Source resolve(String href, String base) throws
TransformerException {
}
}
+ /**
+ * Tells whether the configured {@code ACCESS_EXTERNAL_STYLESHEET}
restriction forbids resolving the given scheme.
+ * JAXP applies that attribute only when no custom {@link URIResolver}
returns a {@link Source}, and Camel always
+ * installs this resolver, so it must apply the same limit itself. Only
the standard external protocols are
+ * governed; Camel's {@code classpath:}, {@code ref:} and {@code bean:}
schemes are internal lookups outside the
+ * JAXP model and are always resolved. Always {@code false} when external
access is unrestricted
+ * ({@code allowedExternalProtocols == null}).
+ */
+ private boolean isExternalAccessDenied(String scheme) {
+ if (allowedExternalProtocols == null) {
+ return false;
+ }
+ // scheme carries a trailing ':' (e.g. "http:")
+ String protocol = scheme.endsWith(":") ? scheme.substring(0,
scheme.length() - 1) : scheme;
+ return EXTERNAL_PROTOCOLS.contains(protocol) &&
!allowedExternalProtocols.contains(protocol);
Review Comment:
Good defensive point — done in cb0625aa. `checkExternalAccessAllowed` now
lowercases the protocol with `toLowerCase(Locale.ROOT)` before the allow-list
comparison. As you noted it isn't reachable today (`ResourceHelper.getScheme`
is case-sensitive, so `FILE:/…` isn't recognised as a scheme at all — it's
treated as a relative/classpath reference that then fails to resolve), but the
lowercasing keeps the guard robust if scheme detection ever changes.
_Claude Code on behalf of oscerd_
##########
components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltBuilder.java:
##########
@@ -73,6 +74,9 @@ public class XsltBuilder implements Processor {
private ResultHandlerFactory resultHandlerFactory = new
StringResultHandlerFactory();
private boolean failOnNullBody = true;
private URIResolver uriResolver;
+ // the resolver installed on the transformer for runtime document()
resolution; enforces the factory's
+ // ACCESS_EXTERNAL_STYLESHEET restriction while stylesheet compilation
(xsl:include/import) stays unrestricted
+ private volatile URIResolver runtimeUriResolver;
Review Comment:
Done in cb0625aa — `setUriResolver()` now resets `runtimeUriResolver` to
`null`, so a resolver set after the first exchange is picked up instead of the
cached wrapper.
_Claude Code on behalf of oscerd_
##########
components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltUriResolver.java:
##########
@@ -74,6 +123,14 @@ public Source resolve(String href, String base) throws
TransformerException {
String scheme = ResourceHelper.getScheme(href);
if (scheme != null) {
+ if (isExternalAccessDenied(scheme)) {
+ // refuse the external resource by returning an empty
document; document() then yields an empty
+ // node-set rather than the resource content, and the
processor does not fall back to its own resolver
+ LOG.warn("Refusing to resolve external resource {} for the
XSLT document() function: it is not permitted"
+ + " by the transformer factory's
ACCESS_EXTERNAL_STYLESHEET restriction",
+ href);
+ return new StreamSource(new StringReader(DENIED_DOCUMENT));
Review Comment:
Agreed and changed in cb0625aa — the resolver now throws a
`TransformerException` for a denied external protocol instead of returning an
empty document. I verified the XSLTC behaviour you described:
`TransformerImpl.retrieveDocument` catches the resolver exception and returns
`null`, and `LoadDocument` then fails the transform without reading the
resource (confirmed end-to-end against an existing absolute `file:` target —
the file is not read). Since XSLTC surfaces its own document-retrieval error
rather than the resolver's message, I also log the denial as a WARN so the
reason stays diagnosable.
_Claude Code on behalf of oscerd_
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc:
##########
@@ -2921,3 +2921,28 @@ This is the intended behaviour and makes the `keepOpen`
toggle deterministic, bu
relied on the setter being inert until an exchange arrived must be aware of
the change.
Setting `keepOpen` back to `false` remains deferred: the half-open timer
attempts to close the circuit on
its next tick (after `halfOpenAfter` milliseconds, default 30 s).
+
+=== camel-xslt - external document() access is denied by default
Review Comment:
Done in cb0625aa — moved the entry into the "Upgrading Camel 4.22 to 4.23"
section (it was rendering under `== ThrottlingExceptionRoutePolicy`).
Documented that `camel-xslt-saxon` is affected the same way
(`XsltSaxonEndpoint` sets the same deny-all attribute, Saxon reports it via
`getAttribute`, `XsltSaxonBuilder extends XsltBuilder`) and ran its suite —
green apart from an unrelated pre-existing flake
(`XsltSaxonJsonBodyTest.testJsonBodyDisabled`, which also fails on `main`
without this change). The relative-`document()`-in-a-`file:`-stylesheet case
(point 2) is now called out in both the upgrade guide and the component page
(classpath noted unaffected) and pinned by
`XsltDocumentExternalAccessTest#relativeDocumentInAFileStylesheetIsDeniedByDefault`.
_Claude Code on behalf of oscerd_
--
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]
