zabetak commented on code in PR #2892:
URL: https://github.com/apache/calcite/pull/2892#discussion_r962695246
##########
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##########
@@ -215,6 +238,27 @@ private static String convertNodeToString(Node node)
throws TransformerException
return writer.toString();
}
+ private static Node getDocumentNode(final String xml) {
+ final DocumentBuilderFactory documentBuilderFactory =
DocumentBuilderFactory.newInstance();
Review Comment:
Do we need to create a factory on every call to this method? Since the
configuration does not change it may be safe to reuse the instance (per
application or per thread).
##########
core/src/test/java/org/apache/calcite/test/SqlXmlFunctionsTest.java:
##########
@@ -36,6 +36,18 @@
*/
class SqlXmlFunctionsTest {
+ private static final String XML = "<document>string</document>";
+ private static final String FILE =
System.getProperty("os.name").startsWith("Windows")
+ ? "file:///C:/Windows/System32/drivers/etc/hosts"
+ : "file:///etc/hosts";
Review Comment:
Is it necessary to use an existing file and make the tests OS dependent? I
suppose in the test we could create a tmp file/dir and work on it.
Moreover it seems a bit risky to attempt to modify system files like
`/etc/hosts` even if it is for testing. It would be pretty bad if for some
reason Calcite modifies `/etc/hosts` while I am running the tests on my machine.
##########
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##########
@@ -215,6 +238,27 @@ private static String convertNodeToString(Node node)
throws TransformerException
return writer.toString();
}
+ private static Node getDocumentNode(final String xml) {
+ final DocumentBuilderFactory documentBuilderFactory =
DocumentBuilderFactory.newInstance();
+ documentBuilderFactory.setXIncludeAware(false);
+ documentBuilderFactory.setExpandEntityReferences(false);
+ documentBuilderFactory.setNamespaceAware(true);
+ try {
+
documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ documentBuilderFactory
+ .setFeature("http://apache.org/xml/features/disallow-doctype-decl";,
true);
+ } catch (final ParserConfigurationException e) {
+ throw new IllegalArgumentException("Document Builder configuration
failed", e);
Review Comment:
The exception does not depend on the arguments of the method neither on
configurations from the user thus it is a bit misleading to use
`IllegalArgumentException`. `IllegalStateException` seems more appropriate here.
##########
core/src/main/java/org/apache/calcite/runtime/XmlFunctions.java:
##########
@@ -60,11 +67,24 @@
public class XmlFunctions {
private static final ThreadLocal<@Nullable XPathFactory> XPATH_FACTORY =
- ThreadLocal.withInitial(XPathFactory::newInstance);
+ ThreadLocal.withInitial(() -> {
+ final XPathFactory xPathFactory = XPathFactory.newInstance();
+ try {
+ xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING,
true);
+ } catch (XPathFactoryConfigurationException e) {
+ throw new IllegalStateException("XPath Factory configuration
failed", e);
Review Comment:
I am wondering if we should make this an unrecoverable error or not. If we
leave it as is then we are breaking applications that cannot create a "secure"
XPathFactory. Alternatively we could catch the exception and issue a
`LOG.warn`. Not sure what is the best approach.
(same comment applies to a few other places in this PR)
--
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]
