From d0ca4c49f7375d49357aaa94329a1efe78abff11 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sat, 11 Mar 2017 23:39:12 +0100 Subject: [PATCH] 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 --- .../listeners/AbstractQueryParams.java | 59 ++++++++++++------- .../impl/test/ExpressionParserTest.java | 16 ++--- 2 files changed, 44 insertions(+), 31 deletions(-) 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 f7a72bca4b..df9705cf76 100644 --- 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 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 ab14b63808..2551e3ee33 100644 --- 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(); } } -- 2.36.6