From: Dinesh Bhor <dinesh.bhor@nttdata.com>
Date: Tue, 9 Feb 2016 11:20:26 +0000 (-0800)
Subject: Fix 500 error if 'offset' is out of range
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=31ebbc04a9b7bb1d302c98cc61a33d2e1020c25a;p=openstack-build%2Fcinder-build.git

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

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