From 536d091d8540dbfca95a921be0a04d981b8761ac Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Mar 2017 12:54:27 +0000 Subject: [PATCH 1/2] Fix HTML showing up on the breaking change page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 41fa1586353374d611f7b7e7690b03c13b830880 added a proper Jinja filter for formatting lists, which was better than the previous macro-based solution. It didn’t, however, account for HTML properly. It did the default Jinja thing of escaping everything. Since we render lists of placeholders by putting HTML before and after each item, this didn’t work (the HTML got escaped and appeared on the page). So this commit does the escaping of HTML outside Jinja, in the user-submitted bits of the input only, then passes the whole thing through as a `Markup` instance which doesn’t get escaped by Jinja. --- app/__init__.py | 9 +++++---- tests/app/test_jinja_filters.py | 7 +++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 726e2a38b..89eb1f55c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -30,6 +30,7 @@ from notifications_python_client.errors import HTTPError from notifications_utils import logging, request_id, formatters from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.recipients import validate_phone_number, InvalidPhoneError +from notifications_utils.field import escape_html from pygments import highlight from pygments.formatters.html import HtmlFormatter from pygments.lexers.javascript import JavascriptLexer @@ -363,17 +364,17 @@ def formatted_list( if prefix_plural: prefix_plural += ' ' - items = list(items) + items = list(map(escape_html, items)) if len(items) == 1: - return '{prefix}{before_each}{items[0]}{after_each}'.format(**locals()) + return Markup('{prefix}{before_each}{items[0]}{after_each}'.format(**locals())) elif items: formatted_items = ['{}{}{}'.format(before_each, item, after_each) for item in items] first_items = separator.join(formatted_items[:-1]) last_item = formatted_items[-1] - return ( + return Markup(( '{prefix_plural}{first_items} {conjunction} {last_item}' - ).format(**locals()) + ).format(**locals())) def nl2br(value): diff --git a/tests/app/test_jinja_filters.py b/tests/app/test_jinja_filters.py index 2d24fc00e..39e337761 100644 --- a/tests/app/test_jinja_filters.py +++ b/tests/app/test_jinja_filters.py @@ -1,5 +1,6 @@ import pytest +from flask import Markup from app import formatted_list @@ -11,6 +12,12 @@ from app import formatted_list ([1], {'prefix': 'foo', 'prefix_plural': 'bar'}, 'foo ‘1’'), ([1, 2, 3], {'before_each': 'a', 'after_each': 'b'}, 'a1b, a2b and a3b'), ([1, 2, 3], {'conjunction': 'foo'}, '‘1’, ‘2’ foo ‘3’'), + (['&'], {'before_each': '', 'after_each': ''}, '&'), + ([1, 2, 3], {'before_each': '', 'after_each': ''}, '1, 2 and 3'), ]) def test_formatted_list(items, kwargs, expected_output): assert formatted_list(items, **kwargs) == expected_output + + +def test_formatted_list_returns_markup(): + assert isinstance(formatted_list([0]), Markup) From 5081d83c1992360e9fc22b9a9246f7ed02b6b8dc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Mar 2017 13:08:27 +0000 Subject: [PATCH 2/2] Fix count of columns on breaking change page For some reason we were rebuilding `new_template` as a dictionary, without the `placeholders` attribute. This meant that we were never actually counting the placeholders, just counting the length of `None` and adding 1 to it. So this commit fixes that, beefs up the tests, and makes sure that everything is pluralised properly. --- app/main/views/templates.py | 7 +------ app/templates/views/templates/breaking-change.html | 3 ++- tests/app/main/views/test_templates.py | 12 ++++++++++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index b0f5a4a76..31bf4a390 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -262,12 +262,7 @@ def edit_service_template(service_id, template_id): return render_template( 'views/templates/breaking-change.html', template_change=template_change, - new_template={ - 'name': form.name.data, - 'subject': subject, - 'content': form.template_content.data, - 'id': new_template.id - }, + new_template=new_template, column_headings=list(ascii_uppercase[:len(new_template.placeholders) + 1]), example_rows=[ first_column_headings[new_template.template_type] + list(new_template.placeholders), diff --git a/app/templates/views/templates/breaking-change.html b/app/templates/views/templates/breaking-change.html index 6ca08fa25..b6c76140e 100644 --- a/app/templates/views/templates/breaking-change.html +++ b/app/templates/views/templates/breaking-change.html @@ -41,7 +41,8 @@

When you send messages using this template you’ll need - {{ new_template.placeholders|length + 1 }} columns of data: + {{ new_template.placeholders|length + 1 }} + column{{ 's' if new_template.placeholders|length > 0 else '' }} of data:

diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 6ecf09309..639b86280 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -9,6 +9,7 @@ from freezegun import freeze_time from notifications_python_client.errors import HTTPError from tests.conftest import service_one as create_sample_service from tests import validate_route_permission, template_json, single_notification_json +from tests.app.test_utils import normalize_spaces from app.main.views.templates import get_last_use_message, get_human_readable_delta @@ -262,7 +263,7 @@ def test_should_show_interstitial_when_making_breaking_change( data={ 'id': template_id, 'name': "new name", - 'template_content': "hello", + 'template_content': "hello ((name)) lets talk about ((thing))", 'template_type': 'email', 'subject': 'reminder', 'service': service_id, @@ -276,10 +277,17 @@ def test_should_show_interstitial_when_making_breaking_change( assert page.find('a', {'class': 'page-footer-back-link'})['href'] == url_for(".edit_service_template", service_id=service_id, template_id=template_id) + for index, p in enumerate([ + 'You removed ((date))', + 'You added ((name))', + 'When you send messages using this template you’ll need 3 columns of data:', + ]): + assert normalize_spaces(page.select('main p')[index].text) == p + for key, value in { 'name': 'new name', 'subject': 'reminder', - 'template_content': 'hello', + 'template_content': 'hello ((name)) lets talk about ((thing))', 'confirm': 'true' }.items(): assert page.find('input', {'name': key})['value'] == value