Improve AbstractQueryParams XML parsing

Use a shared, hardened, DocumentBuilderFactory and do not pass
the XML via a field but rather as an argument.

Change-Id: Id4a1792598b7d202149e62de6fc8292308c2c170
Signed-off-by: Robert Varga <rovarga@cisco.com>

diff --git a/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/streams/listeners/AbstractQueryParams.java b/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/streams/listeners/AbstractQueryParams.java
index f7a72bca4b6728383ae3f694e0f1a8cbb9c9dd2a..df9705cf762aeaac1d4bf9cf04f6361ebbe77473 100644 (file)
--- a/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/streams/listeners/AbstractQueryParams.java
+++ b/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/streams/listeners/AbstractQueryParams.java
@@ -9,8 +9,9 @@ package org.opendaylight.netconf.sal.streams.listeners;
 
 import java.io.StringReader;
 import java.util.Date;
-import javax.xml.parsers.DocumentBuilder;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
 import javax.xml.xpath.XPathFactory;
@@ -23,12 +24,30 @@ import org.xml.sax.InputSource;
  *
  */
 abstract class AbstractQueryParams extends AbstractNotificationsData {
+    // FIXME: BUG-7956: switch to using UntrustedXML
+    private static final DocumentBuilderFactory DBF;
+    static {
+        final DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
+        f.setCoalescing(true);
+        f.setExpandEntityReferences(false);
+        f.setIgnoringElementContentWhitespace(true);
+        f.setIgnoringComments(true);
+        f.setXIncludeAware(false);
+        try {
+            f.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+            f.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            f.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        } catch (final ParserConfigurationException e) {
+            throw new ExceptionInInitializerError(e);
+        }
+        DBF = f;
+    }
 
-    protected Date start = null;
-    protected Date stop = null;
-    protected String filter = null;
-
-    private String xml;
+    // FIXME: these should be final
+    private Date start = null;
+    private Date stop = null;
+    private String filter = null;
 
     /**
      * Set query parameters for listener
@@ -57,11 +76,10 @@ abstract class AbstractQueryParams extends AbstractNotificationsData {
      *         false otherwise
      */
     protected <T extends BaseListenerInterface> boolean checkQueryParams(final String xml, final T listener) {
-        this.xml = xml;
         final Date now = new Date();
         if (this.stop != null) {
             if ((this.start.compareTo(now) < 0) && (this.stop.compareTo(now) > 0)) {
-                return checkFilter();
+                return checkFilter(xml);
             }
             if (this.stop.compareTo(now) < 0) {
                 try {
@@ -73,10 +91,10 @@ abstract class AbstractQueryParams extends AbstractNotificationsData {
         } else if (this.start != null) {
             if (this.start.compareTo(now) < 0) {
                 this.start = null;
-                return checkFilter();
+                return checkFilter(xml);
             }
         } else {
-            return checkFilter();
+            return checkFilter(xml);
         }
         return false;
     }
@@ -87,15 +105,15 @@ abstract class AbstractQueryParams extends AbstractNotificationsData {
      * @param change
      *            - data of notification
      */
-    private boolean checkFilter() {
+    private boolean checkFilter(final String xml) {
         if (this.filter == null) {
             return true;
-        } else {
-            try {
-                return parseFilterParam();
-            } catch (final Exception e) {
-                throw new RestconfDocumentedException("Problem while parsing filter.", e);
-            }
+        }
+
+        try {
+            return parseFilterParam(xml);
+        } catch (final Exception e) {
+            throw new RestconfDocumentedException("Problem while parsing filter.", e);
         }
     }
 
@@ -106,11 +124,10 @@ abstract class AbstractQueryParams extends AbstractNotificationsData {
      *         notifiaction
      * @throws Exception
      */
-    private boolean parseFilterParam() throws Exception {
-        final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-        final DocumentBuilder builder = factory.newDocumentBuilder();
-        final Document docOfXml = builder.parse(new InputSource(new StringReader(this.xml)));
+    private boolean parseFilterParam(final String xml) throws Exception {
+        final Document docOfXml = DBF.newDocumentBuilder().parse(new InputSource(new StringReader(xml)));
         final XPath xPath = XPathFactory.newInstance().newXPath();
+        // FIXME: BUG-7956: xPath.setNamespaceContext(nsContext);
         return (boolean) xPath.compile(this.filter).evaluate(docOfXml, XPathConstants.BOOLEAN);
     }
 }

diff --git a/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/ExpressionParserTest.java b/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/ExpressionParserTest.java
index ab14b638080160d0546f4785dc1d1cb0ba458526..2551e3ee3316db03e8bf15da7f58de139997d180 100644 (file)
--- a/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/ExpressionParserTest.java
+++ b/restconf/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/ExpressionParserTest.java
@@ -10,7 +10,7 @@ package org.opendaylight.controller.sal.restconf.impl.test;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileReader;
-import java.lang.reflect.Field;
+import java.io.IOException;
 import java.lang.reflect.Method;
 import java.util.Collection;
 import org.junit.Assert;
@@ -134,6 +134,8 @@ public class ExpressionParserTest {
         Mockito.when(path.getLastPathArgument()).thenReturn(pathValue);
         final ListenerAdapter listener = Notificator.createListener(path, "streamName", NotificationOutputType.JSON);
         listener.setQueryParams(null, null, filter);
+
+        // FIXME: do not use reflection here
         final Class<?> superclass = listener.getClass().getSuperclass().getSuperclass();
         Method m = null;
         for (final Method method : superclass.getDeclaredMethods()) {
@@ -145,15 +147,11 @@ public class ExpressionParserTest {
             throw new Exception("Methode parseFilterParam doesn't exist in " + superclass.getName());
         }
         m.setAccessible(true);
-        final Field xmlField = superclass.getDeclaredField("xml");
-        xmlField.setAccessible(true);
-        xmlField.set(listener, readFile(xml));
-        return (boolean) m.invoke(listener, null);
+        return (boolean) m.invoke(listener, readFile(xml));
     }
 
-    private String readFile(final File xml) throws Exception {
-        final BufferedReader br = new BufferedReader(new FileReader(xml));
-        try {
+    private static String readFile(final File xml) throws IOException {
+        try (final BufferedReader br = new BufferedReader(new FileReader(xml))) {
             final StringBuilder sb = new StringBuilder();
             String line = br.readLine();
 
@@ -163,8 +161,6 @@ public class ExpressionParserTest {
                 line = br.readLine();
             }
             return sb.toString();
-        } finally {
-            br.close();
         }
     }