From 597c6da8578ac75b464c20d6bf5b36a3923e3728 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jan 2017 12:22:24 +0000 Subject: [PATCH 1/2] Sanitize HTML in templates using utils We can no longer trust that the content of templates stored in the database is safe. Utils now has code to sanitise the content of templates. This commit: - updates utils to bring this code in - modifies some integration tests to make sure everything is working (there are more extensive unit tests in utils) --- requirements.txt | 2 +- tests/app/main/views/test_jobs.py | 2 +- tests/app/main/views/test_send.py | 2 +- tests/app/main/views/test_templates.py | 8 ++++---- tests/conftest.py | 10 +++++----- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/requirements.txt b/requirements.txt index dd5d88e03..7aa73ef3f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,4 +29,4 @@ whitenoise==1.0.6 #manages static assets # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 -git+https://github.com/alphagov/notifications-utils.git@13.1.1#egg=notifications-utils==13.1.1 +git+https://github.com/alphagov/notifications-utils.git@13.2.0#egg=notifications-utils==13.2.0 diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 3dda7fa4f..68701fe88 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -112,7 +112,7 @@ def test_should_show_page_for_one_job( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text.strip() == 'thisisatest.csv' assert page.find('div', {'class': 'sms-message-wrapper'}).text.strip() == ( - '{}: Your vehicle tax is about to expire'.format(service_one['name']) + '{}: Template content with & entity'.format(service_one['name']) ) assert ' '.join(page.find('tbody').find('tr').text.split()) == ( '07123456789 Delivered 1 January at 11:10am' diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 452a04ae3..4a99bb590 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -479,7 +479,7 @@ def test_should_show_preview_letter_message( mock_get_service_letter_template.assert_called_with(service_id, template_id) assert mock_letter_preview.call_args[0][0]['message'] == ( '

Subject

\n' - '

Your vehicle tax is about to expire

' + '

Template <em>content</em> with & entity

' ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 5680ffd9b..4ca915753 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -35,7 +35,7 @@ def test_should_show_page_for_one_template( assert response.status_code == 200 assert "Two week reminder" in response.get_data(as_text=True) - assert "Your vehicle tax is about to expire" in response.get_data(as_text=True) + assert "Template <em>content</em> with & entity" in response.get_data(as_text=True) assert "Use priority queue?" not in response.get_data(as_text=True) mock_get_service_template.assert_called_with( service_id, template_id) @@ -63,7 +63,7 @@ def test_should_show_page_template_with_priority_select_if_platform_admin( assert response.status_code == 200 assert "Two week reminder" in response.get_data(as_text=True) - assert "Your vehicle tax is about to expire" in response.get_data(as_text=True) + assert "Template <em>content</em> with & entity" in response.get_data(as_text=True) assert "Use priority queue?" in response.get_data(as_text=True) mock_get_service_template.assert_called_with( service_id, template_id) @@ -419,7 +419,7 @@ def test_should_show_delete_template_page_with_time_block(app_, assert 'Test template was last used 10 minutes ago. Are you sure you want to delete it?' in content assert 'Are you sure' in content assert 'Two week reminder' in content - assert 'Your vehicle tax is about to expire' in content + assert 'Template <em>content</em> with & entity' in content mock_get_service_template.assert_called_with(service_id, template_id) @@ -453,7 +453,7 @@ def test_should_show_delete_template_page_with_never_used_block(app_, assert 'Two week reminder has never been used. Are you sure you want to delete it?' in content assert 'Are you sure' in content assert 'Two week reminder' in content - assert 'Your vehicle tax is about to expire' in content + assert 'Template <em>content</em> with & entity' in content mock_get_service_template.assert_called_with(service_id, template_id) diff --git a/tests/conftest.py b/tests/conftest.py index 054952fd9..c04ea82ac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -232,7 +232,7 @@ def mock_get_services_with_one_service(mocker, fake_uuid, user=None): def mock_get_service_template(mocker): def _get(service_id, template_id, version=None): template = template_json( - service_id, template_id, "Two week reminder", "sms", "Your vehicle tax is about to expire") + service_id, template_id, "Two week reminder", "sms", "Template content with & entity") if version: template.update({'version': version}) return {'data': template} @@ -248,7 +248,7 @@ def mock_get_service_template_with_priority(mocker): def _get(service_id, template_id, version=None): template = template_json( - service_id, template_id, "Two week reminder", "sms", "Your vehicle tax is about to expire", + service_id, template_id, "Two week reminder", "sms", "Template content with & entity", process_type='priority') if version: template.update({'version': version}) @@ -268,7 +268,7 @@ def mock_get_deleted_template(mocker): template_id, "Two week reminder", "sms", - "Your vehicle tax is about to expire", + "Template content with & entity", archived=True ) if version: @@ -325,7 +325,7 @@ def mock_get_template_versions(mocker, fake_uuid, user=None): def mock_get_service_template_with_placeholders(mocker): def _get(service_id, template_id): template = template_json( - service_id, template_id, "Two week reminder", "sms", "((name)), your vehicle tax is about to expire" + service_id, template_id, "Two week reminder", "sms", "((name)), Template content with & entity" ) return {'data': template} @@ -377,7 +377,7 @@ def mock_get_service_letter_template(mocker): template_id, "Two week reminder", "letter", - "Your vehicle tax is about to expire", "Subject") + "Template content with & entity", "Subject") return {'data': template} return mocker.patch( From 04966e40519bdfbbb68bebecef77c78a486d3784 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Jan 2017 16:51:53 +0000 Subject: [PATCH 2/2] Test that template content passed to API is dirty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t want sanitized content going into the database, because sometimes we need the content unsanitised. The path from admin to the API is where the template goes on it’s way to the database. So let’s make sure we’re not sanitizing it at this stage. --- tests/app/main/views/test_templates.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 4ca915753..1f1d0a5a9 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -128,7 +128,7 @@ def test_should_redirect_when_saving_a_template(app_, mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) template_id = fake_uuid name = "new name" - content = "template content" + content = "template content with & entity" data = { 'id': template_id, 'name': name, @@ -165,7 +165,7 @@ def test_should_edit_content_when_process_type_is_priority_not_platform_admin( data = { 'id': template_id, 'name': "new name", - 'template_content': "change the content", + 'template_content': "new template content with & entity", 'template_type': 'sms', 'service': service['id'], 'process_type': 'priority' @@ -178,7 +178,14 @@ def test_should_edit_content_when_process_type_is_priority_not_platform_admin( assert response.location == url_for( '.view_template', service_id=service['id'], template_id=template_id, _external=True) mock_update_service_template.assert_called_with( - template_id, "new name", 'sms', "change the content", service['id'], None, 'priority') + template_id, + "new name", + 'sms', + "new template content with & entity", + service['id'], + None, + 'priority' + ) def test_should_403_when_edit_template_with_process_type_of_priority_for_non_platform_admin( @@ -197,7 +204,7 @@ def test_should_403_when_edit_template_with_process_type_of_priority_for_non_pla data = { 'id': template_id, 'name': "new name", - 'template_content': "template content", + 'template_content': "template content with & entity", 'template_type': 'sms', 'service': service['id'], 'process_type': 'priority' @@ -226,7 +233,7 @@ def test_should_403_when_create_template_with_process_type_of_priority_for_non_p data = { 'id': template_id, 'name': "new name", - 'template_content': "template content", + 'template_content': "template content with & entity", 'template_type': 'sms', 'service': service['id'], 'process_type': 'priority' @@ -362,7 +369,7 @@ def test_should_redirect_when_saving_a_template_email(app_, service_id = fake_uuid template_id = fake_uuid name = "new name" - content = "template content ((thing)) ((date))" + content = "template content with & entity ((thing)) ((date))" subject = "subject" data = { 'id': template_id,