From fe6610c221d046fd8f4f7c5e186e71281bdf6e5e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Nov 2018 11:29:52 +0000 Subject: [PATCH 1/4] Fix content security policy for the CDN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CDN URLs aren’t in included in the content security policy. So browsers will refuse to load them. This commit: - adds each of the CDN URLs to the - only prepend URLs in CSS files with `/static/` if we’re running locally (because the CDN URLs are like `static.example.com` not `example.com/static`) --- app/__init__.py | 17 ++++++++++++----- app/config.py | 8 ++++++++ gulpfile.babel.js | 2 +- tests/app/main/views/test_headers.py | 18 ++++++++++-------- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 79899c5ca..c32f45ef5 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -496,17 +496,24 @@ def save_service_or_org_after_request(response): # https://www.owasp.org/index.php/List_of_useful_HTTP_headers def useful_headers_after_request(response): + notify_environment = os.environ['NOTIFY_ENVIRONMENT'] response.headers.add('X-Frame-Options', 'deny') response.headers.add('X-Content-Type-Options', 'nosniff') response.headers.add('X-XSS-Protection', '1; mode=block') response.headers.add('Content-Security-Policy', ( - "default-src 'self' 'unsafe-inline';" - "script-src 'self' *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "default-src 'self' {} 'unsafe-inline';" + "script-src 'self' {} *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" "connect-src 'self' *.google-analytics.com;" "object-src 'self';" - "font-src 'self' data:;" - "img-src 'self' *.google-analytics.com *.notifications.service.gov.uk {} data:;" - "frame-src 'self' www.youtube.com;".format(get_cdn_domain()) + "font-src 'self' {} data:;" + "img-src 'self' {} *.google-analytics.com *.notifications.service.gov.uk {} data:;" + "frame-src 'self' www.youtube.com;".format( + configs[notify_environment].ASSET_DOMAIN, + configs[notify_environment].ASSET_DOMAIN, + configs[notify_environment].ASSET_DOMAIN, + configs[notify_environment].ASSET_DOMAIN, + get_cdn_domain(), + ) )) if 'Cache-Control' in response.headers: del response.headers['Cache-Control'] diff --git a/app/config.py b/app/config.py index 2db0dfddb..202a4939c 100644 --- a/app/config.py +++ b/app/config.py @@ -74,6 +74,7 @@ class Config(object): REDIS_URL = os.environ.get('REDIS_URL') REDIS_ENABLED = os.environ.get('REDIS_ENABLED') == '1' + ASSET_DOMAIN = '' ASSET_PATH = '/static/' @@ -94,6 +95,8 @@ class Development(Config): ANTIVIRUS_API_HOST = 'http://localhost:6016' ANTIVIRUS_API_KEY = 'test-key' + ASSET_PATH = '/static/' + class Test(Development): DEBUG = True @@ -109,6 +112,8 @@ class Test(Development): ANTIVIRUS_API_HOST = 'https://test-antivirus' ANTIVIRUS_API_KEY = 'test-antivirus-secret' + ASSET_DOMAIN = 'static.example.com' + class Preview(Config): HTTP_PROTOCOL = 'https' @@ -119,6 +124,7 @@ class Preview(Config): MOU_BUCKET_NAME = 'notify.works-mou' NOTIFY_ENVIRONMENT = 'preview' CHECK_PROXY_HEADER = False + ASSET_DOMAIN = 'static.notify.works' ASSET_PATH = 'https://static.notify.works/' @@ -132,6 +138,7 @@ class Staging(Config): MOU_BUCKET_NAME = 'staging-notify.works-mou' NOTIFY_ENVIRONMENT = 'staging' CHECK_PROXY_HEADER = False + ASSET_DOMAIN = 'static.staging-notify.works' ASSET_PATH = 'https://static.staging-notify.works/' @@ -145,6 +152,7 @@ class Live(Config): MOU_BUCKET_NAME = 'notifications.service.gov.uk-mou' NOTIFY_ENVIRONMENT = 'live' CHECK_PROXY_HEADER = False + ASSET_DOMAIN = 'static.notifications.service.gov.uk' ASSET_PATH = 'https://static.notifications.service.gov.uk/' diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 73d7e2375..da3176d02 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -37,7 +37,7 @@ gulp.task('copy:govuk_template:css', () => gulp.src(paths.template + 'assets/sty })) .on('error', plugins.sass.logError) .pipe(plugins.cssUrlAdjuster({ - prependRelative: '/static/', + prependRelative: process.env.NOTIFY_ADMIN_ENVIRONMENT == 'config.Development' ? '/static/' : '/', })) .pipe(gulp.dest(paths.dist + 'stylesheets/')) ); diff --git a/tests/app/main/views/test_headers.py b/tests/app/main/views/test_headers.py index 1ef2a794c..95e2c8d8e 100644 --- a/tests/app/main/views/test_headers.py +++ b/tests/app/main/views/test_headers.py @@ -8,12 +8,13 @@ def test_owasp_useful_headers_set(client, mocker): assert response.headers['X-Content-Type-Options'] == 'nosniff' assert response.headers['X-XSS-Protection'] == '1; mode=block' assert response.headers['Content-Security-Policy'] == ( - "default-src 'self' 'unsafe-inline';" - "script-src 'self' *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "default-src 'self' static.example.com 'unsafe-inline';" + "script-src 'self' static.example.com *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" "connect-src 'self' *.google-analytics.com;" "object-src 'self';" - "font-src 'self' data:;" - "img-src 'self' *.google-analytics.com *.notifications.service.gov.uk static-logos.test.com data:;" + "font-src 'self' static.example.com data:;" + "img-src " + "'self' static.example.com *.google-analytics.com *.notifications.service.gov.uk static-logos.test.com data:;" "frame-src 'self' www.youtube.com;" ) @@ -25,11 +26,12 @@ def test_headers_non_ascii_characters_are_replaced(client, mocker): assert response.status_code == 200 assert response.headers['Content-Security-Policy'] == ( - "default-src 'self' 'unsafe-inline';" - "script-src 'self' *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "default-src 'self' static.example.com 'unsafe-inline';" + "script-src 'self' static.example.com *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" "connect-src 'self' *.google-analytics.com;" "object-src 'self';" - "font-src 'self' data:;" - "img-src 'self' *.google-analytics.com *.notifications.service.gov.uk static-logos??.test.com data:;" + "font-src 'self' static.example.com data:;" + "img-src " + "'self' static.example.com *.google-analytics.com *.notifications.service.gov.uk static-logos??.test.com data:;" "frame-src 'self' www.youtube.com;" ) From bc6b9c7af7909a7b8b1108c88e54dcf743e61a67 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Nov 2018 11:41:13 +0000 Subject: [PATCH 2/4] Use named arguments for clearer string formatting Helps when the string is long. Also helps disambiguate between the CDN domain used for the logos and those for CSS/JS. --- app/__init__.py | 17 +++++++---------- app/main/views/email_branding.py | 6 +++--- app/main/views/index.py | 4 ++-- app/main/views/service_settings.py | 4 ++-- app/utils.py | 2 +- tests/app/main/views/test_headers.py | 4 ++-- tests/app/test_utils.py | 6 +++--- 7 files changed, 20 insertions(+), 23 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c32f45ef5..0797204ce 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -70,7 +70,7 @@ from app.notify_client.complaint_api_client import complaint_api_client from app.notify_client.platform_stats_api_client import platform_stats_api_client from app.notify_client.template_folder_api_client import template_folder_api_client from app.commands import setup_commands -from app.utils import get_cdn_domain, id_safe +from app.utils import get_logo_cdn_domain, id_safe login_manager = LoginManager() csrf = CSRFProtect() @@ -501,18 +501,15 @@ def useful_headers_after_request(response): response.headers.add('X-Content-Type-Options', 'nosniff') response.headers.add('X-XSS-Protection', '1; mode=block') response.headers.add('Content-Security-Policy', ( - "default-src 'self' {} 'unsafe-inline';" - "script-src 'self' {} *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "default-src 'self' {asset_domain} 'unsafe-inline';" + "script-src 'self' {asset_domain} *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" "connect-src 'self' *.google-analytics.com;" "object-src 'self';" - "font-src 'self' {} data:;" - "img-src 'self' {} *.google-analytics.com *.notifications.service.gov.uk {} data:;" + "font-src 'self' {asset_domain} data:;" + "img-src 'self' {asset_domain} *.google-analytics.com *.notifications.service.gov.uk {logo_domain} data:;" "frame-src 'self' www.youtube.com;".format( - configs[notify_environment].ASSET_DOMAIN, - configs[notify_environment].ASSET_DOMAIN, - configs[notify_environment].ASSET_DOMAIN, - configs[notify_environment].ASSET_DOMAIN, - get_cdn_domain(), + asset_domain=configs[notify_environment].ASSET_DOMAIN, + logo_domain=get_logo_cdn_domain(), ) )) if 'Cache-Control' in response.headers: diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 28851a13a..307575930 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -11,7 +11,7 @@ from app.main.s3_client import ( persist_logo, upload_logo, ) -from app.utils import AgreementInfo, get_cdn_domain, user_is_platform_admin +from app.utils import AgreementInfo, get_logo_cdn_domain, user_is_platform_admin @main.route("/email-branding", methods=['GET', 'POST']) @@ -81,7 +81,7 @@ def update_email_branding(branding_id, logo=None): 'views/email-branding/manage-branding.html', form=form, email_branding=email_branding, - cdn_url=get_cdn_domain(), + cdn_url=get_logo_cdn_domain(), logo=logo ) @@ -126,7 +126,7 @@ def create_email_branding(logo=None): return render_template( 'views/email-branding/manage-branding.html', form=form, - cdn_url=get_cdn_domain(), + cdn_url=get_logo_cdn_domain(), logo=logo ) diff --git a/app/main/views/index.py b/app/main/views/index.py index 24aa8eb44..7cb508e14 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -16,7 +16,7 @@ from app import email_branding_client from app.main import main from app.main.forms import SearchTemplatesForm from app.main.views.sub_navigation_dictionaries import features_nav -from app.utils import AgreementInfo, get_cdn_domain +from app.utils import AgreementInfo, get_logo_cdn_domain @main.route('/') @@ -108,7 +108,7 @@ def email_template(): colour = email_branding['colour'] brand_name = email_branding['text'] brand_colour = colour - brand_logo = ('https://{}/{}'.format(get_cdn_domain(), email_branding['logo']) + brand_logo = ('https://{}/{}'.format(get_logo_cdn_domain(), email_branding['logo']) if email_branding['logo'] else None) govuk_banner = branding_type in ['govuk', 'both'] brand_banner = branding_type == 'org_banner' diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 6166d61cc..965b516c6 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -54,7 +54,7 @@ from app.main.forms import ( from app.utils import ( AgreementInfo, email_safe, - get_cdn_domain, + get_logo_cdn_domain, user_has_permissions, user_is_platform_admin, ) @@ -991,7 +991,7 @@ def get_branding_as_value_and_label(email_branding): def get_branding_as_dict(email_branding): return { branding['id']: { - 'logo': 'https://{}/{}'.format(get_cdn_domain(), branding['logo']), + 'logo': 'https://{}/{}'.format(get_logo_cdn_domain(), branding['logo']), 'colour': branding['colour'] } for branding in email_branding } diff --git a/app/utils.py b/app/utils.py index d11b1c53e..26e945901 100644 --- a/app/utils.py +++ b/app/utils.py @@ -354,7 +354,7 @@ def email_or_sms_not_enabled(template_type, permissions): return (template_type in ['email', 'sms']) and (template_type not in permissions) -def get_cdn_domain(): +def get_logo_cdn_domain(): parsed_uri = urlparse(current_app.config['ADMIN_BASE_URL']) if parsed_uri.netloc.startswith('localhost'): diff --git a/tests/app/main/views/test_headers.py b/tests/app/main/views/test_headers.py index 95e2c8d8e..853bbba1a 100644 --- a/tests/app/main/views/test_headers.py +++ b/tests/app/main/views/test_headers.py @@ -1,5 +1,5 @@ def test_owasp_useful_headers_set(client, mocker): - mocker.patch('app.get_cdn_domain', return_value='static-logos.test.com') + mocker.patch('app.get_logo_cdn_domain', return_value='static-logos.test.com') response = client.get('/') @@ -20,7 +20,7 @@ def test_owasp_useful_headers_set(client, mocker): def test_headers_non_ascii_characters_are_replaced(client, mocker): - mocker.patch('app.get_cdn_domain', return_value='static-logos€æ.test.com') + mocker.patch('app.get_logo_cdn_domain', return_value='static-logos€æ.test.com') response = client.get('/') diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 48136844f..75fb651c9 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -16,7 +16,7 @@ from app.utils import ( generate_next_dict, generate_notifications_csv, generate_previous_dict, - get_cdn_domain, + get_logo_cdn_domain, ) from tests.conftest import fake_uuid @@ -278,13 +278,13 @@ def test_generate_notifications_csv_calls_twice_if_next_link( def test_get_cdn_domain_on_localhost(client, mocker): mocker.patch.dict('app.current_app.config', values={'ADMIN_BASE_URL': 'http://localhost:6012'}) - domain = get_cdn_domain() + domain = get_logo_cdn_domain() assert domain == 'static-logos.notify.tools' def test_get_cdn_domain_on_non_localhost(client, mocker): mocker.patch.dict('app.current_app.config', values={'ADMIN_BASE_URL': 'https://some.admintest.com'}) - domain = get_cdn_domain() + domain = get_logo_cdn_domain() assert domain == 'static-logos.admintest.com' From 23cc182b6fb036dbb1b6e0d502653bce4d401f2d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Nov 2018 12:07:48 +0000 Subject: [PATCH 3/4] Get config from current app --- app/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0797204ce..fbdd3b43c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -496,7 +496,6 @@ def save_service_or_org_after_request(response): # https://www.owasp.org/index.php/List_of_useful_HTTP_headers def useful_headers_after_request(response): - notify_environment = os.environ['NOTIFY_ENVIRONMENT'] response.headers.add('X-Frame-Options', 'deny') response.headers.add('X-Content-Type-Options', 'nosniff') response.headers.add('X-XSS-Protection', '1; mode=block') @@ -508,7 +507,7 @@ def useful_headers_after_request(response): "font-src 'self' {asset_domain} data:;" "img-src 'self' {asset_domain} *.google-analytics.com *.notifications.service.gov.uk {logo_domain} data:;" "frame-src 'self' www.youtube.com;".format( - asset_domain=configs[notify_environment].ASSET_DOMAIN, + asset_domain=current_app.config['ASSET_DOMAIN'], logo_domain=get_logo_cdn_domain(), ) )) From 4e53cafd46d27fdbe6bbc9c804e3885464dcb8e9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 29 Nov 2018 12:18:00 +0000 Subject: [PATCH 4/4] Use NOTIFY_ENVIRONMENT to test if running locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This matches what we tell people, in the README, to put in their local environment.sh It doesn’t matter what Jenkins sets this to, as long as it’s not `'development'`. We think Jenkins isn’t setting it at all (in which case the value will be `undefined`) --- gulpfile.babel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gulpfile.babel.js b/gulpfile.babel.js index da3176d02..435b5dcf6 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -37,7 +37,7 @@ gulp.task('copy:govuk_template:css', () => gulp.src(paths.template + 'assets/sty })) .on('error', plugins.sass.logError) .pipe(plugins.cssUrlAdjuster({ - prependRelative: process.env.NOTIFY_ADMIN_ENVIRONMENT == 'config.Development' ? '/static/' : '/', + prependRelative: process.env.NOTIFY_ENVIRONMENT == 'development' ? '/static/' : '/', })) .pipe(gulp.dest(paths.dist + 'stylesheets/')) );