diff --git a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java index a65331302..73ddadda0 100644 --- a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java @@ -9,17 +9,18 @@ * ****************************************************************************** */ package org.eclipse.openvsx.mirror; +import jakarta.annotation.Nullable; import org.eclipse.openvsx.UrlConfigService; import org.eclipse.openvsx.admin.AdminService; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.NamingUtil; +import org.eclipse.openvsx.util.XmlUtil; import org.jobrunr.jobs.annotations.Job; import org.jobrunr.jobs.lambdas.JobRequestHandler; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.RequestEntity; @@ -28,14 +29,7 @@ import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; -import org.xml.sax.InputSource; -import org.xml.sax.SAXException; - -import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; -import java.io.IOException; -import java.io.StringReader; + import java.net.URI; import java.time.LocalDate; import java.time.format.DateTimeFormatter; @@ -58,9 +52,6 @@ public class DataMirrorJobRequestHandler implements JobRequestHandler dataMirrorService, RepositoryService repositories, @@ -79,7 +70,7 @@ public DataMirrorJobRequestHandler( } @Override - @Job(name="Data Mirror") + @Job(name="Data Mirror", retries = 0) public void run(DataMirrorJobRequest jobRequest) throws Exception { if (data == null) { return; @@ -87,7 +78,7 @@ public void run(DataMirrorJobRequest jobRequest) throws Exception { logger.debug(">> Starting DataMirrorJob"); var sitemap = getSitemap(); - if(sitemap == null) { + if (sitemap == null) { logger.error("failed to fetch sitemap"); return; } @@ -99,7 +90,7 @@ public void run(DataMirrorJobRequest jobRequest) throws Exception { } catch (Exception e) { logger.error("failed to mirror data", e); throw e; - }finally { + } finally { logger.debug("<< Completed DataMirrorJob"); } } @@ -107,7 +98,7 @@ public void run(DataMirrorJobRequest jobRequest) throws Exception { private List processUrls(NodeList urls, UserData mirrorUser) { var extensionIds = new ArrayList(); var progress = jobContext().progressBar(urls.getLength()); - for(var i = 0; i < urls.getLength(); i++) { + for (var i = 0; i < urls.getLength(); i++) { var url = (Element) urls.item(i); var extensionId = getExtensionId(url); var id = NamingUtil.fromExtensionId(extensionId); @@ -134,7 +125,7 @@ private List processUrls(NodeList urls, UserData mirrorUser) { private void deleteOtherExtensions(List extensionIds, UserData mirrorUser) { var notMatchingExtensions = repositories.findAllNotMatchingByExtensionId(extensionIds); - for(var extension : notMatchingExtensions) { + for (var extension : notMatchingExtensions) { var extensionId = NamingUtil.toExtensionId(extension); jobContext().logger().info("deleting " + extensionId); try { @@ -150,21 +141,16 @@ private void deleteOtherExtensions(List extensionIds, UserData mirrorUse } } - private Document getSitemap() throws IOException, SAXException, ParserConfigurationException { + private @Nullable Document getSitemap() { var requestUrl = URI.create(createApiUrl(urlConfigService.getMirrorServerUrl(), "sitemap.xml")); var request = new RequestEntity(HttpMethod.GET, requestUrl); var response = backgroundRestTemplate.exchange(request, String.class); var body = response.getStatusCode().is2xxSuccessful() ? response.getBody() : null; - if(body == null) { + if (body == null) { return null; } - try(var reader = new StringReader(body)) { - var factory = DocumentBuilderFactory.newInstance(); - factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - var builder = factory.newDocumentBuilder(); - return builder.parse(new InputSource(reader)); - } + return XmlUtil.safeParse(body); } private String getExtensionId(Element url) { diff --git a/server/src/main/java/org/eclipse/openvsx/util/XmlUtil.java b/server/src/main/java/org/eclipse/openvsx/util/XmlUtil.java new file mode 100644 index 000000000..f3c893198 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/util/XmlUtil.java @@ -0,0 +1,123 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.util; + +import jakarta.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import java.io.IOException; +import java.io.StringReader; + +public class XmlUtil { + + private static final Logger LOGGER = LoggerFactory.getLogger(XmlUtil.class); + + private XmlUtil() {} + + /** + * Parses the provided string as an XML document using a safe {@code DocumentBuilder} + * instance to prevent XXE attacks. + * + * @param input the XML document as string + * @return a {@code Document} instance if parsing succeeded, or {@code null} otherwise. + */ + public static @Nullable Document safeParse(String input) { + try (var reader = new StringReader(input)) { + var builder = safeDocumentBuilder(); + if (builder != null) { + return builder.parse(new InputSource(reader)); + } + } catch (SAXException | IOException e) { + LOGGER.error("Failed to parse XML Document: {}", e.getMessage()); + } + + return null; + } + + private static DocumentBuilder safeDocumentBuilder() { + // construct a safe DocumentBuilder to prevent XXE attacks + // https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxp-documentbuilderfactory-saxparserfactory-and-dom4j + + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + String[] featuresToEnable = { + // This is the PRIMARY defense. If DTDs (doctypes) are disallowed, almost all + // XML entity attacks are prevented + // Xerces 2 only - http://xerces.apache.org/xerces2-j/features.html#disallow-doctype-decl + "http://apache.org/xml/features/disallow-doctype-decl", + }; + + String[] featuresToDisable = { + // Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-general-entities + // Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-general-entities + // JDK7+ - http://xml.org/sax/features/external-general-entities + // This feature has to be used together with the following one, otherwise it will not protect you from XXE for sure + "http://xml.org/sax/features/external-general-entities", + + // Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-parameter-entities + // Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-parameter-entities + // JDK7+ - http://xml.org/sax/features/external-parameter-entities + // This feature has to be used together with the previous one, otherwise it will not protect you from XXE for sure + "http://xml.org/sax/features/external-parameter-entities", + + // Disable external DTDs as well + "http://apache.org/xml/features/nonvalidating/load-external-dtd" + }; + + for (String feature : featuresToEnable) { + try { + dbf.setFeature(feature, true); + } catch (ParserConfigurationException e) { + LOGGER.debug("The feature '{}' is not supported by your XML processor.", feature); + } + } + + for (String feature : featuresToDisable) { + try { + dbf.setFeature(feature, false); + } catch (ParserConfigurationException e) { + LOGGER.debug("The feature '{}' is not supported by your XML processor.", feature); + } + } + + try { + // Add these as per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks" + dbf.setXIncludeAware(false); + dbf.setExpandEntityReferences(false); + + // As stated in the documentation, "Feature for Secure Processing (FSP)" is the central mechanism that will + // help you safeguard XML processing. It instructs XML processors, such as parsers, validators, + // and transformers, to try and process XML securely, and the FSP can be used as an alternative to + // dbf.setExpandEntityReferences(false); to allow some safe level of Entity Expansion + // Exists from JDK6. + dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + var builder = dbf.newDocumentBuilder(); + // disable explicit logging in the xml parser + builder.setErrorHandler(null); + return builder; + } catch (ParserConfigurationException e) { + LOGGER.error("Could not build a safe XML processor: {}", e.getMessage()); + } + + return null; + } +} diff --git a/server/src/test/java/org/eclipse/openvsx/util/XmlUtilTest.java b/server/src/test/java/org/eclipse/openvsx/util/XmlUtilTest.java new file mode 100644 index 000000000..579626e69 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/util/XmlUtilTest.java @@ -0,0 +1,105 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.util; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +public class XmlUtilTest { + + @Test + public void parseSafeXml() { + var input = """ + + + test + + """; + + var document = XmlUtil.safeParse(input); + assertThat(document).isNotNull(); + assertThat(document.getDocumentElement()).isNotNull(); + } + + @Test + void shouldRejectXmlWithInlineDoctypeEntityDeclaration() { + // Tests the disallow-doctype-decl feature: any DOCTYPE declaration + // must be rejected to prevent local entity injection + var input = """ + + ]> + + + """; + + assertThat(XmlUtil.safeParse(input)).isNull(); + } + + @Test + public void shouldRejectXmlWithInlineDoctypeEntityDeclaration2() { + var input = """ + + ]> + + &xxe; + + """; + + var document = XmlUtil.safeParse(input); + assertThat(document).isNull(); + } + + @Test + void shouldRejectXmlWithExternalGeneralEntityReference() { + // Tests the external-general-entities feature: external system entity + // references (classic XXE attack vector) must be rejected + var input = """ + + ]> + + &xxe; + + """; + + assertThat(XmlUtil.safeParse(input)).isNull(); + } + + @Test + void shouldRejectXmlWithExternalParameterEntityReference() { + // Tests the external-parameter-entities feature: parameter entity + // references that pull in external content must be rejected + var input = """ + + %remote;]> + + + """; + + assertThat(XmlUtil.safeParse(input)).isNull(); + } + + @Test + void shouldRejectXmlWithExternalDtdReference() { + // Tests the ACCESS_EXTERNAL_DTD attribute: external DTD system + // identifiers must not be fetched + var input = """ + + + + + """; + + assertThat(XmlUtil.safeParse(input)).isNull(); + } +}