gnodet-bot commented on code in PR #26904:
URL: https://github.com/apache/camel/pull/26904#discussion_r4105012674
##########
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:
⚠️ **Defense-in-depth: case-insensitive protocol comparison.**
`EXTERNAL_PROTOCOLS` contains lowercase entries (`"http"`, `"file"`, etc.), but
`ResourceHelper.getScheme()` preserves the original casing from the `href`. If
an attacker passes `FILE:/etc/passwd` or `Http://evil.com`, the extracted
protocol would be `FILE` or `Http`, which won't match the lowercase set —
`isExternalAccessDenied` returns `false` and the href proceeds to resolution.
In practice, Camel's resource loading likely fails for uppercase schemes (it
checks `"file:".equals(scheme)` etc.), so this is not an actively exploitable
bypass today. But for a security guard, case-insensitive comparison is the
right defensive choice — it prevents any future resource loader change from
accidentally creating a bypass.
```suggestion
String protocol = scheme.endsWith(":") ? scheme.substring(0,
scheme.length() - 1) : scheme;
return
EXTERNAL_PROTOCOLS.contains(protocol.toLowerCase(java.util.Locale.ROOT)) &&
!allowedExternalProtocols.contains(protocol.toLowerCase(java.util.Locale.ROOT));
```
##########
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:
💡 **Nit: stale cache when `setUriResolver` is called.** `runtimeUriResolver`
is lazily computed from `uriResolver`, but `setUriResolver()` (line 408) does
not reset `runtimeUriResolver` to `null`. If `setUriResolver()` were ever
called after the first exchange, the cached `runtimeUriResolver` would still
wrap the old resolver.
In practice this doesn't bite today — `setUriResolver()` is only called
during endpoint initialization, before any exchange is processed. But adding
`this.runtimeUriResolver = null;` to `setUriResolver()` would make the contract
explicit and prevent a subtle bug if the lifecycle ever changes.
--
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]
