fix xss with service letter contact blocks

service contact blocks contain new lines - and jinja2 normally ignores
newlines (as in it keeps them as new lines) - but we need to turn them
into `<br>` tags so that we can show the formatting that the user has
added. We were previously just doing `{{ block | nl2br | safe }}`. nl2br
turns the new lines into `<br>` tags, and then `safe` tells jinja that
it doesn't need to escape the html.

this causes issues if the user adds `<script>alert(1)</script>` to their
contact block (or some other evil xss hack), where that will get let
through due to the safe flag

To solve this, use `Markup(html='escape')` to sanitise any html, and
then convert new lines to <br>.

bump utils

another xss

app/__init__.py

@@ -8,6 +8,7 @@ from time import monotonic
 import ago
 import jinja2
 from flask import (
+    Markup,
     current_app,
     flash,
     g,
@@ -26,13 +27,14 @@ from govuk_frontend_jinja.flask_ext import init_govuk_frontend
 from itsdangerous import BadSignature
 from notifications_python_client.errors import HTTPError
 from notifications_utils import formatters, logging, request_helper
-from notifications_utils.formatters import formatted_list
+from notifications_utils.field import Field
 from notifications_utils.recipients import (
     InvalidPhoneError,
     format_phone_number_human_readable,
     validate_phone_number,
 )
 from notifications_utils.sanitise_text import SanitiseASCII
+from notifications_utils.take import Take
 from notifications_utils.timezones import utc_string_to_aware_gmt_datetime
 from werkzeug.exceptions import HTTPException as WerkzeugHTTPException
 from werkzeug.exceptions import abort
@@ -467,7 +469,14 @@ def format_notification_status_as_url(status, notification_type):
 
 
 def nl2br(value):
-    return formatters.nl2br(value) if value else ''
+    if value:
+        return Markup(Take(Field(
+            value,
+            html='escape',
+        )).then(
+            formatters.nl2br
+        ))
+    return ''
 
 
 @login_manager.user_loader
@@ -740,7 +749,7 @@ def add_template_filters(application):
         format_notification_status_as_time,
         format_notification_status_as_field_status,
         format_notification_status_as_url,
-        formatted_list,
+        formatters.formatted_list,
         nl2br,
         format_phone_number_human_readable,
         format_thousands,

app/main/validators.py

@@ -1,6 +1,7 @@
 import re
 
 from notifications_utils.field import Field
+from notifications_utils.formatters import formatted_list
 from notifications_utils.recipients import (
     InvalidEmailError,
     validate_email_address,
@@ -9,7 +10,6 @@ from notifications_utils.sanitise_text import SanitiseSMS
 from wtforms import ValidationError
 from wtforms.validators import Email
 
-from app import formatted_list
 from app.main._blacklisted_passwords import blacklisted_passwords
 from app.utils import Spreadsheet, is_gov_user
 

app/models/service.py

@@ -1,7 +1,4 @@
-from flask import Markup, abort, current_app
+from flask import abort, current_app
-from notifications_utils.field import Field
-from notifications_utils.formatters import nl2br
-from notifications_utils.take import Take
 from werkzeug.utils import cached_property
 
 from app.models import JSONModel
@@ -358,13 +355,11 @@ class Service(JSONModel):
 
     @property
     def default_letter_contact_block_html(self):
+        # import in the function to prevent cyclical imports
+        from app import nl2br
+
         if self.default_letter_contact_block:
-            return Markup(Take(Field(
+            return nl2br(self.default_letter_contact_block['contact_block'])
-                self.default_letter_contact_block['contact_block'],
-                html='escape',
-            )).then(
-                nl2br
-            ))
         return ''
 
     def edit_letter_contact_block(self, id, contact_block, is_default):

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 | safe if current_service.default_sms_sender else 'None'}}
+            {{ current_service.default_sms_sender | string | nl2br if current_service.default_sms_sender else 'None'}}
             {% if current_service.count_sms_senders > 1 %}
               <div class="hint">
                 {{ '…and %d more' | format(current_service.count_sms_senders - 1) }}

app/templates/views/service-settings/letter-contact-details.html

@@ -32,7 +32,7 @@
     {% for item in letter_contact_details %}
       <div class="user-list-item">
         <p>
-          {{ item.contact_block | nl2br | safe }}
+          {{ item.contact_block | nl2br }}
         </p>
         <p class="hint">
           {%- if item.is_default -%}

requirements-app.txt

@@ -23,5 +23,5 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@36.4.0#egg=notifications-utils==36.4.0
+git+https://github.com/alphagov/notifications-utils.git@36.4.1#egg=notifications-utils==36.4.1
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha

requirements.txt

@@ -25,7 +25,7 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@36.4.0#egg=notifications-utils==36.4.0
+git+https://github.com/alphagov/notifications-utils.git@36.4.1#egg=notifications-utils==36.4.1
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha
 
 ## The following requirements were added by pip freeze: