From 3478f1e121d84d15558d338a32315f13250cf3bb Mon Sep 17 00:00:00 2001
From: Matthew Sherborne
Date: Tue, 26 Feb 2013 10:35:42 +1000
Subject: 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
---
.../api/openstack/compute/contrib/test_cells.py | 8 +++
.../api/openstack/compute/contrib/test_hosts.py | 8 +++
.../compute/contrib/test_security_groups.py | 22 ++++--
.../api/openstack/compute/test_server_actions.py | 8 +++
nova/tests/api/openstack/compute/test_servers.py | 8 +++
nova/tests/api/openstack/test_common.py | 14 +++-
nova/tests/api/openstack/test_wsgi.py | 16 +++++
nova/tests/api/openstack/test_xmlutil.py | 64 ++++++++++++++++++
nova/tests/test_utils.py | 78 ----------------------
nova/tests/utils.py | 17 +++++
10 files changed, 155 insertions(+), 88 deletions(-)
(limited to 'nova/tests')
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):
'true')
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 = ("""
+
+
+ hey
+ there
+
+ """).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 = ("""
+
+
+
+ html with dtd
+ """)
+
+ parser = xmlutil.ProtectedExpatParser(forbid_dtd=False,
+ forbid_entities=True)
+ self.assertRaises(ValueError,
+ minidom.parseString,
+ xml_string, parser)
+
+ def test_external_file(self):
+ xml_string = """
+ ]>
+ ⅇ"""
+
+ parser = xmlutil.ProtectedExpatParser(forbid_dtd=False,
+ forbid_entities=True)
+ self.assertRaises(ValueError,
+ minidom.parseString,
+ xml_string, parser)
+
+ def test_notation(self):
+ xml_string = """
+
+
+ ]>
+
+ """
+
+ 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 = ("""
-
-
- hey
- there
-
- """).strip()
-
- def killer_body():
- return (("""
-
- ]>
-
-
- %(d)s
-
- """) % {
- '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('"foo"', utils.xhtml_escape('"foo"'))
self.assertEqual(''foo'', 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 = ("""
-
-
-
-