From 64b5f03dcd7a9081e524391acfe22ce96d4ee274 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Mar 2018 10:41:58 +0000 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20populate=20support=20form=20wit?= =?UTF-8?q?h=20arbitrary=20text?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I don’t think it’s a massive risk (we’re certainly mitigating against any XSS), but having a page on a GOV.UK domain where you can prefill text on the page from a query string probably isn’t great. So this commit restricts prefilling the support form to a set of named questions. --- app/main/views/feedback.py | 17 ++++++++++++- app/templates/views/pricing.html | 2 +- app/templates/views/terms-of-use.html | 4 +-- app/utils.py | 11 ++++++++ tests/app/main/views/test_feedback.py | 36 ++++++++++++++++++++++++--- 5 files changed, 62 insertions(+), 8 deletions(-) diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index 48e76c94a..fec897efa 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -13,11 +13,26 @@ from app import ( ) from app.main import main from app.main.forms import Feedback, Problem, SupportType, Triage +from app.utils import AgreementInfo QUESTION_TICKET_TYPE = 'ask-question-give-feedback' PROBLEM_TICKET_TYPE = "report-problem" +def get_prefilled_message(): + agreement_info = AgreementInfo.from_current_user() + return { + 'agreement': ( + agreement_info.as_request_for_agreement() + ), + 'agreement-with-owner': ( + agreement_info.as_request_for_agreement(with_owner=True) + ), + }.get( + request.args.get('body'), '' + ) + + @main.route('/feedback', methods=['GET']) def old_feedback(): return redirect(url_for('.support')) @@ -135,7 +150,7 @@ def feedback(ticket_type): return redirect(url_for('.thanks', urgent=urgent, anonymous=anonymous)) if not form.feedback.data: - form.feedback.data = request.args.get('body', '') + form.feedback.data = get_prefilled_message() return render_template( 'views/support/{}.html'.format(ticket_type), diff --git a/app/templates/views/pricing.html b/app/templates/views/pricing.html index e16c26d17..fba01cfec 100644 --- a/app/templates/views/pricing.html +++ b/app/templates/views/pricing.html @@ -125,7 +125,7 @@

How to pay

You can find details of how to pay for Notify in our data sharing and financial agreement.

- Contact us to get a copy of the agreement + Contact us to get a copy of the agreement {% if agreement_info.agreement_signed %} ({{ agreement_info.owner }} has already accepted it). {% else %} diff --git a/app/templates/views/terms-of-use.html b/app/templates/views/terms-of-use.html index c6de90ced..f90530c14 100644 --- a/app/templates/views/terms-of-use.html +++ b/app/templates/views/terms-of-use.html @@ -28,10 +28,10 @@ Terms of use {% if agreement_info.owner %} ({{ agreement_info.owner }}) must also accept our data sharing and financial agreement. - Contact us to get a copy. + Contact us to get a copy. {% else %} must also accept our data sharing and financial agreement. - Contact us to get a copy. + Contact us to get a copy. {% endif %}

{% endif %} diff --git a/app/utils.py b/app/utils.py index a7075adbb..320117b4f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -464,6 +464,17 @@ class AgreementInfo: else: return 'Can’t tell' + def as_request_for_agreement(self, with_owner=False): + if with_owner and self.owner: + return ( + 'Please send me a copy of the GOV.UK Notify data sharing ' + 'and financial agreement for {} to sign.'.format(self.owner) + ) + return ( + 'Please send me a copy of the GOV.UK Notify data sharing ' + 'and financial agreement.' + ) + @staticmethod def get_matching_function(email_address_or_domain): diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index 5dc5da3c5..5e0740b80 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -7,6 +7,7 @@ from flask import url_for from freezegun import freeze_time from notifications_utils.clients import DeskproError from tests.conftest import ( + active_user_with_permissions, mock_get_services, mock_get_services_with_no_services, mock_get_services_with_one_service, @@ -85,29 +86,56 @@ def test_get_feedback_page(client, ticket_type, expected_status_code): assert response.status_code == expected_status_code +@pytest.mark.parametrize('prefilled_body, expected_textarea', [ + ( + 'agreement', + ( + 'Please send me a copy of the GOV.UK Notify data sharing ' + 'and financial agreement.' + ) + ), + ( + 'agreement-with-owner', + ( + 'Please send me a copy of the GOV.UK Notify data sharing ' + 'and financial agreement for Marine Management ' + 'Organisation to sign.' + ) + ), + ( + 'foo', + '' + ), +]) @freeze_time('2016-12-12 12:00:00.000000') def test_get_feedback_page_with_prefilled_body( client_request, mocker, + fake_uuid, + prefilled_body, + expected_textarea, ): + user = active_user_with_permissions(fake_uuid) + user.email_address = 'test@marinemanagement.org.uk' + mocker.patch('app.user_api_client.get_user', return_value=user) mock_post = mocker.patch('app.main.views.feedback.deskpro_client.create_ticket') page = client_request.get( 'main.feedback', ticket_type=QUESTION_TICKET_TYPE, - body='Please send cat pictures ', + body=prefilled_body, ) assert page.select_one('textarea').text == ( - 'Please send cat pictures ' + expected_textarea ) client_request.post( 'main.feedback', ticket_type=QUESTION_TICKET_TYPE, - body='Please send cat pictures ', + body='agreement', _data={'feedback': 'blah', 'name': 'Example', 'email_address': 'test@example.com'} ) message = mock_post.call_args[1]['message'] assert message.endswith('blah') - assert 'cat pictures' not in message + assert 'Please send' not in message @freeze_time('2016-12-12 12:00:00.000000')