davsclaus commented on code in PR #26344: URL: https://github.com/apache/camel/pull/26344#discussion_r4064940184
########## 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); + } + + // The hardened Configuration must fail closed on the DOCTYPE (or at least never resolve the external + // entity); either way the contents of the local file must not leak into the result or the error. + assertFalse(outcome.contains(SECRET), "External entity was resolved - local file content leaked"); + } + + @Test + public void testBenignSourceBodyStillEvaluates() { Review Comment: Optional: a `SAXSource` case would pin down that Saxon applies the `ParseOptions` features to a caller-supplied `XMLReader` as well (it does today — see `ActiveSAXSource.deliver()` in Saxon 13), so a future Saxon upgrade can't silently regress it. This is the probe I used to verify it; it passes with this PR and fails on `main`: ```java @Test public void testExternalEntityIsNotResolvedForSaxSourceWithCallerXmlReader() 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>"; // a plain, non-hardened JDK XMLReader supplied by the caller SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setNamespaceAware(true); XMLReader reader = factory.newSAXParser().getXMLReader(); Exchange exchange = new DefaultExchange(context); exchange.getIn().setBody(new SAXSource(reader, new InputSource(new StringReader(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); } assertFalse(outcome.contains(SECRET), "External entity was resolved via SAXSource - local file content leaked"); } ``` Also optional: since Saxon does fail closed (`SXXP0003`), an `assertThrows` would make the intended behaviour explicit — but the lenient "never leaks" assertion is a fair trade-off across Saxon versions, so fine as is. ########## 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: Please **don't** apply gnodet-bot's suggestion above — the `SAXSource` limitation it describes doesn't exist. Saxon 13.0's `ActiveSAXSource.deliver()` applies the `ParseOptions` parser features to a caller-supplied `XMLReader` too (and restores them afterwards), and I verified it empirically: a plain JDK `XMLReader` wrapped in a `SAXSource` with the XXE payload is rejected with `SXXP0003` on this branch and leaks on `main`. Documenting a gap that isn't there would send users off to harden something already covered. Separately (optional): the `xquery` component/language pages don't mention the secure default or the `configuration` escape hatch, the way `camel-xslt-saxon` documents `secureProcessing`. A one-liner in `xquery-language.adoc` pointing here would help, but the upgrade guide already covers it. -- 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]
