openstack/cinder
Code Issues Proposed changes

Fix 500 error if 'offset' is out of range

Browse Source 
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
This commit is contained in:
Dinesh Bhor 2016-02-09 03:20:26 -08:00 committed by dineshbhor
parent 6b86441015
commit 31ebbc04a9
18 changed files with 133 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
cinder/api/common.py
View File

@ -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):

5
cinder/api/contrib/quota_classes.py
View File

@ -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)

2
cinder/api/contrib/quotas.py
View File

@ -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)

3
cinder/api/contrib/volume_type_encryption.py
View File

@ -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)

27
cinder/api/openstack/wsgi.py
View File

@ -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."""

16
cinder/tests/unit/api/contrib/test_backups.py
View File

@ -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')

11
cinder/tests/unit/api/contrib/test_consistencygroups.py
View File

@ -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()

6
cinder/tests/unit/api/contrib/test_qos_specs_manage.py
View File

@ -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)

2
cinder/tests/unit/api/contrib/test_quotas.py
View File

@ -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

2
cinder/tests/unit/api/contrib/test_quotas_classes.py
View File

@ -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')

6
cinder/tests/unit/api/contrib/test_volume_type_encryption.py
View File

@ -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

18
cinder/tests/unit/api/openstack/test_wsgi.py
View File

@ -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))

9
cinder/tests/unit/api/test_common.py
View File

@ -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."""

7
cinder/tests/unit/api/v2/test_snapshots.py
View File

@ -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."""

6
cinder/tests/unit/api/v2/test_types.py
View File

@ -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

14
cinder/tests/unit/api/v2/test_volumes.py
View File

@ -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 []

22
cinder/tests/unit/test_utils.py
View File

@ -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))

28
cinder/utils.py
View File

@ -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