From 267b58a66d7fd8eb6dc09200d33ca643d330c886 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 14 Apr 2017 08:52:02 +0100 Subject: [PATCH] Stop template subjects getting saved encoded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is another problem with sanitising HTML, this with with it getting encoded where it shouldn’t be. The result was, when editing a template, the API getting sent an encoded rather than raw version of the subject (for letters and emails). The reason this happened is because BeautifulSoup behaves in an unexpected way. When accessing the `value` attribute of an `input` BeautifulSoup returns an unencoded version of the contents. In other words it returns what the user would see in the page, not what is in the raw HTML of the page. This meant that we were trying too hard to see an `&` instead of a `&` in our tests[1]. So things were actually working fine before adding the call to `escape_html`[2], but from the output of the tests it didn’t look like HTML was getting escaped. So this commit fixes the bug by removing the call to `escape_html` and adding a test that looks at the raw HTML, to complement the existing test which looks at just the `value` attribute. 1. Relevant test added here: https://github.com/alphagov/notifications-admin/pull/1178/files#diff-f2eb304b93cc383727c0ab7fc8fbd464R289 2. Call added here: https://github.com/alphagov/notifications-admin/pull/1178/files#diff-f0af582449ebf426f27f37e38f310057R252 --- app/main/views/templates.py | 2 +- tests/app/main/views/test_templates.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index cb6831304..203ae0b64 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -276,7 +276,7 @@ def edit_service_template(service_id, template_id): if form.process_type.data != template['process_type']: abort_403_if_not_admin_user() - subject = escape_html(form.subject.data) if hasattr(form, 'subject') else None + subject = form.subject.data if hasattr(form, 'subject') else None new_template = get_template({ 'name': form.name.data, 'content': form.template_content.data, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index d4777468f..b321ca50a 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -360,7 +360,7 @@ def test_should_show_interstitial_when_making_breaking_change( 'name': "new name", 'template_content': "hello lets talk about ((thing))", 'template_type': 'email', - 'subject': 'reminder & ((name))', + 'subject': 'reminder \'" & ((name))', 'service': service_id, 'process_type': 'normal' } @@ -377,12 +377,18 @@ def test_should_show_interstitial_when_making_breaking_change( for key, value in { 'name': 'new name', - 'subject': 'reminder & ((name))', + 'subject': 'reminder \'" & ((name))', 'template_content': 'hello lets talk about ((thing))', 'confirm': 'true' }.items(): assert page.find('input', {'name': key})['value'] == value + # BeautifulSoup returns the value attribute as unencoded, let’s make + # sure that it is properly encoded in the HTML + assert str(page.find('input', {'name': 'subject'})) == ( + """""" + ) + def test_should_not_create_too_big_template( logged_in_client, @@ -450,7 +456,7 @@ def test_should_redirect_when_saving_a_template_email( template_id = fake_uuid name = "new name" content = "template content with & entity ((thing)) ((date))" - subject = "subject" + subject = "subject & entity" data = { 'id': template_id, 'name': name,