gnodet-bot commented on code in PR #26344: URL: https://github.com/apache/camel/pull/26344#discussion_r4063828428
########## components/camel-saxon/src/test/java/org/apache/camel/component/xquery/XQueryXxeTest.java: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.xquery; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.camel.Exchange; +import org.apache.camel.support.DefaultExchange; +import org.apache.camel.test.junit6.CamelTestSupport; +import org.apache.camel.util.xml.StringSource; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * Verifies that {@link XQueryBuilder} does not resolve XML external entities when the message body already arrives as a + * {@link javax.xml.transform.Source} (which bypasses Camel's hardened SAX/StAX type converters and is handed straight + * to Saxon). + */ +public class XQueryXxeTest extends CamelTestSupport { + + private static final String SECRET = "CANARY-XQUERY-XXE-do-not-disclose"; + + @TempDir + Path tempDir; + + @Override + public boolean isUseRouteBuilder() { + return false; + } + + @Test + public void testExternalEntityIsNotResolvedForSourceBody() throws Exception { + Path secret = Files.writeString(tempDir.resolve("secret.txt"), SECRET); + + String payload = "<?xml version=\"1.0\"?>\n" + + "<!DOCTYPE data [ <!ENTITY xxe SYSTEM \"" + secret.toUri() + "\"> ]>\n" + + "<order status=\"pending\">&xxe;</order>"; + + Exchange exchange = new DefaultExchange(context); + // a Source-typed body is returned unchanged by getSource() and parsed directly by Saxon + exchange.getIn().setBody(new StringSource(payload)); + + XQueryBuilder xquery = XQueryBuilder.xquery("//order").asString(); + xquery.init(context); + + String outcome; + try { + outcome = String.valueOf(xquery.evaluate(exchange)); + } catch (Exception e) { + outcome = stackTraceOf(e); Review Comment: ⚠️ **Coverage gap — `SAXSource` with pre-wired `XMLReader` is not tested.** The test covers `StringSource` (a `StreamSource` subtype), which is the primary attack path. But `getSource()` can also return a `SAXSource` (it's the second preference after StAX). When the body is a `SAXSource` wrapping an `XMLReader` that the caller already configured, Saxon's `buildDocumentTree` passes that reader through directly — `ParseOptions.withParserFeature(...)` is **not applied** to a pre-existing `XMLReader`. The XXE fix therefore has no effect on that path. Consider adding a second test case that constructs a `SAXSource` directly and verifies the same outcome, or at minimum add a comment explaining the limitation. If the `SAXSource` path is out of scope, a note in the class-level Javadoc would suffice. ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc: ########## @@ -2365,3 +2365,21 @@ it had no effect. The option is kept for backward compatibility of existing endp deprecated and will be removed in a future release. Routes that set `maxRetryTimeout` can simply drop it; behaviour is unchanged. + +=== camel-saxon - external XML entity resolution disabled by default in XQuery + +The XQuery language and the `xquery` component now build their default Saxon `Configuration` with a +hardened XML parser that does not accept a `DOCTYPE` declaration and does not resolve external +general or parameter entities or external DTDs. This aligns XQuery with the parser configuration +already applied to the `String`, `byte[]` and `InputStream` body paths through Camel's +`XmlConverter`, and with `camel-xslt-saxon` (which already defaults `secureProcessing` to `true`). + +Previously, when a message body reached XQuery as an already-built `javax.xml.transform.Source` (for +example after a `convertBodyTo(Source.class)`), it was parsed with the Saxon parser defaults, which +resolved external entities. A body carrying a `DOCTYPE` declaration on that path is now rejected with +a parse error. Bodies without a `DOCTYPE` are unaffected. + +A deployment that genuinely needs to parse documents with a `DOCTYPE` or external entities can supply Review Comment: 💡 **Suggestion:** Add a sentence about the `SAXSource`-with-XMLReader limitation so users who construct that body type know they are responsible for their reader's security settings. ```suggestion A deployment that genuinely needs to parse documents with a `DOCTYPE` or external entities can supply its own pre-configured Saxon `Configuration` through the `configuration` option of the `xquery` endpoint or language (or `XQueryBuilder.setConfiguration(...)`); a user-supplied `Configuration` is used as-is and is not modified. Note: when a message body arrives as a `SAXSource` with a pre-configured `XMLReader`, Saxon uses that reader as-is; the hardened `ParseOptions` are not applied to a reader the caller already owns. In that case, the caller is responsible for configuring the reader's entity-resolution settings. ``` -- 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]
