[bug-63005] disable the reading of DTDs altogether

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1849123 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 134 insertions, 166 deletions

diff --git a/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java b/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java
index 6b0bd55840..cf9fbf71ad 100644
--- a/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java
+++ b/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java
@@ -105,6 +105,7 @@ public final class DocumentHelper {
         //entity expansions to 1 in trySetXercesSecurityManager
         documentBuilderFactory.setExpandEntityReferences(false);
         trySetFeature(documentBuilderFactory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        trySetFeature(documentBuilderFactory, POIXMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL, true);
         trySetFeature(documentBuilderFactory, POIXMLConstants.FEATURE_LOAD_DTD_GRAMMAR, false);
         trySetFeature(documentBuilderFactory, POIXMLConstants.FEATURE_LOAD_EXTERNAL_DTD, false);
         trySetXercesSecurityManager(documentBuilderFactory);
diff --git a/src/ooxml/testcases/org/apache/poi/ooxml/util/TestDocumentHelper.java b/src/ooxml/testcases/org/apache/poi/ooxml/util/TestDocumentHelper.java
index febec72b5f..63c6610f7e 100644
--- a/src/ooxml/testcases/org/apache/poi/ooxml/util/TestDocumentHelper.java
+++ b/src/ooxml/testcases/org/apache/poi/ooxml/util/TestDocumentHelper.java
@@ -39,6 +39,7 @@ public class TestDocumentHelper {
     public void testDocumentBuilderFactory() throws Exception {
         try {
             assertTrue(DocumentHelper.documentBuilderFactory.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
+            assertTrue(DocumentHelper.documentBuilderFactory.getFeature(POIXMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL));
             assertFalse(DocumentHelper.documentBuilderFactory.getFeature(POIXMLConstants.FEATURE_LOAD_DTD_GRAMMAR));
             assertFalse(DocumentHelper.documentBuilderFactory.getFeature(POIXMLConstants.FEATURE_LOAD_EXTERNAL_DTD));
         } catch(AbstractMethodError e) {
diff --git a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestContentType.java b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestContentType.java
index 91187ceb9f..7a0d0369ec 100644
--- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestContentType.java
+++ b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestContentType.java
@@ -19,163 +19,150 @@ package org.apache.poi.openxml4j.opc;
 
 import java.io.InputStream;
 
-import junit.framework.TestCase;
-
 import org.apache.poi.openxml4j.OpenXML4JTestDataSamples;
 import org.apache.poi.openxml4j.exceptions.InvalidFormatException;
 import org.apache.poi.openxml4j.opc.internal.ContentType;
-import org.apache.poi.xwpf.usermodel.XWPFRelation;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
 
 /**
  * Tests for content type (ContentType class).
  *
  * @author Julien Chable
  */
-public final class TestContentType extends TestCase {
-
-	/**
-	 * Check rule M1.13: Package implementers shall only create and only
-	 * recognize parts with a content type; format designers shall specify a
-	 * content type for each part included in the format. Content types for
-	 * package parts shall fit the definition and syntax for media types as
-	 * specified in RFC 2616, \u00A73.7.
-	 */
-	public void testContentTypeValidation() throws InvalidFormatException {
-		String[] contentTypesToTest = new String[] { "text/xml",
-				"application/pgp-key", "application/vnd.hp-PCLXL",
-				"application/vnd.lotus-1-2-3" };
-		for (String contentType : contentTypesToTest) {
-			new ContentType(contentType);
-		}
-	}
-
-	/**
-	 * Check rule M1.13 : Package implementers shall only create and only
-	 * recognize parts with a content type; format designers shall specify a
-	 * content type for each part included in the format. Content types for
-	 * package parts shall fit the definition and syntax for media types as
-	 * specified in RFC 2616, \u00A3.7.
-	 *
-	 * Check rule M1.14: Content types shall not use linear white space either
-	 * between the type and subtype or between an attribute and its value.
-	 * Content types also shall not have leading or trailing white spaces.
-	 * Package implementers shall create only such content types and shall
-	 * require such content types when retrieving a part from a package; format
-	 * designers shall specify only such content types for inclusion in the
-	 * format.
-	 */
-	public void testContentTypeValidationFailure() {
-		String[] contentTypesToTest = new String[] { "text/xml/app", "",
-				"test", "text(xml/xml", "text)xml/xml", "text<xml/xml",
-				"text>/xml", "text@/xml", "text,/xml", "text;/xml",
-				"text:/xml", "text\\/xml", "t/ext/xml", "t\"ext/xml",
-				"text[/xml", "text]/xml", "text?/xml", "tex=t/xml",
-				"te{xt/xml", "tex}t/xml", "te xt/xml",
-				"text" + (char) 9 + "/xml", "text xml", " text/xml " };
-		for (String contentType : contentTypesToTest) {
-			try {
-				new ContentType(contentType);
-			} catch (InvalidFormatException e) {
-				continue;
-			}
-			fail("Must have fail for content type: '" + contentType + "' !");
-		}
-	}
-
-   /**
-    * Parameters are allowed, provides that they meet the
-    *  criteria of rule [01.2]
-    * Invalid parameters are verified as incorrect in 
-    *  {@link #testContentTypeParameterFailure()}
-    */
-   public void testContentTypeParam() throws InvalidFormatException {
-      String[] contentTypesToTest = new String[] { "mail/toto;titi=tata",
-               "text/xml;a=b;c=d", "text/xml;key1=param1;key2=param2",
-               "application/pgp-key;version=\"2\"", 
-               "application/x-resqml+xml;version=2.0;type=obj_global2dCrs"
-      };
-      for (String contentType : contentTypesToTest) {
-          new ContentType(contentType);
-      }
-   }
-   
-	/**
-	 * Check rule [O1.2]: Format designers might restrict the usage of
-	 * parameters for content types.
-	 */
-	public void testContentTypeParameterFailure() {
-		String[] contentTypesToTest = new String[] { 
-		        "mail/toto;\"titi=tata\"", // quotes not allowed like that
+public final class TestContentType {
+
+    /**
+     * Check rule M1.13: Package implementers shall only create and only
+     * recognize parts with a content type; format designers shall specify a
+     * content type for each part included in the format. Content types for
+     * package parts shall fit the definition and syntax for media types as
+     * specified in RFC 2616, \u00A73.7.
+     */
+    @Test
+    public void testContentTypeValidation() throws InvalidFormatException {
+        String[] contentTypesToTest = new String[]{"text/xml",
+                "application/pgp-key", "application/vnd.hp-PCLXL",
+                "application/vnd.lotus-1-2-3"};
+        for (String contentType : contentTypesToTest) {
+            new ContentType(contentType);
+        }
+    }
+
+    /**
+     * Check rule M1.13 : Package implementers shall only create and only
+     * recognize parts with a content type; format designers shall specify a
+     * content type for each part included in the format. Content types for
+     * package parts shall fit the definition and syntax for media types as
+     * specified in RFC 2616, \u00A3.7.
+     * <p>
+     * Check rule M1.14: Content types shall not use linear white space either
+     * between the type and subtype or between an attribute and its value.
+     * Content types also shall not have leading or trailing white spaces.
+     * Package implementers shall create only such content types and shall
+     * require such content types when retrieving a part from a package; format
+     * designers shall specify only such content types for inclusion in the
+     * format.
+     */
+    @Test
+    public void testContentTypeValidationFailure() {
+        String[] contentTypesToTest = new String[]{"text/xml/app", "",
+                "test", "text(xml/xml", "text)xml/xml", "text<xml/xml",
+                "text>/xml", "text@/xml", "text,/xml", "text;/xml",
+                "text:/xml", "text\\/xml", "t/ext/xml", "t\"ext/xml",
+                "text[/xml", "text]/xml", "text?/xml", "tex=t/xml",
+                "te{xt/xml", "tex}t/xml", "te xt/xml",
+                "text" + (char) 9 + "/xml", "text xml", " text/xml "};
+        for (String contentType : contentTypesToTest) {
+            try {
+                new ContentType(contentType);
+            } catch (InvalidFormatException e) {
+                continue;
+            }
+            fail("Must have fail for content type: '" + contentType + "' !");
+        }
+    }
+
+    /**
+     * Parameters are allowed, provides that they meet the
+     * criteria of rule [01.2]
+     * Invalid parameters are verified as incorrect in
+     * {@link #testContentTypeParameterFailure()}
+     */
+    @Test
+    public void testContentTypeParam() throws InvalidFormatException {
+        String[] contentTypesToTest = new String[]{"mail/toto;titi=tata",
+                "text/xml;a=b;c=d", "text/xml;key1=param1;key2=param2",
+                "application/pgp-key;version=\"2\"",
+                "application/x-resqml+xml;version=2.0;type=obj_global2dCrs"
+        };
+        for (String contentType : contentTypesToTest) {
+            new ContentType(contentType);
+        }
+    }
+
+    /**
+     * Check rule [O1.2]: Format designers might restrict the usage of
+     * parameters for content types.
+     */
+    @Test
+    public void testContentTypeParameterFailure() {
+        String[] contentTypesToTest = new String[]{
+                "mail/toto;\"titi=tata\"", // quotes not allowed like that
                 "mail/toto;titi = tata", // spaces not allowed
                 "text/\u0080" // characters above ASCII are not allowed
         };
-		for (String contentType : contentTypesToTest) {
-			try {
-				new ContentType(contentType);
-			} catch (InvalidFormatException e) {
-				continue;
-			}
-			fail("Must have fail for content type: '" + contentType + "' !");
-		}
-	}
-
-	/**
-	 * Check rule M1.15: The package implementer shall require a content type
-	 * that does not include comments and the format designer shall specify such
-	 * a content type.
-	 */
-	public void testContentTypeCommentFailure() {
-		String[] contentTypesToTest = new String[] { "text/xml(comment)" };
-		for (String contentType : contentTypesToTest) {
-			try {
-				new ContentType(contentType);
-			} catch (InvalidFormatException e) {
-				continue;
-			}
-			fail("Must have fail for content type: '" + contentType + "' !");
-		}
-	}
-	
-	/**
-	 * OOXML content types don't need entities, but we shouldn't
-	 * barf if we get one from a third party system that added them
-	 */
-	public void testFileWithContentTypeEntities() throws Exception {
-        InputStream is = OpenXML4JTestDataSamples.openSampleStream("ContentTypeHasEntities.ooxml");
-        OPCPackage p = OPCPackage.open(is);
-        
-        // Check we found the contents of it
-        boolean foundCoreProps = false, foundDocument = false, foundTheme1 = false;
-        for (PackagePart part : p.getParts()) {
-            if (part.getPartName().toString().equals("/docProps/core.xml")) {
-                assertEquals(ContentTypes.CORE_PROPERTIES_PART, part.getContentType());
-                foundCoreProps = true;
+        for (String contentType : contentTypesToTest) {
+            try {
+                new ContentType(contentType);
+            } catch (InvalidFormatException e) {
+                continue;
             }
-            if (part.getPartName().toString().equals("/word/document.xml")) {
-                assertEquals(XWPFRelation.DOCUMENT.getContentType(), part.getContentType());
-                foundDocument = true;
-            }
-            if (part.getPartName().toString().equals("/word/theme/theme1.xml")) {
-                assertEquals(XWPFRelation.THEME.getContentType(), part.getContentType());
-                foundTheme1 = true;
+            fail("Must have fail for content type: '" + contentType + "' !");
+        }
+    }
+
+    /**
+     * Check rule M1.15: The package implementer shall require a content type
+     * that does not include comments and the format designer shall specify such
+     * a content type.
+     */
+    @Test
+    public void testContentTypeCommentFailure() {
+        String[] contentTypesToTest = new String[]{"text/xml(comment)"};
+        for (String contentType : contentTypesToTest) {
+            try {
+                new ContentType(contentType);
+            } catch (InvalidFormatException e) {
+                continue;
             }
+            fail("Must have fail for content type: '" + contentType + "' !");
         }
-        assertTrue("Core not found in " + p.getParts(), foundCoreProps);
-        assertTrue("Document not found in " + p.getParts(), foundDocument);
-        assertTrue("Theme1 not found in " + p.getParts(), foundTheme1);
-	}
-	
-	/**
-	 * Check that we can open a file where there are valid
-	 *  parameters on a content type
-	 */
-	public void testFileWithContentTypeParams() throws Exception {
+    }
+
+    /**
+     * OOXML content types don't need entities and we shouldn't
+     * barf if we get one from a third party system that added them
+     */
+    @Test(expected = InvalidFormatException.class)
+    public void testFileWithContentTypeEntities() throws Exception {
+        InputStream is = OpenXML4JTestDataSamples.openSampleStream("ContentTypeHasEntities.ooxml");
+        OPCPackage.open(is);
+    }
+
+    /**
+     * Check that we can open a file where there are valid
+     * parameters on a content type
+     */
+    @Test
+    public void testFileWithContentTypeParams() throws Exception {
         InputStream is = OpenXML4JTestDataSamples.openSampleStream("ContentTypeHasParameters.ooxml");
 
         OPCPackage p = OPCPackage.open(is);
-        
+
         final String typeResqml = "application/x-resqml+xml";
-        
+
         // Check the types on everything
         for (PackagePart part : p.getParts()) {
             final String contentType = part.getContentType();
@@ -207,8 +194,7 @@ public final class TestContentType extends TestCase {
                 assertEquals(2, length);
                 assertEquals("2.0", details.getParameter("version"));
                 assertEquals("obj_global1dCrs", details.getParameter("type"));
-            }
-            else if (part.getPartName().toString().equals("/global2dCrs.xml")) {
+            } else if (part.getPartName().toString().equals("/global2dCrs.xml")) {
                 assertTrue(part.getContentType().startsWith(typeResqml));
                 assertEquals(typeResqml, details.toString(false));
                 assertEquals(true, hasParameters);
diff --git a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackageCoreProperties.java b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackageCoreProperties.java
index c490048442..17bf1828af 100644
--- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackageCoreProperties.java
+++ b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackageCoreProperties.java
@@ -249,8 +249,8 @@ public final class TestPackageCoreProperties {
         // Get the Core Properties
         PackagePropertiesPart props = (PackagePropertiesPart)p.getPackageProperties();
         
-        // Check
-        assertEquals("Stefan Kopf", props.getCreatorProperty().get());
+        // used to resolve a vale but now we ignore DTD entities for security reasons
+        assertFalse(props.getCreatorProperty().isPresent());
         
         p.close();
     }
diff --git a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestRelationships.java b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestRelationships.java
index ebbc309ffa..846eac2a1a 100644
--- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestRelationships.java
+++ b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestRelationships.java
@@ -418,28 +418,8 @@ public class TestRelationships extends TestCase {
             if (pr.getRelationshipType().equals(PackageRelationshipTypes.EXTENDED_PROPERTIES))
                 foundExtPropRel = true;
         }
-        assertTrue("Core/Doc Relationship not found in " + p.getRelationships(), foundDocRel);
-        assertTrue("Core Props Relationship not found in " + p.getRelationships(), foundCorePropRel);
-        assertTrue("Ext Props Relationship not found in " + p.getRelationships(), foundExtPropRel);
-        
-        // Should have normal work parts
-        boolean foundCoreProps = false, foundDocument = false, foundTheme1 = false;
-        for (PackagePart part : p.getParts()) {
-            if (part.getPartName().toString().equals("/docProps/core.xml")) {
-                assertEquals(ContentTypes.CORE_PROPERTIES_PART, part.getContentType());
-                foundCoreProps = true;
-            }
-            if (part.getPartName().toString().equals("/word/document.xml")) {
-                assertEquals(XWPFRelation.DOCUMENT.getContentType(), part.getContentType());
-                foundDocument = true;
-            }
-            if (part.getPartName().toString().equals("/word/theme/theme1.xml")) {
-                assertEquals(XWPFRelation.THEME.getContentType(), part.getContentType());
-                foundTheme1 = true;
-            }
-        }
-        assertTrue("Core not found in " + p.getParts(), foundCoreProps);
-        assertTrue("Document not found in " + p.getParts(), foundDocument);
-        assertTrue("Theme1 not found in " + p.getParts(), foundTheme1);
+        assertFalse("Core/Doc Relationship not found in " + p.getRelationships(), foundDocRel);
+        assertFalse("Core Props Relationship not found in " + p.getRelationships(), foundCorePropRel);
+        assertFalse("Ext Props Relationship not found in " + p.getRelationships(), foundExtPropRel);
     }
 }
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
index ef83956468..379109303b 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
@@ -1969,7 +1969,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
             fail("should have thrown SAXParseException");
         } catch (SAXParseException e) {
             assertNotNull(e.getMessage());
-            assertTrue(e.getMessage().contains("more than \"1\" entity"));
+            assertTrue(e.getMessage().contains("DOCTYPE is disallowed when the feature"));
         }
     }