From 4d65b94c771f44e17c8a8914972b13a1745c4a00 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Aug 2020 15:05:56 +0100 Subject: [PATCH] Fix HTML being escaped in preview of email subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EmailPreviewTemplate.subject` returns a string of HTML, with any user-submitted HTML already escaped: https://github.com/alphagov/notifications-utils/blob/b5a61bfb7b6b4c55898407304f2758a8f4be8585/notifications_utils/template.py#L672 What won’t be escaped is the HTML needed to redact the placeholders. We generate this HTML so we know its safe, and doesn’t need to be escaped. However when we pass it to Jinja, Jinja doesn’t know this, so will try to escape it. This means users will see the raw HTML. We can get around this by using Flask’s `Markup` class to tell Jinja that the string is already sanitised and doesn’t need escaping again. Text message templates don’t have this problem because they already return `Markup`: https://github.com/alphagov/notifications-utils/blob/b5a61bfb7b6b4c55898407304f2758a8f4be8585/notifications_utils/template.py#L288 Letter templates don’t suffer from this problem (because they don’t support redaction) but without making the same change they would still double-escape ampersands, greater-than symbols, and so on. --- app/main/views/jobs.py | 9 +++++---- tests/app/main/views/test_activity.py | 23 ++++++++++++++++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 0e5f37266..f60f6879f 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -3,6 +3,7 @@ from functools import partial from flask import ( + Markup, Response, abort, flash, @@ -438,14 +439,14 @@ def get_preview_of_content(notification): )) if notification['template']['template_type'] == 'email': - return EmailPreviewTemplate( + return Markup(EmailPreviewTemplate( notification['template'], notification['personalisation'], redact_missing_personalisation=True, - ).subject + ).subject) if notification['template']['template_type'] == 'letter': - return LetterPreviewTemplate( + return Markup(LetterPreviewTemplate( notification['template'], notification['personalisation'], - ).subject + ).subject) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index fffb23471..4c6461fbb 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -648,29 +648,46 @@ def test_html_contains_links_for_failed_notifications( assert normalize_spaces(link_text) == 'Technical failure' +@pytest.mark.parametrize('notification_type, expected_row_contents', ( + ('sms', ( + '07123456789 hello & welcome hidden' + )), + ('email', ( + 'example@gov.uk hidden, hello & welcome' + )), + ('letter', ( + # Letters don’t support redaction, but this test is still + # worthwhile to show that the ampersand is not double-escaped + '1 Example Street ((name)), hello & welcome' + )), +)) def test_redacts_templates_that_should_be_redacted( client_request, mocker, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, + notification_type, + expected_row_contents, ): notifications = create_notifications( status='technical-failure', - content='hello ((name))', + content='hello & welcome ((name))', + subject='((name)), hello & welcome', personalisation={'name': 'Jo'}, redact_personalisation=True, + template_type=notification_type, ) mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) page = client_request.get( 'main.view_notifications', service_id=SERVICE_ONE_ID, - message_type='sms', + message_type=notification_type, ) assert normalize_spaces(page.select('tbody tr th')[0].text) == ( - '07123456789 hello hidden' + expected_row_contents )