[GitHub] [daffodil] tuxji commented on a change in pull request #576: Enhance XML Loader to tolerate whitespace

[GitBox <gi...@apache.org>]

tuxji commented on a change in pull request #576:
URL: https://github.com/apache/daffodil/pull/576#discussion_r641720175



##########
File path: daffodil-lib/src/test/scala/org/apache/daffodil/xml/test/unit/TestXMLLoader.scala
##########
@@ -137,4 +203,66 @@ b&"<>]]>"""))
       xmlFromDafLoaderNormalized.text)
 
   }
+
+  @Test def testLoaderToleratesXMLWithLeadingWhitespace(): Unit = {
+    val xmlTextWithLeadingWhitespace = "    \n" + "<data>foo</data>\n"
+    val loader = new DaffodilXMLLoader()
+    val ss = StringSchemaSource(xmlTextWithLeadingWhitespace)
+    val xml = loader.load(ss, None, addPositionAttributes = false)
+    assertEquals("foo", xml.text)
+  }
+
+  @Test def testLoaderToleratesXMLWithLeadingComments(): Unit = {
+    val xmlText = "    \n" +
+      "  <!-- a comment -->  \n  <data>foo</data>\n"
+    val loader = new DaffodilXMLLoader()
+    val ss = StringSchemaSource(xmlText)
+    val xml = loader.load(ss, None, addPositionAttributes = false)
+    assertEquals("foo", xml.text)
+  }
+
+  @Test def testLoaderToleratesXMLWithPI(): Unit = {
+    val xmlText = "    \n" +
+    "<?aProcInstr yadda yadda ?>\n" +
+      "  <!-- a comment -->  \n  <data>foo</data>\n"
+    val loader = new DaffodilXMLLoader()
+    val ss = StringSchemaSource(xmlText)
+    val xml = loader.load(ss, None, addPositionAttributes = false)
+    assertEquals("foo", xml.text)
+  }
+
+  @Test def testLoaderCatchesVarousBadXML(): Unit = {
+    val xmlText = "    \n" + // no prolog some whitespace (tolerated)
+      "&AnEntityRef;\n" + // entity refs not allowed
+      "random text\n" + // just text not allowed
+      "<data>foo</data>\n" +
+    "<!-- comment afterwards --><another>element</another>\n&AnotherEntityRef;\nmore random text\n" // other bad stuff.
+    val teh = new TestErrorHandler()
+    val loader = new DaffodilXMLLoader(teh)
+    val ss = StringSchemaSource(xmlText)
+    val xml = loader.load(ss, None, addPositionAttributes = false)
+    val msgs = teh.exceptions.map{ _.getMessage() }.mkString("\n")
+    println(msgs)
+    assertTrue(msgs.contains("non-empty text nodes not allowed"))
+    assertTrue(msgs.contains("random text"))
+    assertTrue(msgs.contains("more random text"))
+    assertTrue(msgs.contains("exactly one element"))
+  }
 }
+
+class TestErrorHandler extends org.xml.sax.ErrorHandler {
+
+  val exceptions = new ArrayBuffer[SAXParseException]
+
+  def warning(exception: SAXParseException) = {
+    exceptions += exception
+  }
+
+  def error(exception: SAXParseException) = {
+    exceptions += exception
+  }
+
+  def fatalError(exception: SAXParseException) = {
+    exceptions += exception
+  }
+}

Review comment:
       No newline at end of file (but up to you).

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/xml/DaffodilConstructingLoader.scala
##########
@@ -288,10 +291,91 @@ class DaffodilConstructingLoader private[xml] (uri: URI,
     text(pos, s)
   }
 
+  /**
+   * Same CRLF/CR => LF processing as text gets.
+   */
   override def comment(pos: Int, s: String): Comment = {
     Comment(text(pos, s).text)
   }
 
+  /**
+   * Same CRLF/CR => LF processing as text gets.
+   */
+  override def procInstr(pos: Int, target: String, txt: String) =
+    ProcInstr(target, text(pos, txt).text)
+
+  /**
+   * Override of document to make it tolerant of the start of the file
+   * being whitespace instead of a "<" character
+   *
+   * This does not handle DOCTYPEs (aka DTDs) at all. Hence, is not
+   * a true replacement (bug fix) on the original ConstructingParser method
+   * that it overrides.
+   */

Review comment:
       Suppose upstream takes your document() change to tolerate whitespace before the first `<` character and other big fixes, could you also suggest they take a change to make it easier to load XML securely?  Maybe the upstream PR could let us override a doctype function or something like that so we don't need to duplicate any upstream code in our constructing parser implementation here.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org