fix: fail closed when entities disabled

Author: martinblech

tests/test_xmltodict.py

@@ -410,7 +410,7 @@ def force_list(path, key, value):
     assert parse(xml, force_list=force_list, dict_constructor=dict) == expectedResult
 
 
-def test_disable_entities_true_ignores_xmlbomb():
+def test_disable_entities_true_rejects_xmlbomb():
     xml = """
     <!DOCTYPE xmlbomb [
         <!ENTITY a "1234567890" >
@@ -419,13 +419,8 @@ def test_disable_entities_true_ignores_xmlbomb():
     ]>
     <bomb>&c;</bomb>
     """
-    expectedResult = {'bomb': None}
-    try:
-        parse_attempt = parse(xml, disable_entities=True)
-    except expat.ExpatError:
-        assert True
-    else:
-        assert parse_attempt == expectedResult
+    with pytest.raises(expat.ExpatError, match="entities are disabled"):
+        parse(xml, disable_entities=True)
 
 
 def test_disable_entities_false_returns_xmlbomb():
@@ -442,20 +437,15 @@ def test_disable_entities_false_returns_xmlbomb():
     assert parse(xml, disable_entities=False) == expectedResult
 
 
-def test_disable_entities_true_ignores_external_dtd():
+def test_disable_entities_true_rejects_external_dtd():
     xml = """
     <!DOCTYPE external [
         <!ENTITY ee SYSTEM "http://www.python.org/">
     ]>
     <root>&ee;</root>
     """
-    expectedResult = {'root': None}
-    try:
-        parse_attempt = parse(xml, disable_entities=True)
-    except expat.ExpatError:
-        assert True
-    else:
-        assert parse_attempt == expectedResult
+    with pytest.raises(expat.ExpatError, match="entities are disabled"):
+        parse(xml, disable_entities=True)
 
 
 def test_disable_entities_true_attempts_external_dtd():
@@ -482,6 +472,16 @@ def raising_external_ref_handler(*args, **kwargs):
     expat.ParserCreate = ParserCreate
 
 
+def test_disable_entities_allows_comments_by_default():
+    xml = """
+    <a>
+        <!-- ignored -->
+        <b>1</b>
+    </a>
+    """
+    assert parse(xml) == {'a': {'b': '1'}}
+
+
 def test_comments():
     xml = """
     <a>

xmltodict.py

@@ -354,10 +354,23 @@ def parse(xml_input, encoding=None, expat=expat, process_namespaces=False,
         parser.CommentHandler = handler.comments
     parser.buffer_text = True
     if disable_entities:
-        # Anything not handled ends up here and entities aren't expanded.
-        parser.DefaultHandler = lambda x: None
-        # Expects an integer return; zero means failure -> expat.ExpatError.
-        parser.ExternalEntityRefHandler = lambda *x: 1
+        def _forbid_entities(*_args, **_kwargs):
+            raise expat.ExpatError("xmltodict.parse(): entities are disabled")
+
+        def _forbid_entities_default(text):
+            if not text:
+                return
+            stripped = text.lstrip()
+            if stripped.startswith('<!--'):
+                return
+            if stripped.startswith('<!') or stripped.startswith('&'):
+                _forbid_entities()
+
+        # Reject DTD/entity constructs explicitly instead of ignoring them.
+        parser.DefaultHandler = _forbid_entities_default
+        parser.EntityDeclHandler = _forbid_entities
+        parser.StartDoctypeDeclHandler = _forbid_entities
+        parser.ExternalEntityRefHandler = _forbid_entities
     if hasattr(xml_input, 'read'):
         parser.ParseFile(xml_input)
     elif isgenerator(xml_input):