From 31ebbc04a9b7bb1d302c98cc61a33d2e1020c25a Mon Sep 17 00:00:00 2001 From: Dinesh Bhor Date: Tue, 9 Feb 2016 03:20:26 -0800 Subject: [PATCH] Fix 500 error if 'offset' is out of range If large value is passed as offset to snapshots, volumes, backups, consistencygroups and qos_specs list api then it throws 500 internal server error. Moved existing validate_integer() method from cinder.api.openstack.wsgi.Controller to cinder.utils so that it can also be used for validating offset param for integer value in _get_offset_param() method and return 400 error if value is out of range. Please refer: https://bugs.launchpad.net/cinder/+bug/1535708/comments/1 APIImpact: Return 400 status code if offset is out of range. Closes-Bug: #1535708 Change-Id: I07a5292645f24c381217d43b71510b3352964b8b --- cinder/api/common.py | 14 ++-------- cinder/api/contrib/quota_classes.py | 5 ++-- cinder/api/contrib/quotas.py | 2 +- cinder/api/contrib/volume_type_encryption.py | 3 +- cinder/api/openstack/wsgi.py | 27 ------------------ cinder/tests/unit/api/contrib/test_backups.py | 16 +++++++++++ .../api/contrib/test_consistencygroups.py | 11 ++++++++ .../unit/api/contrib/test_qos_specs_manage.py | 6 ++++ cinder/tests/unit/api/contrib/test_quotas.py | 2 +- .../unit/api/contrib/test_quotas_classes.py | 2 +- .../contrib/test_volume_type_encryption.py | 6 ++-- cinder/tests/unit/api/openstack/test_wsgi.py | 18 ------------ cinder/tests/unit/api/test_common.py | 9 ++++++ cinder/tests/unit/api/v2/test_snapshots.py | 7 +++++ cinder/tests/unit/api/v2/test_types.py | 6 ++++ cinder/tests/unit/api/v2/test_volumes.py | 14 ++++++++++ cinder/tests/unit/test_utils.py | 22 +++++++++++++++ cinder/utils.py | 28 +++++++++++++++++++ 18 files changed, 133 insertions(+), 65 deletions(-) diff --git a/cinder/api/common.py b/cinder/api/common.py index eb1f1cae3..b19e12242 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -25,6 +25,7 @@ import webob from cinder.api.openstack import wsgi from cinder.api import xmlutil +import cinder.db from cinder import exception from cinder.i18n import _ import cinder.policy @@ -137,17 +138,8 @@ def _get_marker_param(params): def _get_offset_param(params): """Extract offset id from request's dictionary (defaults to 0) or fail.""" - try: - offset = int(params.pop('offset', 0)) - except ValueError: - msg = _('offset param must be an integer') - raise webob.exc.HTTPBadRequest(explanation=msg) - - if offset < 0: - msg = _('offset param must be positive') - raise webob.exc.HTTPBadRequest(explanation=msg) - - return offset + offset = params.pop('offset', 0) + return utils.validate_integer(offset, 'offset', 0, cinder.db.MAX_INT) def limited(items, request, max_limit=None): diff --git a/cinder/api/contrib/quota_classes.py b/cinder/api/contrib/quota_classes.py index dbb081e52..086d2c36d 100644 --- a/cinder/api/contrib/quota_classes.py +++ b/cinder/api/contrib/quota_classes.py @@ -22,6 +22,7 @@ from cinder import db from cinder import exception from cinder.i18n import _ from cinder import quota +from cinder import utils QUOTAS = quota.QUOTAS @@ -80,8 +81,8 @@ class QuotaClassSetsController(wsgi.Controller): for key, value in body['quota_class_set'].items(): if key in QUOTAS: try: - value = self.validate_integer(value, key, min_value=-1, - max_value=db.MAX_INT) + value = utils.validate_integer(value, key, min_value=-1, + max_value=db.MAX_INT) db.quota_class_update(context, quota_class, key, value) except exception.QuotaClassNotFound: db.quota_class_create(context, quota_class, key, value) diff --git a/cinder/api/contrib/quotas.py b/cinder/api/contrib/quotas.py index 788d3f830..84016d87d 100644 --- a/cinder/api/contrib/quotas.py +++ b/cinder/api/contrib/quotas.py @@ -269,7 +269,7 @@ class QuotaSetsController(wsgi.Controller): if key in NON_QUOTA_KEYS: continue - value = self.validate_integer( + value = utils.validate_integer( body['quota_set'][key], key, min_value=-1, max_value=db.MAX_INT) diff --git a/cinder/api/contrib/volume_type_encryption.py b/cinder/api/contrib/volume_type_encryption.py index 5ffc7d026..7750aa090 100644 --- a/cinder/api/contrib/volume_type_encryption.py +++ b/cinder/api/contrib/volume_type_encryption.py @@ -24,6 +24,7 @@ from cinder import db from cinder import exception from cinder.i18n import _ from cinder import rpc +from cinder import utils from cinder.volume import volume_types authorize = extensions.extension_authorizer('volume', @@ -58,7 +59,7 @@ class VolumeTypeEncryptionController(wsgi.Controller): def _check_encryption_input(self, encryption, create=True): if encryption.get('key_size') is not None: - encryption['key_size'] = self.validate_integer( + encryption['key_size'] = utils.validate_integer( encryption['key_size'], 'key_size', min_value=0, max_value=db.MAX_INT) diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py index dd3df2d27..5e966756b 100644 --- a/cinder/api/openstack/wsgi.py +++ b/cinder/api/openstack/wsgi.py @@ -1496,33 +1496,6 @@ class Controller(object): except exception.InvalidInput as error: raise webob.exc.HTTPBadRequest(explanation=error.msg) - @staticmethod - def validate_integer(value, name, min_value=None, max_value=None): - """Make sure that value is a valid integer, potentially within range. - - :param value: the value of the integer - :param name: the name of the integer - :param min_length: the min_length of the integer - :param max_length: the max_length of the integer - :returns: integer - """ - try: - value = int(value) - except (TypeError, ValueError, UnicodeEncodeError): - raise webob.exc.HTTPBadRequest(explanation=( - _('%s must be an integer.') % name)) - - if min_value is not None and value < min_value: - raise webob.exc.HTTPBadRequest( - explanation=(_('%(value_name)s must be >= %(min_value)d') % - {'value_name': name, 'min_value': min_value})) - if max_value is not None and value > max_value: - raise webob.exc.HTTPBadRequest( - explanation=(_('%(value_name)s must be <= %(max_value)d') % - {'value_name': name, 'max_value': max_value})) - - return value - class Fault(webob.exc.HTTPException): """Wrap webob.exc.HTTPException to provide API friendly response.""" diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 031ffb4cd..042ddb4ab 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -250,6 +250,14 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id2) db.backup_destroy(context.get_admin_context(), backup_id1) + def test_list_backups_with_offset_out_of_range(self): + url = '/v2/fake/backups?offset=252452434242342434' + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + self.assertEqual(400, res.status_int) + def test_list_backups_with_marker(self): backup_id1 = self._create_backup() backup_id2 = self._create_backup() @@ -553,6 +561,14 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id2) db.backup_destroy(context.get_admin_context(), backup_id1) + def test_list_backups_detail_with_offset_out_of_range(self): + url = '/v2/fake/backups/detail?offset=234534543657634523' + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + self.assertEqual(400, res.status_int) + @mock.patch('cinder.db.service_get_all_by_topic') @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_name_and_description') diff --git a/cinder/tests/unit/api/contrib/test_consistencygroups.py b/cinder/tests/unit/api/contrib/test_consistencygroups.py index ecb5a2a17..54c6cb73e 100644 --- a/cinder/tests/unit/api/contrib/test_consistencygroups.py +++ b/cinder/tests/unit/api/contrib/test_consistencygroups.py @@ -248,6 +248,17 @@ class ConsistencyGroupsAPITestCase(test.TestCase): consistencygroup2.destroy() consistencygroup3.destroy() + @ddt.data(False, True) + def test_list_consistencygroups_with_offset_out_of_range(self, is_detail): + url = '/v2/fake/consistencygroups?offset=234523423455454' + if is_detail: + url = '/v2/fake/consistencygroups/detail?offset=234523423455454' + req = webob.Request.blank(url) + req.method = 'GET' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + self.assertEqual(400, res.status_int) + @ddt.data(False, True) def test_list_consistencygroups_with_limit_and_offset(self, is_detail): consistencygroup1 = self._create_consistencygroup() diff --git a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py index c22b09d6c..62306778b 100644 --- a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py +++ b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py @@ -227,6 +227,12 @@ class QoSSpecManageApiTest(test.TestCase): self.assertEqual(3, len(res['qos_specs'])) + def test_index_with_offset_out_of_range(self): + url = '/v2/fake/qos-specs?offset=356576877698707' + req = fakes.HTTPRequest.blank(url, use_admin_context=True) + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.index, + req) + def test_index_with_limit_and_offset(self): url = '/v2/fake/qos-specs?limit=2&offset=1' req = fakes.HTTPRequest.blank(url, use_admin_context=True) diff --git a/cinder/tests/unit/api/contrib/test_quotas.py b/cinder/tests/unit/api/contrib/test_quotas.py index b691dc249..94d0e8382 100644 --- a/cinder/tests/unit/api/contrib/test_quotas.py +++ b/cinder/tests/unit/api/contrib/test_quotas.py @@ -224,7 +224,7 @@ class QuotaSetsControllerTest(QuotaSetsControllerTestBase): @mock.patch( 'cinder.api.openstack.wsgi.Controller.validate_string_length') @mock.patch( - 'cinder.api.openstack.wsgi.Controller.validate_integer') + 'cinder.utils.validate_integer') def test_update_limit(self, mock_validate_integer, mock_validate): mock_validate_integer.return_value = 10 diff --git a/cinder/tests/unit/api/contrib/test_quotas_classes.py b/cinder/tests/unit/api/contrib/test_quotas_classes.py index 7f39882b1..3792bac70 100644 --- a/cinder/tests/unit/api/contrib/test_quotas_classes.py +++ b/cinder/tests/unit/api/contrib/test_quotas_classes.py @@ -110,7 +110,7 @@ class QuotaClassSetsControllerTest(test.TestCase): self.assertDictMatch(body, result) @mock.patch('cinder.api.openstack.wsgi.Controller.validate_string_length') - @mock.patch('cinder.api.openstack.wsgi.Controller.validate_integer') + @mock.patch('cinder.utils.validate_integer') def test_update_limit(self, mock_validate_integer, mock_validate): mock_validate_integer.return_value = 5 volume_types.create(self.ctxt, 'fake_type') diff --git a/cinder/tests/unit/api/contrib/test_volume_type_encryption.py b/cinder/tests/unit/api/contrib/test_volume_type_encryption.py index 9cf51cf19..27144499e 100644 --- a/cinder/tests/unit/api/contrib/test_volume_type_encryption.py +++ b/cinder/tests/unit/api/contrib/test_volume_type_encryption.py @@ -18,11 +18,11 @@ import mock from oslo_serialization import jsonutils import webob -from cinder.api.openstack import wsgi from cinder import context from cinder import db from cinder import test from cinder.tests.unit.api import fakes +from cinder import utils def return_volume_type_encryption(context, volume_type_id): @@ -199,7 +199,7 @@ class VolumeTypeEncryptionTest(test.TestCase): db.volume_type_destroy(context.get_admin_context(), volume_type['id']) def test_create_json(self): - with mock.patch.object(wsgi.Controller, + with mock.patch.object(utils, 'validate_integer') as mock_validate_integer: mock_validate_integer.return_value = 128 self._create('fake_cipher', 'front-end', 128, 'fake_encryptor') @@ -500,7 +500,7 @@ class VolumeTypeEncryptionTest(test.TestCase): self.assertEqual(expected, jsonutils.loads(res.body)) db.volume_type_destroy(context.get_admin_context(), volume_type['id']) - @mock.patch('cinder.api.openstack.wsgi.Controller.validate_integer') + @mock.patch('cinder.utils.validate_integer') def test_update_item(self, mock_validate_integer): mock_validate_integer.return_value = 512 volume_type = self._default_volume_type diff --git a/cinder/tests/unit/api/openstack/test_wsgi.py b/cinder/tests/unit/api/openstack/test_wsgi.py index 5186ce779..a4ef22949 100644 --- a/cinder/tests/unit/api/openstack/test_wsgi.py +++ b/cinder/tests/unit/api/openstack/test_wsgi.py @@ -1060,21 +1060,3 @@ class ValidBodyTest(test.TestCase): body = {'name': 'a' * 255 + " "} self.controller.validate_name_and_description(body) self.assertEqual('a' * 255, body['name']) - - def test_validate_integer_greater_than_max_int_limit(self): - value = (2 ** 31) + 1 - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.validate_integer, - value, 'limit', min_value=-1, max_value=(2 ** 31)) - - def test_validate_integer_less_than_min_int_limit(self): - value = -12 - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.validate_integer, - value, 'limit', min_value=-1, max_value=(2 ** 31)) - - def test_validate_integer_invalid_limit(self): - value = "should_be_int" - self.assertRaises(webob.exc.HTTPBadRequest, - self.controller.validate_integer, - value, 'limit', min_value=-1, max_value=(2 ** 31)) diff --git a/cinder/tests/unit/api/test_common.py b/cinder/tests/unit/api/test_common.py index 915983776..d4b8dc355 100644 --- a/cinder/tests/unit/api/test_common.py +++ b/cinder/tests/unit/api/test_common.py @@ -80,6 +80,12 @@ class LimiterTest(test.TestCase): self.assertRaises( webob.exc.HTTPBadRequest, common.limited, self.tiny, req) + def test_limiter_offset_out_of_range(self): + """Test offset key works with a offset out of range.""" + req = webob.Request.blank('/?offset=123456789012346456') + self.assertRaises( + webob.exc.HTTPBadRequest, common.limited, self.tiny, req) + def test_limiter_offset_bad(self): """Test offset key works with a BAD offset.""" req = webob.Request.blank(u'/?offset=\u0020aa') @@ -135,6 +141,9 @@ class LimiterTest(test.TestCase): self.assertEqual(items[3:1003], common.limited(items, req)) req = webob.Request.blank('/?offset=3000&limit=10') self.assertEqual([], common.limited(items, req)) + req = webob.Request.blank('/?offset=30034522235674530&limit=10') + self.assertRaises( + webob.exc.HTTPBadRequest, common.limited, items, req) def test_limiter_custom_max_limit(self): """Test a max_limit other than 1000.""" diff --git a/cinder/tests/unit/api/v2/test_snapshots.py b/cinder/tests/unit/api/v2/test_snapshots.py index 8cffcf775..f7ebae7e7 100644 --- a/cinder/tests/unit/api/v2/test_snapshots.py +++ b/cinder/tests/unit/api/v2/test_snapshots.py @@ -401,6 +401,13 @@ class SnapshotApiTest(test.TestCase): self.controller.index, req) + # Test that we get an exception HTTPBadRequest(400) with an offset + # greater than the maximum offset value. + url = '/v2/snapshots?limit=1&offset=323245324356534235' + req = fakes.HTTPRequest.blank(url) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.index, req) + def _assert_list_next(self, expected_query=None, project='fakeproject', **kwargs): """Check a page of snapshots list.""" diff --git a/cinder/tests/unit/api/v2/test_types.py b/cinder/tests/unit/api/v2/test_types.py index 981ace027..14aaea428 100644 --- a/cinder/tests/unit/api/v2/test_types.py +++ b/cinder/tests/unit/api/v2/test_types.py @@ -153,6 +153,12 @@ class VolumeTypesApiTest(test.TestCase): self.assertEqual(2, len(res['volume_types'])) + def test_volume_types_index_with_offset_out_of_range(self): + url = '/v2/fake/types?offset=424366766556787' + req = fakes.HTTPRequest.blank(url) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.index, req) + def test_volume_types_index_with_limit_and_offset(self): req = fakes.HTTPRequest.blank('/v2/fake/types?limit=2&offset=1') req.environ['cinder.context'] = self.ctxt diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 265286948..00c36b724 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -914,6 +914,14 @@ class VolumeApiTest(test.TestCase): self.controller.index, req) + # Test that we get an exception HTTPBadRequest(400) with an offset + # greater than the maximum offset value. + url = '/v2/volumes?limit=2&offset=43543564546567575' + req = fakes.HTTPRequest.blank(url) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.index, + req) + def test_volume_detail_with_marker(self): def stub_volume_get_all_by_project(context, project_id, marker, limit, sort_keys=None, sort_dirs=None, @@ -1005,6 +1013,12 @@ class VolumeApiTest(test.TestCase): self.controller.detail, req) + url = '/v2/volumes/detail?limit=2&offset=4536546546546467' + req = fakes.HTTPRequest.blank(url) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.detail, + req) + def test_volume_with_limit_zero(self): def stub_volume_get_all(context, marker, limit, **kwargs): return [] diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index e4b16722b..043b856c7 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -24,6 +24,7 @@ from oslo_config import cfg from oslo_utils import timeutils import six from six.moves import range +import webob.exc import cinder from cinder import exception @@ -1401,3 +1402,24 @@ class TestComparableMixin(test.TestCase): def test_compare(self): self.assertEqual(NotImplemented, self.one._compare(1, self.one._cmpkey)) + + +class TestValidateInteger(test.TestCase): + + def test_validate_integer_greater_than_max_int_limit(self): + value = (2 ** 31) + 1 + self.assertRaises(webob.exc.HTTPBadRequest, + utils.validate_integer, + value, 'limit', min_value=-1, max_value=(2 ** 31)) + + def test_validate_integer_less_than_min_int_limit(self): + value = -12 + self.assertRaises(webob.exc.HTTPBadRequest, + utils.validate_integer, + value, 'limit', min_value=-1, max_value=(2 ** 31)) + + def test_validate_integer_invalid_limit(self): + value = "should_be_int" + self.assertRaises(webob.exc.HTTPBadRequest, + utils.validate_integer, + value, 'limit', min_value=-1, max_value=(2 ** 31)) diff --git a/cinder/utils.py b/cinder/utils.py index 3476912a2..77305c63e 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -53,6 +53,7 @@ from oslo_utils import strutils from oslo_utils import timeutils import retrying import six +import webob.exc from cinder import exception from cinder.i18n import _, _LE, _LW @@ -1057,3 +1058,30 @@ def calculate_virtual_free_capacity(total_capacity, # account the reserved space. free = free_capacity - math.floor(total * reserved) return free + + +def validate_integer(value, name, min_value=None, max_value=None): + """Make sure that value is a valid integer, potentially within range. + + :param value: the value of the integer + :param name: the name of the integer + :param min_length: the min_length of the integer + :param max_length: the max_length of the integer + :returns: integer + """ + try: + value = int(value) + except (TypeError, ValueError, UnicodeEncodeError): + raise webob.exc.HTTPBadRequest(explanation=( + _('%s must be an integer.') % name)) + + if min_value is not None and value < min_value: + raise webob.exc.HTTPBadRequest( + explanation=(_('%(value_name)s must be >= %(min_value)d') % + {'value_name': name, 'min_value': min_value})) + if max_value is not None and value > max_value: + raise webob.exc.HTTPBadRequest( + explanation=(_('%(value_name)s must be <= %(max_value)d') % + {'value_name': name, 'max_value': max_value})) + + return value -- 2.45.2