Use HTTPS for loading DTD

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-2022: Suite.xml files make web request when suite uses DTD over HTTPS (Krishnan Mahadevan)
 Fixed: GITHUB-2017: Don't use child injectors for Guice tests (Joe Barnett)
 New:   GITHUB-2003: Add to @Test interface fields IDs and issues (Krishnan Mahadevan)
 Fixed: GITHUB-2009: Test Timeout not respected in parallel="methods" mode (Krishnan Mahadevan)

src/main/java/org/testng/GuiceHelper.java

@@ -77,7 +77,7 @@ private static Module getParentModule(ITestContext context) {
 
   public static Injector createInjector(ITestContext context, List<Module> moduleInstances) {
     Module parentModule = getParentModule(context);
-    List fullModules = Lists.newArrayList(moduleInstances);
+    List<Module> fullModules = Lists.newArrayList(moduleInstances);
     if (parentModule != null) {
       fullModules.add(parentModule);
     }

src/main/java/org/testng/internal/RuntimeBehavior.java

@@ -9,6 +9,7 @@ public final class RuntimeBehavior {
   public static final String TESTNG_MODE_DRYRUN = "testng.mode.dryrun";
   private static final String TEST_CLASSPATH = "testng.test.classpath";
   private static final String SKIP_CALLER_CLS_LOADER = "skip.caller.clsLoader";
+  public static final String TESTNG_USE_UNSECURE_URL = "testng.dtd.http";
   public static final String SHOW_TESTNG_STACK_FRAMES = "testng.show.stack.frames";
 
   private RuntimeBehavior() {}
@@ -17,6 +18,10 @@ public static boolean showTestNGStackFrames() {
     return Boolean.getBoolean(SHOW_TESTNG_STACK_FRAMES);
   }
 
+  public static boolean useHttpUrlForDtd() {
+    return Boolean.getBoolean(TESTNG_USE_UNSECURE_URL);
+  }
+
   public static String getDefaultLineSeparator() {
     return System.getProperty("line.separator");
   }

src/main/java/org/testng/xml/DefaultXmlWeaver.java

@@ -30,7 +30,7 @@ class DefaultXmlWeaver implements IWeaveXml {
   public String asXml(XmlSuite xmlSuite) {
     XMLStringBuffer xsb = new XMLStringBuffer();
     xsb.setDefaultComment(defaultComment);
-    xsb.setDocType("suite SYSTEM \"" + Parser.TESTNG_DTD_URL + '\"');
+    xsb.setDocType("suite SYSTEM \"" + Parser.HTTPS_TESTNG_DTD_URL + '\"');
     Properties p = new Properties();
     p.setProperty("name", xmlSuite.getName());
     if (xmlSuite.getVerbose() != null) {

src/main/java/org/testng/xml/LaunchSuite.java

@@ -121,7 +121,7 @@ private CustomizedSuite(
      */
     protected XMLStringBuffer createContentBuffer() {
       XMLStringBuffer suiteBuffer = new XMLStringBuffer();
-      suiteBuffer.setDocType("suite SYSTEM \"" + Parser.TESTNG_DTD_URL + "\"");
+      suiteBuffer.setDocType("suite SYSTEM \"" + Parser.HTTPS_TESTNG_DTD_URL + "\"");
 
       Properties attrs = new Properties();
       attrs.setProperty("parallel", XmlSuite.ParallelMode.NONE.toString());

src/main/java/org/testng/xml/Parser.java

@@ -1,5 +1,7 @@
 package org.testng.xml;
 
+import java.util.Arrays;
+import java.util.Collections;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 
@@ -22,10 +24,18 @@ public class Parser {
   public static final String TESTNG_DTD = "testng-1.0.dtd";
 
   /** The URL to the deprecated TestNG DTD. */
-  public static final String DEPRECATED_TESTNG_DTD_URL = "http://beust.com/testng/" + TESTNG_DTD;
+  private static final String OLD_TESTNG_DTD_URL = "http://beust.com/testng/" + TESTNG_DTD;
+  private static final String HTTPS_OLD_TESTNG_DTD_URL = "https://beust.com/testng/" + TESTNG_DTD;
 
   /** The URL to the TestNG DTD. */
-  public static final String TESTNG_DTD_URL = "http://testng.org/" + TESTNG_DTD;
+  private static final String TESTNG_DTD_URL = "http://testng.org/" + TESTNG_DTD;
+  public static final String HTTPS_TESTNG_DTD_URL = "https://testng.org/" + TESTNG_DTD;
+
+  private static final List<String> URLS = Collections.unmodifiableList(Arrays.asList(
+      OLD_TESTNG_DTD_URL,
+      HTTPS_OLD_TESTNG_DTD_URL,
+      TESTNG_DTD_URL,
+      HTTPS_TESTNG_DTD_URL));
 
   /** The default file name for the TestNG test suite if none is specified (testng.xml). */
   public static final String DEFAULT_FILENAME = "testng.xml";
@@ -40,6 +50,10 @@ public class Parser {
     }
   }
 
+  static boolean isUnRecognizedPublicId(String publicId) {
+    return !URLS.contains(publicId);
+  }
+
   /**
    * The file name of the xml suite being parsed. This may be null if the Parser has not been
    * initialized with a file name. TODO CQ This member is never used.

src/main/java/org/testng/xml/TestNGContentHandler.java

@@ -6,6 +6,7 @@
 import org.testng.TestNGException;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
+import org.testng.internal.RuntimeBehavior;
 import org.testng.internal.Utils;
 import org.testng.log4testng.Logger;
 import org.xml.sax.Attributes;
@@ -89,31 +90,37 @@ public TestNGContentHandler(String fileName, boolean loadClasses) {
   @Override
   public InputSource resolveEntity(String systemId, String publicId)
       throws IOException, SAXException {
-    InputSource result;
-    if (Parser.DEPRECATED_TESTNG_DTD_URL.equals(publicId)
-        || Parser.TESTNG_DTD_URL.equals(publicId)) {
-      m_validate = true;
-      InputStream is = getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD);
-      if (null == is) {
-        is = Thread.currentThread().getContextClassLoader().getResourceAsStream(Parser.TESTNG_DTD);
-        if (null == is) {
-          System.out.println(
-              "WARNING: couldn't find in classpath "
-                  + publicId
-                  + "\n"
-                  + "Fetching it from the Web site.");
-          result = super.resolveEntity(systemId, publicId);
-        } else {
-          result = new InputSource(is);
-        }
+    if (Parser.isUnRecognizedPublicId(publicId)) {
+      //The Url is not TestNG recognized
+      boolean isHttps = publicId != null && publicId.trim().toLowerCase().startsWith("https");
+      if (isHttps || RuntimeBehavior.useHttpUrlForDtd()) {
+        return super.resolveEntity(systemId, publicId);
       } else {
-        result = new InputSource(is);
+        String msg = "TestNG by default disables loading DTD from unsecure Urls. " +
+            "If you need to explicitly load the DTD from a http url, please do so " +
+            "by using the JVM argument [-D" + RuntimeBehavior.TESTNG_USE_UNSECURE_URL + "=true]";
+        throw new TestNGException(msg);
       }
-    } else {
-      result = super.resolveEntity(systemId, publicId);
     }
-
-    return result;
+    m_validate = true;
+    InputStream is = loadDtdUsingClassLoader();
+    if (is != null) {
+      return new InputSource(is);
+    }
+    System.out.println(
+        "WARNING: couldn't find in classpath "
+            + publicId
+            + "\n"
+            + "Fetching it from " + Parser.HTTPS_TESTNG_DTD_URL);
+    return super.resolveEntity(systemId, Parser.HTTPS_TESTNG_DTD_URL);
+  }
+
+  private InputStream loadDtdUsingClassLoader() {
+    InputStream is = getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD);
+    if (is != null) {
+      return is;
+    }
+    return Thread.currentThread().getContextClassLoader().getResourceAsStream(Parser.TESTNG_DTD);
   }
 
   /** Parse <suite-file> */
@@ -400,18 +407,15 @@ public void xmlPackages(boolean start, Attributes attributes) {
   public void xmlMethodSelectors(boolean start, Attributes attributes) {
     if (start) {
       m_currentSelectors = new ArrayList<>();
+      return;
+    }
+    if (m_locations.peek() == Location.TEST) {
+      m_currentTest.setMethodSelectors(m_currentSelectors);
     } else {
-      switch (m_locations.peek()) {
-        case TEST:
-          m_currentTest.setMethodSelectors(m_currentSelectors);
-          break;
-        default:
-          m_currentSuite.setMethodSelectors(m_currentSelectors);
-          break;
-      }
-
-      m_currentSelectors = null;
+      m_currentSuite.setMethodSelectors(m_currentSelectors);
     }
+
+    m_currentSelectors = null;
   }
 
   /** Parse <selector-class> */
@@ -450,7 +454,7 @@ private void xmlMethod(boolean start) {
   }
 
   /** Parse <run> */
-  public void xmlRun(boolean start, Attributes attributes) throws SAXException {
+  public void xmlRun(boolean start, Attributes attributes) {
     if (start) {
       m_currentRuns = Lists.newArrayList();
     } else {
@@ -466,15 +470,15 @@ public void xmlRun(boolean start, Attributes attributes) throws SAXException {
   }
 
   /** Parse <group> */
-  public void xmlGroup(boolean start, Attributes attributes) throws SAXException {
+  public void xmlGroup(boolean start, Attributes attributes) {
     if (start) {
       m_currentTest.addXmlDependencyGroup(
           attributes.getValue("name"), attributes.getValue("depends-on"));
     }
   }
 
   /** Parse <groups> */
-  public void xmlGroups(boolean start, Attributes attributes) throws SAXException {
+  public void xmlGroups(boolean start, Attributes attributes) {
     if (start) {
       m_currentGroups = new XmlGroups();
       m_currentIncludedGroups = Lists.newArrayList();
@@ -493,14 +497,14 @@ public void xmlGroups(boolean start, Attributes attributes) throws SAXException
    * something when the tag opens, the code is inlined below in the startElement() method.
    */
   @Override
-  public void startElement(String uri, String localName, String qName, Attributes attributes)
-      throws SAXException {
+  public void startElement(String uri, String localName, String qName, Attributes attributes) {
     if (!m_validate && !m_hasWarn) {
-      Logger.getLogger(TestNGContentHandler.class)
-          .warn(
-              "It is strongly recommended to add "
-                  + "\"<!DOCTYPE suite SYSTEM \"http://testng.org/testng-1.0.dtd\" >\" at the top of your file, "
-                  + "otherwise TestNG may fail or not work as expected.");
+      String msg = String.format(
+          "It is strongly recommended to add "
+              + "\"<!DOCTYPE suite SYSTEM \"%s\" >\" at the top of your file, "
+              + "otherwise TestNG may fail or not work as expected.", Parser.HTTPS_TESTNG_DTD_URL
+      );
+      Logger.getLogger(TestNGContentHandler.class).warn(msg);
       m_hasWarn = true;
     }
     String name = attributes.getValue("name");
@@ -649,8 +653,8 @@ private void pushLocation(Location l) {
     m_locations.push(l);
   }
 
-  private Location popLocation() {
-    return m_locations.pop();
+  private void popLocation() {
+    m_locations.pop();
   }
 
   private List<Integer> stringToList(String in) {
@@ -663,7 +667,7 @@ private List<Integer> stringToList(String in) {
   }
 
   @Override
-  public void endElement(String uri, String localName, String qName) throws SAXException {
+  public void endElement(String uri, String localName, String qName) {
     if ("suite".equals(qName)) {
       xmlSuite(false, null);
     } else if ("suite-file".equals(qName)) {
@@ -722,7 +726,7 @@ private boolean areWhiteSpaces(char[] ch, int start, int length) {
   }
 
   @Override
-  public void characters(char ch[], int start, int length) {
+  public void characters(char[] ch, int start, int length) {
     if (null != m_currentLanguage && !areWhiteSpaces(ch, start, length)) {
       m_currentExpression += new String(ch, start, length);
     }
@@ -744,7 +748,7 @@ private static String expandValue(String value) {
       if (result == null) {
         result = new StringBuilder(value.substring(startPosition, startIndex));
       } else {
-        result.append(value.substring(startPosition, startIndex));
+        result.append(value, startPosition, startIndex);
       }
       String propertyValue = System.getProperty(property);
       if (propertyValue == null) {