From 0bb0ec1da99cc6fdfab069d438d0e38ba5a2c293 Mon Sep 17 00:00:00 2001 From: Arvid Norlander Date: Thu, 30 May 2019 10:36:26 +0200 Subject: [PATCH] Rework SRI flag to be on the urls() method instead of the bundle object. This makes it work better with nested bundles and removes the possibility of mixed settings. This commit also adds unit tests for the bundle class. Part of implementing #494 --- src/webassets/bundle.py | 27 ++--- src/webassets/ext/jinja2.py | 5 +- src/webassets/utils.py | 40 +++++-- tests/test_bundle_urls.py | 204 +++++++++++++++++++++++++++++++++++- 4 files changed, 246 insertions(+), 30 deletions(-) diff --git a/src/webassets/bundle.py b/src/webassets/bundle.py index 77414a0b..c9a0dcea 100644 --- a/src/webassets/bundle.py +++ b/src/webassets/bundle.py @@ -12,7 +12,7 @@ from .exceptions import BundleError, BuildError from .utils import cmp_debug_levels, hash_func from .env import ConfigurationContext, DictConfigStorage, BaseEnvironment -from .utils import is_url, calculate_sri +from .utils import is_url, calculate_sri_on_file __all__ = ('Bundle', 'get_all_bundle_files',) @@ -118,7 +118,6 @@ def __init__(self, *contents, **options): self.remove_duplicates = options.pop('remove_duplicates', True) self.extra = options.pop('extra', {}) self.merge = options.pop('merge', True) - self.calculate_sri = options.pop('calculate_sri', False) self._config = BundleConfig(self) self._config.update(options.pop('config', {})) @@ -738,6 +737,8 @@ def _urls(self, ctx, extra_filters, *args, **kwargs): """Return a list of urls for this bundle, and all subbundles, and, when it becomes necessary, start a build process. """ + # Check if we should calculate SRI + calculate_sri = kwargs.pop('calculate_sri', False) # Look at the debug value to see if this bundle should return the # source urls (in debug mode), or a single url of the bundle in built @@ -765,9 +766,9 @@ def _urls(self, ctx, extra_filters, *args, **kwargs): if ctx.auto_build: self._build(ctx, extra_filters=extra_filters, force=False, *args, **kwargs) - if self.calculate_sri: + if calculate_sri: return [{'uri': self._make_output_url(ctx), - 'sri': calculate_sri(ctx.resolver.resolve_output_to_path(ctx, self.output, self))}] + 'sri': calculate_sri_on_file(ctx.resolver.resolve_output_to_path(ctx, self.output, self))}] else: return [self._make_output_url(ctx)] else: @@ -780,29 +781,31 @@ def _urls(self, ctx, extra_filters, *args, **kwargs): urls.extend(org._urls( wrap(ctx, cnt), merge_filters(extra_filters, self.filters), - *args, **kwargs)) + *args, + calculate_sri=calculate_sri, + **kwargs)) elif is_url(cnt): # Can't calculate SRI for non file - if self.calculate_sri: - urls.append({'uri': cnt}) + if calculate_sri: + urls.append({'uri': cnt, 'sri': None}) else: urls.append(cnt) else: sri = None try: url = ctx.resolver.resolve_source_to_url(ctx, cnt, org) - if self.calculate_sri: - sri = calculate_sri(ctx.resolver.resolve_output_to_path(ctx, cnt, org)) + if calculate_sri: + sri = calculate_sri_on_file(ctx.resolver.resolve_output_to_path(ctx, cnt, org)) except ValueError: # If we cannot generate a url to a path outside the # media directory. So if that happens, we copy the # file into the media directory. external = pull_external(ctx, cnt) url = ctx.resolver.resolve_source_to_url(ctx, external, org) - if self.calculate_sri: - sri = calculate_sri(ctx.resolver.resolve_output_to_path(ctx, external, org)) + if calculate_sri: + sri = calculate_sri_on_file(ctx.resolver.resolve_output_to_path(ctx, external, org)) - if self.calculate_sri: + if calculate_sri: urls.append({'uri': url, 'sri': sri}) else: urls.append(url) diff --git a/src/webassets/ext/jinja2.py b/src/webassets/ext/jinja2.py index 7a2de1ff..defeb9e7 100644 --- a/src/webassets/ext/jinja2.py +++ b/src/webassets/ext/jinja2.py @@ -177,15 +177,14 @@ def _render_assets(self, filter, output, dbg, depends, files, caller=None): 'output': output, 'filters': filter, 'debug': dbg, - 'depends': depends, - 'calculate_sri': True, + 'depends': depends } bundle = self.BundleClass( *self.resolve_contents(files, env), **bundle_kwargs) # Retrieve urls (this may or may not cause a build) with bundle.bind(env): - urls = bundle.urls() + urls = bundle.urls(calculate_sri=True) # For each url, execute the content of this template tag (represented # by the macro ```caller`` given to use by Jinja2). diff --git a/src/webassets/utils.py b/src/webassets/utils.py index 0f3884b8..985f5cec 100644 --- a/src/webassets/utils.py +++ b/src/webassets/utils.py @@ -36,6 +36,14 @@ set = set +try: + FileNotFoundError +except NameError: + FileNotFoundError = IOError +else: + FileNotFoundError = FileNotFoundError + + from webassets.six import StringIO @@ -214,16 +222,28 @@ def is_url(s): return bool(parsed.scheme and parsed.netloc) and len(parsed.scheme) > 1 -def calculate_sri(input): - """Calculate SRI string""" - BUF_SIZE = 65536 +def calculate_sri(data): + """Calculate SRI string for data buffer.""" hash = hashlib.sha384() - with open(input, 'rb') as f: - while True: - data = f.read(BUF_SIZE) - if not data: - break - hash.update(data) + hash.update(data) hash = hash.digest() hash_base64 = base64.b64encode(hash).decode() - return 'sha384-{}'.format(hash_base64) \ No newline at end of file + return 'sha384-{}'.format(hash_base64) + + +def calculate_sri_on_file(file_name): + """Calculate SRI string if file can be found. Otherwise silently return None""" + BUF_SIZE = 65536 + hash = hashlib.sha384() + try: + with open(file_name, 'rb') as f: + while True: + data = f.read(BUF_SIZE) + if not data: + break + hash.update(data) + hash = hash.digest() + hash_base64 = base64.b64encode(hash).decode() + return 'sha384-{}'.format(hash_base64) + except FileNotFoundError: + return None diff --git a/tests/test_bundle_urls.py b/tests/test_bundle_urls.py index 06af96a2..5910d92a 100644 --- a/tests/test_bundle_urls.py +++ b/tests/test_bundle_urls.py @@ -6,14 +6,16 @@ from __future__ import with_statement -from nose.tools import assert_raises, assert_equal from nose import SkipTest +from nose.tools import assert_raises, assert_equal +from tests.test_bundle_build import AppendFilter from webassets import Bundle from webassets.exceptions import BundleError from webassets.test import TempEnvironmentHelper, TempDirHelper -from tests.test_bundle_build import AppendFilter +# SRI for an empty file +_EMPTY_FILE_SRI = 'sha384-OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb' class BaseUrlsTester(TempEnvironmentHelper): @@ -34,15 +36,19 @@ def setup(self): self.build_called = build_called = [] self.makeurl_called = makeurl_called = [] env = self.env + class MockBundle(Bundle): def __init__(self, *a, **kw): Bundle.__init__(self, *a, **kw) self.env = env + def _build(self, *a, **kw): build_called.append(self.output) + def _make_url(self, *a, **kw): makeurl_called.append(self.output) return Bundle._make_url(self, *a, **kw) + self.MockBundle = MockBundle @@ -130,6 +136,23 @@ def test_external_refs(self): assert len(urls) == 1 assert_regex(urls[0], r'.*/webassets-external/[\da-z]*_foo.css') + def test_external_refs_calculate_sri(self): + """If a bundle contains absolute paths outside of the + media directory, to generate a url they are copied in. + """ + try: + from nose.tools import assert_regex + except ImportError: + raise SkipTest("Assertion method only present in 2.7+") + self.env.debug = True + with TempDirHelper() as h: + h.create_files(['foo.css']) + bundle = self.mkbundle(h.path('foo.css')) + urls = bundle.urls(calculate_sri=True) + assert len(urls) == 1 + assert_regex(urls[0]['uri'], r'.*/webassets-external/[\da-z]*_foo.css') + assert urls[0]['sri'] == _EMPTY_FILE_SRI + class TestUrlsWithDebugFalse(BaseUrlsTester): """Test url generation in production mode - everything is always built. @@ -191,11 +214,76 @@ def test_root_debug_true_and_child_debug_false(self): None of this has any effect, since Environment.debug=False. """ bundle = self.MockBundle( - '1', '2', - self.MockBundle('a', output='child1', debug=False), - output='rootout', debug=True) + '1', '2', + self.MockBundle('a', output='child1', debug=False), + output='rootout', debug=True) assert_equal(bundle.urls(), ['/rootout']) + def test_simple_bundle_with_sri(self): + bundle = self.MockBundle('a', 'b', 'c', output='out') + assert bundle.urls(calculate_sri=True) == [{'uri': '/out', 'sri': None}] + assert len(self.build_called) == 1 + + def test_nested_bundle_with_sri(self): + bundle = self.MockBundle( + 'a', self.MockBundle('d', 'childout'), 'c', output='out') + assert bundle.urls(calculate_sri=True) == [{'uri': '/out', 'sri': None}] + assert len(self.build_called) == 1 + + def test_container_bundle_with_sri(self): + """A bundle that has only child bundles and does not specify + an output target of its own will simply build its child + bundles separately. + """ + bundle = self.MockBundle( + self.MockBundle('a', output='child1'), + self.MockBundle('a', output='child2')) + assert bundle.urls(calculate_sri=True) == [{'uri': '/child1', 'sri': None}, + {'uri': '/child2', 'sri': None}] + assert len(self.build_called) == 2 + + def test_source_bundle_with_sri(self): + """If a bundle does neither specify an output target nor any + filters, its file are always sourced directly. + """ + bundle = self.MockBundle('a', self.MockBundle('d', output='childout')) + # The child bundle generates a file, thus we end up being able to actually calculate the SRI in this case. + assert bundle.urls(calculate_sri=True) == [ + {'uri': '/a', 'sri': 'sha384-OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb'}, + {'uri': '/childout', 'sri': None}] + assert len(self.build_called) == 1 + + def test_root_bundle_switching_to_merge_with_sri(self): + """A bundle explicitly says it wants to be merged, wanting to override + the global "debug=False" setting. This is ineffectual (and anyway + does not affect url generation). + """ + bundle = self.MockBundle('1', '2', output='childout', debug='merge') + assert_equal(bundle.urls(calculate_sri=True), [{'uri': '/childout', 'sri': None}]) + assert len(self.build_called) == 1 + + def test_root_bundle_switching_to_debug_true_with_sri(self): + """A bundle explicitly says it wants to be processed in debug + mode, wanting overriding the global "debug=False" setting. This is + ineffectual. + """ + bundle = self.MockBundle('1', '2', output='childout', debug=True) + assert_equal(bundle.urls(calculate_sri=True), [{'uri': '/childout', 'sri': None}]) + assert len(self.build_called) == 1 + + def test_root_debug_true_and_child_debug_false_with_sri(self): + """The root bundle explicitly says it wants to be processed in + debug mode, overriding the global "debug" setting, and a child + bundle asks for debugging to be disabled again. + + None of this has any effect, since Environment.debug=False. + """ + bundle = self.MockBundle( + '1', '2', + self.MockBundle('a', output='child1', debug=False), + output='rootout', debug=True) + assert_equal(bundle.urls(calculate_sri=True), [{'uri': '/rootout', 'sri': None}]) + class TestUrlsWithDebugTrue(BaseUrlsTester): """Test url generation in debug mode. @@ -269,6 +357,80 @@ def test_child_bundle_switching(self): assert_equal(bundle.urls(), ['/a', '/childout', '/c']) assert len(self.build_called) == 1 + def test_simple_bundle_with_sri(self): + bundle = self.MockBundle('a', 'b', 'c', output='out') + assert_equal(bundle.urls(calculate_sri=True), + [{'sri': _EMPTY_FILE_SRI, 'uri': '/a'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/b'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/c'}]) + assert_equal(len(self.build_called), 0) + + def test_nested_bundle_with_sri(self): + bundle = self.MockBundle( + 'a', self.MockBundle('1', '2', output='childout'), 'c', output='out') + assert bundle.urls(calculate_sri=True) == [{'sri': _EMPTY_FILE_SRI, 'uri': '/a'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/1'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/2'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/c'}] + assert len(self.build_called) == 0 + + def test_container_bundle_with_sri(self): + """A bundle that has only sub bundles and does not specify + an output target of its own. + """ + bundle = self.MockBundle( + self.MockBundle('a', output='child1'), + self.MockBundle('a', output='child2')) + assert bundle.urls(calculate_sri=True) == [{'sri': _EMPTY_FILE_SRI, 'uri': '/a'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/a'}] + assert len(self.build_called) == 0 + + def test_url_source_with_sri(self): + """[Regression] Test a Bundle that contains a source URL. + """ + bundle = self.MockBundle('http://test.de', output='out') + assert_equal(bundle.urls(calculate_sri=True), [{'sri': None, 'uri': 'http://test.de'}]) + assert_equal(len(self.build_called), 0) + + # This is the important test. It proves that the url source + # was handled separately, and not processed like any other + # source file, which would be passed through makeurl(). + # This is a bit convoluted to test because the code that + # converts a bundle content into an url operates just fine + # on a url source, so there is no easy other way to determine + # whether the url source was treated special. + assert_equal(len(self.makeurl_called), 0) + + def test_root_bundle_switching_to_debug_false_with_sri(self): + """A bundle explicitly says it wants to be processed with + debug=False, overriding the global "debug=True" setting. + """ + bundle = self.MockBundle('1', '2', output='childout', debug=False) + assert_equal(bundle.urls(calculate_sri=True), [{'sri': None, 'uri': '/childout'}]) + assert len(self.build_called) == 1 + + def test_root_bundle_switching_to_merge_with_sri(self): + """A bundle explicitly says it wants to be merged, overriding + the global "debug=True" setting. + """ + bundle = self.MockBundle('1', '2', output='childout', debug='merge') + assert_equal(bundle.urls(calculate_sri=True), [{'sri': None, 'uri': '/childout'}]) + assert len(self.build_called) == 1 + + def test_child_bundle_switching_with_sri(self): + """A child bundle explicitly says it wants to be processed in + "merge" mode, overriding the global "debug=True" setting, with the + root bundle not having an opinion. + """ + bundle = self.MockBundle( + 'a', self.MockBundle('1', '2', output='childout', debug='merge'), + 'c', output='out') + assert_equal(bundle.urls(calculate_sri=True), + [{'sri': _EMPTY_FILE_SRI, 'uri': '/a'}, + {'sri': None, 'uri': '/childout'}, + {'sri': _EMPTY_FILE_SRI, 'uri': '/c'}]) + assert len(self.build_called) == 1 + class TestUrlsWithDebugMerge(BaseUrlsTester): @@ -307,3 +469,35 @@ def test_root_bundle_switching_to_debug_true(self): 'c', output='out') assert_equal(bundle.urls(), ['/out']) assert len(self.build_called) == 1 + + def test_simple_bundle_with_sri(self): + bundle = self.MockBundle('a', 'b', 'c', output='out') + assert bundle.urls(calculate_sri=True) == [{'sri': None, 'uri': '/out'}] + assert len(self.build_called) == 1 + + def test_nested_bundle_with_sri(self): + bundle = self.MockBundle('a', self.MockBundle('d', 'childout'), 'c', output='out') + assert bundle.urls(calculate_sri=True) == [{'sri': None, 'uri': '/out'}] + assert len(self.build_called) == 1 + + def test_child_bundle_switching_to_debug_false_with_sri(self): + """A child bundle explicitly says it wants to be processed in + full production mode, with overriding the global "debug" setting. + + This makes no difference to the urls that are generated. + """ + bundle = self.MockBundle( + 'a', self.MockBundle('1', '2', output='childout', debug=False), + 'c', output='out') + assert_equal(bundle.urls(calculate_sri=True), [{'sri': None, 'uri': '/out'}]) + assert len(self.build_called) == 1 + + def test_root_bundle_switching_to_debug_true_with_sri(self): + """A bundle explicitly says it wants to be processed in debug mode, + overriding the global ``debug=merge"`` setting. This is ineffectual. + """ + bundle = self.MockBundle( + 'a', self.MockBundle('1', '2', output='childout', debug=True), + 'c', output='out') + assert_equal(bundle.urls(calculate_sri=True), [{'sri': None, 'uri': '/out'}]) + assert len(self.build_called) == 1