From 62b5ebc718b914751883f7ec8297819648fc3e79 Mon Sep 17 00:00:00 2001 From: Drew Varner Date: Tue, 3 Nov 2015 13:39:04 -0600 Subject: [PATCH] Assert problems in Glance raised by Bandit Fix all assert problems raised by Bandit. Asserts are potentially problematic, since Python optimization sometimes removes them, so code needs to remain safe and functional without the assert. Two asserts are safe to skip, so they are deleted for improved error messages. Three asserts are probably necessary, and are converted to exceptions. Two asserts are probably necessary, and are instead made to fail safely, and `# nosec` is added to the assert line. This also enables the assert test in bandit's configuration. Change-Id: Ic69a204ceb15cac234c6b6bca3d950256a98016d Partial-bug: 1511862 --- bandit.yaml | 2 +- glance/api/v2/images.py | 1 - glance/common/rpc.py | 3 +-- glance/db/__init__.py | 5 +++-- glance/db/sqlalchemy/api.py | 12 ++++++++---- glance/db/sqlalchemy/artifacts.py | 10 ++++++---- .../versions/015_quote_swift_credentials.py | 4 +++- .../017_quote_encrypted_swift_credentials.py | 4 +++- glance/tests/unit/common/test_rpc.py | 2 +- glance/tests/unit/test_migrations.py | 2 +- glance/tests/unit/v2/test_images_resource.py | 4 ++-- 11 files changed, 29 insertions(+), 20 deletions(-) diff --git a/bandit.yaml b/bandit.yaml index 701ada5108..c99c2a11e8 100644 --- a/bandit.yaml +++ b/bandit.yaml @@ -33,7 +33,7 @@ profiles: include: - any_other_function_with_shell_equals_true - # - assert_used # TODO: enable this test + - assert_used - blacklist_calls - blacklist_import_func diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index bb7949c3bd..5de26d6ea5 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -144,7 +144,6 @@ class ImagesController(object): for change in changes: change_method_name = '_do_%s' % change['op'] - assert hasattr(self, change_method_name) change_method = getattr(self, change_method_name) change_method(req, image, change) diff --git a/glance/common/rpc.py b/glance/common/rpc.py index 4a6c6685ba..9363293c59 100644 --- a/glance/common/rpc.py +++ b/glance/common/rpc.py @@ -114,7 +114,7 @@ class Controller(object): :params excluded: List of methods to exclude. :params refiner: Callable to use as filter for methods. - :raises: AssertionError: If refiner is not callable. + :raises TypeError: If refiner is not callable. """ funcs = filter(lambda x: not x.startswith("_"), dir(resource)) @@ -126,7 +126,6 @@ class Controller(object): funcs = [f for f in funcs if f not in excluded] if refiner: - assert callable(refiner), "Refiner must be callable" funcs = filter(refiner, funcs) for name in funcs: diff --git a/glance/db/__init__.py b/glance/db/__init__.py index 9dd600d13b..f54d4f4a26 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -163,8 +163,9 @@ class ImageRepo(object): def get(self, image_id): try: db_api_image = dict(self.db_api.image_get(self.context, image_id)) - assert not db_api_image['deleted'] - except (exception.ImageNotFound, exception.Forbidden, AssertionError): + if db_api_image['deleted']: + raise exception.ImageNotFound() + except (exception.ImageNotFound, exception.Forbidden): msg = _("No image found with ID %s") % image_id raise exception.ImageNotFound(msg) tags = self.db_api.image_tag_get_all(self.context, image_id) diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index 796764a948..3e3fc66639 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -330,17 +330,21 @@ def _paginate_query(query, model, limit, sort_keys, marker=None, # the actual primary key, rather than assuming its id LOG.warn(_LW('Id not in sort_keys; is sort_keys unique?')) - assert(not (sort_dir and sort_dirs)) + assert(not (sort_dir and sort_dirs)) # nosec + # nosec: This function runs safely if the assertion fails. # Default the sort direction to ascending - if sort_dirs is None and sort_dir is None: + if sort_dir is None: sort_dir = 'asc' # Ensure a per-column sort direction if sort_dirs is None: - sort_dirs = [sort_dir for _sort_key in sort_keys] + sort_dirs = [sort_dir] * len(sort_keys) - assert(len(sort_dirs) == len(sort_keys)) + assert(len(sort_dirs) == len(sort_keys)) # nosec + # nosec: This function runs safely if the assertion fails. + if len(sort_dirs) < len(sort_keys): + sort_dirs += [sort_dir] * (len(sort_keys) - len(sort_dirs)) # Add sorting for current_sort_key, current_sort_dir in zip(sort_keys, sort_dirs): diff --git a/glance/db/sqlalchemy/artifacts.py b/glance/db/sqlalchemy/artifacts.py index a2f94f60f4..e61ad22c0b 100644 --- a/glance/db/sqlalchemy/artifacts.py +++ b/glance/db/sqlalchemy/artifacts.py @@ -328,14 +328,16 @@ def _get_all(context, session, filters=None, marker=None, def _do_paginate_query(query, sort_keys=None, sort_dirs=None, marker=None, limit=None): # Default the sort direction to ascending - if sort_dirs is None: - sort_dir = 'asc' + sort_dir = 'asc' # Ensure a per-column sort direction if sort_dirs is None: - sort_dirs = [sort_dir for _sort_key in sort_keys] + sort_dirs = [sort_dir] * len(sort_keys) - assert(len(sort_dirs) == len(sort_keys)) + assert(len(sort_dirs) == len(sort_keys)) # nosec + # nosec: This function runs safely if the assertion fails. + if len(sort_dirs) < len(sort_keys): + sort_dirs += [sort_dir] * (len(sort_keys) - len(sort_dirs)) # Add sorting for current_sort_key, current_sort_dir in zip(sort_keys, sort_dirs): diff --git a/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py b/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py index 6b5ac87f8c..8cb54b2b0a 100644 --- a/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py +++ b/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py @@ -98,7 +98,9 @@ def legacy_parse_uri(uri, to_quote): raise exception.BadStoreUri(message=reason) pieces = urlparse.urlparse(uri) - assert pieces.scheme in ('swift', 'swift+http', 'swift+https') + if pieces.scheme not in ('swift', 'swift+http', 'swift+https'): + raise exception.BadStoreUri(message="Unacceptable scheme: '%s'" % + pieces.scheme) scheme = pieces.scheme netloc = pieces.netloc path = pieces.path.lstrip('/') diff --git a/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py b/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py index b7a93af59c..7347daa4bb 100644 --- a/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py +++ b/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py @@ -162,7 +162,9 @@ def legacy_parse_uri(uri, to_quote): raise exception.BadStoreUri(message=reason) pieces = urlparse.urlparse(uri) - assert pieces.scheme in ('swift', 'swift+http', 'swift+https') + if pieces.scheme not in ('swift', 'swift+http', 'swift+https'): + raise exception.BadStoreUri(message="Unacceptable scheme: '%s'" % + pieces.scheme) scheme = pieces.scheme netloc = pieces.netloc path = pieces.path.lstrip('/') diff --git a/glance/tests/unit/common/test_rpc.py b/glance/tests/unit/common/test_rpc.py index 17113ddedc..8ae6c7d2d7 100644 --- a/glance/tests/unit/common/test_rpc.py +++ b/glance/tests/unit/common/test_rpc.py @@ -108,7 +108,7 @@ class TestRPCController(base.IsolatedUnitTest): controller = rpc.Controller() # Not callable - self.assertRaises(AssertionError, + self.assertRaises(TypeError, controller.register, res, refiner="get_all_images") diff --git a/glance/tests/unit/test_migrations.py b/glance/tests/unit/test_migrations.py index 2a2a703756..7963603215 100644 --- a/glance/tests/unit/test_migrations.py +++ b/glance/tests/unit/test_migrations.py @@ -420,7 +420,7 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): invalid_scheme_uri = ('http://acct:usr:pass@example.com' '/container/obj-id') - self.assertRaises(AssertionError, + self.assertRaises(exception.BadStoreUri, legacy_parse_uri_fn, invalid_scheme_uri, True) diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index c5c5a2e77f..27a3fcc743 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1899,8 +1899,8 @@ class TestImagesController(base.IsolatedUnitTest): change = {'op': 'test', 'path': 'options', 'value': 'puts'} try: self.controller.update(request, UUID1, [change]) - except AssertionError: - pass # AssertionError is the desired behavior + except AttributeError: + pass # AttributeError is the desired behavior else: self.fail('Failed to raise AssertionError on %s' % change)