From f3fa6a67e12b44434556d2eecd7a2e3b4c0ce619 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 22 Jan 2020 16:13:25 +0000 Subject: [PATCH] fix one more place where senders weren't sanitised make sure everything is using the `nl2br` formatter that properly wraps it in markdown to keep everything sanitised nicely. Also write a couple of tests --- app/main/views/send.py | 3 ++- app/main/views/templates.py | 5 ++--- app/templates/views/service-settings.html | 2 +- tests/app/main/views/test_service_settings.py | 15 ++++++++++++- tests/app/main/views/test_templates.py | 22 ++++++++++++++++++- tests/conftest.py | 2 +- 6 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 30a351f22..8764406c4 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -31,6 +31,7 @@ from xlrd.xldate import XLDateError from app import ( current_service, job_api_client, + nl2br, notification_api_client, service_api_client, ) @@ -272,7 +273,7 @@ def get_sender_context(sender_details, template_type): if context['default_id'] == context.get('receives_text_message', None): context['default_and_receives'] = context['default_id'] - context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] + context['value_and_label'] = [(sender['id'], nl2br(sender[sender_format])) for sender in sender_details] return context diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 3d4f18ee4..6d61d4755 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -4,14 +4,13 @@ from functools import partial from dateutil.parser import parse from flask import abort, flash, redirect, render_template, request, url_for from flask_login import current_user -from markupsafe import Markup from notifications_python_client.errors import HTTPError from notifications_utils import LETTER_MAX_PAGE_COUNT -from notifications_utils.formatters import nl2br from notifications_utils.pdf import is_letter_too_long from app import ( current_service, + nl2br, service_api_client, template_folder_api_client, template_statistics_client, @@ -844,7 +843,7 @@ def get_template_sender_form_dict(service_id, template): if not service_senders: context['no_senders'] = True - context['value_and_label'] = [(sender['id'], Markup(nl2br(sender[sender_format]))) for sender in service_senders] + context['value_and_label'] = [(sender['id'], nl2br(sender[sender_format])) for sender in service_senders] context['value_and_label'].insert(0, ('', 'Blank')) # Add blank option to start of list context['current_choice'] = template['service_letter_contact'] if template['service_letter_contact'] else '' diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index d308068c8..52efd838d 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -136,7 +136,7 @@ {% call settings_row(if_has_permission='sms') %} {{ text_field('Text message senders') }} {% call field(status='default' if current_service.default_sms_sender == "None" else '') %} - {{ current_service.default_sms_sender | string | nl2br if current_service.default_sms_sender else 'None'}} + {{ current_service.default_sms_sender | nl2br if current_service.default_sms_sender else 'None'}} {% if current_service.count_sms_senders > 1 %}
{{ '…and %d more' | format(current_service.count_sms_senders - 1) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2f4cdae33..2f38ebcb3 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1977,7 +1977,7 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( 'Blank Make default', '1 Example Street (default) Change 1234', '2 Example Street Change 5678', - '3 Example Street Change 9457', + 'foobaz Change 9457', ], ), ( 'main.service_sms_senders', @@ -2756,6 +2756,19 @@ def test_default_box_shows_on_non_default_sender_details_while_editing( ) +def test_sender_details_are_escaped(client_request, mocker, fake_uuid): + letter_contact_block = create_letter_contact_block(contact_block='foo\n\n
\n\nbar') + mocker.patch('app.service_api_client.get_letter_contacts', return_value=[letter_contact_block]) + + page = client_request.get( + 'main.service_letter_contact_details', + service_id=SERVICE_ONE_ID, + ) + + # get the second row (first is the default Blank sender) + assert 'foo
bar' in normalize_spaces(page.select('.user-list-item')[1].text) + + @pytest.mark.parametrize('sms_sender, expected_link_text, partial_href', [ ( create_sms_sender(is_default=False), diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index fde9b8f0b..b2262c213 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -2207,7 +2207,6 @@ def test_add_sender_link_only_appears_on_services_with_no_senders( mocker, contact_block_data, mock_get_service_letter_template, - no_letter_contact_blocks ): mocker.patch('app.service_api_client.get_letter_contacts', return_value=contact_block_data) page = client_request.get( @@ -2221,3 +2220,24 @@ def test_add_sender_link_only_appears_on_services_with_no_senders( service_id=SERVICE_ONE_ID, from_template=fake_uuid, ) + + +def test_set_template_sender_escapes_letter_contact_block_names( + client_request, + fake_uuid, + mocker, + mock_get_service_letter_template, +): + letter_contact_block = create_letter_contact_block(contact_block='foo\n\n