From 6a07b5dc109ce9088d52b52d6061bec2cc9a2285 Mon Sep 17 00:00:00 2001
From: PJ Fanning <pjfanning@users.noreply.github.com>
Date: Sun, 18 Dec 2022 13:25:10 +0100
Subject: [PATCH] HADOOP-18575. Make XML transformer factory more lenient
 (#5224)

Due diligence followup to
HADOOP-18469. Add secure XML parser factories to XMLUtils (#4940)

Contributed by P J Fanning
---
 .../java/org/apache/hadoop/util/XMLUtils.java | 34 ++++++++++++++++---
 .../org/apache/hadoop/util/TestXMLUtils.java  | 12 +++++++
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/XMLUtils.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/XMLUtils.java
index a8040ad90a..7aa4b2bcc5 100644
--- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/XMLUtils.java
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/XMLUtils.java
@@ -29,6 +29,8 @@
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.xml.sax.SAXException;
 
 import java.io.*;
@@ -41,6 +43,9 @@
 @InterfaceStability.Unstable
 public class XMLUtils {
 
+  private static final Logger LOG =
+          LoggerFactory.getLogger(XMLUtils.class);
+
   public static final String DISALLOW_DOCTYPE_DECL =
       "http://apache.org/xml/features/disallow-doctype-decl";
   public static final String LOAD_EXTERNAL_DECL =
@@ -138,8 +143,8 @@ public static TransformerFactory newSecureTransformerFactory()
           throws TransformerConfigurationException {
     TransformerFactory trfactory = TransformerFactory.newInstance();
     trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
-    trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
-    trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
+    bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
+    bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
     return trfactory;
   }
 
@@ -156,8 +161,29 @@ public static SAXTransformerFactory newSecureSAXTransformerFactory()
           throws TransformerConfigurationException {
     SAXTransformerFactory trfactory = (SAXTransformerFactory) SAXTransformerFactory.newInstance();
     trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
-    trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
-    trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
+    bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
+    bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
     return trfactory;
   }
+
+  /**
+   * Set an attribute value on a {@link TransformerFactory}. If the TransformerFactory
+   * does not support the attribute, the method just returns <code>false</code> and
+   * logs the issue at debug level.
+   *
+   * @param transformerFactory to update
+   * @param name of the attribute to set
+   * @param value to set on the attribute
+   * @return whether the attribute was successfully set
+   */
+  static boolean bestEffortSetAttribute(TransformerFactory transformerFactory,
+                                        String name, Object value) {
+    try {
+      transformerFactory.setAttribute(name, value);
+      return true;
+    } catch (Throwable t) {
+      LOG.debug("Issue setting TransformerFactory attribute {}: {}", name, t.toString());
+    }
+    return false;
+  }
 }
diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestXMLUtils.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestXMLUtils.java
index ec1b74338a..0ebec96213 100644
--- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestXMLUtils.java
+++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestXMLUtils.java
@@ -20,10 +20,12 @@
 import java.io.InputStream;
 import java.io.StringReader;
 import java.io.StringWriter;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.SAXParser;
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 import javax.xml.transform.stream.StreamSource;
@@ -31,6 +33,7 @@
 import org.apache.hadoop.test.AbstractHadoopTestBase;
 
 import org.assertj.core.api.Assertions;
+import org.junit.Assert;
 import org.junit.Test;
 import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
@@ -128,6 +131,15 @@ public void testExternalDtdWithSecureSAXTransformerFactory() throws Exception {
     }
   }
 
+  @Test
+  public void testBestEffortSetAttribute() throws Exception {
+    TransformerFactory factory = TransformerFactory.newInstance();
+    Assert.assertFalse("unexpected attribute results in return of false",
+            XMLUtils.bestEffortSetAttribute(factory, "unsupportedAttribute false", "abc"));
+    Assert.assertTrue("expected attribute results in return of false",
+            XMLUtils.bestEffortSetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, ""));
+  }
+
   private static InputStream getResourceStream(final String filename) {
     return TestXMLUtils.class.getResourceAsStream(filename);
   }