diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 9113045788c..2a6d26cee7a 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -177,6 +177,8 @@ Other Changes * SOLR-16324: Upgrade commons-configuration2 to 2.8.0 and commons-text to 1.8 (Kevin Risden) +* SOLR-16296: XmlConfigFile and QueryElevationComponent now use SafeXMLParsing. (Haythem Khiri) + Build --------------------- * SOLR-16204: Change Lucene dependency to Lucene 9.1.0 (Elia Porciani via Alessandro Benedetti) diff --git a/solr/core/src/java/org/apache/solr/core/XmlConfigFile.java b/solr/core/src/java/org/apache/solr/core/XmlConfigFile.java index 1e88fa9bf38..080e2ee0bd6 100644 --- a/solr/core/src/java/org/apache/solr/core/XmlConfigFile.java +++ b/solr/core/src/java/org/apache/solr/core/XmlConfigFile.java @@ -22,6 +22,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.Set; import java.util.SortedMap; @@ -30,18 +31,14 @@ import java.util.TreeSet; import java.util.function.Function; import javax.xml.namespace.QName; -import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; import javax.xml.xpath.XPath; import javax.xml.xpath.XPathConstants; import javax.xml.xpath.XPathExpressionException; import javax.xml.xpath.XPathFactory; -import org.apache.commons.io.IOUtils; import org.apache.solr.cloud.ZkSolrResourceLoader; import org.apache.solr.common.SolrException; import org.apache.solr.common.util.DOMUtil; -import org.apache.solr.common.util.XMLErrorLogger; +import org.apache.solr.util.SafeXMLParsing; import org.apache.solr.util.SystemIdResolver; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,37 +56,23 @@ */ public class XmlConfigFile { // formerly simply "Config" private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - private static final XMLErrorLogger xmllog = new XMLErrorLogger(log); static final XPathFactory xpathFactory = XPathFactory.newInstance(); private final Document doc; - private final Document origDoc; // with unsubstituted properties private final String prefix; private final String name; private final SolrResourceLoader loader; private final Properties substituteProperties; private int zkVersion = -1; - /** Builds a config from a resource name with no xpath prefix. Does no property substitution. */ - public XmlConfigFile(SolrResourceLoader loader, String name) - throws ParserConfigurationException, IOException, SAXException { - this(loader, name, null, null); - } - - /** Builds a config. Does no property substitution. */ - public XmlConfigFile(SolrResourceLoader loader, String name, InputSource is, String prefix) - throws ParserConfigurationException, IOException, SAXException { - this(loader, name, is, prefix, null); - } - public XmlConfigFile( SolrResourceLoader loader, String name, InputSource is, String prefix, Properties substituteProps) - throws ParserConfigurationException, IOException, SAXException { + throws IOException { this( loader, s -> { @@ -106,7 +89,7 @@ public XmlConfigFile( } /** - * Builds a config: + * Builds a config. * *

Note that the 'name' parameter is used to obtain a valid input stream if no valid one is * provided through 'is'. If no valid stream is provided, a valid SolrResourceLoader instance @@ -131,16 +114,13 @@ public XmlConfigFile( String prefix, Properties substituteProps) throws IOException { - if (null == loader) throw new NullPointerException("loader"); - this.loader = loader; - + this.loader = Objects.requireNonNull(loader); this.substituteProperties = substituteProps; this.name = name; this.prefix = (prefix != null && !prefix.endsWith("/")) ? prefix + '/' : prefix; - try { - javax.xml.parsers.DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - if (is == null) { + try { + if (is == null && fileSupplier != null) { InputStream in = fileSupplier.apply(name); if (in instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) { zkVersion = ((ZkSolrResourceLoader.ZkByteArrayInputStream) in).getStat().getVersion(); @@ -150,34 +130,18 @@ public XmlConfigFile( is.setSystemId(SystemIdResolver.createSystemIdFromResourceName(name)); } - // only enable xinclude, if a SystemId is available - if (is.getSystemId() != null) { - try { - dbf.setXIncludeAware(true); - dbf.setNamespaceAware(true); - } catch (UnsupportedOperationException e) { - log.warn("{} XML parser doesn't support XInclude option", name); - } - } - - final DocumentBuilder db = dbf.newDocumentBuilder(); - db.setEntityResolver(new SystemIdResolver(loader)); - db.setErrorHandler(xmllog); - try { - doc = db.parse(is); - origDoc = doc; - } finally { - // some XML parsers are broken and don't close the byte stream (but they should according to - // spec) - IOUtils.closeQuietly(is.getByteStream()); - } - if (substituteProps != null) { - DOMUtil.substituteProperties(doc, getSubstituteProperties()); + if (is != null) { + doc = SafeXMLParsing.parseConfigXML(log, loader, is); + } else { + doc = SafeXMLParsing.parseConfigXML(log, loader, name); } - } catch (ParserConfigurationException | SAXException e) { + } catch (SAXException e) { SolrException.log(log, "Exception during parsing file: " + name, e); throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); } + if (substituteProps != null) { + DOMUtil.substituteProperties(doc, getSubstituteProperties()); + } } /* diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java index 6caf39f2bd8..711dd4d0cb8 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java @@ -44,6 +44,7 @@ import java.util.List; import java.util.Map; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.Queue; import java.util.Set; import java.util.SortedSet; @@ -79,7 +80,6 @@ import org.apache.solr.common.util.StrUtils; import org.apache.solr.core.SolrCore; import org.apache.solr.core.SolrResourceNotFoundException; -import org.apache.solr.core.XmlConfigFile; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.response.transform.ElevatedMarkerFactory; import org.apache.solr.response.transform.ExcludedMarkerFactory; @@ -90,11 +90,14 @@ import org.apache.solr.search.SortSpec; import org.apache.solr.search.grouping.GroupingSpecification; import org.apache.solr.util.RefCounted; +import org.apache.solr.util.SafeXMLParsing; import org.apache.solr.util.plugin.SolrCoreAware; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.w3c.dom.Document; import org.w3c.dom.Node; import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; /** * A component to elevate some documents to the top of the result set. @@ -376,10 +379,10 @@ protected long getConfigVersion(SolrCore core) { * * @return The loaded {@link ElevationProvider}; not null. */ - private ElevationProvider loadElevationProvider(SolrCore core) throws Exception { - XmlConfigFile cfg; + private ElevationProvider loadElevationProvider(SolrCore core) throws IOException, SAXException { + Document xmlDocument; try { - cfg = new XmlConfigFile(core.getResourceLoader(), configFileName); + xmlDocument = SafeXMLParsing.parseConfigXML(log, core.getResourceLoader(), configFileName); } catch (SolrResourceNotFoundException e) { String msg = "Missing config file \"" + configFileName + "\""; if (Files.exists(Path.of(core.getDataDir(), configFileName))) { @@ -402,9 +405,7 @@ private ElevationProvider loadElevationProvider(SolrCore core) throws Exception } throw e; } - ElevationProvider elevationProvider = loadElevationProvider(cfg); - assert elevationProvider != null; - return elevationProvider; + return Objects.requireNonNull(loadElevationProvider(xmlDocument)); } /** @@ -413,17 +414,20 @@ private ElevationProvider loadElevationProvider(SolrCore core) throws Exception * @throws RuntimeException If the config does not provide an XML content of the expected format * (either {@link RuntimeException} or {@link org.apache.solr.common.SolrException}). */ - protected ElevationProvider loadElevationProvider(XmlConfigFile config) { + protected ElevationProvider loadElevationProvider(Document doc) { + if (!doc.getDocumentElement().getNodeName().equals("elevate")) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "Root element must be "); + } Map elevationBuilderMap = new LinkedHashMap<>(); + NodeList nodes = doc.getDocumentElement().getElementsByTagName("query"); XPath xpath = XPathFactory.newInstance().newXPath(); - NodeList nodes = (NodeList) config.evaluate("elevate/query", XPathConstants.NODESET); for (int i = 0; i < nodes.getLength(); i++) { Node node = nodes.item(i); String queryString = DOMUtil.getAttr(node, "text", "missing query 'text'"); String matchString = DOMUtil.getAttr(node, "match"); ElevatingQuery elevatingQuery = new ElevatingQuery(queryString, isSubsetMatchPolicy(matchString)); - NodeList children; try { children = (NodeList) xpath.evaluate("doc", node, XPathConstants.NODESET); diff --git a/solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java b/solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java index d9bce120a06..783fa038ee8 100644 --- a/solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java +++ b/solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java @@ -21,6 +21,7 @@ import java.io.InputStream; import java.io.Reader; import java.io.StringReader; +import java.util.Objects; import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -51,36 +52,56 @@ public final class SafeXMLParsing { private SafeXMLParsing() {} /** - * Parses a config file from ResourceLoader. Xinclude and external entities are enabled, but - * cannot escape the resource loader. + * Parses a config file from a Solr config based on ResourceLoader. Xinclude and external entities + * are enabled, but cannot escape the resource loader. */ public static Document parseConfigXML(Logger log, ResourceLoader loader, String file) throws SAXException, IOException { try (InputStream in = loader.openResource(file)) { + DocumentBuilder db = configDocumentBuilder(loader, log); + return db.parse(in, SystemIdResolver.createSystemIdFromResourceName(file)); + } + } + + /** + * Parses a config file from a Solr config based on InputSource. Xinclude and external entities + * are enabled, but cannot escape the resource loader. + */ + public static Document parseConfigXML(Logger log, ResourceLoader loader, InputSource is) + throws SAXException, IOException { + return configDocumentBuilder(loader, log).parse(is); + } + + private static DocumentBuilder configDocumentBuilder(ResourceLoader loader, Logger log) { + try { + Objects.requireNonNull(loader); final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - dbf.setValidating(false); dbf.setNamespaceAware(true); trySetDOMFeature(dbf, XMLConstants.FEATURE_SECURE_PROCESSING, true); + // only enable xinclude, if systemId is available. this assumes it is the case as loader + // provides one. try { dbf.setXIncludeAware(true); } catch (UnsupportedOperationException e) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "XML parser doesn't support XInclude option", e); } - final DocumentBuilder db = dbf.newDocumentBuilder(); db.setEntityResolver(new SystemIdResolver(loader)); db.setErrorHandler(new XMLErrorLogger(log)); - return db.parse(in, SystemIdResolver.createSystemIdFromResourceName(file)); + return db; } catch (ParserConfigurationException pce) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "XML parser cannot be configured", pce); + } catch (UnsupportedOperationException e) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "XML parser doesn't support XInclude option", e); } } /** - * Parses the given InputStream as XML, disabling any external entities with secure processing - * enabled. The given InputStream is not closed. + * Parses non-Solr configs as XML based on the given InputStream, disabling any external entities + * with secure processing enabled. The given InputStream is not closed. */ public static Document parseUntrustedXML(Logger log, InputStream in) throws SAXException, IOException {