Makes safe xml data calls raise 400 http error instead of 500

When we parse incoming XML safely, if there was an error raised it would
be an expat.Expat() error, which would bubble up to the api and turn
into a HTTP 500 (Internal Error)

It turns out that all the places we use the safe_xml parsing are in
Deserializers, close to the API, so in this patch we just change the
error it raises straight to nova.exception.MalformedRequest().

This causes the api to fail with the proper 400 (Malformed Request) when
it encounters corrupt XML. This is caught at
nova.api.openstack.wsgi._process_stack and __call__.

We also take the opportunity to move the new safe parser from nova.utils
to nova.api.openstack.xmlutil as the openstack api is the only thing
that uses it.

Fixes: bug #1133111
Change-Id: Ifa2ed7ee128241cfe8dbcdc5bd75194d96b6cdb5

10 files changed, 155 insertions, 88 deletions

diff --git a/nova/tests/api/openstack/compute/contrib/test_cells.py b/nova/tests/api/openstack/compute/contrib/test_cells.py
index 89ce4cd5a..bf6bff27c 100644
--- a/nova/tests/api/openstack/compute/contrib/test_cells.py
+++ b/nova/tests/api/openstack/compute/contrib/test_cells.py
@@ -27,6 +27,7 @@ from nova import exception
 from nova.openstack.common import timeutils
 from nova import test
 from nova.tests.api.openstack import fakes
+from nova.tests import utils
 
 
 FAKE_CELLS = [
@@ -394,3 +395,10 @@ class TestCellsXMLDeserializer(test.TestCase):
         deserializer = cells_ext.CellDeserializer()
         result = deserializer.deserialize(intext)
         self.assertEqual(dict(body=expected), result)
+
+    def test_with_corrupt_xml(self):
+        deserializer = cells_ext.CellDeserializer()
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                deserializer.deserialize,
+                utils.killer_xml_body())
diff --git a/nova/tests/api/openstack/compute/contrib/test_hosts.py b/nova/tests/api/openstack/compute/contrib/test_hosts.py
index 85f93a9d5..5678933dc 100644
--- a/nova/tests/api/openstack/compute/contrib/test_hosts.py
+++ b/nova/tests/api/openstack/compute/contrib/test_hosts.py
@@ -21,9 +21,11 @@ from nova.compute import power_state
 from nova.compute import vm_states
 from nova import context as context_maker
 from nova import db
+from nova import exception
 from nova.openstack.common import log as logging
 from nova import test
 from nova.tests import fake_hosts
+from nova.tests import utils
 
 LOG = logging.getLogger(__name__)
 
@@ -390,3 +392,9 @@ class HostSerializerTest(test.TestCase):
         result = self.deserializer.deserialize(intext)
 
         self.assertEqual(dict(body=exemplar), result)
+
+    def test_corrupt_xml(self):
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                self.deserializer.deserialize,
+                utils.killer_xml_body())
diff --git a/nova/tests/api/openstack/compute/contrib/test_security_groups.py b/nova/tests/api/openstack/compute/contrib/test_security_groups.py
index 4919d461f..02aa96956 100644
--- a/nova/tests/api/openstack/compute/contrib/test_security_groups.py
+++ b/nova/tests/api/openstack/compute/contrib/test_security_groups.py
@@ -30,6 +30,7 @@ from nova.openstack.common import jsonutils
 from nova import quota
 from nova import test
 from nova.tests.api.openstack import fakes
+from nova.tests import utils
 
 CONF = cfg.CONF
 FAKE_UUID = 'a47ae74e-ab08-447f-8eee-ffd43fc46c16'
@@ -727,13 +728,6 @@ class TestSecurityGroupRules(test.TestCase):
         self.assertEquals(security_group_rule['to_port'], 81)
 
     def test_create_by_invalid_cidr_json(self):
-        rules = {
-                  "security_group_rule": {
-                        "ip_protocol": "tcp",
-                        "from_port": "22",
-                        "to_port": "22",
-                        "parent_group_id": self.sg2['id'],
-                        "cidr": "10.2.3.124/2433"}}
         rule = security_group_rule_template(
                 ip_protocol="tcp",
                 from_port=22,
@@ -1146,6 +1140,13 @@ class TestSecurityGroupRulesXMLDeserializer(test.TestCase):
         }
         self.assertEquals(request['body'], expected)
 
+    def test_corrupt_xml(self):
+        """Should throw a 400 error on corrupt xml."""
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                self.deserializer.deserialize,
+                utils.killer_xml_body())
+
 
 class TestSecurityGroupXMLDeserializer(test.TestCase):
 
@@ -1192,6 +1193,13 @@ class TestSecurityGroupXMLDeserializer(test.TestCase):
         }
         self.assertEquals(request['body'], expected)
 
+    def test_corrupt_xml(self):
+        """Should throw a 400 error on corrupt xml."""
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                self.deserializer.deserialize,
+                utils.killer_xml_body())
+
 
 class TestSecurityGroupXMLSerializer(test.TestCase):
     def setUp(self):
diff --git a/nova/tests/api/openstack/compute/test_server_actions.py b/nova/tests/api/openstack/compute/test_server_actions.py
index 62a688962..754e103d4 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -32,6 +32,7 @@ from nova import test
 from nova.tests.api.openstack import fakes
 from nova.tests.image import fake
 from nova.tests import matchers
+from nova.tests import utils
 
 CONF = cfg.CONF
 CONF.import_opt('password_length', 'nova.utils')
@@ -1146,3 +1147,10 @@ class TestServerActionXMLDeserializer(test.TestCase):
                           self.deserializer.deserialize,
                           serial_request,
                           'action')
+
+    def test_corrupt_xml(self):
+        """Should throw a 400 error on corrupt xml."""
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                self.deserializer.deserialize,
+                utils.killer_xml_body())
diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py
index b77814003..2dfefc541 100644
--- a/nova/tests/api/openstack/compute/test_servers.py
+++ b/nova/tests/api/openstack/compute/test_servers.py
@@ -51,6 +51,7 @@ from nova.tests.api.openstack import fakes
 from nova.tests import fake_network
 from nova.tests.image import fake
 from nova.tests import matchers
+from nova.tests import utils
 
 CONF = cfg.CONF
 CONF.import_opt('password_length', 'nova.utils')
@@ -3859,6 +3860,13 @@ class TestServerCreateRequestXMLDeserializer(test.TestCase):
         }
         self.assertEquals(request['body'], expected)
 
+    def test_corrupt_xml(self):
+        """Should throw a 400 error on corrupt xml."""
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                self.deserializer.deserialize,
+                utils.killer_xml_body())
+
 
 class TestAddressesXMLSerialization(test.TestCase):
 
diff --git a/nova/tests/api/openstack/test_common.py b/nova/tests/api/openstack/test_common.py
index 227044572..a6344c09f 100644
--- a/nova/tests/api/openstack/test_common.py
+++ b/nova/tests/api/openstack/test_common.py
@@ -28,7 +28,7 @@ from nova.api.openstack import common
 from nova.api.openstack import xmlutil
 from nova import exception
 from nova import test
-from nova.tests import utils as test_utils
+from nova.tests import utils
 
 
 NS = "{http://docs.openstack.org/compute/api/v1.1}"
@@ -297,7 +297,7 @@ class MiscFunctionsTest(test.TestCase):
             self.fail("webob.exc.HTTPConflict was not raised")
 
     def test_check_img_metadata_properties_quota_valid_metadata(self):
-        ctxt = test_utils.get_test_admin_context()
+        ctxt = utils.get_test_admin_context()
         metadata1 = {"key": "value"}
         actual = common.check_img_metadata_properties_quota(ctxt, metadata1)
         self.assertEqual(actual, None)
@@ -311,7 +311,7 @@ class MiscFunctionsTest(test.TestCase):
         self.assertEqual(actual, None)
 
     def test_check_img_metadata_properties_quota_inv_metadata(self):
-        ctxt = test_utils.get_test_admin_context()
+        ctxt = utils.get_test_admin_context()
         metadata1 = {"a" * 260: "value"}
         self.assertRaises(webob.exc.HTTPBadRequest,
                 common.check_img_metadata_properties_quota, ctxt, metadata1)
@@ -512,3 +512,11 @@ class MetadataXMLSerializationTest(test.TestCase):
         """.replace("  ", "").replace("\n", ""))
 
         self.assertEqual(expected.toxml(), actual.toxml())
+
+    def test_metadata_deserializer(self):
+        """Should throw a 400 error on corrupt xml."""
+        deserializer = common.MetadataXMLDeserializer()
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                deserializer.deserialize,
+                utils.killer_xml_body())
diff --git a/nova/tests/api/openstack/test_wsgi.py b/nova/tests/api/openstack/test_wsgi.py
index a18dc78d5..374aa1162 100644
--- a/nova/tests/api/openstack/test_wsgi.py
+++ b/nova/tests/api/openstack/test_wsgi.py
@@ -7,6 +7,7 @@ from nova.api.openstack import wsgi
 from nova import exception
 from nova import test
 from nova.tests.api.openstack import fakes
+from nova.tests import utils
 
 
 class RequestTest(test.TestCase):
@@ -272,6 +273,21 @@ class ResourceTest(test.TestCase):
                                                  '<fooAction>true</fooAction>')
         self.assertEqual(controller._action_foo, method)
 
+    def test_get_method_action_corrupt_xml(self):
+        class Controller(wsgi.Controller):
+            @wsgi.action('fooAction')
+            def _action_foo(self, req, id, body):
+                return body
+
+        controller = Controller()
+        resource = wsgi.Resource(controller)
+        self.assertRaises(
+                exception.MalformedRequestBody,
+                resource.get_method,
+                None, 'action',
+                'application/xml',
+                utils.killer_xml_body())
+
     def test_get_method_action_bad_body(self):
         class Controller(wsgi.Controller):
             @wsgi.action('fooAction')
diff --git a/nova/tests/api/openstack/test_xmlutil.py b/nova/tests/api/openstack/test_xmlutil.py
index bd7f24233..3ed6a86fc 100644
--- a/nova/tests/api/openstack/test_xmlutil.py
+++ b/nova/tests/api/openstack/test_xmlutil.py
@@ -16,9 +16,12 @@
 #    under the License.
 
 from lxml import etree
+from xml.dom import minidom
 
 from nova.api.openstack import xmlutil
+from nova import exception
 from nova import test
+from nova.tests import utils as tests_utils
 
 
 class SelectorTest(test.TestCase):
@@ -720,3 +723,64 @@ class MiscellaneousXMLUtilTests(test.TestCase):
         tmpl = xmlutil.MasterTemplate(root, 1)
         result = tmpl.serialize(dict(wrapper=dict(a='foo', b='bar')))
         self.assertEqual(result, expected_xml)
+
+    def test_safe_parse_xml(self):
+
+        normal_body = ("""
+                 <?xml version="1.0" ?><foo>
+                    <bar>
+                        <v1>hey</v1>
+                        <v2>there</v2>
+                    </bar>
+                </foo>""").strip()
+
+        dom = xmlutil.safe_minidom_parse_string(normal_body)
+        self.assertEqual(normal_body, str(dom.toxml()))
+
+        self.assertRaises(exception.MalformedRequestBody,
+                          xmlutil.safe_minidom_parse_string,
+                          tests_utils.killer_xml_body())
+
+
+class SafeParserTestCase(test.TestCase):
+    def test_external_dtd(self):
+        xml_string = ("""<?xml version="1.0" encoding="utf-8"?>
+                <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
+                 "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
+                <html>
+                   <head/>
+                     <body>html with dtd</body>
+                   </html>""")
+
+        parser = xmlutil.ProtectedExpatParser(forbid_dtd=False,
+                                            forbid_entities=True)
+        self.assertRaises(ValueError,
+                          minidom.parseString,
+                          xml_string, parser)
+
+    def test_external_file(self):
+        xml_string = """<!DOCTYPE external [
+                <!ENTITY ee SYSTEM "file:///PATH/TO/root.xml">
+                ]>
+                <root>&ee;</root>"""
+
+        parser = xmlutil.ProtectedExpatParser(forbid_dtd=False,
+                                            forbid_entities=True)
+        self.assertRaises(ValueError,
+                          minidom.parseString,
+                          xml_string, parser)
+
+    def test_notation(self):
+        xml_string = """<?xml version="1.0" standalone="no"?>
+                        <!-- comment data -->
+                        <!DOCTYPE x [
+                        <!NOTATION notation SYSTEM "notation.jpeg">
+                        ]>
+                        <root attr1="value1">
+                        </root>"""
+
+        parser = xmlutil.ProtectedExpatParser(forbid_dtd=False,
+                                            forbid_entities=True)
+        self.assertRaises(ValueError,
+                          minidom.parseString,
+                          xml_string, parser)
diff --git a/nova/tests/test_utils.py b/nova/tests/test_utils.py
index c601bb0af..af6a9b9aa 100644
--- a/nova/tests/test_utils.py
+++ b/nova/tests/test_utils.py
@@ -23,7 +23,6 @@ import os
 import os.path
 import StringIO
 import tempfile
-from xml.dom import minidom
 
 import mox
 import netaddr
@@ -450,39 +449,6 @@ class GenericUtilsTestCase(test.TestCase):
                 self.assertEqual(fake_execute.uid, 2)
             self.assertEqual(fake_execute.uid, os.getuid())
 
-    def test_safe_parse_xml(self):
-
-        normal_body = ("""
-                 <?xml version="1.0" ?><foo>
-                    <bar>
-                        <v1>hey</v1>
-                        <v2>there</v2>
-                    </bar>
-                </foo>""").strip()
-
-        def killer_body():
-            return (("""<!DOCTYPE x [
-                    <!ENTITY a "%(a)s">
-                    <!ENTITY b "%(b)s">
-                    <!ENTITY c "%(c)s">]>
-                <foo>
-                    <bar>
-                        <v1>%(d)s</v1>
-                    </bar>
-                </foo>""") % {
-                'a': 'A' * 10,
-                'b': '&a;' * 10,
-                'c': '&b;' * 10,
-                'd': '&c;' * 9999,
-            }).strip()
-
-        dom = utils.safe_minidom_parse_string(normal_body)
-        self.assertEqual(normal_body, str(dom.toxml()))
-
-        self.assertRaises(ValueError,
-                          utils.safe_minidom_parse_string,
-                          killer_body())
-
     def test_xhtml_escape(self):
         self.assertEqual('&quot;foo&quot;', utils.xhtml_escape('"foo"'))
         self.assertEqual('&apos;foo&apos;', utils.xhtml_escape("'foo'"))
@@ -992,47 +958,3 @@ class StringLengthTestCase(test.TestCase):
         self.assertRaises(exception.InvalidInput,
                           utils.check_string_length,
                           'a' * 256, 'name', max_length=255)
-
-
-class SafeParserTestCase(test.TestCase):
-    def test_external_dtd(self):
-        xml_string = ("""<?xml version="1.0" encoding="utf-8"?>
-                <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
-                 "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
-                <html>
-                   <head/>
-                     <body>html with dtd</body>
-                   </html>""")
-
-        parser = utils.ProtectedExpatParser(forbid_dtd=False,
-                                            forbid_entities=True)
-        self.assertRaises(ValueError,
-                          minidom.parseString,
-                          xml_string, parser)
-
-    def test_external_file(self):
-        xml_string = """<!DOCTYPE external [
-                <!ENTITY ee SYSTEM "file:///PATH/TO/root.xml">
-                ]>
-                <root>&ee;</root>"""
-
-        parser = utils.ProtectedExpatParser(forbid_dtd=False,
-                                            forbid_entities=True)
-        self.assertRaises(ValueError,
-                          minidom.parseString,
-                          xml_string, parser)
-
-    def test_notation(self):
-        xml_string = """<?xml version="1.0" standalone="no"?>
-                        <!-- comment data -->
-                        <!DOCTYPE x [
-                        <!NOTATION notation SYSTEM "notation.jpeg">
-                        ]>
-                        <root attr1="value1">
-                        </root>"""
-
-        parser = utils.ProtectedExpatParser(forbid_dtd=False,
-                                            forbid_entities=True)
-        self.assertRaises(ValueError,
-                          minidom.parseString,
-                          xml_string, parser)
diff --git a/nova/tests/utils.py b/nova/tests/utils.py
index 755d49be1..e9248c7b7 100644
--- a/nova/tests/utils.py
+++ b/nova/tests/utils.py
@@ -183,3 +183,20 @@ def cleanup_dns_managers():
     for manager in test_dns_managers:
         manager.delete_dns_file()
     test_dns_managers = []
+
+
+def killer_xml_body():
+    return (("""<!DOCTYPE x [
+            <!ENTITY a "%(a)s">
+            <!ENTITY b "%(b)s">
+            <!ENTITY c "%(c)s">]>
+        <foo>
+            <bar>
+                <v1>%(d)s</v1>
+            </bar>
+        </foo>""") % {
+        'a': 'A' * 10,
+        'b': '&a;' * 10,
+        'c': '&b;' * 10,
+        'd': '&c;' * 9999,
+    }).strip()